Layout API prototype, for discussion

Layout API prototype, for discussion

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/56b8b21d1552dc4cc45b0e05ea3d519afa7246a7

[ojan, 2015-04-23 20:28:31]

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

[ojan, 2015-04-23 20:28:32]

Still trying to wrap my head around the relayoutSubtreeRoot thing. Other than
that...I think I get most of what this is trying to do. Still digesting.

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/layout.dart
File sky/framework/layout.dart (right):

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/layout.d...
sky/framework/layout.dart:35: _children.add(child);
Why add the children to a list instead of calling paint() directly here?

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/layout.d...
sky/framework/layout.dart:54: const kLayoutDirections = 4;
FYI, this is dead

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/node.dart
File sky/framework/node.dart (right):

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/node.dar...
sky/framework/node.dart:8: int _order = 0;
bikeshed: I might call this _treeDepth. It took me a while to understand this
and I needed the comment above to figure out what this was.

[Hixie, 2015-04-23 20:45:13]

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/layout.dart
File sky/framework/layout.dart (right):

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/layout.d...
sky/framework/layout.dart:35: _children.add(child);
On 2015/04/23 20:28:32, ojan wrote:
> Why add the children to a list instead of calling paint() directly here?

If it's not dirty, you don't need to paint it. We could call _doPaint()
directly, though. I just figured it was best to not actually nest the calls to
paint() if we don't have to. I don't have a strong opinion here.

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/layout.d...
sky/framework/layout.dart:54: const kLayoutDirections = 4;
On 2015/04/23 20:28:32, ojan wrote:
> FYI, this is dead

Acknowledged.

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/node.dart
File sky/framework/node.dart (right):

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/node.dar...
sky/framework/node.dart:8: int _order = 0;
On 2015/04/23 20:28:32, ojan wrote:
> bikeshed: I might call this _treeDepth. It took me a while to understand this
> and I needed the comment above to figure out what this was.

Makes sense. It's not quite the depth (e.g. if you lose a layer above you, your
numbers aren't decreased), though. Not sure what the right term would be.

[ojan, 2015-04-23 21:12:36]

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/layout.dart
File sky/framework/layout.dart (right):

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/layout.d...
sky/framework/layout.dart:35: _children.add(child);
On 2015/04/23 at 20:45:13, Hixie wrote:
> On 2015/04/23 20:28:32, ojan wrote:
> > Why add the children to a list instead of calling paint() directly here?
> 
> If it's not dirty, you don't need to paint it. We could call _doPaint()
directly, though. I just figured it was best to not actually nest the calls to
paint() if we don't have to. I don't have a strong opinion here.

Extra lists == moar slow. So we would only want to do this if it gained us
thing.

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/node.dart
File sky/framework/node.dart (right):

https://codereview.chromium.org/1093633002/diff/160001/sky/framework/node.dar...
sky/framework/node.dart:8: int _order = 0;
On 2015/04/23 at 20:45:13, Hixie wrote:
> On 2015/04/23 20:28:32, ojan wrote:
> > bikeshed: I might call this _treeDepth. It took me a while to understand
this
> > and I needed the comment above to figure out what this was.
> 
> Makes sense. It's not quite the depth (e.g. if you lose a layer above you,
your numbers aren't decreased), though. Not sure what the right term would be.

Hmmm...then I think I don't understand what this does.

[ojan, 2015-05-09 01:23:34]

https://codereview.chromium.org/1093633002/diff/180001/sky/framework/layout.dart
File sky/framework/layout.dart (right):

https://codereview.chromium.org/1093633002/diff/180001/sky/framework/layout.d...
sky/framework/layout.dart:123: relayout();
I was talking Elliott through this design and we realized that this part doesn't
actually work. Specifically, imagine you have the following structure:

auto-height-container
  auto-height-child
    grand-child-that-gets-marked-dirty

You'll mark the grand-child dirty and then when you go to lay it out, you'll
call relayout on auto-height-container instead since that would be the
relayoutSubtreeRoot. The problem is that the relayout calls all force a full
layout without any pruning, so you end up doing a non-pruned layout walk, which
is way too expensive.

