Add asserts to catch potential misuses of the rendering framework.

New asserts:
- verify that after layout, the size fits the constraints
- verify that after layout, the size isn't infinite
- verify that you don't set the size in performLayout() if you have
  sizedByParent set
- verify that nobody reads your size during layout except you, or your
  parent if they said parentUsesSize:true

Fixes some bugs found by those asserts:
- RenderBlock, RenderStack, and RenderScaffold were not always setting
  parentUsesSize correctly
- RenderScaffold was setting its slot entries to null rather than
  removing them when the slot went away, which led to null derefs in
  certain circumstances

Also, rename a local variable in RenderStack.performLayout() because
it was shadowing a variable on the object itself, which was really
confusing when I first tried to debug this function...

R=abarth@chromium.org

Review URL: https://codereview.chromium.org/1213473003.

engine/core/painting/Size.dart

@@ -22,6 +22,8 @@ class Size {
   Size operator +(Size other) => new Size(width + other.width, height + other.height);
   Size operator -(Size other) => new Size(width - other.width, height - other.height);
 
+  bool get isInfinite => width >= double.INFINITY || height >= double.INFINITY;
+
   // does the equivalent of "return new Point(0,0) + this"
   Point toPoint() => new Point(this.width, this.height);
 

examples/rendering/sector_layout.dart

@@ -75,6 +75,17 @@ abstract class RenderSector extends RenderObject {
   }
 
   SectorConstraints get constraints => super.constraints;
+  bool debugDoesMeetConstraints() {
+    assert(constraints != null);
+    assert(deltaRadius != null);
+    assert(deltaRadius < double.INFINITY);
+    assert(deltaTheta != null);
+    assert(deltaTheta < double.INFINITY);
+    return constraints.minDeltaRadius <= deltaRadius &&
+           deltaRadius <= math.max(constraints.minDeltaRadius, constraints.maxDeltaRadius) &&
+           constraints.minDeltaTheta <= deltaTheta &&
+           deltaTheta <= math.max(constraints.minDeltaTheta, constraints.maxDeltaTheta);
+  }
   void performResize() {
     // default behaviour for subclasses that have sizedByParent = true
     deltaRadius = constraints.constrainDeltaRadius(0.0);

sdk/lib/rendering/README.md

@@ -85,6 +85,13 @@ preinitialise the `parentData` member to set its values before you add
 a node to its parent, you can preemptively call that future parent's
 `setParentData()` method with the future child as the argument.
 
+TODO(ianh): Discuss putting per-child configuration information for
+the parent on the child's parentData.
+
+If you change a child's parentData dynamically, you must also call
+markNeedsLayout() on the parent, otherwise the new information will
+not take effect until something else triggers a layout.
+
 ### Box Model
 
 #### Dimensions

sdk/lib/rendering/block.dart

@@ -84,12 +84,11 @@ class RenderBlock extends RenderBox with ContainerRenderObjectMixin<RenderBox, B
   void performLayout() {
     assert(constraints is BoxConstraints);
     double width = constraints.constrainWidth(constraints.maxWidth);
-    bool usesChildSize = !constraints.hasTightHeight;
     BoxConstraints innerConstraints = _getInnerConstraintsForWidth(width);
     double y = 0.0;
     RenderBox child = firstChild;
     while (child != null) {
-      child.layout(innerConstraints, parentUsesSize: usesChildSize);
+      child.layout(innerConstraints, parentUsesSize: true);
       assert(child.parentData is BlockParentData);
       child.parentData.position = new Point(0.0, y);
       y += child.size.height;

sdk/lib/rendering/box.dart

@@ -193,6 +193,11 @@ class BoxConstraints extends Constraints {
   bool get hasTightHeight => minHeight >= maxHeight;
   bool get isTight => hasTightWidth && hasTightHeight;
 
+  bool contains(Size size) {
+    return (minWidth <= size.width) && (size.width <= math.max(minWidth, maxWidth)) &&
+           (minHeight <= size.height) && (size.height <= math.max(minHeight, maxHeight));
+  }
+
   bool operator ==(other) {
     if (identical(this, other))
       return true;
@@ -290,25 +295,13 @@ abstract class RenderBox extends RenderObject {
     return null;
   }
 
-  // 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 => super.constraints;
+  bool debugDoesMeetConstraints() {
+    assert(constraints != null);
+    assert(_size != null);
+    assert(!_size.isInfinite);
+    return constraints.contains(_size);
+  }
   void performResize() {
     // default behaviour for subclasses that have sizedByParent = true
     size = constraints.constrain(Size.zero);
@@ -329,19 +322,34 @@ abstract class RenderBox extends RenderObject {
   }
   void hitTestChildren(HitTestResult result, { Point position }) { }
 
+  // TODO(ianh): move size up to before constraints
   // 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;
+  Size get size {
+    if (_size is _DebugSize) {
+      final _DebugSize _size = this._size; // TODO(ianh): Remove this once the analyzer is cleverer
+      assert(_size._owner == this);
+      if (RenderObject.debugActiveLayout != null) {
+        // we are always allowed to access our own size (for print debugging and asserts if nothing else)
+        // other than us, the only object that's allowed to read our size is our parent, if they're said they will
+        assert(debugDoingThisResize || debugDoingThisLayout ||
+               (RenderObject.debugActiveLayout == parent && _size._canBeUsedByParent));
+      }
+      assert(_size == this._size); // TODO(ianh): Remove this once the analyzer is cleverer
+    }
+    return _size;
+  }
   void set size(Size value) {
     assert(RenderObject.debugDoingLayout);
-    assert(_debugDoingThisLayout);
+    assert((sizedByParent && debugDoingThisResize) ||
+           (!sizedByParent && debugDoingThisLayout));
     if (value is _DebugSize) {
       assert(value._canBeUsedByParent);
       assert(value._owner.parent == this);
     }
-    _size = inDebugBuild ? new _DebugSize(value, this, _debugCanParentUseSize) : value;
+    _size = inDebugBuild ? new _DebugSize(value, this, debugCanParentUseSize) : value;
   }
 
   String debugDescribeSettings(String prefix) => '${super.debugDescribeSettings(prefix)}${prefix}size: ${size}\n';
@@ -1134,11 +1142,8 @@ class RenderView extends RenderObject with RenderObjectWithChildMixin<RenderBox>
     assert(_size.height < double.INFINITY);
     assert(_size.width < double.INFINITY);
 
-    if (child != null) {
+    if (child != null)
       child.layout(new BoxConstraints.tight(_size));
-      assert(child.size.width == width);
-      assert(child.size.height == height);
-    }
   }
 
   void rotate({ int oldAngle, int newAngle, Duration time }) {

sdk/lib/rendering/object.dart

@@ -80,11 +80,20 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget {
   static List<RenderObject> _nodesNeedingLayout = new List<RenderObject>();
   static bool _debugDoingLayout = false;
   static bool get debugDoingLayout => _debugDoingLayout;
+  bool _debugDoingThisResize = false;
+  bool get debugDoingThisResize => _debugDoingThisResize;
+  bool _debugDoingThisLayout = false;
+  bool get debugDoingThisLayout => _debugDoingThisLayout;
+  static RenderObject _debugActiveLayout = null;
+  static RenderObject get debugActiveLayout => _debugActiveLayout;
+  bool _debugCanParentUseSize;
+  bool get debugCanParentUseSize => _debugCanParentUseSize;
   bool _needsLayout = true;
   bool get needsLayout => _needsLayout;
   RenderObject _relayoutSubtreeRoot;
   Constraints _constraints;
   Constraints get constraints => _constraints;
+  bool debugDoesMeetConstraints(); // override this in a subclass to verify that your state matches the constraints object
   bool debugAncestorsAlreadyMarkedNeedsLayout() {
     if (_relayoutSubtreeRoot == null)
       return true; // we haven't yet done layout even once, so there's nothing for us to do
@@ -152,7 +161,14 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget {
   void layoutWithoutResize() {
     try {
       assert(_relayoutSubtreeRoot == this);
+      _debugCanParentUseSize = false;
+      _debugDoingThisLayout = true;
+      RenderObject debugPreviousActiveLayout = _debugActiveLayout;
+      _debugActiveLayout = this;
       performLayout();
+      _debugActiveLayout = debugPreviousActiveLayout;
+      _debugDoingThisLayout = false;
+      _debugCanParentUseSize = null;
     } catch (e, stack) {
       print('Exception raised during layout of ${this}: ${e}');
       print(stack);
@@ -172,9 +188,20 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget {
       return;
     _constraints = constraints;
     _relayoutSubtreeRoot = relayoutSubtreeRoot;
-    if (sizedByParent)
+    _debugCanParentUseSize = parentUsesSize;
+    if (sizedByParent) {
+      _debugDoingThisResize = true;
       performResize();
+      _debugDoingThisResize = false;
+    }
+    _debugDoingThisLayout = true;
+    RenderObject debugPreviousActiveLayout = _debugActiveLayout;
+    _debugActiveLayout = this;
     performLayout();
+    _debugActiveLayout = debugPreviousActiveLayout;
+    _debugDoingThisLayout = false;
+    _debugCanParentUseSize = null;
+    assert(debugDoesMeetConstraints());
     _needsLayout = false;
     markNeedsPaint();
     assert(parent == this.parent); // TODO(ianh): Remove this once the analyzer is cleverer
@@ -250,6 +277,8 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget {
 
 
   String toString([String prefix = '']) {
+    RenderObject debugPreviousActiveLayout = _debugActiveLayout;
+    _debugActiveLayout = null;
     String header = '${runtimeType}';
     if (_relayoutSubtreeRoot != null && _relayoutSubtreeRoot != this) {
       int count = 1;
@@ -265,7 +294,9 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget {
     if (!attached)
       header += ' DETACHED';
     prefix += '  ';
-    return '${header}\n${debugDescribeSettings(prefix)}${debugDescribeChildren(prefix)}';
+    String result = '${header}\n${debugDescribeSettings(prefix)}${debugDescribeChildren(prefix)}';
+    _debugActiveLayout = debugPreviousActiveLayout;
+    return result;
   }
   String debugDescribeSettings(String prefix) => '${prefix}parentData: ${parentData}\n${prefix}constraints: ${constraints}\n';
   String debugDescribeChildren(String prefix) => '';

sdk/lib/rendering/stack.dart

@@ -150,45 +150,45 @@ class RenderStack extends RenderBox with ContainerRenderObjectMixin<RenderBox, S
     child = firstChild;
     while (child != null) {
       assert(child.parentData is StackParentData);
-      final StackParentData parentData = child.parentData;
+      final StackParentData childData = child.parentData;
 
-      if (parentData.isPositioned) {
+      if (childData.isPositioned) {
         BoxConstraints childConstraints = innerConstraints;
 
-        if (parentData.left != null && parentData.right != null)
-          childConstraints = childConstraints.applyWidth(parentData.right - parentData.left);
-        else if (parentData.left != null)
-          childConstraints = childConstraints.applyMaxWidth(size.width - parentData.left);
-        else if (parentData.right != null)
-          childConstraints = childConstraints.applyMaxWidth(size.width - parentData.right);
+        if (childData.left != null && childData.right != null)
+          childConstraints = childConstraints.applyWidth(childData.right - childData.left);
+        else if (childData.left != null)
+          childConstraints = childConstraints.applyMaxWidth(size.width - childData.left);
+        else if (childData.right != null)
+          childConstraints = childConstraints.applyMaxWidth(size.width - childData.right);
 
-        if (parentData.top != null && parentData.bottom != null)
-          childConstraints = childConstraints.applyHeight(parentData.bottom - parentData.top);
-        else if (parentData.top != null)
-          childConstraints = childConstraints.applyMaxHeight(size.height - parentData.top);
-        else if (parentData.bottom != null)
-          childConstraints = childConstraints.applyMaxHeight(size.width - parentData.bottom);
+        if (childData.top != null && childData.bottom != null)
+          childConstraints = childConstraints.applyHeight(childData.bottom - childData.top);
+        else if (childData.top != null)
+          childConstraints = childConstraints.applyMaxHeight(size.height - childData.top);
+        else if (childData.bottom != null)
+          childConstraints = childConstraints.applyMaxHeight(size.width - childData.bottom);
 
-        child.layout(childConstraints);
+        child.layout(childConstraints, parentUsesSize: true);
 
         double x = 0.0;
-        if (parentData.left != null)
-          x = parentData.left;
-        else if (parentData.right != null)
-          x = size.width - parentData.right - child.size.width;
+        if (childData.left != null)
+          x = childData.left;
+        else if (childData.right != null)
+          x = size.width - childData.right - child.size.width;
         assert(x >= 0.0 && x + child.size.width <= size.width);
 
         double y = 0.0;
-        if (parentData.top != null)
-          y = parentData.top;
-        else if (parentData.bottom != null)
-          y = size.height - parentData.bottom - child.size.height;
+        if (childData.top != null)
+          y = childData.top;
+        else if (childData.bottom != null)
+          y = size.height - childData.bottom - child.size.height;
         assert(y >= 0.0 && y + child.size.height <= size.height);
 
-        parentData.position = new Point(x, y);
+        childData.position = new Point(x, y);
       }
 
-      child = parentData.nextSibling;
+      child = childData.nextSibling;
     }
   }
 

sdk/lib/widgets/scaffold.dart

@@ -39,9 +39,12 @@ class RenderScaffold extends RenderBox {
       return;
     if (old != null)
       dropChild(old);
-    _slots[slot] = value;
-    if (value != null)
+    if (value == null) {
+      _slots.remove(slot);
+    } else {
+      _slots[slot] = value;
       adoptChild(value);
+    }
     markNeedsLayout();
   }
 
@@ -118,7 +121,7 @@ class RenderScaffold extends RenderBox {
     if (_slots[ScaffoldSlots.floatingActionButton] != null) {
       RenderBox floatingActionButton = _slots[ScaffoldSlots.floatingActionButton];
       Size area = new Size(size.width - kButtonX, size.height - kButtonY);
-      floatingActionButton.layout(new BoxConstraints.loose(area));
+      floatingActionButton.layout(new BoxConstraints.loose(area), parentUsesSize: true);
       assert(floatingActionButton.parentData is BoxParentData);
       floatingActionButton.parentData.position = (area - floatingActionButton.size).toPoint();
     }