Fix preferred logical widths of orthogonal writing modes

Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available when preferred logical
widths are needed.

This patch makes the previous partial fix for shrink-to-fit
inline-block[1] unnecessary, so the code and flags added in the patch
are removed.

[1] https://codereview.chromium.org/1121173002/

BUG=550963, 473429
Committed: https://crrev.com/3d7b19bbee5aac29c8efa9df62f08264c478041b
Cr-Commit-Position: refs/heads/master@{#373144}

[kojii, 2015-12-31 05:29:44]

Description was changed from

==========
WIP: Fix shrink-to-fit when children's writing-mode is orthogonal

BUG=550963, 473429
==========

to

==========
WIP: Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available.

BUG=550963, 473429
==========

[kojii, 2015-12-31 07:03:41]

Description was changed from

==========
WIP: Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available.

BUG=550963, 473429
==========

to

==========
WIP: Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available when preferred
logical widths are needed.

BUG=550963, 473429
==========

[kojii, 2015-12-31 07:16:29]

Description was changed from

==========
WIP: Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available when preferred
logical widths are needed.

BUG=550963, 473429
==========

to

==========
WIP: Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available when preferred logical
widths are needed.

This patch makes a partial fix for shrink-to-fit inline-block[1]
unnecessary, so the code and flags added in the patch are removed.

[1] https://codereview.chromium.org/1121173002/

BUG=550963, 473429
==========

[kojii, 2015-12-31 09:23:42]

Description was changed from

==========
WIP: Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available when preferred logical
widths are needed.

This patch makes a partial fix for shrink-to-fit inline-block[1]
unnecessary, so the code and flags added in the patch are removed.

[1] https://codereview.chromium.org/1121173002/

BUG=550963, 473429
==========

to

==========
Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available when preferred logical
widths are needed.

This patch makes a partial fix for shrink-to-fit inline-block[1]
unnecessary, so the code and flags added in the patch are removed.

[1] https://codereview.chromium.org/1121173002/

BUG=550963, 473429
==========

[kojii, 2015-12-31 09:25:51]

kojii@chromium.org changed reviewers:
+ cbiesinger@chromium.org, eae@chromium.org, esprehn@chromium.org,
leviw@chromium.org, ojan@chromium.org

[kojii, 2015-12-31 09:25:55]

PTAL. The one we discussed in BlinkOn5. Appreciate feedback in advance, there
are some code that I'm not really familiar with.

[kojii, 2016-01-08 19:46:21]

Gentle reminder after the vacation.

[cbiesinger, 2016-01-15 05:31:47]

OK, this looks essentially right to me, though I haven't done a detailed review
yet.

However this does need some documentation. Maybe add some comments in FrameView
where you call layoutOrthogonalWritingModeRoots() about how this works.

Also, in LayoutBlock::computeChildPreferredLogicalWidths(), can we
ASSERT(!child.needsLayout()) for the orthogonal flow case or do we still need
the "WithoutLayout" codepath?

https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:107:
computeChildPreferredLogicalWidths(*child, minPreferredLogicalWidth,
maxPreferredLogicalWidth);
Hmm I wonder why this didn't call that function in the past...

[kojii, 2016-01-15 09:09:11]

Thank you for reviewing this!

On 2016/01/15 05:31:47, cbiesinger wrote:
> OK, this looks essentially right to me, though I haven't done a detailed
review
> yet.

Thank you, "essentially right" is great word at this point.

> However this does need some documentation. Maybe add some comments in
FrameView
> where you call layoutOrthogonalWritingModeRoots() about how this works.

Agreed, OrthogonalWritingModeRootList is the class for this, I'll write up there
and point to it from FrameView. I was hesitant to do before hearing some good
feedback on the overall direction.

> Also, in LayoutBlock::computeChildPreferredLogicalWidths(), can we
> ASSERT(!child.needsLayout()) for the orthogonal flow case or do we still need
> the "WithoutLayout" codepath?

Yes, like "height: 100%" can be better layout in the current code flow because
it relies on parent being layout, and isn't affected by child layout.

FrameView::layoutOrthogonalWritingModeRoots() has some conditions to skip. If
the orthogonal roots meet this criteria, they go to the current code path.

https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:107:
computeChildPreferredLogicalWidths(*child, minPreferredLogicalWidth,
maxPreferredLogicalWidth);
On 2016/01/15 05:31:47, cbiesinger wrote:
> Hmm I wonder why this didn't call that function in the past...

Great, thank you for the review.

[ojan, 2016-01-22 02:22:35]

Agree that this is essentially right. Christian and/or Levi should probably do
the detailed review, but at a high level, this looks good to me!

https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1724: ||
!root->styleRef().logicalHeight().isIntrinsicOrAuto()) {
If it's any of these last three, maybe we just shouldn't put it in
m_orthogonalWritingModeRootList in the first place?

https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1727: LayoutState
layoutState(*root);
I'm not 100% sure creating a LayoutState without dealing with any of the
ancestors is correct. I guess that's the fundamental assumption of this approach
though. :) Taking a quick look at the LayoutState code, I don't see a problem,
but Levi and Morten should probably take a close look here.

