Fix broken preferred widths optimization involving subtree layout roots

Fix broken preferred widths optimization involving subtree layout roots

Back in an old, poorly explained commit, we added logic in
LayoutBox::computePreferredLogicalWidths to bail when in a subtree layout.
This may have been to cover up a bug whereby we'd get the value wrong in
this case, or it may have been an optimization:
https://chromium.googlesource.com/chromium/blink/+/0d46936cccddf2caa4d484e27fbccfe2520ca214%5E%21/#F8

This code can result in a box not updating its preferred widths even when it
should in the case that its a subtree layout root that was marked for layout 
of its children, then subsequently marked for layout itself.

We shouldn't be calling computePreferredLogicalWidths if we don't need to,
instead of having this crazy short-circuit logic inside it.

BUG=497178
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197060

[leviw_travelin_and_unemployed, 2015-06-11 22:56:34]

leviw@chromium.org changed reviewers:
+ esprehn@chromium.org, jchaffraix@chromium.org

[leviw_travelin_and_unemployed, 2015-06-12 00:00:30]

leviw@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2015-06-12 00:42:26]

https://codereview.chromium.org/1184563002/diff/1/Source/core/layout/LayoutBo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1184563002/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBox.cpp:1950: if (node() && !parent()->needsLayout() &&
view()->frameView() && view()->frameView()->isLayoutRoot(*this))
Maybe just delete the early-out? Does it matter?

[esprehn, 2015-06-12 00:44:52]

https://codereview.chromium.org/1184563002/diff/1/Source/core/layout/LayoutBo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1184563002/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBox.cpp:1950: if (node() && !parent()->needsLayout() &&
view()->frameView() && view()->frameView()->isLayoutRoot(*this))
On 2015/06/12 at 00:42:26, chrishtr wrote:
> Maybe just delete the early-out? Does it matter?

It'd be nice to avoid computing preferredLogicalWidths when it's not needed.

[leviw_travelin_and_unemployed, 2015-06-12 00:46:05]

On 2015/06/12 at 00:42:26, chrishtr wrote:
>
https://codereview.chromium.org/1184563002/diff/1/Source/core/layout/LayoutBo...
> File Source/core/layout/LayoutBox.cpp (right):
> 
>
https://codereview.chromium.org/1184563002/diff/1/Source/core/layout/LayoutBo...
> Source/core/layout/LayoutBox.cpp:1950: if (node() && !parent()->needsLayout()
&& view()->frameView() && view()->frameView()->isLayoutRoot(*this))
> Maybe just delete the early-out? Does it matter?

I'm happy to give it a try. I mentioned the possibility in the description. We
don't have particularly good perf coverage for the benefits of subtree layouts,
hence my kid gloves.

[leviw_travelin_and_unemployed, 2015-06-12 00:47:15]

I'll defer to the crowd, but I think the optimization is likely worthwhile.

[ojan, 2015-06-12 01:06:15]

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

[ojan, 2015-06-12 01:06:16]

Hmmm...I'm a bit confused about this optimization. Avoiding computing preferred
width where we otherwise would is definitely a win. But in this case, I don't
see how we'd end up computing preferred width in the first place. Being a layout
root means that you have fixed width, so we won't compute your preferred width
in this function anyways?

In looking into this I noticed that we don't take min-width/max-width/flex into
account in objectIsRelayoutBoundary, which means that we'll treat something with
min-width:min-content as a layout boundary when we shouldn't.

https://codereview.chromium.org/1184563002/diff/1/LayoutTests/fast/layout/nes...
File LayoutTests/fast/layout/nested-subtree-layout-preferred-widths.html
(right):

https://codereview.chromium.org/1184563002/diff/1/LayoutTests/fast/layout/nes...
LayoutTests/fast/layout/nested-subtree-layout-preferred-widths.html:26: failing
due the the non-deterministic way we iterate over subtree layout roots.
Lol. Good luck with that.

[ojan, 2015-06-12 01:10:45]

I looked up the patch that added this. There's no real explanation for it.

https://chromium.googlesource.com/chromium/blink/+/0d46936cccddf2caa4d484e27f...

[ojan, 2015-06-12 01:12:18]

But it did come with a test. Does floating-textfield-relayout pass if you get
rid of the early return? If not, then there's more we need to understand here
about why the code works this way.

[leviw_travelin_and_unemployed, 2015-06-12 18:54:44]

Code removed. Tests pass. PTAL.

[ojan, 2015-06-12 19:41:07]

Lgtm, but please update the change description now that we know this isn't a
preferred width optimization.

[leviw_travelin_and_unemployed, 2015-06-12 20:14:31]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2015-06-12 20:14:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184563002/20001

[commit-bot: I haz the power, 2015-06-12 20:18:50]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197060