Fold out-of-flow objects into anonymous blocks when removing children

Fold out-of-flow objects into anonymous blocks when removing children

Sometimes when we remove a child we end up with a float or out-of-flow object beside
an anonymous block. When that happens, fold the float/out-of-flow object into the
anonymous box as that is where it would have lived if they were siblings in the
first place.

BUG=322039
Committed: https://crrev.com/44402d152cd0bb788db3136ab08c3c76ec348cd1
Cr-Commit-Position: refs/heads/master@{#357886}

[rhogan, 2015-10-22 18:51:10]

Description was changed from

==========
bug 322039

BUG=322039
==========

to

==========
Add subsequent floats to anonymous block if their siblings become inline

BUG=322039
==========

[rhogan, 2015-10-29 18:23:29]

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

[mstensho (USE GERRIT), 2015-10-29 20:00:26]

https://codereview.chromium.org/1406163003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/block/float/add-inline-before-float-and-into-anonymous-block-container.html
(right):

https://codereview.chromium.org/1406163003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/block/float/add-inline-before-float-and-into-anonymous-block-container.html:13:
display: inline;
Not needed. 'float' overrides this to 'block'.

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

https://codereview.chromium.org/1406163003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:372: static void
addSubsequentFloatingSiblingsToBlock(LayoutBlock* block, LayoutBlock* container)
FloatingOrOutOfFlowSiblings

https://codereview.chromium.org/1406163003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:448:
addSubsequentFloatingSiblingsToBlock(toLayoutBlock(afterChild), this);
Why would there be subsequent floats or out-of-flow objects after an anonymous
block? Don't we already have code to wrap them inside such anonymous blocks?

Oh, I see... Nice testcase. :)

No, wait a minute!

The TC goes like this:

LayoutBlock (anonymous)
    BR
LayoutBlock (DIV #first .block)
LayoutBlock (DIV .float)

Then we change #first from block to inline-block. First we detach the old layout
object:

LayoutBlock (anonymous)
    BR
LayoutBlock (DIV .float)

I guess my question is: why don't we wrap the float inside the anonymous block
right away here? Why is there stuff like this to clean up when inserting the new
#first inline-block layout object? Shouldn't the final lines in
LayoutBlock::removeChild() take care of that?

[rhogan, 2015-10-29 22:53:39]

On 2015/10/29 at 20:00:26, mstensho wrote:
> I guess my question is: why don't we wrap the float inside the anonymous block
right away here? Why is there stuff like this to clean up when inserting the new
#first inline-block layout object? Shouldn't the final lines in
LayoutBlock::removeChild() take care of that?

Sure, that makes sense - moved it to removeChild instead!

[mstensho (USE GERRIT), 2015-10-30 08:15:14]

https://codereview.chromium.org/1406163003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1406163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:738: if (prev &&
prev->isAnonymousBlock())
Don't we also need something similar for |next|?

<div id="outer">
    <div id="float" style="float:left;"></div>
    <div id="killMe"></div>
    Ost.
</div>

Now remove #killMe. I imagine we'd end up with

LayoutBlockFlow DIV #outer
    LayoutBlockFlow DIV #float
    LayoutBlockFlow anonymous
        LayoutText "Ost."

There should no longer be any need for the anonymous block at this point.

Admittedly, the rendering will be the same either way with this test, but isn't
the idea (what you've been working on lately) that we should be strict about
removing anonymous blocks when they are no longer needed?

[rhogan, 2015-10-30 19:28:16]

https://codereview.chromium.org/1406163003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1406163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:738: if (prev &&
prev->isAnonymousBlock())
On 2015/10/30 at 08:15:14, mstensho wrote:
> Don't we also need something similar for |next|?
> 
> <div id="outer">
>     <div id="float" style="float:left;"></div>
>     <div id="killMe"></div>
>     Ost.
> </div>
> 
> Now remove #killMe. I imagine we'd end up with
> 
> LayoutBlockFlow DIV #outer
>     LayoutBlockFlow DIV #float
>     LayoutBlockFlow anonymous
>         LayoutText "Ost."
> 
> There should no longer be any need for the anonymous block at this point.
> 

