Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2113483002:
Make RootScroller set the outer viewport scroll layer in the compositor (Closed)
|
Created:
4 years, 5 months ago by bokan Modified:
4 years, 5 months ago CC:
aelias_OOO_until_Jul13, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, cc-bugs_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, dtapuska+blinkwatch_chromium.org, eae+blinkwatch, jam, kenneth.christiansen, kinuko+watch, mlamouri+watch-content_chromium.org, nzolghadr+blinkwatch_chromium.org, piman+watch_chromium.org, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake RootScroller set the outer viewport scroll layer in the compositor
This CL makes the outer viewport scroll layer in the compositor match the
RootScroller element's scroll layer. It adds a callback in RootScroller
that registers the viewport's layers each time compositing in the
FrameView is updated. The WebViewImpl::registerViewportLayersWithCompositor
method queries RootScroller for the layer to use for the outer viewport
scroll layer.
Without enabling the RootScroller API, this will always fall back to the
FrameView's scroll layer so it'll only have an effect with experimental
flags enabled.
BUG=505516
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/243c701b03206899b372d0b7898b49fc7a725704
Cr-Commit-Position: refs/heads/master@{#404890}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Rebase #Patch Set 3 : Addressed comments #Patch Set 4 : Plumb root scroller into compositor #Patch Set 5 : RootScroller changes outer viewport scroll layer #Dependent Patchsets: Messages
Total messages: 41 (18 generated)
Description was changed from ========== Add plumbing for setting a root scroller layer in the compositor This CL adds plumbing and a mirror implementation of the root scroller concept to the compositor. Blink already has the notion of a "root scroller", an element whose scrolling controls the URL bar and produces effects like overscroll glow. This CL adds a mirror RootScrollerController on the compositor side and adds plumbing to set the root scroller layer on the compositor to match the root scroller element set in Blink. This doesn't yet implement any of the root scroller API functionality on the compositor. The only functional change is that once a layer is set as the root scroller, scrolls won't chain past it. The root scroller API is currently not enabled in stable builds so this will only ever be the inner viewport scroll layer. BUG=505516 ========== to ========== Add plumbing for setting a root scroller layer in the compositor This CL adds plumbing and a mirror implementation of the root scroller concept to the compositor. Blink already has the notion of a "root scroller", an element whose scrolling controls the URL bar and produces effects like overscroll glow. This CL adds a mirror RootScrollerController on the compositor side and adds plumbing to set the root scroller layer on the compositor to match the root scroller element set in Blink. This doesn't yet implement any of the root scroller API functionality on the compositor. The only functional change is that once a layer is set as the root scroller, scrolls won't chain past it. The root scroller API is currently not enabled in stable builds so this will only ever be the inner viewport scroll layer. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Description was changed from ========== Add plumbing for setting a root scroller layer in the compositor This CL adds plumbing and a mirror implementation of the root scroller concept to the compositor. Blink already has the notion of a "root scroller", an element whose scrolling controls the URL bar and produces effects like overscroll glow. This CL adds a mirror RootScrollerController on the compositor side and adds plumbing to set the root scroller layer on the compositor to match the root scroller element set in Blink. This doesn't yet implement any of the root scroller API functionality on the compositor. The only functional change is that once a layer is set as the root scroller, scrolls won't chain past it. The root scroller API is currently not enabled in stable builds so this will only ever be the inner viewport scroll layer. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add plumbing for setting a root scroller layer in the compositor This CL adds plumbing and a mirror implementation of the root scroller concept to the compositor. Blink already has the notion of a "root scroller", an element whose scrolling controls the URL bar and produces effects like overscroll glow. This CL adds a mirror RootScrollerController on the compositor side and adds plumbing to set the root scroller layer on the compositor to match the root scroller element set in Blink. This doesn't yet implement any of the root scroller API functionality on the compositor. The only functional change is that once a layer is set as the root scroller, scrolls won't chain past it. The root scroller API is currently not enabled in stable builds so this will only ever be the inner viewport scroll layer. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
bokan@chromium.org changed reviewers: + aelias@chromium.org
ptal
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2113483002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2113483002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:1880: if (!page()->mainFrame() || !page()->mainFrame()->isLocalFrame()) I'm out of the loop on the root scroller work: is it possible to have a root scroller in a non-main frame?
https://codereview.chromium.org/2113483002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2113483002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:1880: if (!page()->mainFrame() || !page()->mainFrame()->isLocalFrame()) On 2016/06/29 22:34:29, dcheng wrote: > I'm out of the loop on the root scroller work: is it possible to have a root > scroller in a non-main frame? It is in the sense that an iframe can set document.rootScroller so the machinery is still there to handle that but it has no visible effect right now so I chose to simply not tell the compositor about it for remote frames.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
aelias@chromium.org changed reviewers: - dcheng@chromium.org
https://codereview.chromium.org/2113483002/diff/1/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/2113483002/diff/1/cc/trees/layer_tree_host.h#... cc/trees/layer_tree_host.h:216: void RegisterViewportLayers(scoped_refptr<Layer> overscroll_elasticity_layer, Can you make it an argument to this (and likewise down the commit flow)? We always end up regretting it later when related state isn't committed atomically. https://codereview.chromium.org/2113483002/diff/1/cc/trees/layer_tree_host.h#... cc/trees/layer_tree_host.h:218: scoped_refptr<Layer> inner_viewport_scroll_layer, I'm having trouble understanding the intended breakdown of what's tied to the root scroller and what to the inner viewport layer, could you lay it out? https://codereview.chromium.org/2113483002/diff/1/cc/trees/root_scroller_cont... File cc/trees/root_scroller_controller.h (right): https://codereview.chromium.org/2113483002/diff/1/cc/trees/root_scroller_cont... cc/trees/root_scroller_controller.h:28: LayerImpl* root_scroller_layer_; Please change this to an id, that avoids so many lifetime management headaches and we really should not be persistently storing raw LayerImpl pointers anywhere. https://codereview.chromium.org/2113483002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h (right): https://codereview.chromium.org/2113483002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h:115: bool m_changedSinceLastCompositingUpdate; I doubt that this dirty bit is really needed. Can you just compare every time to the old value (e.g. the one held by LayerTreeHost)?
https://codereview.chromium.org/2113483002/diff/1/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/2113483002/diff/1/cc/trees/layer_tree_host.h#... cc/trees/layer_tree_host.h:216: void RegisterViewportLayers(scoped_refptr<Layer> overscroll_elasticity_layer, On 2016/06/29 22:44:08, aelias wrote: > Can you make it an argument to this (and likewise down the commit flow)? We > always end up regretting it later when related state isn't committed atomically. Done. I moved registerViewportLayers in Blink out from VisualViewport and into WebViewImpl since it's really more general than VisualViewport. I've split that off into a separate patch. https://codereview.chromium.org/2113483002/diff/1/cc/trees/layer_tree_host.h#... cc/trees/layer_tree_host.h:218: scoped_refptr<Layer> inner_viewport_scroll_layer, On 2016/06/29 22:44:08, aelias wrote: > I'm having trouble understanding the intended breakdown of what's tied to the > root scroller and what to the inner viewport layer, could you lay it out? The root scroller is really just designating which layer is the outer viewport. In a future patch I'll add plumbing in cc::Viewport to swap the outer viewport when the root scroller changes. When a layer is designated as the root scroller, we shouldn't chain scrolls past it and scrolling it should move top controls and activate overscroll glow. We still want it to scroll as part of cc::Viewport and in tandem with the inner viewport since we still want to scroll inner first when pinch zoomed in. You can see the main thread version of this in https://codereview.chromium.org/2128553002/ which is about to land. https://codereview.chromium.org/2113483002/diff/1/cc/trees/root_scroller_cont... File cc/trees/root_scroller_controller.h (right): https://codereview.chromium.org/2113483002/diff/1/cc/trees/root_scroller_cont... cc/trees/root_scroller_controller.h:28: LayerImpl* root_scroller_layer_; On 2016/06/29 22:44:08, aelias wrote: > Please change this to an id, that avoids so many lifetime management headaches > and we really should not be persistently storing raw LayerImpl pointers > anywhere. Done. https://codereview.chromium.org/2113483002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h (right): https://codereview.chromium.org/2113483002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h:115: bool m_changedSinceLastCompositingUpdate; On 2016/06/29 22:44:08, aelias wrote: > I doubt that this dirty bit is really needed. Can you just compare every time > to the old value (e.g. the one held by LayerTreeHost)? Done.
Description was changed from ========== Add plumbing for setting a root scroller layer in the compositor This CL adds plumbing and a mirror implementation of the root scroller concept to the compositor. Blink already has the notion of a "root scroller", an element whose scrolling controls the URL bar and produces effects like overscroll glow. This CL adds a mirror RootScrollerController on the compositor side and adds plumbing to set the root scroller layer on the compositor to match the root scroller element set in Blink. This doesn't yet implement any of the root scroller API functionality on the compositor. The only functional change is that once a layer is set as the root scroller, scrolls won't chain past it. The root scroller API is currently not enabled in stable builds so this will only ever be the inner viewport scroll layer. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add plumbing for setting a root scroller layer in the compositor This CL adds plumbing and a mirror implementation of the root scroller concept to the compositor. Blink already has the notion of a "root scroller", an element whose scrolling controls the URL bar and produces effects like overscroll glow. This CL adds a mirror RootScrollerController on the compositor side and adds plumbing to set the root scroller layer on the compositor to match the root scroller element set in Blink. This doesn't yet implement any of the root scroller API functionality on the compositor. The only functional change is that once a layer is set as the root scroller, scrolls won't chain past it. The root scroller API is currently not enabled in stable builds so this will only ever be the outer viewport scroll layer. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
https://codereview.chromium.org/2113483002/diff/1/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/2113483002/diff/1/cc/trees/layer_tree_host.h#... cc/trees/layer_tree_host.h:218: scoped_refptr<Layer> inner_viewport_scroll_layer, On 2016/07/07 at 21:20:45, bokan wrote: > On 2016/06/29 22:44:08, aelias wrote: > > I'm having trouble understanding the intended breakdown of what's tied to the > > root scroller and what to the inner viewport layer, could you lay it out? > > The root scroller is really just designating which layer is the outer viewport. In a future patch I'll add plumbing in cc::Viewport to swap the outer viewport when the root scroller changes. So, we have another layer right above it called the outer_viewport_scroll_layer. So I assume you mean this is the outer viewport container layer. But we already have a canonical reference to that: outer_viewport_scroll_layer()->scroll_clip_layer_id(). Why is this one needed? I'm not necessarily super committed to the existing way it's structured, but I do think we shouldn't pass it in redundantly via two different APIs.
https://codereview.chromium.org/2113483002/diff/1/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/2113483002/diff/1/cc/trees/layer_tree_host.h#... cc/trees/layer_tree_host.h:218: scoped_refptr<Layer> inner_viewport_scroll_layer, On 2016/07/07 22:03:15, aelias wrote: > On 2016/07/07 at 21:20:45, bokan wrote: > > On 2016/06/29 22:44:08, aelias wrote: > > > I'm having trouble understanding the intended breakdown of what's tied to > the > > > root scroller and what to the inner viewport layer, could you lay it out? > > > > The root scroller is really just designating which layer is the outer > viewport. In a future patch I'll add plumbing in cc::Viewport to swap the outer > viewport when the root scroller changes. > > So, we have another layer right above it called the outer_viewport_scroll_layer. > So I assume you mean this is the outer viewport container layer. But we > already have a canonical reference to that: > outer_viewport_scroll_layer()->scroll_clip_layer_id(). Why is this one needed? > > I'm not necessarily super committed to the existing way it's structured, but I > do think we shouldn't pass it in redundantly via two different APIs. No, it's basically an indirection. If the page doesn't set a rootScroller element, root_scroller_layer here would be outer_viewport_scroll_layer (and that's what cc::Viewport will set as the outer viewport). But if the page does set a rootScroller, root_scroller_layer will point to the scroll layer of some other scroller (e.g. a <div>) in the hierarchy and now cc::Viewport will set that as its "outer viewport". The outer viewport scroll/container layers remain in the tree hierarchy though. Perhaps cc::Viewport should use the visual/layout terminology to make this less confusing. We're not replacing the outer viewport layers in the tree at all (we could, conceptually, but I tried that and found doing layer surgery to be difficult). What we're doing is specifying which scroll layer in the tree moves top controls/overscroll. So maybe what I should have said above is that "root scroller specifies which layer is the layout viewport" and layout viewport doesn't necessarily have to be the outer viewport. E.g. When the page loads: inner_container page_scale inner_scroll <--------------------- outer_container | outer_scroll <----------- cc::Viewport uses these as root layer "visual" and "layout" layers #div container #div scroll .... Now the page does document.rootScroller = $('#div'). The #div scrolling layer becomes the "root scroller" but the outer viewport layers remain part of the hierarchy inner_container page_scale inner_scroll <--------------------+ outer_container | outer_scroll cc::Viewport uses these as root layer "visual" and "layout" layers #div container | #div scroll <-----------+ .... Does that make sense?
I understand wanting to avoid layer surgery, but we already have a top-level reference "outer_viewport_scroll_layer" passed in RegisterViewportLayers, and I still don't follow why it wouldn't work to switch that one to point to "#div scroll", leaving the old "outer_scroll" layer idle in the tree.
On 2016/07/07 23:41:52, aelias wrote:
> I understand wanting to avoid layer surgery, but we already have a top-level
> reference "outer_viewport_scroll_layer" passed in RegisterViewportLayers, and
I
> still don't follow why it wouldn't work to switch that one to point to "#div
> scroll", leaving the old "outer_scroll" layer idle in the tree.
Ah, ok, I think I see what you're saying. So setting the
outer_viewport_scroll_layer to rootScroller doesn't remove the current outer
viewport scroll layer (or change the structure of the layer tree in any way), it
just changes which layer is referred to as the outer viewport, right? So in my
previous picture, setting #div as the rootScroller does this:
inner_container <----------------Inner Viewport Container Layer
page_scale
inner_scroll <---------------Inner Viewport Scroll Layer
frameview_container
frameview_scroll
root layer
#div container <-----Outer Viewport Container Layer
#div scroll <------Outer Viewport Scroll Layer
Does that sound right? In that case, I think you're right in that it could work
and is probably a better approach. I'll give that a try instead. Thanks.
Right, that's what I had in mind. Let me know when the new patch is uploaded.
Description was changed from ========== Add plumbing for setting a root scroller layer in the compositor This CL adds plumbing and a mirror implementation of the root scroller concept to the compositor. Blink already has the notion of a "root scroller", an element whose scrolling controls the URL bar and produces effects like overscroll glow. This CL adds a mirror RootScrollerController on the compositor side and adds plumbing to set the root scroller layer on the compositor to match the root scroller element set in Blink. This doesn't yet implement any of the root scroller API functionality on the compositor. The only functional change is that once a layer is set as the root scroller, scrolls won't chain past it. The root scroller API is currently not enabled in stable builds so this will only ever be the outer viewport scroll layer. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Make RootScroller set the outer viewport scroll layer in the compositor This CL makes the outer viewport scroll layer in the compositor match the RootScroller element's scroll layer. It adds a callback in RootScroller that registers the viewport's layers each time compositing in the FrameView is updated. The WebViewImpl::registerViewportLayersWithCompositor method queries RootScroller for the layer to use for the outer viewport scroll layer. Without enabling the RootScroller API, this will always fall back to the FrameView's scroll layer so it'll only have an effect with experimental flags enabled. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
bokan@chromium.org changed reviewers: + tdresser@chromium.org - aelias@chromium.org
Tim, Alexandre was originally reviewing this but he's now OOO for a few weeks and his suggestion means none of this is in CC anymore anyway. The point of this CL is to hook up the RootScroller to the compositor. This patch has no effect if the RootScroller API isn't turned on with a flag. When it is turned on and the root scroller is changed, it's broken horribly when scrolling on the compositor but I'd like to land this "plumbing" change and I'll fix the issues one at a time on CC side. Sound good?
aelias@chromium.org changed reviewers: + aelias@chromium.org
I'm still checking sporadically, lgtm.
On 2016/07/12 02:00:20, aelias_OOO_until_July_25 wrote: > I'm still checking sporadically, lgtm. Dave, do you still want my review?
On 2016/07/12 12:17:36, tdresser wrote: > On 2016/07/12 02:00:20, aelias_OOO_until_July_25 wrote: > > I'm still checking sporadically, lgtm. > > Dave, do you still want my review? I'll land this as is. If you have time and want to look over I'm happy to make any improvements :)
The CQ bit was checked by bokan@chromium.org
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 bokan@chromium.org
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_asan_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 bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make RootScroller set the outer viewport scroll layer in the compositor This CL makes the outer viewport scroll layer in the compositor match the RootScroller element's scroll layer. It adds a callback in RootScroller that registers the viewport's layers each time compositing in the FrameView is updated. The WebViewImpl::registerViewportLayersWithCompositor method queries RootScroller for the layer to use for the outer viewport scroll layer. Without enabling the RootScroller API, this will always fall back to the FrameView's scroll layer so it'll only have an effect with experimental flags enabled. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Make RootScroller set the outer viewport scroll layer in the compositor This CL makes the outer viewport scroll layer in the compositor match the RootScroller element's scroll layer. It adds a callback in RootScroller that registers the viewport's layers each time compositing in the FrameView is updated. The WebViewImpl::registerViewportLayersWithCompositor method queries RootScroller for the layer to use for the outer viewport scroll layer. Without enabling the RootScroller API, this will always fall back to the FrameView's scroll layer so it'll only have an effect with experimental flags enabled. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make RootScroller set the outer viewport scroll layer in the compositor This CL makes the outer viewport scroll layer in the compositor match the RootScroller element's scroll layer. It adds a callback in RootScroller that registers the viewport's layers each time compositing in the FrameView is updated. The WebViewImpl::registerViewportLayersWithCompositor method queries RootScroller for the layer to use for the outer viewport scroll layer. Without enabling the RootScroller API, this will always fall back to the FrameView's scroll layer so it'll only have an effect with experimental flags enabled. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Make RootScroller set the outer viewport scroll layer in the compositor This CL makes the outer viewport scroll layer in the compositor match the RootScroller element's scroll layer. It adds a callback in RootScroller that registers the viewport's layers each time compositing in the FrameView is updated. The WebViewImpl::registerViewportLayersWithCompositor method queries RootScroller for the layer to use for the outer viewport scroll layer. Without enabling the RootScroller API, this will always fall back to the FrameView's scroll layer so it'll only have an effect with experimental flags enabled. BUG=505516 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/243c701b03206899b372d0b7898b49fc7a725704 Cr-Commit-Position: refs/heads/master@{#404890} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/243c701b03206899b372d0b7898b49fc7a725704 Cr-Commit-Position: refs/heads/master@{#404890} |
