Introduce BlockViewport.

Introduce BlockViewport. This class lets you build Widgets dynamically during layout.

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/5f6218024a1aa9f615054ee765ffe03daa6558c2

[abarth-chromium, 2015-07-08 21:56:19]

abarth@chromium.org changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2015-07-08 21:56:20]

https://codereview.chromium.org/1222913013/diff/1/sky/sdk/lib/widgets/block_v...
File sky/sdk/lib/widgets/block_viewport.dart (right):

https://codereview.chromium.org/1222913013/diff/1/sky/sdk/lib/widgets/block_v...
sky/sdk/lib/widgets/block_viewport.dart:23: BlockViewport({ IndexedBuilder
this.builder, this.startOffset, this.token, String key })
Is IndexedBuilder needed here?

https://codereview.chromium.org/1222913013/diff/1/sky/sdk/lib/widgets/block_v...
sky/sdk/lib/widgets/block_viewport.dart:28: Object token;
final?

https://codereview.chromium.org/1222913013/diff/1/sky/sdk/lib/widgets/block_v...
sky/sdk/lib/widgets/block_viewport.dart:87: int right = _offsets.length-1;
We usually put spaces around binary operators like -

https://codereview.chromium.org/1222913013/diff/1/sky/sdk/lib/widgets/block_v...
sky/sdk/lib/widgets/block_viewport.dart:99: }
Seems like a function we should factor out to somewhere more general.

http://www.dartdocs.org/documentation/collection_helpers/0.9.2/index.html#col...

Is the official version, which we probably don't want to use directly.

https://codereview.chromium.org/1222913013/diff/1/sky/sdk/lib/widgets/block_v...
sky/sdk/lib/widgets/block_viewport.dart:103: root.markNeedsLayout();
Do we need to do a layout every time we sync?  I would have thought |token|
would control that.

https://codereview.chromium.org/1222913013/diff/1/sky/sdk/lib/widgets/block_v...
sky/sdk/lib/widgets/block_viewport.dart:139: BoxConstraints innerConstraints =
new BoxConstraints.tightFor(width: constraints.maxWidth);
There's a bug here if constraints.minWidth is greater than maxWidth

https://codereview.chromium.org/1222913013/diff/1/sky/sdk/lib/widgets/widget....
File sky/sdk/lib/widgets/widget.dart (right):

https://codereview.chromium.org/1222913013/diff/1/sky/sdk/lib/widgets/widget....
sky/sdk/lib/widgets/widget.dart:677: void
doRetainStatefulNode(RenderObjectWrapper newNode) {
doRetainStatefulNode -> retainStatefulRenderObjectWrapper ?

[abarth-chromium, 2015-07-09 03:15:10]

LGTM

I understood what this function is doing much better the second time.  After you
land this, I'd like to take a pass reorganizing the code a bit to confirm my
understanding.

https://codereview.chromium.org/1222913013/diff/40001/sky/sdk/lib/widgets/blo...
File sky/sdk/lib/widgets/block_viewport.dart (right):

https://codereview.chromium.org/1222913013/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:18: bool operator ==(_Key other) =>
other is _Key && other.type == type && other.key == key;
operator ==(_Key other)

should be

operator ==(other)

https://codereview.chromium.org/1222913013/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:40: static const omit = const Object();
// used as a slot when it's not yet time to attach the child
omit -> _omit

Presumably we don't want other people depending on this object.

https://codereview.chromium.org/1222913013/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:62: for (var child in
_childrenByKey.values) {
s/var/Widget/

https://codereview.chromium.org/1222913013/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:158: double endOffset = startOffset +
height;
I'd mark these as final.  Normally I wouldn't bother, but this function is
complex enough to warrant it.

https://codereview.chromium.org/1222913013/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:224: root.startOffset =
_offsets[index]-startOffset;
s/-/ - /

[Hixie, 2015-07-09 16:20:57]

Committed patchset #4 (id:60001) manually as
5f6218024a1aa9f615054ee765ffe03daa6558c2 (presubmit successful).