I would expect https://codereview.chromium.org/1423573002 to take care of that -
there is only floats/out-of-flows and anonymous blocks on the line after the
child is removed, so makeChildrenInlineIfPossible() collapses the anonymous
block. (Checked and it does.)

[mstensho (USE GERRIT), 2015-11-02 12:31:58]

https://codereview.chromium.org/1406163003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1406163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:738: if (prev &&
prev->isAnonymousBlock())
On 2015/10/30 19:28:16, rhogan wrote:
> On 2015/10/30 at 08:15:14, mstensho wrote:
> > Don't we also need something similar for |next|?
> > 
> > <div id="outer">
> >     <div id="float" style="float:left;"></div>
> >     <div id="killMe"></div>
> >     Ost.
> > </div>
> > 
> > Now remove #killMe. I imagine we'd end up with
> > 
> > LayoutBlockFlow DIV #outer
> >     LayoutBlockFlow DIV #float
> >     LayoutBlockFlow anonymous
> >         LayoutText "Ost."
> > 
> > There should no longer be any need for the anonymous block at this point.
> > 
> 
> I would expect https://codereview.chromium.org/1423573002 to take care of that
-
> there is only floats/out-of-flows and anonymous blocks on the line after the
> child is removed, so makeChildrenInlineIfPossible() collapses the anonymous
> block. (Checked and it does.)

Actually, the test in this CL also passes with only that CL (but
http://jsfiddle.net/U76jj/3/ doesn't), so we need a new test.

[rhogan, 2015-11-02 19:38:05]

https://codereview.chromium.org/1406163003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1406163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:738: if (prev &&
prev->isAnonymousBlock())
On 2015/11/02 at 12:31:57, mstensho wrote:
> On 2015/10/30 19:28:16, rhogan wrote:
> > On 2015/10/30 at 08:15:14, mstensho wrote:
> > > Don't we also need something similar for |next|?
> > > 
> > > <div id="outer">
> > >     <div id="float" style="float:left;"></div>
> > >     <div id="killMe"></div>
> > >     Ost.
> > > </div>
> > > 
> > > Now remove #killMe. I imagine we'd end up with
> > > 
> > > LayoutBlockFlow DIV #outer
> > >     LayoutBlockFlow DIV #float
> > >     LayoutBlockFlow anonymous
> > >         LayoutText "Ost."
> > > 
> > > There should no longer be any need for the anonymous block at this point.
> > > 
> > 
> > I would expect https://codereview.chromium.org/1423573002 to take care of
that -
> > there is only floats/out-of-flows and anonymous blocks on the line after the
> > child is removed, so makeChildrenInlineIfPossible() collapses the anonymous
> > block. (Checked and it does.)
> 
> Actually, the test in this CL also passes with only that CL (but
http://jsfiddle.net/U76jj/3/ doesn't), so we need a new test.

Oops, fixed the test - it was missing a <div> before the <br>.

[mstensho (USE GERRIT), 2015-11-02 21:35:24]

On 2015/11/02 19:38:05, rhogan wrote:
>
https://codereview.chromium.org/1406163003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):
> 
>
https://codereview.chromium.org/1406163003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBlock.cpp:738: if (prev &&
> prev->isAnonymousBlock())
> On 2015/11/02 at 12:31:57, mstensho wrote:
> > On 2015/10/30 19:28:16, rhogan wrote:
> > > On 2015/10/30 at 08:15:14, mstensho wrote:
> > > > Don't we also need something similar for |next|?
> > > > 
> > > > <div id="outer">
> > > >     <div id="float" style="float:left;"></div>
> > > >     <div id="killMe"></div>
> > > >     Ost.
> > > > </div>
> > > > 
> > > > Now remove #killMe. I imagine we'd end up with
> > > > 
> > > > LayoutBlockFlow DIV #outer
> > > >     LayoutBlockFlow DIV #float
> > > >     LayoutBlockFlow anonymous
> > > >         LayoutText "Ost."
> > > > 
> > > > There should no longer be any need for the anonymous block at this
point.
> > > > 
> > > 
> > > I would expect https://codereview.chromium.org/1423573002 to take care of
> that -
> > > there is only floats/out-of-flows and anonymous blocks on the line after
the
> > > child is removed, so makeChildrenInlineIfPossible() collapses the
anonymous
> > > block. (Checked and it does.)
> > 
> > Actually, the test in this CL also passes with only that CL (but
> http://jsfiddle.net/U76jj/3/ doesn't), so we need a new test.
> 
> Oops, fixed the test - it was missing a <div> before the <br>.

Right, now my test, with the same missing DIV. :)

