Plumb PinchVirtualViewport layers into CC.

Plumb PinchVirtualViewport layers into CC.

This CL plumbs pointers from the inner/outer-viewport layers and page scale
layer into CC from Blink. At present this has no effect on CC unless
--enable-pinch-virtual-viewport is specified on the command line.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223305

[wjmaclean, 2013-09-09 20:58:00]

This CL is an updated version of https://codereview.chromium.org/16679011/, but
is sufficiently different that I'm opening a new review for it. The major
difference is that the scrollbar Layer/LayerImpl's are created via WebViewImpl
and not in LayerTreeHost.

Depends on https://codereview.chromium.org/23464035/ to land first.

[enne (OOO), 2013-09-10 17:03:04]

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
content/renderer/gpu/render_widget_compositor.cc:493: const WebKit::WebLayer*
innerViewportClipLayer,
I understand setting the inner and outer viewport scroll layers, as you'll need
this to figure out what the total scroll offset is at certain points in LT(H)I.

I'm a little bit less sure about what the inner viewport clip layer is for.  I
assume you're going to use this to hook up to the solid color scrollbars to help
eliminate the UpdateSolidColorScrollbars function?  How are you going to find
them?  For these "pinch-aware" solid color scrollbars, could you set the clip
layer explicitly on them?

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
content/renderer/gpu/render_widget_compositor.cc:494: const WebKit::WebLayer*
pageScaleLayerLayer,
I'd like it if this patch could help make parts of the non-pinch viewport path
be more explicit, and also unify the way both of these paths work so that we
have fewer conditional paths in the compositor.

Is it possible to have both paths go through this? In particular, it seems both
paths need the page scale layer?  Also, it seems like both paths could set inner
viewport scroll layer (as root scroll layer for the non-pinch viewport path)
with outer viewport scroll layer being the only optional layer here.

[wjmaclean, 2013-09-11 00:19:25]

Here are some answers to the comments/questions ... let me know if what I'm
proposing seems reasonable.

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
content/renderer/gpu/render_widget_compositor.cc:493: const WebKit::WebLayer*
innerViewportClipLayer,
On 2013/09/10 17:03:04, enne wrote:
> I understand setting the inner and outer viewport scroll layers, as you'll
need
> this to figure out what the total scroll offset is at certain points in
LT(H)I.
> 
> I'm a little bit less sure about what the inner viewport clip layer is for.  I
> assume you're going to use this to hook up to the solid color scrollbars to
help
> eliminate the UpdateSolidColorScrollbars function?  How are you going to find
> them?  For these "pinch-aware" solid color scrollbars, could you set the clip
> layer explicitly on them?

Actually, the clip layer probably does not need to be passed at all (though the
original intent was likely as you indicate). The existing code in
tree_synchronizer that notifies scroll layers about their scrollbars should work
fine for the inner-viewport solid-color scrollbars, and my idea was that the
scroll layer would look after transmitting the necessary information to the
scroll bars, as per the current model. In that way the mechanism for pushing
information to the scrollbars won't be specific to inner/out viewport (or any
other scroll-layer type). But each scroll layer would need to know its clip
layer; I had intended this would be set (in a later cl) in LayerTreeHost.

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
content/renderer/gpu/render_widget_compositor.cc:494: const WebKit::WebLayer*
pageScaleLayerLayer,
On 2013/09/10 17:03:04, enne wrote:
> I'd like it if this patch could help make parts of the non-pinch viewport path
> be more explicit, and also unify the way both of these paths work so that we
> have fewer conditional paths in the compositor.

Hmm, ok. I guess I was still operating on the principle of keeping all the pinch
stuff isolated, at least until all the bits have landed, and we feel it's
working correctly.

> Is it possible to have both paths go through this? In particular, it seems
both
> paths need the page scale layer?  Also, it seems like both paths could set
inner
> viewport scroll layer (as root scroll layer for the non-pinch viewport path)
> with outer viewport scroll layer being the only optional layer here.

I'm not quite sure how we would run both code paths through here, since most of
this doesn't exist if the non-pinch pathway is used. We could presumably find
the root scroll layer and pass pointers to it and its parent (to avoid the need
to discover these in LayerTreeHost), does that sound helpful? The rest of the
layer pointers would be 0 in the non-pinch case.

Does that sound reasonable?

[enne (OOO), 2013-09-11 00:49:46]

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
content/renderer/gpu/render_widget_compositor.cc:493: const WebKit::WebLayer*
innerViewportClipLayer,
On 2013/09/11 00:19:25, wjmaclean wrote:

> Actually, the clip layer probably does not need to be passed at all (though
the
> original intent was likely as you indicate). The existing code in
> tree_synchronizer that notifies scroll layers about their scrollbars should
work
> fine for the inner-viewport solid-color scrollbars, and my idea was that the
> scroll layer would look after transmitting the necessary information to the
> scroll bars, as per the current model. In that way the mechanism for pushing
> information to the scrollbars won't be specific to inner/out viewport (or any
> other scroll-layer type). But each scroll layer would need to know its clip
> layer; I had intended this would be set (in a later cl) in LayerTreeHost.

