Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(97)

Issue 1351743002: Avoid scrollbar construction/destruction thrashing during flex layout. (Closed)

Created:
5 years, 3 months ago by szager1
Modified:
5 years, 3 months ago
Reviewers:
pdr., skobes
CC:
blink-reviews, dshwang, slimming-paint-reviews_chromium.org, blink-reviews-paint_chromium.org, ojan
Base URL:
https://chromium.googlesource.com/chromium/blink@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : Fix suppression behavior. #

Patch Set 4 : Add ScrollbarManager #

Unified diffs Side-by-side diffs Delta from patch set Stats (+225 lines, -120 lines) Patch
M LayoutTests/fast/repaint/destroy-scrollbar-expected.txt View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/paint/DeprecatedPaintLayerScrollableArea.h View 1 2 3 5 chunks +48 lines, -10 lines 0 comments Download
M Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp View 1 2 3 15 chunks +175 lines, -108 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
szager1
5 years, 3 months ago (2015-09-16 23:37:47 UTC) #1
skobes
I'm not a big fan of this change. It's confusing to have two different ways ...
5 years, 3 months ago (2015-09-17 00:01:16 UTC) #2
szager1
On 2015/09/17 00:01:16, skobes wrote: > I'm not a big fan of this change. It's ...
5 years, 3 months ago (2015-09-17 00:20:46 UTC) #3
szager1
> > That doesn't work, because we need to have constructed scrollbars so we can ...
5 years, 3 months ago (2015-09-17 00:21:54 UTC) #4
szager1
On 2015/09/17 00:01:16, skobes wrote: > I'm not a big fan of this change. It's ...
5 years, 3 months ago (2015-09-17 00:26:52 UTC) #5
skobes
On 2015/09/17 00:20:46, szager1 wrote: > On 2015/09/17 00:01:16, skobes wrote: > > I'm not ...
5 years, 3 months ago (2015-09-17 00:29:52 UTC) #6
skobes
On 2015/09/17 00:29:52, skobes wrote: > On 2015/09/17 00:20:46, szager1 wrote: > > On 2015/09/17 ...
5 years, 3 months ago (2015-09-17 00:33:59 UTC) #7
szager1
On 2015/09/17 00:33:59, skobes wrote: > FrameView has a multi pass update process for scrollbar ...
5 years, 3 months ago (2015-09-21 21:21:16 UTC) #8
szager1
FYI, here is a version of the patch that passes tests. I'm now working on ...
5 years, 3 months ago (2015-09-23 00:52:49 UTC) #9
szager1
The latest patch adds a ScrollbarManager class nested inside DPLSA, which handles the "suppress" functionality. ...
5 years, 3 months ago (2015-09-23 05:55:37 UTC) #10
skobes
On 2015/09/23 05:55:37, szager1 wrote: > The latest patch adds a ScrollbarManager class nested inside ...
5 years, 3 months ago (2015-09-23 16:01:30 UTC) #11
szager1
On 2015/09/23 16:01:30, skobes wrote: > On 2015/09/23 05:55:37, szager1 wrote: > > The latest ...
5 years, 3 months ago (2015-09-23 16:49:22 UTC) #12
szager1
5 years, 3 months ago (2015-09-23 22:00:08 UTC) #13
Post-blink-merge, migrating this to:

https://codereview.chromium.org/1357423007

Powered by Google App Engine
This is Rietveld 408576698