Am I reading the code wrong?

[Hixie, 2015-05-13 20:29:21]

https://codereview.chromium.org/1093633002/diff/180001/sky/framework/layout.dart
File sky/framework/layout.dart (right):

https://codereview.chromium.org/1093633002/diff/180001/sky/framework/layout.d...
sky/framework/layout.dart:123: relayout();
On 2015/05/09 01:23:34, ojan wrote:
> I was talking Elliott through this design and we realized that this part
doesn't
> actually work. Specifically, imagine you have the following structure:
> 
> auto-height-container
>   auto-height-child
>     grand-child-that-gets-marked-dirty
> 
> You'll mark the grand-child dirty and then when you go to lay it out, you'll
> call relayout on auto-height-container instead since that would be the
> relayoutSubtreeRoot. The problem is that the relayout calls all force a full
> layout without any pruning, so you end up doing a non-pruned layout walk,
which
> is way too expensive.
> 
> Am I reading the code wrong?

Hm, yeah.

I don't see a way to work around this. Any ideas? Specifically, the
auto-height-container doesn't know that any of its grandchildren are
needsLayout, so it can't know whether or what to prune, and the
grand-child-that-gets-marked-dirty has no way to walk up to the
auto-height-child to mark it as descendantNeedsLayout, which would give the
auto-height-container the information it needs to prune.

[ojan, 2015-05-17 00:56:00]

On 2015/05/13 at 20:29:21, ianh wrote:
>
https://codereview.chromium.org/1093633002/diff/180001/sky/framework/layout.dart
> File sky/framework/layout.dart (right):
> 
>
https://codereview.chromium.org/1093633002/diff/180001/sky/framework/layout.d...
> sky/framework/layout.dart:123: relayout();
> On 2015/05/09 01:23:34, ojan wrote:
> > I was talking Elliott through this design and we realized that this part
doesn't
> > actually work. Specifically, imagine you have the following structure:
> > 
> > auto-height-container
> >   auto-height-child
> >     grand-child-that-gets-marked-dirty
> > 
> > You'll mark the grand-child dirty and then when you go to lay it out, you'll
> > call relayout on auto-height-container instead since that would be the
> > relayoutSubtreeRoot. The problem is that the relayout calls all force a full
> > layout without any pruning, so you end up doing a non-pruned layout walk,
which
> > is way too expensive.
> > 
> > Am I reading the code wrong?
> 
> Hm, yeah.
> 
> I don't see a way to work around this. Any ideas? Specifically, the
auto-height-container doesn't know that any of its grandchildren are
needsLayout, so it can't know whether or what to prune, and the
grand-child-that-gets-marked-dirty has no way to walk up to the
auto-height-child to mark it as descendantNeedsLayout, which would give the
auto-height-container the information it needs to prune.

I can think of ways around this...but none that I'm happy with. Here's one
approach...instead of saving a relayoutSubtreeRoot, you save a
relayoutAncestorChain that goes from the grand-child to the relayoutSubtreeRoot.
Then, when you dirty the grand-child, you can dirty the ancestor chain up to the
relayoutSubtreeRoot. It's basically recreating parent pointers, but they're
scoped instead of connecting the whole tree together. The cure may be worse than
the disease here.

[Hixie, 2015-05-18 18:04:03]

That sounds too expensive, yeah. Also it doesn't really efficiently handle the
case of two branches in the same subtree being dirty.

[Hixie, 2015-05-19 20:26:38]

I've added parent.

[abarth-chromium, 2015-05-20 17:51:31]

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

[abarth-chromium, 2015-05-20 17:51:32]

LGTM

Maybe don't land paint.dart yet?

[Hixie, 2015-05-20 17:52:39]

Committed patchset #20 (id:380001) manually as
56b8b21d1552dc4cc45b0e05ea3d519afa7246a7 (presubmit successful).

[Hixie, 2015-05-20 18:14:35]

Moved the paint part to https://codereview.chromium.org/1143053003 for now.