Yeah, I was thinking something along those lines too, that the clip layer would
have to be set on the scroll layer (or scroll bar, maybe?), although maybe from
Blink and not right in LayerTreeHost since that's where the clip layer is
established.

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
content/renderer/gpu/render_widget_compositor.cc:494: const WebKit::WebLayer*
pageScaleLayerLayer,
On 2013/09/11 00:19:25, wjmaclean wrote:

> Hmm, ok. I guess I was still operating on the principle of keeping all the
pinch
> stuff isolated, at least until all the bits have landed, and we feel it's
> working correctly.

If you want to keep it separate as an intermediate step, that's fine.  I'm just
looking for an eye to how this will look eventually, and when I see stuff like
"if pinch use this layer and if not use this other layer" in this patch, that
code seems like it could go away eventually.

> I'm not quite sure how we would run both code paths through here, since most
of
> this doesn't exist if the non-pinch pathway is used. We could presumably find
> the root scroll layer and pass pointers to it and its parent (to avoid the
need
> to discover these in LayerTreeHost), does that sound helpful? The rest of the
> layer pointers would be 0 in the non-pinch case.

You said above that you're thinking of removing the clip layer.  So, that leaves
page scale and the two scroll layers.  The non-pinch case has a page scale layer
and one scroll layer.  So, if by "most of this" you mean "one of the scroll
layers", then yes?

But, yes.  I think this would eventually just become RegisterViewportLayers, and
the other path could use it to set up some layer relationships that are
currently implicitly discovered from the tree itself.

[wjmaclean, 2013-09-11 17:30:37]

PTAL

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
content/renderer/gpu/render_widget_compositor.cc:493: const WebKit::WebLayer*
innerViewportClipLayer,
On 2013/09/11 00:49:46, enne wrote:
> On 2013/09/11 00:19:25, wjmaclean wrote:
> 
> > Actually, the clip layer probably does not need to be passed at all (though
> the
> > original intent was likely as you indicate). The existing code in
> > tree_synchronizer that notifies scroll layers about their scrollbars should
> work
> > fine for the inner-viewport solid-color scrollbars, and my idea was that the
> > scroll layer would look after transmitting the necessary information to the
> > scroll bars, as per the current model. In that way the mechanism for pushing
> > information to the scrollbars won't be specific to inner/out viewport (or
any
> > other scroll-layer type). But each scroll layer would need to know its clip
> > layer; I had intended this would be set (in a later cl) in LayerTreeHost.
> 
> Yeah, I was thinking something along those lines too, that the clip layer
would
> have to be set on the scroll layer (or scroll bar, maybe?), although maybe
from
> Blink and not right in LayerTreeHost since that's where the clip layer is
> established.

Hmm, ok ... if we're setting this on the Blink side, does adding something like
WebLayer::setScrollClipLayer() seem ok? Then we can plumb the pointer through on
the CC-side.

https://codereview.chromium.org/23922006/diff/1/content/renderer/gpu/render_w...
content/renderer/gpu/render_widget_compositor.cc:494: const WebKit::WebLayer*
pageScaleLayerLayer,
On 2013/09/11 00:49:46, enne wrote:
> On 2013/09/11 00:19:25, wjmaclean wrote:
> 
> > Hmm, ok. I guess I was still operating on the principle of keeping all the
> pinch
> > stuff isolated, at least until all the bits have landed, and we feel it's
> > working correctly.
> 
> If you want to keep it separate as an intermediate step, that's fine.  I'm
just
> looking for an eye to how this will look eventually, and when I see stuff like
> "if pinch use this layer and if not use this other layer" in this patch, that
> code seems like it could go away eventually.
> 
> > I'm not quite sure how we would run both code paths through here, since most
> of
> > this doesn't exist if the non-pinch pathway is used. We could presumably
find
> > the root scroll layer and pass pointers to it and its parent (to avoid the
> need
> > to discover these in LayerTreeHost), does that sound helpful? The rest of
the
> > layer pointers would be 0 in the non-pinch case.
> 
> You said above that you're thinking of removing the clip layer.  So, that
leaves
> page scale and the two scroll layers.  The non-pinch case has a page scale
layer
> and one scroll layer.  So, if by "most of this" you mean "one of the scroll
> layers", then yes?
> 
> But, yes.  I think this would eventually just become RegisterViewportLayers,
and
> the other path could use it to set up some layer relationships that are
> currently implicitly discovered from the tree itself.

Done.

I've renamed the WebLayerTreeView interface function to just be
registerViewportLayers(), and this now gets called in both paths. Perhaps in
some not-too-distant future, when the pinch work has stabilized, we can
re-factor PinchViewports into something like "ViewportManager", and move some of
the logic out of WebViewImpl::setRootGraphicsLayer().

