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

Issue 1370373003: Revert of Avoid scrollbar construction/destruction thrashing during flex layout. (Closed)

Created:
5 years, 2 months ago by Lei Zhang
Modified:
5 years, 2 months ago
Reviewers:
pdr., skobes, szager1
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of Avoid scrollbar construction/destruction thrashing during flex layout. (patchset #4 id:60001 of https://codereview.chromium.org/1357423007/ ) Reason for revert: 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. Original issue's 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. > > Committed: https://crrev.com/662a21f7bbebca99b8d968a8104da7fb53af820c > Cr-Commit-Position: refs/heads/master@{#351130} TBR=pdr@chromium.org,skobes@chromium.org,szager@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=528940 Committed: https://crrev.com/1a157eb11dda435aefcad65d3e49ecc36112c02b Cr-Commit-Position: refs/heads/master@{#351223}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+121 lines, -223 lines) Patch
M third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.h View 5 chunks +10 lines, -53 lines 0 comments Download
M third_party/WebKit/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp View 15 chunks +111 lines, -170 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Lei Zhang
Created Revert of Avoid scrollbar construction/destruction thrashing during flex layout.
5 years, 2 months ago (2015-09-29 01:13:11 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1370373003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1370373003/1
5 years, 2 months ago (2015-09-29 01:13:54 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 2 months ago (2015-09-29 01:15:37 UTC) #3
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/1a157eb11dda435aefcad65d3e49ecc36112c02b Cr-Commit-Position: refs/heads/master@{#351223}
5 years, 2 months ago (2015-09-29 01:16:31 UTC) #4
Lei Zhang
So I reverted because oilpan bots were on the main waterfall and they were red. ...
5 years, 2 months ago (2015-09-29 03:06:26 UTC) #5
szager1
OK, thanks for letting me know. On Mon, Sep 28, 2015 at 8:06 PM <thestig@chromium.org> ...
5 years, 2 months ago (2015-09-29 04:10:54 UTC) #6
Lei Zhang
5 years, 2 months ago (2015-09-29 08:45:08 UTC) #7
Message was sent while issue was closed.
But since you know this CL breaks oilpan, please try to fix it rather than
reverting the revert.

Powered by Google App Engine
This is Rietveld 408576698