Formatting contexts should always expand to enclose floats

Formatting contexts should always expand to enclose floats

If a block that creates a formatting context contains floats in its descendants that
overhang, it should expand to enclose them. We were failing to do this when the float
overhung the formatting context but not its direct ancestor.

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

[rhogan, 2015-04-25 17:16:58]

robhogan@gmail.com changed reviewers:
+ leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-04-28 17:38:16]

Clever fix! I know your previous attempts netted a 5% perf reduction... it looks
like this one should be pretty neutral! Can you confirm what your results were
locally?

[rhogan, 2015-04-30 20:07:00]

On 2015/04/28 at 17:38:16, leviw wrote:
> Clever fix! I know your previous attempts netted a 5% perf reduction... it
looks like this one should be pretty neutral! Can you confirm what your results
were locally?

This one nets a performance improvement of 10 to 20% in some of the float tests.
This improvement is specifically due to the root element now expanding to
enclose floats even though they're nested in descendants so the root element no
longer gets as many layouts.

[leviw_travelin_and_unemployed, 2015-04-30 21:18:41]

On 2015/04/30 at 20:07:00, robhogan wrote:
> On 2015/04/28 at 17:38:16, leviw wrote:
> > Clever fix! I know your previous attempts netted a 5% perf reduction... it
looks like this one should be pretty neutral! Can you confirm what your results
were locally?
> 
> This one nets a performance improvement of 10 to 20% in some of the float
tests. This improvement is specifically due to the root element now expanding to
enclose floats even though they're nested in descendants so the root element no
longer gets as many layouts.

Geeeeeeeeez! LGTM, my friend. Great work.

[rhogan, 2015-05-01 18:31:10]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-05-01 18:31:28]

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

[commit-bot: I haz the power, 2015-05-01 18:35:41]

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

[commit-bot: I haz the power, 2015-05-01 18:35:42]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/54173)

[rhogan, 2015-05-01 18:56:26]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-05-01 18:56:26]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1106533002/#ps40001
(title: "Updated")

[commit-bot: I haz the power, 2015-05-01 18:56:44]

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

[commit-bot: I haz the power, 2015-05-01 20:48:18]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194831

[rhogan, 2015-05-22 18:45:51]

Hi Levi,

I'd like to have another go at this - revised approach available for your
review.

This new approach is more intrusive. Now we always pass the lowest float in a
block up the line until we reach a block formatting context. It doesn't result
in a material change in performance from my local testing.

Thanks,
RObert

[leviw_travelin_and_unemployed, 2015-05-22 20:52:02]

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
File Source/core/layout/FloatingObjects.h (right):

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.h:113: unsigned
m_isLowestNonOverhangingFloatInChild : 1;
Nit: I'd put this above the assertion-only member.

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.h:198: FloatingObject* m_lowestFloatObject;
m_lowestFloatingObject?

Would it maybe make sense to roll this into the lowest float bottom cache?
(Understanding of course we'd have to look at both values to answer the
question).

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.cpp:510: void
LayoutBlockFlow::addLowestFloatFromChildren(LayoutBlockFlow* block)
This is a little weird, as this will bubble up the tree from parent to parent,
right? So while we just got the value from the child, it could actually be in an
arbitrary descendant.

[rhogan, 2015-05-23 11:56:51]

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
File Source/core/layout/FloatingObjects.h (right):

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.h:198: FloatingObject* m_lowestFloatObject;
On 2015/05/22 at 20:52:01, leviw wrote:
> m_lowestFloatingObject?
> 
> Would it maybe make sense to roll this into the lowest float bottom cache?
(Understanding of course we'd have to look at both values to answer the
question).

Yes, I think it might. I'll try it and see.

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.cpp:510: void
LayoutBlockFlow::addLowestFloatFromChildren(LayoutBlockFlow* block)
On 2015/05/22 at 20:52:02, leviw wrote:
> This is a little weird, as this will bubble up the tree from parent to parent,
right? So while we just got the value from the child, it could actually be in an
arbitrary descendant.

Exactly, which would be the case with a genuinely overhanging float in an
arbitrary descendant already.