[enne (OOO), 2013-09-11 20:13:03]

https://codereview.chromium.org/23922006/diff/8001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/23922006/diff/8001/cc/trees/layer_tree_impl.c...
cc/trees/layer_tree_impl.cc:121: if (settings().use_pinch_virtual_viewport)
style nit: {}

https://codereview.chromium.org/23922006/diff/8001/cc/trees/layer_tree_impl.c...
cc/trees/layer_tree_impl.cc:267: void LayerTreeImpl::SetPinchViewportLayerIds(
Should this DCHECK that these exist? It seems like an error to have ids but not
have layers.

https://codereview.chromium.org/23922006/diff/8001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/23922006/diff/8001/cc/trees/layer_tree_impl.h...
cc/trees/layer_tree_impl.h:121: void SetPinchViewportLayerIds(int
page_scale_layer_id,
bikeshed: s/Pinch// to match the LTH functions?

https://codereview.chromium.org/23922006/diff/8001/content/renderer/gpu/rende...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/23922006/diff/8001/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:498: // The innerviewport layer
will only exist when using pinch.
I think it's the outer viewport layer that's optional and not the inner.  The
inner is affected by pinch, but the outer one is not.  So, I'd say the inner one
already exists.

[wjmaclean, 2013-09-12 18:16:46]

PTAL?

https://codereview.chromium.org/23922006/diff/8001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/23922006/diff/8001/cc/trees/layer_tree_impl.c...
cc/trees/layer_tree_impl.cc:121: if (settings().use_pinch_virtual_viewport)
On 2013/09/11 20:13:03, enne wrote:
> style nit: {}

Done.

https://codereview.chromium.org/23922006/diff/8001/cc/trees/layer_tree_impl.c...
cc/trees/layer_tree_impl.cc:267: void LayerTreeImpl::SetPinchViewportLayerIds(
On 2013/09/11 20:13:03, enne wrote:
> Should this DCHECK that these exist? It seems like an error to have ids but
not
> have layers.

Done.

We need to check the validity of the layer id for the outer viewport scroll
layer, as (eventually) we will call this in both code paths.

https://codereview.chromium.org/23922006/diff/8001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/23922006/diff/8001/cc/trees/layer_tree_impl.h...
cc/trees/layer_tree_impl.h:121: void SetPinchViewportLayerIds(int
page_scale_layer_id,
On 2013/09/11 20:13:03, enne wrote:
> bikeshed: s/Pinch// to match the LTH functions?

Done.

Definitely! :-)

https://codereview.chromium.org/23922006/diff/8001/content/renderer/gpu/rende...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/23922006/diff/8001/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:498: // The innerviewport layer
will only exist when using pinch.
On 2013/09/11 20:13:03, enne wrote:
> I think it's the outer viewport layer that's optional and not the inner.  The
> inner is affected by pinch, but the outer one is not.  So, I'd say the inner
one
> already exists.

Yes, right. I keep forgetting we call the newly created layer "inner" so as not
to have to re-organize the existing layers, but conceptually it's the out
viewport layer that's optional.

[enne (OOO), 2013-09-12 18:57:01]

lgtm

https://codereview.chromium.org/23922006/diff/16001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/23922006/diff/16001/cc/trees/layer_tree_impl....
cc/trees/layer_tree_impl.cc:289: page_scale_layer_ = 0;
s/0/NULL/g

https://codereview.chromium.org/23922006/diff/16001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/23922006/diff/16001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:500: // The outer viewport
layer will only exist when using pinch.
pinch => pinch virtual viewports

https://codereview.chromium.org/23922006/diff/16001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:503: : 0);
0 => NULL

[wjmaclean, 2013-09-12 19:06:17]

https://codereview.chromium.org/23922006/diff/16001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/23922006/diff/16001/cc/trees/layer_tree_impl....
cc/trees/layer_tree_impl.cc:289: page_scale_layer_ = 0;
On 2013/09/12 18:57:02, enne wrote:
> s/0/NULL/g

Done.

https://codereview.chromium.org/23922006/diff/16001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/23922006/diff/16001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:500: // The outer viewport
layer will only exist when using pinch.
On 2013/09/12 18:57:02, enne wrote:
> pinch => pinch virtual viewports

Done.

https://codereview.chromium.org/23922006/diff/16001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:503: : 0);
On 2013/09/12 18:57:02, enne wrote:
> 0 => NULL

Done.

[jamesr, 2013-09-12 21:49:59]

lgtm

[commit-bot: I haz the power, 2013-09-13 01:47:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/23922006/23001

[commit-bot: I haz the power, 2013-09-13 02:15:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-15 15:19:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/23922006/23001

[commit-bot: I haz the power, 2013-09-16 02:55:44]

Change committed as 223305