|
|
Created:
3 years, 9 months ago by skobes Modified:
3 years, 9 months ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews, blink-reviews-frames_chromium.org, kinuko+watch, cbiesinger Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[RLS] Don't create PaintLayerCompositor layers.
In root layer scrolling mode, the main GraphicsLayer in the LayoutView's
CompositedLayerMapping is now connected directly to the VisualViewport scroll
layer (in the main frame), or to the layer of an owning LayoutIFrame.
BUG=698464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2728273002
Cr-Commit-Position: refs/heads/master@{#455954}
Committed: https://chromium.googlesource.com/chromium/src/+/13e9d344a202ab2d800c64f9c3f0beef605651d9
Patch Set 1 #
Total comments: 10
Patch Set 2 : add DCHECK, remove dead branch #Patch Set 3 : address szager's comments #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : rebase #Patch Set 7 : rebase #Messages
Total messages: 60 (49 generated)
Description was changed from ========== [RLS] Don't create PaintLayerCompositor layers. BUG=698464 ========== to ========== [RLS] Don't create PaintLayerCompositor layers. 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...
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
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...
Patchset #1 (id:40001) has been deleted
skobes@chromium.org changed reviewers: + bokan@chromium.org, chrishtr@chromium.org, szager@chromium.org
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...
Patchset #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (left): https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1186: if (!m_rootContentLayer) It seems like the existence of rootContentLayer was previously used to tell if the PLC has been initialised. Was this just an optimisation that doesn't affect correctness in all these cases? Does the PLC under RLS no longer maintain any real state that we don't have to ensure it's been initialised? https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:362: return isMainFrame() ? visualViewport().scrollLayer() : nullptr; Is it true that !rootContentLayer implies rootLayerScrolling? If so, please add a DCHECK. https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1274: else if (m_rootContentLayer) Just some questions for my own benefit: how does this work for the non-RLS case? When is there a rootContentLayer but no overflowControlsHostLayer? If there's no overflowControlsHostLayer, does this just disconnect the RootContentLayer from the FrameScroll layer or is there no FrameScroll layer in that case either?
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/2728273002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1160: // TODO(skobes): When root layer scrolling is enabled, we should not even Remove this comment. https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1234: DisableCompositingQueryAsserts disabler; Why is this now necessary?
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/2728273002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (left): https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1186: if (!m_rootContentLayer) On 2017/03/06 15:42:30, bokan wrote: > It seems like the existence of rootContentLayer was previously used to tell if > the PLC has been initialised. Was this just an optimisation that doesn't affect > correctness in all these cases? Does the PLC under RLS no longer maintain any > real state that we don't have to ensure it's been initialised? PLC still tracks its state in m_rootLayerAttachment. I think the !m_rootContentLayer checks were unnecessary. https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:362: return isMainFrame() ? visualViewport().scrollLayer() : nullptr; On 2017/03/06 15:42:30, bokan wrote: > Is it true that !rootContentLayer implies rootLayerScrolling? If so, please add > a DCHECK. Done. https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1160: // TODO(skobes): When root layer scrolling is enabled, we should not even On 2017/03/06 22:07:47, szager1 wrote: > Remove this comment. Done. https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1234: DisableCompositingQueryAsserts disabler; On 2017/03/06 22:07:47, szager1 wrote: > Why is this now necessary? Done. https://codereview.chromium.org/2728273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1274: else if (m_rootContentLayer) On 2017/03/06 15:42:29, bokan wrote: > Just some questions for my own benefit: how does this work for the non-RLS case? > When is there a rootContentLayer but no overflowControlsHostLayer? If there's no > overflowControlsHostLayer, does this just disconnect the RootContentLayer from > the FrameScroll layer or is there no FrameScroll layer in that case either? Good question. It looks like this branch is a historical artifact - it's no longer possible to have m_rootContentLayer without m_overflowControlsHostLayer. Updated.
lgtm (though I'm not an expert in PLC so please wait for other reviewers)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== [RLS] Don't create PaintLayerCompositor layers. BUG=698464 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RLS] Don't create PaintLayerCompositor layers. In root layer scrolling mode, the main GraphicsLayer in the LayoutView's CompositedLayerMapping is now connected directly to the VisualViewport scroll layer (in the main frame), or to the layer of an owning LayoutIFrame. 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
szager: ping :)
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, szager@chromium.org Link to the patchset: https://codereview.chromium.org/2728273002/#ps200001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by skobes@chromium.org
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": 200001, "attempt_start_ts": 1489105927029730, "parent_rev": "589c696ecd13c722eb3b7eeef6eb43791978a687", "commit_rev": "13e9d344a202ab2d800c64f9c3f0beef605651d9"}
Message was sent while issue was closed.
Description was changed from ========== [RLS] Don't create PaintLayerCompositor layers. In root layer scrolling mode, the main GraphicsLayer in the LayoutView's CompositedLayerMapping is now connected directly to the VisualViewport scroll layer (in the main frame), or to the layer of an owning LayoutIFrame. BUG=698464 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RLS] Don't create PaintLayerCompositor layers. In root layer scrolling mode, the main GraphicsLayer in the LayoutView's CompositedLayerMapping is now connected directly to the VisualViewport scroll layer (in the main frame), or to the layer of an owning LayoutIFrame. BUG=698464 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2728273002 Cr-Commit-Position: refs/heads/master@{#455954} Committed: https://chromium.googlesource.com/chromium/src/+/13e9d344a202ab2d800c64f9c3f0... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001) as https://chromium.googlesource.com/chromium/src/+/13e9d344a202ab2d800c64f9c3f0... |