[rhogan, 2015-05-23 12:12:09]

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
File Source/core/layout/FloatingObjects.h (right):

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.h:198: FloatingObject* m_lowestFloatObject;
On 2015/05/23 at 11:56:51, rhogan wrote:
> On 2015/05/22 at 20:52:01, leviw wrote:
> > m_lowestFloatingObject?
> > 
> > Would it maybe make sense to roll this into the lowest float bottom cache?
(Understanding of course we'd have to look at both values to answer the
question).
> 
> Yes, I think it might. I'll try it and see.

It doesn't fit well with m_lowestFloatBottomCache because the latter cares about
left/right floats and we don't. So I'll just leave it out by itself I think.

[leviw_travelin_and_unemployed, 2015-05-26 23:15:26]

On 2015/05/23 at 12:12:09, robhogan wrote:
>
https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
> File Source/core/layout/FloatingObjects.h (right):
> 
>
https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
> Source/core/layout/FloatingObjects.h:198: FloatingObject* m_lowestFloatObject;
> On 2015/05/23 at 11:56:51, rhogan wrote:
> > On 2015/05/22 at 20:52:01, leviw wrote:
> > > m_lowestFloatingObject?
> > > 
> > > Would it maybe make sense to roll this into the lowest float bottom cache?
(Understanding of course we'd have to look at both values to answer the
question).
> > 
> > Yes, I think it might. I'll try it and see.
> 
> It doesn't fit well with m_lowestFloatBottomCache because the latter cares
about left/right floats and we don't. So I'll just leave it out by itself I
think.

It felt to me like we'd just have to keep two pointers in the cache and look at
both to answer the question of what was the lowest. Does that seem unreasonable
or worse?

[leviw_travelin_and_unemployed, 2015-05-26 23:15:35]

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.cpp:510: void
LayoutBlockFlow::addLowestFloatFromChildren(LayoutBlockFlow* block)
On 2015/05/23 11:56:51, rhogan wrote:
> On 2015/05/22 at 20:52:02, leviw wrote:
> > This is a little weird, as this will bubble up the tree from parent to
parent,
> right? So while we just got the value from the child, it could actually be in
an
> arbitrary descendant.
> 
> Exactly, which would be the case with a genuinely overhanging float in an
> arbitrary descendant already.

I guess I was trying to suggest that we should name this something different to
communicate that this value, while currently being accumulated form the
children, was potentially from an arbitrary descendant.

[rhogan, 2015-05-27 18:06:47]

On 2015/05/26 at 23:15:26, leviw wrote:
> On 2015/05/23 at 12:12:09, robhogan wrote:
> >
https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
> > File Source/core/layout/FloatingObjects.h (right):
> > 
> >
https://codereview.chromium.org/1106533002/diff/120001/Source/core/layout/Flo...
> > Source/core/layout/FloatingObjects.h:198: FloatingObject*
m_lowestFloatObject;
> > On 2015/05/23 at 11:56:51, rhogan wrote:
> > > On 2015/05/22 at 20:52:01, leviw wrote:
> > > > m_lowestFloatingObject?
> > > > 
> > > > Would it maybe make sense to roll this into the lowest float bottom
cache? (Understanding of course we'd have to look at both values to answer the
question).
> > > 
> > > Yes, I think it might. I'll try it and see.
> > 
> > It doesn't fit well with m_lowestFloatBottomCache because the latter cares
about left/right floats and we don't. So I'll just leave it out by itself I
think.
> 
> It felt to me like we'd just have to keep two pointers in the cache and look
at both to answer the question of what was the lowest. Does that seem
unreasonable or worse?

It means keeping around two pointers rather than one, and figuring out which has
the lowest float when we want to decide which one to pass up the chain. So I
think it requires more work and storage than keeping and storing the lowest
float, regardless of type.

[leviw_travelin_and_unemployed, 2015-05-27 18:51:58]

It'd mean storing two pointers as opposed to 2 LayoutUnits and a pointer, right?

https://codereview.chromium.org/1106533002/diff/140001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1106533002/diff/140001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.cpp:522: LayoutSize offset =
isHorizontalWritingMode() ? LayoutSize(-childLogicalLeft, -childLogicalTop) :
LayoutSize(-childLogicalTop, -childLogicalLeft);
This code is kind of confusing. I think it would be clearer if you took the
block's location, transposed it if necessary for its own !horizontalWritingMode,
then transposed it once more for !this->horizontalWritingMode().

