Avoid scrollbar construction/destruction thrashing during flex layout.

Avoid scrollbar construction/destruction thrashing during flex layout.

BUG=528940
R=skobes@chromium.org,pdr@chromium.org

Before this change:

https://codereview.chromium.org/1295933003

... blocks with overflow:auto would delay updating their scrollbars until
after all flex layout was finished.  After the change, scrollbar info is
updated immediately during the course of layout.  Flex items may run
layout multiple times during flex layout; if a flex item has auto scrollbars,
it may create and destroy its scrollbars multiple times.

Aside from being a performance problem, this can cause WebScrollbarThemePainter
to point to a stale Scrollbar object.  If a flex item has scrollbars prior to
layout; then the flex item destroys and creates scrollbars during layout; and
at the end of layout, it still has a scrollbar; then
CompositedDeprecatedPaintLayerMapping::updateOverflowControlsLayers will not
update the WebScrollbarThemePainter with the final Scrollbar object.

We could fix this in updateOverflowControlsLayers, but that wouldn't address
the performance issue of needlessly creating and destroying scrollbars during
flex layout.  This patch avoids destroying scrollbars that are no longer
deemed necessary, until after all flexing is finished.

Committed: https://crrev.com/662a21f7bbebca99b8d968a8104da7fb53af820c
Cr-Commit-Position: refs/heads/master@{#351130}

[szager1, 2015-09-23 22:00:38]

Post-blink-merge, migrated this from:

https://codereview.chromium.org/1351743002/

[szager1, 2015-09-24 18:59:18]

Same logic, but use "attach/detach" rather than suppress.  PTAL.

[skobes, 2015-09-24 20:17:55]

On 2015/09/24 18:59:18, szager1 wrote:
> Same logic, but use "attach/detach" rather than suppress.  PTAL.

Were you able to get any performance numbers?

[szager1, 2015-09-25 19:00:30]

On 2015/09/24 20:17:55, skobes wrote:
> On 2015/09/24 18:59:18, szager1 wrote:
> > Same logic, but use "attach/detach" rather than suppress.  PTAL.
> 
> Were you able to get any performance numbers?

I ran perf numbers over night; there is no discernible difference.

I spent some time looking at fixing this in ScrollingCoordinator; it would
require plumbing it into WebCompositorSupport and into cc, which feels kind of
invasive.

[skobes, 2015-09-26 01:57:06]

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp
(right):

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:655:
m_scrollbarManager.setCanDetachScrollbars(false);
Do we need to update / reconstruct the WebScrollbarLayer in this case?

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:1035:
if (!hasScrollbar && !layerForVerticalScrollbar())
I wonder if ScrollbarManager should own the logic here and in the methods below
for repainting / positioning / hit testing scrollbars.

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:1487:
m_hBarIsAttached = 0;
Don't we need to do something to actually detach the scrollbar here?

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:1496:
// This doesn't hit in any tests, but since the equivalent code in
setHasVerticalScrollbar
Copy/paste error on this comment?

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h
(right):

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h:88:
void setCanDetachScrollbars(bool);
Add a comment here that documents what this api does (from the caller's
perspective).

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h:107:
unsigned m_canDetachScrollbars: 1;
Is there a reason to prefer unsigned:1 to bool?

[szager1, 2015-09-26 04:48:32]

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp
(right):

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:655:
m_scrollbarManager.setCanDetachScrollbars(false);
On 2015/09/26 01:57:06, skobes wrote:
> Do we need to update / reconstruct the WebScrollbarLayer in this case?

No, needsScrollbarReconstruction() returns true only when at least one of the
scrollbars is custom, which means that scrolling will be done on the main thread
with LayoutScrollbar.  So, the WebScrollbarLayer will get destroyed when
CompositedDeprecatedPaintLayerMapping::updateOverflowControlsLayers runs after
layout.

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:1035:
if (!hasScrollbar && !layerForVerticalScrollbar())
On 2015/09/26 01:57:06, skobes wrote:
> I wonder if ScrollbarManager should own the logic here and in the methods
below
> for repainting / positioning / hit testing scrollbars.

I dunno, I envisioned ScrollbarManager as just responsible for the life cycle of
the scrollbars, but I'm not against moving more scrollbar functionality there. 
Would you prefer that?

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:1487:
m_hBarIsAttached = 0;
On 2015/09/26 01:57:06, skobes wrote:
> Don't we need to do something to actually detach the scrollbar here?  

No, this is sufficient.  Setting m_hBarIsAttached to 0 will cause
horizontalScrollbar() to return nullptr, and everything else relies on that,
including hasHorizontalScrollbar().

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:1496:
// This doesn't hit in any tests, but since the equivalent code in
setHasVerticalScrollbar
On 2015/09/26 01:57:06, skobes wrote:
> Copy/paste error on this comment?

Yes, fixed.

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h
(right):

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h:88:
void setCanDetachScrollbars(bool);
On 2015/09/26 01:57:06, skobes wrote:
> Add a comment here that documents what this api does (from the caller's
> perspective).

Done.

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h:107:
unsigned m_canDetachScrollbars: 1;
On 2015/09/26 01:57:06, skobes wrote:
> Is there a reason to prefer unsigned:1 to bool?

No reason, just following convention.

[szager1, 2015-09-26 04:53:26]

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h
(right):

https://codereview.chromium.org/1357423007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h:107:
unsigned m_canDetachScrollbars: 1;
On 2015/09/26 01:57:06, skobes wrote:
> Is there a reason to prefer unsigned:1 to bool?

Actually, after reading some StackOverflow, I retract my previous comment.  bool
member variables are always assigned 8 bits, because they must be
byte-addressable.  But integer types with a specified number of bits are not
byte-addressable, and they are packed efficiently.

[skobes, 2015-09-28 18:01:40]

lgtm

[szager1, 2015-09-28 18:37:53]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2015-09-28 18:42:53]

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

[commit-bot: I haz the power, 2015-09-28 20:49:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-28 20:50:07]

Patchset 4 (id:??) landed as
https://crrev.com/662a21f7bbebca99b8d968a8104da7fb53af820c
Cr-Commit-Position: refs/heads/master@{#351130}

[Lei Zhang, 2015-09-29 01:13:11]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1370373003/ by thestig@chromium.org.

The reason for reverting is: Broke oilpan:

../../third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h:71:5:
error: [blink-gc] Class 'ScrollbarManager' contains invalid fields.
    class ScrollbarManager {
    ^
../../third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h:110:9:
note: [blink-gc] Member field 'm_hBar' in unmanaged class declared here:
        RefPtrWillBeMember<Scrollbar> m_hBar;
        ^
../../third_party/WebKit/Source/platform/heap/Handle.h:919:28: note: expanded
from macro 'RefPtrWillBeMember'
#define RefPtrWillBeMember blink::Member
                           ^
../../third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h:111:9:
note: [blink-gc] Member field 'm_vBar' in unmanaged class declared here:
        RefPtrWillBeMember<Scrollbar> m_vBar;
        ^
../../third_party/WebKit/Source/platform/heap/Handle.h:919:28: note: expanded
from macro 'RefPtrWillBeMember'
#define RefPtrWillBeMember blink::Member
                           ^
1 error generated..