Flowthread should move its floatlists to container when evacuating

Flowthread should move its floatlists to container when evacuating

This ensures that if any of the floats gets deleted the new
container will be able to delete from its new descendants'
float-lists.

BUG=714440
Review-Url: https://codereview.chromium.org/2863093004
Cr-Commit-Position: refs/heads/master@{#470128}
Committed: https://chromium.googlesource.com/chromium/src/+/a925ec8c709dcc3768dfeb349bf19c766d6f5c7d

[rhogan, 2017-05-05 19:02:48]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 19:03:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rhogan, 2017-05-05 19:04:06]

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

[rhogan, 2017-05-05 19:04:07]

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

https://codereview.chromium.org/2863093004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:3171:
MoveAllChildrenTo(to_block_flow, full_remove_insert);
If full_remove_insert is true, the object's floatlist gets cleared so copying
the floats after the fact will be a null op!

[commit-bot: I haz the power, 2017-05-05 20:47:30]

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

[commit-bot: I haz the power, 2017-05-05 20:47:31]

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

[mstensho (USE GERRIT), 2017-05-08 09:56:09]

In https://codereview.chromium.org/1729663002 you did a similar fix for
Populate() (which is the opposite operation of EvacuateAndDestroy()), it seems,
but there you just added a RemoveFloatingObjectsFromDescendants(). Perhaps you
could just do the same here?

If that doesn't make sense, then lgtm. I just prefer to keep it a bit more
symmetric, if at all possible.

[rhogan, 2017-05-08 18:58:04]

On 2017/05/08 at 09:56:09, mstensho wrote:
> In https://codereview.chromium.org/1729663002 you did a similar fix for
Populate() (which is the opposite operation of EvacuateAndDestroy()), it seems,
but there you just added a RemoveFloatingObjectsFromDescendants(). Perhaps you
could just do the same here?
> 
> If that doesn't make sense, then lgtm. I just prefer to keep it a bit more
symmetric, if at all possible.

In my head Populate() is creating a formatting context so we have to clear down
the float lists as its not certain if the descendants should still contain them
whereas with EvacuateAndDestroy() we're removing a formatting context so want to
make the new container aware of them so they can be removed later if necessary -
if the formatting context contained them the new container should. I agree that
RemoveFloatingObjectsFromDescendants() would still work, and it's what I put in
there first.

Since a side-benefit of this CL is fixing the behaviour of is fixing
MoveAllChildrenIncludingFloatsTo, I think I'll go with it. Maybe we can make it
consistent between the two operations later - I'm sure it'll be either you or me
that is back here eventually! :)

[mstensho (USE GERRIT), 2017-05-08 19:12:48]

On 2017/05/08 18:58:04, rhogan wrote:
> On 2017/05/08 at 09:56:09, mstensho wrote:
> > In https://codereview.chromium.org/1729663002 you did a similar fix for
> Populate() (which is the opposite operation of EvacuateAndDestroy()), it
seems,
> but there you just added a RemoveFloatingObjectsFromDescendants(). Perhaps you
> could just do the same here?
> > 
> > If that doesn't make sense, then lgtm. I just prefer to keep it a bit more
> symmetric, if at all possible.
> 
> In my head Populate() is creating a formatting context so we have to clear
down
> the float lists as its not certain if the descendants should still contain
them
> whereas with EvacuateAndDestroy() we're removing a formatting context so want
to
> make the new container aware of them so they can be removed later if necessary
-
> if the formatting context contained them the new container should. I agree
that
> RemoveFloatingObjectsFromDescendants() would still work, and it's what I put
in
> there first.
> 
> Since a side-benefit of this CL is fixing the behaviour of is fixing
> MoveAllChildrenIncludingFloatsTo, I think I'll go with it. Maybe we can make
it
> consistent between the two operations later - I'm sure it'll be either you or
me
> that is back here eventually! :)

OK - still lgtm

[rhogan, 2017-05-08 19:19:38]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2017-05-08 19:21:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 21:16:33]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1494271178123750, "parent_rev":
"cf63bb826e3f5b9bd89fcde86ffdf33134cb4ea4", "commit_rev":
"a925ec8c709dcc3768dfeb349bf19c766d6f5c7d"}

[commit-bot: I haz the power, 2017-05-08 21:16:44]

Description was changed from

==========
Flowthread should move its floatlists to container when evacuating

This ensures that if any of the floats gets deleted the new
container will be able to delete from its new descendants'
float-lists.

BUG=714440
==========

to

==========
Flowthread should move its floatlists to container when evacuating

This ensures that if any of the floats gets deleted the new
container will be able to delete from its new descendants'
float-lists.

BUG=714440

Review-Url: https://codereview.chromium.org/2863093004
Cr-Commit-Position: refs/heads/master@{#470128}
Committed:
https://chromium.googlesource.com/chromium/src/+/a925ec8c709dcc3768dfeb349bf1...
==========

[commit-bot: I haz the power, 2017-05-08 21:16:45]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/a925ec8c709dcc3768dfeb349bf1...