[rhogan, 2015-06-02 17:52:29]

On 2015/05/27 at 18:51:58, leviw wrote:
> It'd mean storing two pointers as opposed to 2 LayoutUnits and a pointer,
right?
Ah I get you now - done.

>
https://codereview.chromium.org/1106533002/diff/140001/Source/core/layout/Lay...
> Source/core/layout/LayoutBlockFlow.cpp:522: LayoutSize offset =
isHorizontalWritingMode() ? LayoutSize(-childLogicalLeft, -childLogicalTop) :
LayoutSize(-childLogicalTop, -childLogicalLeft);
> This code is kind of confusing. I think it would be clearer if you took the
block's location, transposed it if necessary for its own !horizontalWritingMode,
then transposed it once more for !this->horizontalWritingMode().

Updated per your suggestion.

[leviw_travelin_and_unemployed, 2015-06-02 23:05:41]

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
File Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.cpp:239: FloatingObject* floatingObject =
nullptr;
Nit: don't need to pull this out of the loop

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.cpp:293: if (lowestFloatBottomLeft >
lowestFloatBottomRight)
return max(lowestFloatBottomLeft, lowestFloatBottomRight);?

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
File Source/core/layout/FloatingObjects.h (right):

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.h:99: FloatingObject(LayoutBox*, Type, const
LayoutRect&, bool shouldPaint, bool isDescendant, bool
isLowestNonOverhangingFloatInChild);
It makes me a bit sad this adds an additional boolean parameter.

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.cpp:524:
newFloatingObject->setIsLowestNonOverhangingFloatInChild(true);
If new floats get added, it seems possible for the
lowestNonOverhangingFloatInChild bit to get stale. Are you sure that can't
happen?

[leviw_travelin_and_unemployed, 2015-06-02 23:05:41]

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
File Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.cpp:239: FloatingObject* floatingObject =
nullptr;
Nit: don't need to pull this out of the loop

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.cpp:293: if (lowestFloatBottomLeft >
lowestFloatBottomRight)
return max(lowestFloatBottomLeft, lowestFloatBottomRight);?

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
File Source/core/layout/FloatingObjects.h (right):

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.h:99: FloatingObject(LayoutBox*, Type, const
LayoutRect&, bool shouldPaint, bool isDescendant, bool
isLowestNonOverhangingFloatInChild);
It makes me a bit sad this adds an additional boolean parameter.

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.cpp:524:
newFloatingObject->setIsLowestNonOverhangingFloatInChild(true);
If new floats get added, it seems possible for the
lowestNonOverhangingFloatInChild bit to get stale. Are you sure that can't
happen?

[rhogan, 2015-06-03 20:30:55]

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
File Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.cpp:293: if (lowestFloatBottomLeft >
lowestFloatBottomRight)
On 2015/06/02 at 23:05:41, leviw wrote:
> return max(lowestFloatBottomLeft, lowestFloatBottomRight);?

Look again, I'm returning the object not the value! :)

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1106533002/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.cpp:524:
newFloatingObject->setIsLowestNonOverhangingFloatInChild(true);
On 2015/06/02 at 23:05:41, leviw wrote:
> If new floats get added, it seems possible for the
lowestNonOverhangingFloatInChild bit to get stale. Are you sure that can't
happen?

m_floatingObjects gets cleared down at the start of layoutBlockFlow() by
rebuildFloatsFromIntruding() so it won't get stale that way and this is the last
place where floats are added to the block's float list. So the info is good
until the next layout I believe.

[leviw_travelin_and_unemployed, 2015-06-03 22:36:21]

Thanks for clearing that up :) LGTM!

[rhogan, 2015-06-07 18:17:56]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-06-07 18:17:57]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1106533002/#ps180001
(title: "Updated")

[commit-bot: I haz the power, 2015-06-07 18:18:06]

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

[rhogan, 2015-06-07 18:18:10]

The CQ bit was unchecked by robhogan@gmail.com

[rhogan, 2015-06-07 18:18:28]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-06-07 18:18:39]

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

[commit-bot: I haz the power, 2015-06-07 18:26:03]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196633