Move VariableHeightScrollable to sdk/lib/widgets

Move VariableHeightScrollable to sdk/lib/widgets

Added a callback to BlockViewport so that the scrollable can
stay in sync with the viewport's contents.

R=ianh@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/e018ff3b2215d2a12319f3e4446ee57b105e8c99

[hansmuller, 2015-07-10 20:25:53]

hansmuller@chromium.org changed reviewers:
+ ianh@google.com

[hansmuller, 2015-07-10 20:26:57]

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

[hansmuller, 2015-07-10 20:26:57]

PTAL

[abarth-chromium, 2015-07-10 20:29:24]

Looks great to me, but hixie should review.

[Hixie, 2015-07-10 20:55:12]

https://codereview.chromium.org/1227823012/diff/1/sky/sdk/lib/widgets/variabl...
File sky/sdk/lib/widgets/variable_height_scrollable.dart (right):

https://codereview.chromium.org/1227823012/diff/1/sky/sdk/lib/widgets/variabl...
sky/sdk/lib/widgets/variable_height_scrollable.dart:18: this.itemCount
Requiring that we pass this in implies this is a finite variable height
scrollable, not an infinite variable height scrollable.

https://codereview.chromium.org/1227823012/diff/1/sky/sdk/lib/widgets/variabl...
sky/sdk/lib/widgets/variable_height_scrollable.dart:48: // We don't know the
height of all items yet. Grow the ScrollBehavior's
This isn't the right way to do this IMHO. We should make the ScrollBehaviour
know about the "unknown height" state and do the right thing with that state.

https://codereview.chromium.org/1227823012/diff/1/sky/sdk/lib/widgets/variabl...
sky/sdk/lib/widgets/variable_height_scrollable.dart:50: if (contentsCount <
itemCount)
Rather than using itemCount and figuring out if we happen to know the offset of
the last item, we should just have the underlying class expose a bit regarding
whether it knows that it's reached the bottom or not. (It knows that because
when it asks the builder for the N+1th item, the builder returns null.)

[hansmuller, 2015-07-10 22:42:50]

https://codereview.chromium.org/1227823012/diff/1/sky/sdk/lib/widgets/variabl...
File sky/sdk/lib/widgets/variable_height_scrollable.dart (right):

https://codereview.chromium.org/1227823012/diff/1/sky/sdk/lib/widgets/variabl...
sky/sdk/lib/widgets/variable_height_scrollable.dart:18: this.itemCount
On 2015/07/10 20:55:11, Hixie wrote:
> Requiring that we pass this in implies this is a finite variable height
> scrollable, not an infinite variable height scrollable.

Done.

https://codereview.chromium.org/1227823012/diff/1/sky/sdk/lib/widgets/variabl...
sky/sdk/lib/widgets/variable_height_scrollable.dart:48: // We don't know the
height of all items yet. Grow the ScrollBehavior's
On 2015/07/10 20:55:11, Hixie wrote:
> This isn't the right way to do this IMHO. We should make the ScrollBehaviour
> know about the "unknown height" state and do the right thing with that state.

I agree. I've updated OverScrollBehavior to handle contentsHeight == null.
There's no over-scrolling past the maxScrollOffset in this case.

https://codereview.chromium.org/1227823012/diff/1/sky/sdk/lib/widgets/variabl...
sky/sdk/lib/widgets/variable_height_scrollable.dart:50: if (contentsCount <
itemCount)
On 2015/07/10 20:55:12, Hixie wrote:
> Rather than using itemCount and figuring out if we happen to know the offset
of
> the last item, we should just have the underlying class expose a bit regarding
> whether it knows that it's reached the bottom or not. (It knows that because
> when it asks the builder for the N+1th item, the builder returns null.)

Good point. I've added a parameter to the onLayoutChanged callback that
indicates if the end has been seen.

[Hixie, 2015-07-10 22:51:38]

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

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:19: bool lastWidgetReached);
paren on its own line

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:95: bool _lastWidgetReached = false;
lastWidgetReached would store a Widget. If you want to store a boolean, call it
something like didReachLastWidget or some such. Sine you call it a child count
and not a widget count, maybe didReachLastChild?

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:209: haveChildren = false;
i think this also means we've reached the end, no?

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:222: break;
i think you have to also flag the end here?

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:251: assert(haveChildren != null);
assert that if haveChildren is false, that we've also set the "reached the end"
flag.

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:306: _currentStartIndex,
this should be indented with two spaces.

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:309: _lastWidgetReached);
closing paren should be on its own line.

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/var...
File sky/sdk/lib/widgets/variable_height_scrollable.dart (right):

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/var...
sky/sdk/lib/widgets/variable_height_scrollable.dart:47: // We don't know the
height of all items yet. Grow the ScrollBehavior's
delete comment-out code

[hansmuller, 2015-07-10 23:42:06]

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

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:19: bool lastWidgetReached);
On 2015/07/10 22:51:38, Hixie wrote:
> paren on its own line

Done.

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:95: bool _lastWidgetReached = false;
On 2015/07/10 22:51:37, Hixie wrote:
> lastWidgetReached would store a Widget. If you want to store a boolean, call
it
> something like didReachLastWidget or some such. Sine you call it a child count
> and not a widget count, maybe didReachLastChild?

I've changed all the fooWidget names I added to fooChild.

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:209: haveChildren = false;
On 2015/07/10 22:51:37, Hixie wrote:
> i think this also means we've reached the end, no?

Yes.

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:222: break;
On 2015/07/10 22:51:37, Hixie wrote:
> i think you have to also flag the end here?

Oops, yes.

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:251: assert(haveChildren != null);
On 2015/07/10 22:51:37, Hixie wrote:
> assert that if haveChildren is false, that we've also set the "reached the
end"
> flag.

Done.

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:306: _currentStartIndex,
On 2015/07/10 22:51:38, Hixie wrote:
> this should be indented with two spaces.

Done.

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:309: _lastWidgetReached);
On 2015/07/10 22:51:37, Hixie wrote:
> closing paren should be on its own line.

Done.

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/var...
File sky/sdk/lib/widgets/variable_height_scrollable.dart (right):

https://codereview.chromium.org/1227823012/diff/40001/sky/sdk/lib/widgets/var...
sky/sdk/lib/widgets/variable_height_scrollable.dart:47: // We don't know the
height of all items yet. Grow the ScrollBehavior's
On 2015/07/10 22:51:38, Hixie wrote:
> delete comment-out code

Done.

[Hixie, 2015-07-11 00:10:14]

lgtm

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

https://codereview.chromium.org/1227823012/diff/80001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:256: assert(!haveChildren ?
_didReachLastChild : true);
haveChildren || _didReachLastChild

[hansmuller, 2015-07-11 00:13:17]

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

https://codereview.chromium.org/1227823012/diff/80001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:256: assert(!haveChildren ?
_didReachLastChild : true);
On 2015/07/11 00:10:13, Hixie wrote:
> haveChildren || _didReachLastChild

Done.

[hansmuller1, 2015-07-11 00:13:36]

Committed patchset #6 (id:100001) manually as
e018ff3b2215d2a12319f3e4446ee57b105e8c99 (presubmit successful).