Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2736523002:
Defer ChromeClient::attachRootGraphicsLayer until after compositing update. (Closed)
|
Created:
3 years, 9 months ago by skobes Modified:
3 years, 9 months ago CC:
chromium-reviews, pdr+renderingwatchlist_chromium.org, zoltan1, blink-reviews-layout_chromium.org, szager+layoutwatch_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews, kinuko+watch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDefer ChromeClient::attachRootGraphicsLayer until after compositing update.
This paves the way for PLC::rootGraphicsLayer to read from the LayoutView's
CompositedLayerMapping in root layer scrolling mode.
BUG=698464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2736523002
Cr-Commit-Position: refs/heads/master@{#455325}
Committed: https://chromium.googlesource.com/chromium/src/+/115e9610c49a6cc1970918a17bba42ba392de0d6
Patch Set 1 #
Total comments: 2
Patch Set 2 : use updateLifecycleToCompositingCleanPlusScrolling #
Total comments: 4
Patch Set 3 : fold m_pendingChromeClientAttachment into m_rootLayerAttachment #Patch Set 4 : rebase #
Dependent Patchsets: Messages
Total messages: 32 (22 generated)
Description was changed from ========== Defer ChromeClient::attachRootGraphicsLayer until after the compositing update. BUG=698464 ========== to ========== Defer ChromeClient::attachRootGraphicsLayer until after the compositing update. BUG=698464 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Defer ChromeClient::attachRootGraphicsLayer until after the compositing update. BUG=698464 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Defer ChromeClient::attachRootGraphicsLayer until after the compositing update. This paves the way for PLC::rootGraphicsLayer to read from the LayoutView's CompositedLayerMapping in root layer scrolling mode. BUG=698464 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Defer ChromeClient::attachRootGraphicsLayer until after the compositing update. This paves the way for PLC::rootGraphicsLayer to read from the LayoutView's CompositedLayerMapping in root layer scrolling mode. BUG=698464 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Defer ChromeClient::attachRootGraphicsLayer until after compositing update. This paves the way for PLC::rootGraphicsLayer to read from the LayoutView's CompositedLayerMapping in root layer scrolling mode. BUG=698464 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
skobes@chromium.org changed reviewers: + bokan@chromium.org, szager@chromium.org
https://codereview.chromium.org/2736523002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2736523002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:2002: updatePageOverlays(); I think there's ways for compositing to be run without going through WVI::updateAllLifecyclePhases, e.g FrameView::updateLifecycleToCompositingCleanPlusScrolling is called from GestureManager::handleGestureTap, is that ok?
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2736523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): https://codereview.chromium.org/2736523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1241: m_pendingChromeClientAttachment = true; Is this new variable necessary? Can't you just call attachRootLayer only from updateIfNeeded?
https://codereview.chromium.org/2736523002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2736523002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:2002: updatePageOverlays(); On 2017/03/06 16:17:18, bokan wrote: > I think there's ways for compositing to be run without going through > WVI::updateAllLifecyclePhases, e.g > FrameView::updateLifecycleToCompositingCleanPlusScrolling is called from > GestureManager::handleGestureTap, is that ok? Done, although doing this from WebViewImpl::layoutUpdated hits a re-entrancy assertion in FrameView::updateLifecyclePhasesInternal. It looks like it's sufficient to call it only from setPageOverlayColor, since this ensures subsequent invocations of PageOverlay::update will see the root GraphicsLayer. https://codereview.chromium.org/2736523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): https://codereview.chromium.org/2736523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1241: m_pendingChromeClientAttachment = true; On 2017/03/06 22:23:34, szager1 wrote: > Is this new variable necessary? Can't you just call attachRootLayer only from > updateIfNeeded? Not easily - attachRootLayer updates m_rootLayerAttachment, and in the iframe case it calls setNeedsCompositingUpdate. Those things should happen immediately and not be deferred.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2736523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): https://codereview.chromium.org/2736523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1241: m_pendingChromeClientAttachment = true; On 2017/03/06 22:44:23, skobes wrote: > On 2017/03/06 22:23:34, szager1 wrote: > > Is this new variable necessary? Can't you just call attachRootLayer only from > > updateIfNeeded? > > Not easily - attachRootLayer updates m_rootLayerAttachment, and in the iframe > case it calls setNeedsCompositingUpdate. Those things should happen immediately > and not be deferred. Could you add a RootLayerPendingAttachment value to the RootLayerAttachment enum? It looks like, aside from attachRootLayer(), the only places in the code that look at m_rootLayerAttachment are only interested in whether or not it's RootLayerUnattached.
lgtm
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2736523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): https://codereview.chromium.org/2736523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1241: m_pendingChromeClientAttachment = true; On 2017/03/06 22:57:58, szager1 wrote: > On 2017/03/06 22:44:23, skobes wrote: > > On 2017/03/06 22:23:34, szager1 wrote: > > > Is this new variable necessary? Can't you just call attachRootLayer only > from > > > updateIfNeeded? > > > > Not easily - attachRootLayer updates m_rootLayerAttachment, and in the iframe > > case it calls setNeedsCompositingUpdate. Those things should happen > immediately > > and not be deferred. > > Could you add a RootLayerPendingAttachment value to the RootLayerAttachment > enum? It looks like, aside from attachRootLayer(), the only places in the code > that look at m_rootLayerAttachment are only interested in whether or not it's > RootLayerUnattached. Done, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/07 23:05:46, skobes wrote: > https://codereview.chromium.org/2736523002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp > (right): > > https://codereview.chromium.org/2736523002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1241: > m_pendingChromeClientAttachment = true; > On 2017/03/06 22:57:58, szager1 wrote: > > On 2017/03/06 22:44:23, skobes wrote: > > > On 2017/03/06 22:23:34, szager1 wrote: > > > > Is this new variable necessary? Can't you just call attachRootLayer only > > from > > > > updateIfNeeded? > > > > > > Not easily - attachRootLayer updates m_rootLayerAttachment, and in the > iframe > > > case it calls setNeedsCompositingUpdate. Those things should happen > > immediately > > > and not be deferred. > > > > Could you add a RootLayerPendingAttachment value to the RootLayerAttachment > > enum? It looks like, aside from attachRootLayer(), the only places in the > code > > that look at m_rootLayerAttachment are only interested in whether or not it's > > RootLayerUnattached. > > Done, PTAL. MUCH better, thanks. lgtm
The CQ bit was checked by skobes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2736523002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488935836330730,
"parent_rev": "003796caeb8de246a1d5719f439f25f26ca7f4f1", "commit_rev":
"115e9610c49a6cc1970918a17bba42ba392de0d6"}
Message was sent while issue was closed.
Description was changed from ========== Defer ChromeClient::attachRootGraphicsLayer until after compositing update. This paves the way for PLC::rootGraphicsLayer to read from the LayoutView's CompositedLayerMapping in root layer scrolling mode. BUG=698464 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Defer ChromeClient::attachRootGraphicsLayer until after compositing update. This paves the way for PLC::rootGraphicsLayer to read from the LayoutView's CompositedLayerMapping in root layer scrolling mode. BUG=698464 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2736523002 Cr-Commit-Position: refs/heads/master@{#455325} Committed: https://chromium.googlesource.com/chromium/src/+/115e9610c49a6cc1970918a17bba... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/115e9610c49a6cc1970918a17bba... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