[kojii, 2016-01-22 02:33:51]

Thank you Ojan for having a look, replies inline:

https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1724: ||
!root->styleRef().logicalHeight().isIntrinsicOrAuto()) {
On 2016/01/22 at 02:22:35, ojan wrote:
> If it's any of these last three, maybe we just shouldn't put it in
m_orthogonalWritingModeRootList in the first place?

Yeah, thought about it. But then we'll need to check if these properties are
changed and add to/remove from the list. To do so, we'll need to check
writing-mode whenever 'position' or 'height' property were changed.

Given orthogonal is much rarer than changing 'position' or 'height', having
longer list than really necessary looks better to doing such checks. Does this
look reasonable trade-off?

https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1727: LayoutState
layoutState(*root);
On 2016/01/22 at 02:22:35, ojan wrote:
> I'm not 100% sure creating a LayoutState without dealing with any of the
ancestors is correct. I guess that's the fundamental assumption of this approach
though. :) Taking a quick look at the LayoutState code, I don't see a problem,
but Levi and Morten should probably take a close look here.

Yeah, wasn't sure either, but reading around and testing some cases seems to
work better than I expected. Thank you for giving your thoughts. I'll ask Levi
more on this point.

[leviw_travelin_and_unemployed, 2016-01-22 02:51:34]

On 2016/01/22 at 02:33:51, kojii wrote:
> Thank you Ojan for having a look, replies inline:
> 
>
https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/FrameView.cpp:1724: ||
!root->styleRef().logicalHeight().isIntrinsicOrAuto()) {
> On 2016/01/22 at 02:22:35, ojan wrote:
> > If it's any of these last three, maybe we just shouldn't put it in
m_orthogonalWritingModeRootList in the first place?
> 
> Yeah, thought about it. But then we'll need to check if these properties are
changed and add to/remove from the list. To do so, we'll need to check
writing-mode whenever 'position' or 'height' property were changed.
> 
> Given orthogonal is much rarer than changing 'position' or 'height', having
longer list than really necessary looks better to doing such checks. Does this
look reasonable trade-off?
> 
>
https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/FrameView.cpp:1727: LayoutState
layoutState(*root);
> On 2016/01/22 at 02:22:35, ojan wrote:
> > I'm not 100% sure creating a LayoutState without dealing with any of the
ancestors is correct. I guess that's the fundamental assumption of this approach
though. :) Taking a quick look at the LayoutState code, I don't see a problem,
but Levi and Morten should probably take a close look here.
> 
> Yeah, wasn't sure either, but reading around and testing some cases seems to
work better than I expected. Thank you for giving your thoughts. I'll ask Levi
more on this point.

This piggy-backs off the code we use to handle subtree layout roots. It should
be fine here as well.

There are some comments in the LayoutState code that specifically mentions
subtree layout roots, though, that should be updated to mention this use case as
well.

[kojii, 2016-01-22 06:57:39]

On 2016/01/22 02:51:34, leviw wrote:
> >
>
https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/frame/FrameView.cpp:1727: LayoutState
> layoutState(*root);
> > On 2016/01/22 at 02:22:35, ojan wrote:
> > > I'm not 100% sure creating a LayoutState without dealing with any of the
> ancestors is correct. I guess that's the fundamental assumption of this
approach
> though. :) Taking a quick look at the LayoutState code, I don't see a problem,
> but Levi and Morten should probably take a close look here.
> > 
> > Yeah, wasn't sure either, but reading around and testing some cases seems to
> work better than I expected. Thank you for giving your thoughts. I'll ask Levi
> more on this point.
> 
> This piggy-backs off the code we use to handle subtree layout roots. It should
> be fine here as well.

Thank you!

> There are some comments in the LayoutState code that specifically mentions
> subtree layout roots, though, that should be updated to mention this use case
as
> well.

Fixed comments in DepthOrderedLayoutObjectList.h. For other files, searched
"LayoutState" and "subtree" but didn't find comments to be fixed, appreciate to
point out if you have any from top of your head.

[leviw_travelin_and_unemployed, 2016-01-22 07:01:17]

