Clear intruding floats when moving child to inline parent.

Clear intruding floats when moving child to inline parent.

BUG=562208
Committed: https://crrev.com/c2185287e6160a9df8834d2bf409d2aa75573c1c
Cr-Commit-Position: refs/heads/master@{#363870}

[rhogan, 2015-12-07 11:30:28]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2015-12-07 14:07:36]

Would it be possible to come up with an additional, super-simple test, if we add
the right assertions to the right places? :) It would be a useful addition, to
more clearly explain what we're trying to fix here.

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/block/float/clear-intruding-floats-when-moving-to-inline-parent.html
(right):

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/float/clear-intruding-floats-when-moving-to-inline-parent.html:29:
testPassed("crbug.com/562208: Clear intruding floats when moving child to inline
parent.");
This is a crash test. Could you make that clearer? E.g. by adding "Did not
crash" here, or by renaming the test to
clear-intruding-floats-when-moving-to-inline-parent-crash.html

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1014: // If a
child is moving from a block-flow to an inline-flow parent then any floats
currently intruding into
I think we need an example here. When can things like this happen?

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1016: // won't
attempt to remove floats from parents that have inline-flow if we try later.
Yeah... regarding that... Why don't we just attempt to remove floats from
inline-children blocks too? Then we wouldn't have bugs like this, right?

[rhogan, 2015-12-07 17:29:52]

Updated per your comments.

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1016: // won't
attempt to remove floats from parents that have inline-flow if we try later.
On 2015/12/07 at 14:07:35, mstensho wrote:
> Yeah... regarding that... Why don't we just attempt to remove floats from
inline-children blocks too? Then we wouldn't have bugs like this, right?

Performance. I removed it once but then added it back in again:
https://codereview.chromium.org/1397383002.

[mstensho (USE GERRIT), 2015-12-07 21:14:44]

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1016: // won't
attempt to remove floats from parents that have inline-flow if we try later.
On 2015/12/07 17:29:52, rhogan wrote:
> On 2015/12/07 at 14:07:35, mstensho wrote:
> > Yeah... regarding that... Why don't we just attempt to remove floats from
> inline-children blocks too? Then we wouldn't have bugs like this, right?
> 
> Performance. I removed it once but then added it back in again:
> https://codereview.chromium.org/1397383002. 

Why did that cause a performance regression? I don't understand why that has to
be particularly expensive.

Is it because you have a DIV with 30000 floats inside (but no in-flow blocks, so
children will be inline)? Looking at
https://code.google.com/p/chromium/issues/attachmentText?id=537111&aid=537111...

What if we change the test case to contain one in-flow block followed by 30000
float siblings? Then the floats would be in a block-children container, and
you'd have serious performance problems as well. But that's OK, since it's
always been like that, or what?

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1017: if
(child->isLayoutBlockFlow() && toBoxModelObject->childrenInline() &&
!child->childrenInline())
I think something is missing here.

The condition is: if the child to be moved has block children and it is being
moved into a block with inline children... That could for instance mean an
inline-block with block children being moved from one block to another (both
having inline children). Killing the floats of an inline-block is completely
unnecessary, since inline-blocks establish block formatting contexts.

Add "&& !childrenInline()", just to make clear that the old parent had block
children?

https://codereview.chromium.org/1495033002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/block/float/clear-intruding-floats-when-moving-to-inline-parent.html
(right):

https://codereview.chromium.org/1495033002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/block/float/clear-intruding-floats-when-moving-to-inline-parent.html:2:
<script src="../../../resources/js-test.js"></script>
I asked in my previous batch of comments, but got no response:

Would it be possible to come up with an additional, super-simple test, if we add
the right assertions to the right places? :) It would be a useful addition, to
more clearly explain what we're trying to fix here.

https://codereview.chromium.org/1495033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1495033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1016: // to an an
anonymous block. Remove them from their float-lists immediately as
markAllDescendantsWithFloatsForLayout
"them" as in "intruding floats"?

[rhogan, 2015-12-08 18:15:44]

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1016: // won't
attempt to remove floats from parents that have inline-flow if we try later.
On 2015/12/07 at 21:14:44, mstensho wrote:
> On 2015/12/07 17:29:52, rhogan wrote:
> > On 2015/12/07 at 14:07:35, mstensho wrote:
> > > Yeah... regarding that... Why don't we just attempt to remove floats from
> > inline-children blocks too? Then we wouldn't have bugs like this, right?
> > 
> > Performance. I removed it once but then added it back in again:
> > https://codereview.chromium.org/1397383002. 
> 
> Why did that cause a performance regression? I don't understand why that has
to be particularly expensive.

I let markAllDescendantsWithFloatForLayout() descend through elements with
inline children in all cases just because I introduced an edge-case where
elements with inline-children might have intruding floats in their float-lists
temporarily (i.e. before getting cleared at the next layout). So it was an
unnecessary performance regression incurred whenever we removed a float. You're
right - that if a float does indeed intrude many levels down the layout tree it
would take some time to remove it from all of them. But no point in letting it
do that it when it doesn't need to.

https://codereview.chromium.org/1495033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1017: if
(child->isLayoutBlockFlow() && toBoxModelObject->childrenInline() &&
!child->childrenInline())
On 2015/12/07 at 21:14:44, mstensho wrote:
> I think something is missing here.
> 
> The condition is: if the child to be moved has block children and it is being
moved into a block with inline children... That could for instance mean an
> inline-block with block children being moved from one block to another (both
having inline children). Killing the floats of an inline-block is completely
unnecessary, since inline-blocks establish block formatting contexts.
> 
> Add "&& !childrenInline()", just to make clear that the old parent had block
children?

Yes, that's true - it couldn't have any intruding floats from ancestors if all
its siblings were inline.

https://codereview.chromium.org/1495033002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/block/float/clear-intruding-floats-when-moving-to-inline-parent.html
(right):

https://codereview.chromium.org/1495033002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/block/float/clear-intruding-floats-when-moving-to-inline-parent.html:2:
<script src="../../../resources/js-test.js"></script>
On 2015/12/07 at 21:14:44, mstensho wrote:
> I asked in my previous batch of comments, but got no response:
> 
> Would it be possible to come up with an additional, super-simple test, if we
add the right assertions to the right places? :) It would be a useful addition,
to more clearly explain what we're trying to fix here.

OK. Done!

[mstensho (USE GERRIT), 2015-12-08 18:57:27]

lgtm

[rhogan, 2015-12-08 19:17:09]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-12-08 19:18:23]

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

[commit-bot: I haz the power, 2015-12-08 19:35:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 19:35:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[rhogan, 2015-12-08 20:46:30]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-12-08 20:48:52]

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

[commit-bot: I haz the power, 2015-12-09 00:56:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-09 00:56:47]

Description was changed from

==========
Clear intruding floats when moving child to inline parent.

BUG=562208
==========

to

==========
Clear intruding floats when moving child to inline parent.

BUG=562208

Committed: https://crrev.com/c2185287e6160a9df8834d2bf409d2aa75573c1c
Cr-Commit-Position: refs/heads/master@{#363870}
==========

[commit-bot: I haz the power, 2015-12-09 00:56:48]

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