<div id="outer">
    <div id="first"></div>
    <div id="float" style="float:left;"></div>
    <div id="killMe"></div>
    Ost.
</div>

Now remove #killMe. We end up with this:

LayoutBlockFlow DIV #outer
    LayoutBlockFlow DIV #first
    LayoutBlockFlow DIV #float
    LayoutBlockFlow anonymous
        LayoutText "Ost."

But shouldn't really #float be inside of its anonymous sibling?

[rhogan, 2015-11-03 19:28:39]

On 2015/11/02 at 21:35:24, mstensho wrote:
> Now remove #killMe. We end up with this:
> 
> LayoutBlockFlow DIV #outer
>     LayoutBlockFlow DIV #first
>     LayoutBlockFlow DIV #float
>     LayoutBlockFlow anonymous
>         LayoutText "Ost."
> 
> But shouldn't really #float be inside of its anonymous sibling?

Right enough. Updated with a new test.

[rhogan, 2015-11-03 19:29:09]

Description was changed from

==========
Add subsequent floats to anonymous block if their siblings become inline

BUG=322039
==========

to

==========
Add previous and subsequent floats to anon blocks if their siblings become
inline

BUG=322039
==========

[mstensho (USE GERRIT), 2015-11-03 20:53:18]

lgtm with my comments addressed.

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

https://codereview.chromium.org/1406163003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:378: static void
addSubsequentFloatingOrOutOfFlowSiblingsToBlock(LayoutBlock* block, LayoutBlock*
container)
As an antonym to "Previous", maybe "Next" instead of "Subsequent"?

https://codereview.chromium.org/1406163003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:744: if (next &&
next->isAnonymousBlock())
else if?

[mstensho (USE GERRIT), 2015-11-03 21:02:36]

On 2015/11/03 20:53:18, mstensho wrote:
> lgtm with my comments addressed.
> 
>
https://codereview.chromium.org/1406163003/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):
> 
>
https://codereview.chromium.org/1406163003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBlock.cpp:378: static void
> addSubsequentFloatingOrOutOfFlowSiblingsToBlock(LayoutBlock* block,
LayoutBlock*
> container)
> As an antonym to "Previous", maybe "Next" instead of "Subsequent"?
> 
>
https://codereview.chromium.org/1406163003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBlock.cpp:744: if (next &&
> next->isAnonymousBlock())
> else if?

One more thing, that I forgot to mention: I think you need to improve the
description a little.

This is not about stuff becoming inline. It's about stuff ceasing to be blocks.

[rhogan, 2015-11-03 22:00:41]

Description was changed from

==========
Add previous and subsequent floats to anon blocks if their siblings become
inline

BUG=322039
==========

to

==========
Fold out-of-flow objects into anonymous blocks when removing children

Sometimes when we remove a child we end up with a float or out-of-flow object
beside
an anonymous block. When that happens, fold the float/out-of-flow object into
the
anonymous box as that is where it would have lived if they were siblings in the
first place.

BUG=322039
==========

[rhogan, 2015-11-03 22:01:57]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-11-03 22:01:57]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1406163003/#ps110001
(title: "Updated")

[commit-bot: I haz the power, 2015-11-03 22:04:12]

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

[commit-bot: I haz the power, 2015-11-03 22:48:14]

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

[commit-bot: I haz the power, 2015-11-03 22:48:14]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2015-11-04 19:27:46]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-11-04 19:28:22]

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

[commit-bot: I haz the power, 2015-11-04 20:34:31]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-11-04 20:35:17]

Patchset 7 (id:??) landed as
https://crrev.com/44402d152cd0bb788db3136ab08c3c76ec348cd1
Cr-Commit-Position: refs/heads/master@{#357886}

[inferno, 2015-11-05 16:05:04]

A revert of this CL (patchset #7 id:110001) has been created in
https://codereview.chromium.org/1419053004/ by inferno@chromium.org.

The reason for reverting is: Causing use-after-free crashes in entire rendering
pipeline, reverting..