On 2016/01/22 at 06:57:39, kojii wrote:
> On 2016/01/22 02:51:34, leviw wrote:
> > >
> >
https://codereview.chromium.org/1549153002/diff/100001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/frame/FrameView.cpp:1727: LayoutState
> > layoutState(*root);
> > > On 2016/01/22 at 02:22:35, ojan wrote:
> > > > I'm not 100% sure creating a LayoutState without dealing with any of the
> > ancestors is correct. I guess that's the fundamental assumption of this
approach
> > though. :) Taking a quick look at the LayoutState code, I don't see a
problem,
> > but Levi and Morten should probably take a close look here.
> > > 
> > > Yeah, wasn't sure either, but reading around and testing some cases seems
to
> > work better than I expected. Thank you for giving your thoughts. I'll ask
Levi
> > more on this point.
> > 
> > This piggy-backs off the code we use to handle subtree layout roots. It
should
> > be fine here as well.
> 
> Thank you!
> 
> > There are some comments in the LayoutState code that specifically mentions
> > subtree layout roots, though, that should be updated to mention this use
case as
> > well.
> 
> Fixed comments in DepthOrderedLayoutObjectList.h. For other files, searched
"LayoutState" and "subtree" but didn't find comments to be fixed, appreciate to
point out if you have any from top of your head.

I was thinking of this:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[kojii, 2016-01-22 07:41:13]

On 2016/01/22 07:01:17, leviw wrote:
> 
> I was thinking of this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

ahh, missed that one, thank you. Fixed.

[esprehn, 2016-01-31 00:44:56]

The list should not be mutable when iterating, you don't need that complexity.

You just ask the root list to give you a sorted vector back.

DepthSortedLayoutObjectList list;

list.add(o1);
list.add(o2);

Vector<LayoutObjectWithDepth> sortedList;
list.takeSorted(&sortedList);
// list is now empty, you took all the sorted ones.

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/OrthogonalWritingModeRootList.cpp
(right):

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/OrthogonalWritingModeRootList.cpp:32: if
(!m_isAnyRemoved || m_roots.contains(object))
what is this? How can the list be mutated while iterating?

This should be impossible, you take the sorted list after layout and iterate it
directly, you should never have mutations.

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/OrthogonalWritingModeRootList.h
(right):

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/OrthogonalWritingModeRootList.h:35:
LayoutObject* takeDeepestRoot();
lets just merge this all into DepthOrderedLayoutObjectList and not have the two
sub classes.

