Don't invalidate width for children unless scrollbars have changed.

Don't invalidate width for children unless scrollbars have changed.

Previous to this patch, PaintLayerScrollableArea::updateAfterLayout
would call box().scrollbarsChanged(), even if relayout was delayed.
The result is that the box would sometimes force relayout its
children even when the box'es final state had the same logical width
as before.

With this patch, box().scrollbarsChanged() is only called when the
final state of scrollbars differs from the initial state.

BUG=621644
R=cbiesinger@chromium.org,eae@chromium.org

Committed: https://crrev.com/96c111dfbb085888c163c987375ff854b4619c5e
Cr-Commit-Position: refs/heads/master@{#401745}

[eae, 2016-06-23 07:47:27]

OK, I'd like skobes or cbiesinger to take a look as well though as they've both
touched this code more recently than I have.

LGTM

[cbiesinger, 2016-06-23 17:58:15]

lgtm. maybe we should also add a PerformanceTest here?
https://cs.chromium.org/chromium/src/third_party/WebKit/PerformanceTests/Layout/

[skobes, 2016-06-23 18:19:17]

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

[skobes, 2016-06-23 18:19:18]

PLSA is getting crazier and crazier.  At some point we need to step back and
think more holistically about how to solve these problems.  Hopefully LayoutNG
gives us a path forward.

https://codereview.chromium.org/2088323003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):

https://codereview.chromium.org/2088323003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:178: static void
setBoxNeedsLayout(PaintLayerScrollableArea&, bool hadHorizontalScrollbar, bool
hadVerticalScrollbar);
This API is super confusing, if the box needs layout why does it matter whether
we had scrollbars?  Should it be named notifyScrollbarsChanged or similar?

https://codereview.chromium.org/2088323003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:415: bool
hadHorizontalScrollbarBeforeRelayout() const { return
m_hadHorizontalScrollbarBeforeRelayout; }
Do these need to be public?  Add comments please.

https://codereview.chromium.org/2088323003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:497: unsigned
m_needsRelayout : 1;
Please add comments explaining what this means.  How is it different from
box().needsLayout()?

[szager1, 2016-06-23 19:05:31]

https://codereview.chromium.org/2088323003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):

https://codereview.chromium.org/2088323003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:178: static void
setBoxNeedsLayout(PaintLayerScrollableArea&, bool hadHorizontalScrollbar, bool
hadVerticalScrollbar);
On 2016/06/23 18:19:18, skobes wrote:
> This API is super confusing, if the box needs layout why does it matter
whether
> we had scrollbars?  Should it be named notifyScrollbarsChanged or similar?

There are two things going on here:

- Does the box need relayout?
- If so, should the box force relayout of its children?

setBoxNeedsLayout affirms the first condition, i.e., the box definitely needs
relayout.  The second question can't be answered until we know the final state
of scrollbars, which won't be until s_count drops to zero.

I added to the class comment, hopefully that helps.  This is indeed pretty
gross, although I do think these classes do a good job of hiding the grossness
from LayoutFlexibleBox, which has enough of its own grossness already.

I share your optimism about layout ng.

https://codereview.chromium.org/2088323003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:415: bool
hadHorizontalScrollbarBeforeRelayout() const { return
m_hadHorizontalScrollbarBeforeRelayout; }
On 2016/06/23 18:19:18, skobes wrote:
> Do these need to be public?  Add comments please.

I could use a friend declaration to give PreventRelayoutScope access to private
members, but that's not a common idiom in blink code.

Comments added.

https://codereview.chromium.org/2088323003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:497: unsigned
m_needsRelayout : 1;
On 2016/06/23 18:19:18, skobes wrote:
> Please add comments explaining what this means.  How is it different from
> box().needsLayout()?

I added comments to the getters/setters.

[skobes, 2016-06-23 19:08:22]

lgtm

https://codereview.chromium.org/2088323003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):

https://codereview.chromium.org/2088323003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:174: // will be
called as appropriate for all marked PLSA's.
thanks, this helps a lot

[szager1, 2016-06-23 20:28:14]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-06-23 20:28:14]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
cbiesinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2088323003/#ps80001
(title: "nits")

[commit-bot: I haz the power, 2016-06-23 20:29:35]

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

[commit-bot: I haz the power, 2016-06-23 22:35:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-23 22:36:53]

Description was changed from

==========
Don't invalidate width for children unless scrollbars have changed.

Previous to this patch, PaintLayerScrollableArea::updateAfterLayout
would call box().scrollbarsChanged(), even if relayout was delayed.
The result is that the box would sometimes force relayout its
children even when the box'es final state had the same logical width
as before.

With this patch, box().scrollbarsChanged() is only called when the
final state of scrollbars differs from the initial state.

BUG=621644
R=cbiesinger@chromium.org,eae@chromium.org
==========

to

==========
Don't invalidate width for children unless scrollbars have changed.

Previous to this patch, PaintLayerScrollableArea::updateAfterLayout
would call box().scrollbarsChanged(), even if relayout was delayed.
The result is that the box would sometimes force relayout its
children even when the box'es final state had the same logical width
as before.

With this patch, box().scrollbarsChanged() is only called when the
final state of scrollbars differs from the initial state.

BUG=621644
R=cbiesinger@chromium.org,eae@chromium.org

Committed: https://crrev.com/96c111dfbb085888c163c987375ff854b4619c5e
Cr-Commit-Position: refs/heads/master@{#401745}
==========

[commit-bot: I haz the power, 2016-06-23 22:36:54]

Patchset 5 (id:??) landed as
https://crrev.com/96c111dfbb085888c163c987375ff854b4619c5e
Cr-Commit-Position: refs/heads/master@{#401745}