From 9fc0a3f2aec50d6e0a479c1eeaedecaa59e82527 Mon Sep 17 00:00:00 2001 From: Hixie Date: Fri, 12 Jun 2015 17:09:57 -0700 Subject: [PATCH] Verify that callers correctly set parentUsesSize if they use the child's size (and fix a few cases that didn't). This introduces a bunch of code that should only run in debug builds, but we don't have #ifdefs yet. R=abarth@chromium.org Review URL: https://codereview.chromium.org/1182933003. --- engine/core/painting/Size.dart | 1 + examples/widgets/spinning_mixed.dart | 6 +-- sdk/BUILD.gn | 1 + sdk/lib/framework/debug/utils.dart | 12 +++++ sdk/lib/framework/rendering/box.dart | 55 ++++++++++++++++++---- sdk/lib/framework/rendering/object.dart | 4 +- sdk/lib/framework/rendering/paragraph.dart | 8 +++- 7 files changed, 71 insertions(+), 16 deletions(-) create mode 100644 sdk/lib/framework/debug/utils.dart diff --git a/engine/core/painting/Size.dart b/engine/core/painting/Size.dart index ed02a664f91..fefdf4f329d 100644 --- a/engine/core/painting/Size.dart +++ b/engine/core/painting/Size.dart @@ -8,6 +8,7 @@ part of dart.sky; /// Think of this as a vector from Point(0,0) to Point(size.width, size.height) class Size { const Size(this.width, this.height); + Size.copy(Size source) : width = source.width, height = source.height; const Size.fromWidth(this.width) : height = double.INFINITY; const Size.fromHeight(this.height) : width = double.INFINITY; diff --git a/examples/widgets/spinning_mixed.dart b/examples/widgets/spinning_mixed.dart index 324d0429726..e75b36c9156 100644 --- a/examples/widgets/spinning_mixed.dart +++ b/examples/widgets/spinning_mixed.dart @@ -71,12 +71,8 @@ void rotate(double timeStamp) { transformBox.translate(transformBox.size.width / 2.0, transformBox.size.height / 2.0); transformBox.rotateZ(delta); transformBox.translate(-transformBox.size.width / 2.0, -transformBox.size.height / 2.0); - - // tester.checkFrame(); } -final bool debugDisplayList = false; // set this to true to use the test rendering logic - void main() { // Because we're going to use UINodes, we want to initialise its // AppView, not use the default one. We don't really need to do @@ -84,7 +80,7 @@ void main() { // it's good practice in case we happen to not have a // RenderObjectToUINodeAdapter in our tree at startup, or in case we // want a renderViewOverride. - UINodeAppView.initUINodeAppView(renderViewOverride: debugDisplayList ? tester : null); + UINodeAppView.initUINodeAppView(); RenderFlex flexRoot = new RenderFlex(direction: FlexDirection.vertical); diff --git a/sdk/BUILD.gn b/sdk/BUILD.gn index 62d35410101..d4fd712599d 100644 --- a/sdk/BUILD.gn +++ b/sdk/BUILD.gn @@ -45,6 +45,7 @@ dart_pkg("sdk") { "lib/framework/components/tool_bar.dart", "lib/framework/debug/shake-to-reload.sky", "lib/framework/debug/tracing.dart", + "lib/framework/debug/utils.dart", "lib/framework/editing/editable_string.dart", "lib/framework/editing/editable_text.dart", "lib/framework/editing/keyboard.dart", diff --git a/sdk/lib/framework/debug/utils.dart b/sdk/lib/framework/debug/utils.dart new file mode 100644 index 00000000000..8aa2c84b205 --- /dev/null +++ b/sdk/lib/framework/debug/utils.dart @@ -0,0 +1,12 @@ + +final bool inDebugBuild = _initInDebugBuild(); + +bool _initInDebugBuild() { + bool _inDebug = false; + bool setAssert() { + _inDebug = true; + return true; + } + assert(setAssert()); + return _inDebug; +} diff --git a/sdk/lib/framework/rendering/box.dart b/sdk/lib/framework/rendering/box.dart index 3d54eae0e6d..f53fd0bd31b 100644 --- a/sdk/lib/framework/rendering/box.dart +++ b/sdk/lib/framework/rendering/box.dart @@ -4,16 +4,26 @@ import 'dart:math' as math; import 'dart:sky' as sky; -import 'object.dart'; -import '../painting/box_painter.dart'; -import 'package:vector_math/vector_math.dart'; + import 'package:sky/framework/net/image_cache.dart' as image_cache; +import 'package:vector_math/vector_math.dart'; + +import '../debug/utils.dart'; +import '../painting/box_painter.dart'; +import 'object.dart'; export '../painting/box_painter.dart'; // GENERIC BOX RENDERING // Anything that has a concept of x, y, width, height is going to derive from this +// This class should only be used in debug builds +class _DebugSize extends Size { + _DebugSize(Size source, this._owner, this._canBeUsedByParent): super.copy(source); + final RenderBox _owner; + final bool _canBeUsedByParent; +} + class EdgeDims { // used for e.g. padding const EdgeDims(this.top, this.right, this.bottom, this.left); @@ -153,7 +163,10 @@ class BoxConstraints { } Size constrain(Size size) { - return new Size(constrainWidth(size.width), constrainHeight(size.height)); + Size result = new Size(constrainWidth(size.width), constrainHeight(size.height)); + if (size is _DebugSize) + result = new _DebugSize(result, size._owner, size._canBeUsedByParent); + return result; } bool get isInfinite => maxWidth >= double.INFINITY || maxHeight >= double.INFINITY; @@ -220,6 +233,24 @@ abstract class RenderBox extends RenderObject { return constraints.constrainHeight(0.0); } + // This whole block should only be here in debug builds + bool _debugDoingThisLayout = false; + bool _debugCanParentUseSize; + void layoutWithoutResize() { + _debugDoingThisLayout = true; + _debugCanParentUseSize = false; + super.layoutWithoutResize(); + _debugCanParentUseSize = null; + _debugDoingThisLayout = false; + } + void layout(dynamic constraints, { bool parentUsesSize: false }) { + _debugDoingThisLayout = true; + _debugCanParentUseSize = parentUsesSize; + super.layout(constraints, parentUsesSize: parentUsesSize); + _debugCanParentUseSize = null; + _debugDoingThisLayout = false; + } + BoxConstraints get constraints { BoxConstraints result = super.constraints; return result; } void performResize() { // default behaviour for subclasses that have sizedByParent = true @@ -241,11 +272,19 @@ abstract class RenderBox extends RenderObject { } void hitTestChildren(HitTestResult result, { Point position }) { } + // TODO(ianh): In non-debug builds, this should all just be: + // Size size = Size.zero; + // In debug builds, however: Size _size = Size.zero; Size get size => _size; void set size(Size value) { assert(RenderObject.debugDoingLayout); - _size = value; + assert(_debugDoingThisLayout); + if (value is _DebugSize) { + assert(value._canBeUsedByParent); + assert(value._owner.parent == this); + } + _size = inDebugBuild ? new _DebugSize(value, this, _debugCanParentUseSize) : value; } String debugDescribeSettings(String prefix) => '${super.debugDescribeSettings(prefix)}${prefix}size: ${size}\n'; @@ -394,7 +433,7 @@ class RenderConstrainedBox extends RenderProxyBox { void performLayout() { if (child != null) { - child.layout(constraints.apply(_additionalConstraints)); + child.layout(constraints.apply(_additionalConstraints), parentUsesSize: true); size = child.size; } else { performResize(); @@ -439,7 +478,7 @@ class RenderShrinkWrapWidth extends RenderProxyBox { void performLayout() { if (child != null) { - child.layout(_getInnerConstraints(constraints)); + child.layout(_getInnerConstraints(constraints), parentUsesSize: true); size = child.size; } else { performResize(); @@ -663,7 +702,7 @@ class RenderPositionedBox extends RenderShiftedBox { void performLayout() { if (child != null) { - child.layout(constraints.loosen()); + child.layout(constraints.loosen(), parentUsesSize: true); size = constraints.constrain(child.size); assert(child.parentData is BoxParentData); Size delta = size - child.size; diff --git a/sdk/lib/framework/rendering/object.dart b/sdk/lib/framework/rendering/object.dart index 4f817323760..17682201ae4 100644 --- a/sdk/lib/framework/rendering/object.dart +++ b/sdk/lib/framework/rendering/object.dart @@ -128,11 +128,11 @@ abstract class RenderObject extends AbstractNode { _nodesNeedingLayout = new List(); dirtyNodes..sort((a, b) => a.depth - b.depth)..forEach((node) { if (node._needsLayout && node.attached) - node._doLayout(); + node.layoutWithoutResize(); }); _debugDoingLayout = false; } - void _doLayout() { + void layoutWithoutResize() { try { assert(_relayoutSubtreeRoot == this); performLayout(); diff --git a/sdk/lib/framework/rendering/paragraph.dart b/sdk/lib/framework/rendering/paragraph.dart index 16caee3a9d9..3643ebb2120 100644 --- a/sdk/lib/framework/rendering/paragraph.dart +++ b/sdk/lib/framework/rendering/paragraph.dart @@ -211,5 +211,11 @@ class RenderParagraph extends RenderBox { // we should probably expose a way to do precise (inter-glpyh) hit testing - String debugDescribeSettings(String prefix) => '${super.debugDescribeSettings(prefix)}${prefix}style:\n${style.toString("$prefix ")}\n${prefix}text: ${text}\n'; + String debugDescribeSettings(String prefix) { + String result = '${super.debugDescribeSettings(prefix)}'; + if (style != null) + result += '${prefix}style:\n' + style.toString('$prefix ') + '\n'; + result += '${prefix}text: ${text}\n'; + return result; + } }