this should just be takeDeepest()

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/DepthOrderedLayoutObjectList.h
(right):

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/DepthOrderedLayoutObjectList.h:51: //
OrthogonalWritingModeRootList.
just merge them into a single class, DepthOrderedLayoutObjectList

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:245: if (parent()) {
The only times you go through here without a parent are:

you're the LayoutView, you're being first constructed before you get inserted,
so insertedIntoTree handles it.

Which case are you guarding here though?

[kojii, 2016-01-31 01:12:20]

Thank you for the review.

On 2016/01/31 at 00:44:56, esprehn wrote:
> The list should not be mutable when iterating, you don't need that complexity.
> 
> You just ask the root list to give you a sorted vector back.
> 
> DepthSortedLayoutObjectList list;
> 
> list.add(o1);
> list.add(o2);
> 
> Vector<LayoutObjectWithDepth> sortedList;
> list.takeSorted(&sortedList);
> // list is now empty, you took all the sorted ones.

Naming was bad, but LayoutSubtreeRootList empties the list after every
iterations, while OrthogonalWritingModeRootList caches the sorted list unless
the list was changed.

Are you recommending not to cache and sort on every layout? That makes sense
then, but just want to make sure.

If we want to keep the cache but still want to merge the two classes, I can add
"iterate" and "take" methods to the merged class. Does that work better?

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/OrthogonalWritingModeRootList.cpp
(right):

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/OrthogonalWritingModeRootList.cpp:32: if
(!m_isAnyRemoved || m_roots.contains(object))
On 2016/01/31 at 00:44:56, esprehn wrote:
> what is this? How can the list be mutated while iterating?
> 
> This should be impossible, you take the sorted list after layout and iterate
it directly, you should never have mutations.

Just mimic'ed what LayoutSubtreeRootList does, I'll remove in the next patch.

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1549153002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:245: if (parent()) {
On 2016/01/31 at 00:44:56, esprehn wrote:
> The only times you go through here without a parent are:
> 
> you're the LayoutView, you're being first constructed before you get inserted,
so insertedIntoTree handles it.
> 
> Which case are you guarding here though?

This "if" avoids:
* Mark/unmark twice for when first constructed as you said.
* isOrthogonalWritingModeRoot() needs parent() to determine whether it's
orthogonal or not.
so insertedIntoTree() handles when constructed, while this code needs to handle
when styles were changed.

Did I miss something? I actually haven't gone though we always re-attach when
writing-mode is changed, and if that's the case, this code may not run at all.
Are you saying that's the case?

[esprehn, 2016-01-31 01:15:21]

I'd suggest having getSorted() which caches the list and returns a reference to
a Vector, and takeSorted() which gives you back ownership of the cached vector
and clears the list.

[kojii, 2016-01-31 01:21:35]

On 2016/01/31 at 01:15:21, esprehn wrote:
> I'd suggest having getSorted() which caches the list and returns a reference
to a Vector, and takeSorted() which gives you back ownership of the cached
vector and clears the list.

Works good for WritingModeRoots. LayoutSubtreeRootList::removeRoot() handles
modifying during iterate/layout today:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
I'm not sure if this is safe to remove. Levi?

[esprehn, 2016-01-31 01:25:32]

I guess there's some random cases where the layout tree can be modified during
layout, but I don't think we remove layout roots... leviw@ What case does this
happen?

[leviw_travelin_and_unemployed, 2016-01-31 01:33:10]

On 2016/01/31 at 01:21:35, kojii wrote:
> On 2016/01/31 at 01:15:21, esprehn wrote:
> > I'd suggest having getSorted() which caches the list and returns a reference
to a Vector, and takeSorted() which gives you back ownership of the cached
vector and clears the list.
> 
> Works good for WritingModeRoots. LayoutSubtreeRootList::removeRoot() handles
modifying during iterate/layout today:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> I'm not sure if this is safe to remove. Levi?

We shouldn't need that. I think it's vestigial from before I used a "take"
method to pop off the sorted vector.

We shouldn't be removing these during Layout.

[kojii, 2016-01-31 01:35:43]

On 2016/01/31 01:33:10, leviw wrote:
> On 2016/01/31 at 01:21:35, kojii wrote:
> > On 2016/01/31 at 01:15:21, esprehn wrote:
> > > I'd suggest having getSorted() which caches the list and returns a
reference
> to a Vector, and takeSorted() which gives you back ownership of the cached
> vector and clears the list.
> > 
> > Works good for WritingModeRoots. LayoutSubtreeRootList::removeRoot() handles
> modifying during iterate/layout today:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > I'm not sure if this is safe to remove. Levi?
> 
> We shouldn't need that. I think it's vestigial from before I used a "take"
> method to pop off the sorted vector.
> 
> We shouldn't be removing these during Layout.

Thank you, I'll work on the next CL to merge the two classes as suggested.

[kojii, 2016-01-31 11:17:32]

Patchset #12 (id:220001) has been deleted

[kojii, 2016-01-31 14:41:00]

Description was changed from

==========
Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available when preferred logical
widths are needed.

This patch makes a partial fix for shrink-to-fit inline-block[1]
unnecessary, so the code and flags added in the patch are removed.

[1] https://codereview.chromium.org/1121173002/

BUG=550963, 473429
==========

to

==========
Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available when preferred logical
widths are needed.

This patch makes the previous partial fix for shrink-to-fit
inline-block[1] unnecessary, so the code and flags added in the patch
are removed.

[1] https://codereview.chromium.org/1121173002/

BUG=550963, 473429
==========

[kojii, 2016-01-31 23:00:03]

PTAL. Two new findings:

1. The distinction of get/take is removed, since LayoutSubtreeRoot always clear
both unordered/ordered list after each enumeration. It's easy to do "get" only
then clear after enumeration (as long as we don't add while iterating for the
next iteration.)

2. compositing/overflow/repaint-after-losing-scrollbars.html does removal of
LayoutSubtreeRoot while iterating, and not supporting lead to SEGV crash (see
PS12 failures,) so I added it back, and added ASSERT for other than removal. I
do not understand whether the removal in this test is ok or not, but hope this
is cleaner than before.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[kojii, 2016-02-01 00:57:03]

PTAL.

* Removed support for removal-while-iterating.
* Debug ASSERT is still there. Release is likely to crash.
* It depends on leviw@'s CL.

[kojii, 2016-02-01 02:41:47]

Ah, I have to allow calling remove() as long as it doesn't actually remove.

Or should I add contains() and fix callers to call only if contains()?

[leviw_travelin_and_unemployed, 2016-02-01 05:41:37]

On 2016/02/01 at 02:41:47, kojii wrote:
> Ah, I have to allow calling remove() as long as it doesn't actually remove.
> 
> Or should I add contains() and fix callers to call only if contains()?

I'm not dogmatic, but I think it's fine as long as it doesn't remove during
layout.

[kojii, 2016-02-01 05:53:03]

On 2016/02/01 at 05:41:37, leviw wrote:
> On 2016/02/01 at 02:41:47, kojii wrote:
> > Ah, I have to allow calling remove() as long as it doesn't actually remove.
> > 
> > Or should I add contains() and fix callers to call only if contains()?
> 
> I'm not dogmatic, but I think it's fine as long as it doesn't remove during
layout.

Great, then the patch is ready for review.

PTAL?

[leviw_travelin_and_unemployed, 2016-02-02 05:03:40]

Couple little things.

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/DepthOrderedLayoutObjectList.h
(right):

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/DepthOrderedLayoutObjectList.h:27:
ASSERT(!m_assertOnModify);
Instead of needing this bit, since you always get an Object during the assert,
you should be able to follow it back to its document and use the lifecycle.

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/DepthOrderedLayoutObjectList.h:42:
ASSERT(!m_assertOnModify);
As Kojii mentioned, you'd have to allow clearing still.

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/DepthOrderedLayoutObjectList.h:58: :
object(0)
nullptr

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:154: if
(!documentBeingDestroyed() && isOrthogonalWritingModeRoot())
May be worth switching this ordering? I think it's less common?

[kojii, 2016-02-02 23:32:50]

All done, thank you. PTAL.

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/DepthOrderedLayoutObjectList.h
(right):

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/DepthOrderedLayoutObjectList.h:27:
ASSERT(!m_assertOnModify);
On 2016/02/02 at 05:03:40, leviw wrote:
> Instead of needing this bit, since you always get an Object during the assert,
you should be able to follow it back to its document and use the lifecycle.

Done.

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/DepthOrderedLayoutObjectList.h:42:
ASSERT(!m_assertOnModify);
On 2016/02/02 at 05:03:40, leviw wrote:
> As Kojii mentioned, you'd have to allow clearing still.

Done.

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/DepthOrderedLayoutObjectList.h:58: :
object(0)
On 2016/02/02 at 05:03:40, leviw wrote:
> nullptr

Done.

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1549153002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:154: if
(!documentBeingDestroyed() && isOrthogonalWritingModeRoot())
On 2016/02/02 at 05:03:40, leviw wrote:
> May be worth switching this ordering? I think it's less common?

Done.

[leviw_travelin_and_unemployed, 2016-02-03 00:15:52]

lgtm

[kojii, 2016-02-03 02:51:31]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-02-03 02:52:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549153002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549153002/340001

[commit-bot: I haz the power, 2016-02-03 03:00:34]

Committed patchset #17 (id:340001)

[commit-bot: I haz the power, 2016-02-03 03:01:30]

Description was changed from

==========
Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available when preferred logical
widths are needed.

This patch makes the previous partial fix for shrink-to-fit
inline-block[1] unnecessary, so the code and flags added in the patch
are removed.

[1] https://codereview.chromium.org/1121173002/

BUG=550963, 473429
==========

to

==========
Fix preferred logical widths of orthogonal writing modes

To compute preferred logical widths of elements with orthogonal writing
modes, this patch keeps track of all orthogonal writing mode roots in
documents, and layout them before computing preferred widths.

OrthogonalWritingModeRootList keeps a list of orthogonal writing mode
roots in FrameView, and layout them in the depth order. Part of
LayoutSubtreeRootList was extracted to DepthOrderedLayoutObjectList to
share the code.

LayoutBox adds or removes themselves to/from the list as needed in
insertedIntoTree(), willBeRemovedFromTree(), and styleDidChange().

FrameView::performLayout() layouts them in the deepest-first order
before computing preferred widths, so that orthogonal children are
!needsLayout() and have logicalHeight() available when preferred logical
widths are needed.

This patch makes the previous partial fix for shrink-to-fit
inline-block[1] unnecessary, so the code and flags added in the patch
are removed.

[1] https://codereview.chromium.org/1121173002/

BUG=550963, 473429

Committed: https://crrev.com/3d7b19bbee5aac29c8efa9df62f08264c478041b
Cr-Commit-Position: refs/heads/master@{#373144}
==========

[commit-bot: I haz the power, 2016-02-03 03:01:30]

Patchset 17 (id:??) landed as
https://crrev.com/3d7b19bbee5aac29c8efa9df62f08264c478041b
Cr-Commit-Position: refs/heads/master@{#373144}