Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16679011:
Add viewport scrollbar class to support overlay scrollbars for pinch zoom virtual viewport. (Closed)
|
Created:
7 years, 6 months ago by wjmaclean Modified:
7 years, 3 months ago CC:
chromium-reviews, cc-bugs_chromium.org, darin-cc_chromium.org, WRONG-USE-chromium Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd viewport scrollbar class to support overlay scrollbars for pinch zoom virtual viewport.
BUG=none
Patch Set 1 #
Total comments: 10
Patch Set 2 : Plumb viewport layers. #Patch Set 3 : Add scrollbars layers in compositor, plumb layer ids to LayerTreeImpl. #
Total comments: 14
Messages
Total messages: 32 (0 generated)
This is the companion CL to https://codereview.chromium.org/16799005/, and must be landed after it. It provides a cc Scrollbar-derived class for the non-Blink-backed overlay scrollbars.
https://codereview.chromium.org/16679011/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/16679011/diff/1/cc/trees/layer_tree_impl.cc#n... cc/trees/layer_tree_impl.cc:284: // TODO(wjmaclean): when settings().use_pinch_virtual_viewport is specified, Maybe LayerTreeHost can be told the id of the page scale layer and this code can be ignorant about settings and which layer that actually is. https://codereview.chromium.org/16679011/diff/1/webkit/renderer/compositor_bi... File webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc (right): https://codereview.chromium.org/16679011/diff/1/webkit/renderer/compositor_bi... webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc:26: WebScrollbarLayerImpl::WebScrollbarLayerImpl( I am not really sure this is in line with what I was expecting. I was hoping we could be in a state where all scrollbars behaved identically rather than having two different kinds of scrollbars. Is this some intermediate state on the way to that goal?
https://codereview.chromium.org/16679011/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/16679011/diff/1/cc/trees/layer_tree_impl.cc#n... cc/trees/layer_tree_impl.cc:284: // TODO(wjmaclean): when settings().use_pinch_virtual_viewport is specified, On 2013/06/12 19:44:38, enne wrote: > Maybe LayerTreeHost can be told the id of the page scale layer and this code can > be ignorant about settings and which layer that actually is. Yes, that is the ultimate goal. LTH will be told which layer ids to use for inner/outer/page layers, and set that here. That's planned for the next CL. https://codereview.chromium.org/16679011/diff/1/webkit/renderer/compositor_bi... File webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc (right): https://codereview.chromium.org/16679011/diff/1/webkit/renderer/compositor_bi... webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc:26: WebScrollbarLayerImpl::WebScrollbarLayerImpl( On 2013/06/12 19:44:38, enne wrote: > I am not really sure this is in line with what I was expecting. I was hoping we > could be in a state where all scrollbars behaved identically rather than having > two different kinds of scrollbars. > > Is this some intermediate state on the way to that goal? By "behave identically" you mean using cc:Scrollbars using Blink geometry/painters? Or do you mean full-fledged Blink Scrollbar-objects with their own ScrollableAreas, etc. The latter seems hard if we stay with the "Blink-core doesn't know about the viewports" philosophy, though I could be wrong.
https://codereview.chromium.org/16679011/diff/1/webkit/renderer/compositor_bi... File webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc (right): https://codereview.chromium.org/16679011/diff/1/webkit/renderer/compositor_bi... webkit/renderer/compositor_bindings/web_scrollbar_layer_impl.cc:26: WebScrollbarLayerImpl::WebScrollbarLayerImpl( On 2013/06/12 19:54:36, wjmaclean wrote: > On 2013/06/12 19:44:38, enne wrote: > > I am not really sure this is in line with what I was expecting. I was hoping > we > > could be in a state where all scrollbars behaved identically rather than > having > > two different kinds of scrollbars. > > > > Is this some intermediate state on the way to that goal? > > By "behave identically" you mean using cc:Scrollbars using Blink > geometry/painters? Or do you mean full-fledged Blink Scrollbar-objects with > their own ScrollableAreas, etc. > > The latter seems hard if we stay with the "Blink-core doesn't know about the > viewports" philosophy, though I could be wrong. I agree with enne@ that we should avoid adding new CC layer type unless necessary. However I want to support wjmaclean@ on this one. ScrollbarLayer that uses Blink-core ScrollbarTheme to paint has always been a headache in the past. We actually needed to make a quick-hack scrollbar back in M25 age. It just worked. Simple, fast and clean. Also the pinch zoom scrollbar has very different behavior than regular scrollbars -- its thumb resizes all the time during pinch gesture. ScrollbarLayer won't suffice if later we want to paint fancy thumb textures that can't be simply stretched. At this point I can't think of a reason to create full-fledged ScrollableArea. One good reason to implement ScrollableArea is to receive GraphicsLayer::didScroll() notification (which is weird, why didn't we simply add a API to GraphicsLayerClient?), but for the pinch zoom viewport it's better to use the special-cased WebViewImpl::applyScrollAndScale.
There's separate issues here that I'd like to keep differentiated. (1) How are these viewport scrollbars supposed to be painted? This is not actually clear to me from this patch. Is the intention that these will be solid color scrollbars? Are they going to be painted some other way? Also, viewport scrollbars are which scrollbars, again? (Can somebody come up with better naming around the different scrollbars?) (2) How does scrolling on the compositor thread affect the size/position of the thumb for these scrollbars? I am arguing that pinch zoom scrollbars should not have different behavior than regular scrollbars. All scrollbars on the compositor thread are really just about reflecting the rect of some clipping layer into some other layer's space. I think if the scrollbar was informed about these two layers, then "normal" and "pinch" scrollbars could behave identically. (3) How much should Blink know about these scrollbars? I think Blink needs to create GraphicsLayers for them, but I don't think we need to go so far as to create a ScrollableArea.
On 2013/06/12 21:48:37, enne wrote: > There's separate issues here that I'd like to keep differentiated. > > (1) How are these viewport scrollbars supposed to be painted? > > This is not actually clear to me from this patch. Is the intention that these > will be solid color scrollbars? Are they going to be painted some other way? > Also, viewport scrollbars are which scrollbars, again? (Can somebody come up > with better naming around the different scrollbars?) Since the viewport scrollbar's thumbs are intended to be resized from the compositor thread, they have to be solid color scrollbars. We don't have the capability to resize painted thumbs on the compositor thread. Since solid color scrollbars have no dependency on the things that WebScrollbarLayerImpl knows about, such as geometry/painters/etc, this strongly indicates that viewport scrollbars should have nothing to do with webkit/renderer/compositor_bindings/...
On 2013/06/12 21:48:37, enne wrote: > There's separate issues here that I'd like to keep differentiated. > > (1) How are these viewport scrollbars supposed to be painted? > > This is not actually clear to me from this patch. Is the intention that these > will be solid color scrollbars? Are they going to be painted some other way? > Also, viewport scrollbars are which scrollbars, again? (Can somebody come up > with better naming around the different scrollbars?) > > (2) How does scrolling on the compositor thread affect the size/position of the > thumb for these scrollbars? > > I am arguing that pinch zoom scrollbars should not have different behavior than > regular scrollbars. All scrollbars on the compositor thread are really just > about reflecting the rect of some clipping layer into some other layer's space. > I think if the scrollbar was informed about these two layers, then "normal" and > "pinch" scrollbars could behave identically. Not really. Scroll extent calculation is very different for the inner scrollbar. For example, if we have outer viewport of 200px, and a document of 300px, and now we zoom in to 2x (so inner viewport exposes 100px of the document). Then the percentage length of the scrollbar thumb should be 100px(clip size of inner viewport)/300px(content size of outer viewport), instead of 100px(clip size of inner viewport)/200px(content size of inner viewport). And the begin of the thumb should use the combined scroll offset divided by combined max scroll offset. > (3) How much should Blink know about these scrollbars? > > I think Blink needs to create GraphicsLayers for them, but I don't think we need > to go so far as to create a ScrollableArea. Agree
On 2013/06/12 22:08:39, trchen wrote: > On 2013/06/12 21:48:37, enne wrote: > > > > (2) How does scrolling on the compositor thread affect the size/position of > the > > thumb for these scrollbars? > > > > I am arguing that pinch zoom scrollbars should not have different behavior > than > > regular scrollbars. All scrollbars on the compositor thread are really just > > about reflecting the rect of some clipping layer into some other layer's > space. > > I think if the scrollbar was informed about these two layers, then "normal" > and > > "pinch" scrollbars could behave identically. > > Not really. Scroll extent calculation is very different for the inner scrollbar. Yes, really. Scroll extent calculation is very different *now*. I don't think it has to be. A scrollbar thumb's size and position are proportional to some particular clip layer's projection into some other layer's space. For overlay scrollbars unaffected by page scale it's the inner viewport clip layer projected into the root scroll layer. For non-overlay scrollbars affected by page scale, it's the outer viewport clip layer projected into the root scroll layer.
On 2013/06/12 22:22:44, enne wrote: > On 2013/06/12 22:08:39, trchen wrote: > > On 2013/06/12 21:48:37, enne wrote: > > > > > > (2) How does scrolling on the compositor thread affect the size/position of > > the > > > thumb for these scrollbars? > > > > > > I am arguing that pinch zoom scrollbars should not have different behavior > > than > > > regular scrollbars. All scrollbars on the compositor thread are really just > > > about reflecting the rect of some clipping layer into some other layer's > > space. > > > I think if the scrollbar was informed about these two layers, then "normal" > > and > > > "pinch" scrollbars could behave identically. > > > > Not really. Scroll extent calculation is very different for the inner > scrollbar. > > Yes, really. Scroll extent calculation is very different *now*. I don't think > it has to be. > > A scrollbar thumb's size and position are proportional to some particular clip > layer's projection into some other layer's space. For overlay scrollbars > unaffected by page scale it's the inner viewport clip layer projected into the > root scroll layer. For non-overlay scrollbars affected by page scale, it's the > outer viewport clip layer projected into the root scroll layer. Ah now I see what you mean. That sounds like a great idea!
On 2013/06/12 21:51:58, jamesr wrote: > On 2013/06/12 21:48:37, enne wrote: > > There's separate issues here that I'd like to keep differentiated. > > > > (1) How are these viewport scrollbars supposed to be painted? > > > > This is not actually clear to me from this patch. Is the intention that these > > will be solid color scrollbars? Are they going to be painted some other way? > > Also, viewport scrollbars are which scrollbars, again? (Can somebody come up > > with better naming around the different scrollbars?) > > Since the viewport scrollbar's thumbs are intended to be resized from the > compositor thread, they have to be solid color scrollbars. We don't have the > capability to resize painted thumbs on the compositor thread. Since solid color > scrollbars have no dependency on the things that WebScrollbarLayerImpl knows > about, such as geometry/painters/etc, this strongly indicates that viewport > scrollbars should have nothing to do with > webkit/renderer/compositor_bindings/... Solid color scrollbars are hard-edged rectangles and we should make sure this would look acceptable for a large desktop-browser scrollbar. If instead we want something with rounded edges, what would be needed is a "three-patch" thumb with a small rounded top and bottom and repeated 1px-high center.
On 2013/06/12 21:48:37, enne wrote: > There's separate issues here that I'd like to keep differentiated. > > (1) How are these viewport scrollbars supposed to be painted? > > This is not actually clear to me from this patch. The current pair of cl's does actually invoke the PaintPart() on the new scrollbar type. > Is the intention that these will be solid color scrollbars? For desktop, no, I hadn't intended that. > Are they going to be painted some other way? > Also, viewport scrollbars are which scrollbars, again? (Can somebody come up > with better naming around the different scrollbars?) How about (1) Inner-Viewport Scrollbars (IVS), always overlay, and (2) Outer-Viewport Scrollbars (OVS), these only exist on desktop and are non-overlay. In the non-pinch, desktop case let's use the term Main-Frame Scrollbars (MFS). > (2) How does scrolling on the compositor thread affect the size/position of the > thumb for these scrollbars? It will happy though LTH::ApplyScrollAndScale(), as for current MFSs. > I am arguing that pinch zoom scrollbars should not have different behavior than > regular scrollbars. All scrollbars on the compositor thread are really just > about reflecting the rect of some clipping layer into some other layer's space. > I think if the scrollbar was informed about these two layers, then "normal" and > "pinch" scrollbars could behave identically. They are still intended to have identical behaviour. The OVS/IVS scrollbars will get their info from their respective scroll layers in the Layer-tree. > (3) How much should Blink know about these scrollbars? > > I think Blink needs to create GraphicsLayers for them, Done, in the companion CL. > but I don't think we need to go so far as to create a ScrollableArea. That's what I thought, hence the ViewportScrollbar class (perhaps this should be name OuterViewportScrollbars).
On 2013/06/12 21:51:58, jamesr wrote: > On 2013/06/12 21:48:37, enne wrote: > > Since the viewport scrollbar's thumbs are intended to be resized from the > compositor thread, they have to be solid color scrollbars. Do they? Why can't the Outer-Viewport Scrollbars just be re-painted on the main thread, as currently happens for the desktop Main-Frame Scrollbars? > We don't have the > capability to resize painted thumbs on the compositor thread. Since solid color > scrollbars have no dependency on the things that WebScrollbarLayerImpl knows > about, such as geometry/painters/etc, this strongly indicates that viewport > scrollbars should have nothing to do with > webkit/renderer/compositor_bindings/... Except that we want to create GraphicsLayers for them, and get those graphics layers to automatically create the necessary ScrollbarLayers in the Layer-tree ... hence the approach I've taken.
On 2013/06/12 22:32:21, trchen wrote: > On 2013/06/12 22:22:44, enne wrote: > > > > A scrollbar thumb's size and position are proportional to some particular clip > > layer's projection into some other layer's space. For overlay scrollbars > > unaffected by page scale it's the inner viewport clip layer projected into the > > root scroll layer. For non-overlay scrollbars affected by page scale, it's > the > > outer viewport clip layer projected into the root scroll layer. > > Ah now I see what you mean. That sounds like a great idea! Agreed, and this is (I hope) where my CLs lead ... all the necessary information to position the scrollbars and size the thumbs is contained in the geometry found in the inner/outer viewport clip layers, and flows according to existing mechanisms (with some updates to LTH::ApplyScrollAndScale and setting of maximum_scroll_offset on these layers.)
On 2013/06/13 12:44:40, wjmaclean wrote: > On 2013/06/12 21:51:58, jamesr wrote: > > On 2013/06/12 21:48:37, enne wrote: > > > > Since the viewport scrollbar's thumbs are intended to be resized from the > > compositor thread, they have to be solid color scrollbars. > > Do they? Why can't the Outer-Viewport Scrollbars just be re-painted on the main > thread, as currently happens for the desktop Main-Frame Scrollbars? Main thread scrollbar components (specifically thumbs) can't be resized on the compositor thread. The outer viewport scrollbar's thumb needs to be resizable, right? > > > We don't have the > > capability to resize painted thumbs on the compositor thread. Since solid > color > > scrollbars have no dependency on the things that WebScrollbarLayerImpl knows > > about, such as geometry/painters/etc, this strongly indicates that viewport > > scrollbars should have nothing to do with > > webkit/renderer/compositor_bindings/... > > Except that we want to create GraphicsLayers for them, and get those graphics > layers to automatically create the necessary ScrollbarLayers in the Layer-tree > ... hence the approach I've taken. Solid color scrollbars do not require a new construction codepath. The geometry and painter are simply ignored when the solid color layer setting is set. For these scrollbars, it seems that you probably want to do something similar.
On 2013/06/13 21:13:41, jamesr wrote: > On 2013/06/13 12:44:40, wjmaclean wrote: > > On 2013/06/12 21:51:58, jamesr wrote: > > > On 2013/06/12 21:48:37, enne wrote: > > > > > > Since the viewport scrollbar's thumbs are intended to be resized from the > > > compositor thread, they have to be solid color scrollbars. > > > > Do they? Why can't the Outer-Viewport Scrollbars just be re-painted on the > main > > thread, as currently happens for the desktop Main-Frame Scrollbars? > > Main thread scrollbar components (specifically thumbs) can't be resized on the > compositor thread. The outer viewport scrollbar's thumb needs to be resizable, > right? correction: The inner viewport(= pinch zoom viewport) scrollbar needs to be resizable. > > > > > We don't have the > > > capability to resize painted thumbs on the compositor thread. Since solid > > color > > > scrollbars have no dependency on the things that WebScrollbarLayerImpl knows > > > about, such as geometry/painters/etc, this strongly indicates that viewport > > > scrollbars should have nothing to do with > > > webkit/renderer/compositor_bindings/... > > > > Except that we want to create GraphicsLayers for them, and get those graphics > > layers to automatically create the necessary ScrollbarLayers in the Layer-tree > > ... hence the approach I've taken. > > Solid color scrollbars do not require a new construction codepath. The geometry > and painter are simply ignored when the solid color layer setting is set. For > these scrollbars, it seems that you probably want to do something similar. solid_color_scrollbars is a global setting currently. Perhaps we should move it down to individual scrollbar layers.
On 2013/06/13 21:19:44, trchen wrote: > solid_color_scrollbars is a global setting currently. Perhaps we should move it > down to individual scrollbar layers. We still need the global setting to ban use of PrioritizedResources in ScrollbarLayers, until those are phased out. But we could have both a global setting (renamed to "allow_only_solid_color_scrollbars") and a per-layer one.
On 2013/06/13 21:13:41, jamesr wrote: > On 2013/06/13 12:44:40, wjmaclean wrote: > > On 2013/06/12 21:51:58, jamesr wrote: > > Main thread scrollbar components (specifically thumbs) can't be resized on the > compositor thread. The outer viewport scrollbar's thumb needs to be resizable, > right? Yes, but I don't want to give up the possibility of painted scrollbars for desktop, and until impl-side painting of scrollbars is possible, I'm willing to live with thumb-size-jank during pinch gestures, the only time this is really an issue. Not that I'm against the idea of solid color scrollbars on desktop, but I want to be able to do either. > > > > Except that we want to create GraphicsLayers for them, and get those graphics > > layers to automatically create the necessary ScrollbarLayers in the Layer-tree > > ... hence the approach I've taken. > > Solid color scrollbars do not require a new construction codepath. The geometry > and painter are simply ignored when the solid color layer setting is set. For > these scrollbars, it seems that you probably want to do something similar. Even if the construction codepath for solid color scrollbars ignore the painter and geometry, it does not ignore the need to have something backed by a WebCore::Scrollbar* (via WebScrollbar) when creating a WebScrollbarLayer. Since we need the latter to set up the graphics layers in Blink, and since managing the creation of the ViewportScrollbar via compositor_bindings seemed to make sense, I still think this is a reasonable way to go.
On Jun 14, 2013 9:10 AM, <wjmaclean@chromium.org> wrote: > > On 2013/06/13 21:13:41, jamesr wrote: >> >> On 2013/06/13 12:44:40, wjmaclean wrote: >> > On 2013/06/12 21:51:58, jamesr wrote: > > >> Main thread scrollbar components (specifically thumbs) can't be resized on the >> compositor thread. The outer viewport scrollbar's thumb needs to be > > resizable, >> >> right? > > > Yes, but I don't want to give up the possibility of painted scrollbars for > desktop, and until impl-side painting of scrollbars is possible, I'm willing to > live with thumb-size-jank during pinch gestures, the only time this is really an > issue. Not that I'm against the idea of solid color scrollbars on desktop, but I > want to be able to do either. Impl-side painting does not help with resizing and stretching is not going to be acceptable. If you want thumb resizing, you have to design a way to get it. Currently the only way we have to do it is with solid color scrollbars. You can use those or come up with a new way to represent thumbs if you want something more complex, but you can't use painted scrollbars. > https://codereview.chromium.org/16679011/
https://codereview.chromium.org/16679011/diff/1/cc/input/viewport_scrollbar.cc File cc/input/viewport_scrollbar.cc (right): https://codereview.chromium.org/16679011/diff/1/cc/input/viewport_scrollbar.c... cc/input/viewport_scrollbar.cc:71: // TODO(wjmaclean): currently the pinch zoom overlay scrollbars are drawn as How are you going to differentiate this PaintPart function from Android's solid color scrollbars? Aren't they both going to be using these "viewport scrollbars"? Also, I don't mean to bikeshed, but the phrase "viewport scrollbar" doesn't mean that much to me when we have both inner and outer viewport layers. I don't really have a better suggestion though. https://codereview.chromium.org/16679011/diff/1/webkit/renderer/compositor_bi... File webkit/renderer/compositor_bindings/web_compositor_support_impl.h (right): https://codereview.chromium.org/16679011/diff/1/webkit/renderer/compositor_bi... webkit/renderer/compositor_bindings/web_compositor_support_impl.h:40: virtual WebKit::WebScrollbarLayer* createScrollbarLayer( I had a conversation with jamesr who suggested that cc create these scrollbar layers itself based on layer ids passed into LayerTreeHost. That way we don't have to plumb these special scrollbar types all the way through compositor bindings. That seems like a better approach to me too.
I'm now assuming the most sensible thing to do is just make the desktop scrollbars use solid-colour scrollbars like Android. Will revise this CL. See comments below. https://codereview.chromium.org/16679011/diff/1/cc/input/viewport_scrollbar.cc File cc/input/viewport_scrollbar.cc (right): https://codereview.chromium.org/16679011/diff/1/cc/input/viewport_scrollbar.c... cc/input/viewport_scrollbar.cc:71: // TODO(wjmaclean): currently the pinch zoom overlay scrollbars are drawn as On 2013/06/14 20:56:59, enne wrote: > How are you going to differentiate this PaintPart function from Android's solid > color scrollbars? Aren't they both going to be using these "viewport > scrollbars"? Based on my conversations with JamesR, the simplest thing to do (at least in the short term) is to have both Android and desktop use the solid colour scrollbars. Then ViewportScrollbar will just have an empty implementation of PaintPart (actually, I'd rather give cc::Scrollbar an empty default implementation if that's ok, but I'll wait to hear from you on that). > Also, I don't mean to bikeshed, but the phrase "viewport scrollbar" doesn't mean > that much to me when we have both inner and outer viewport layers. I don't > really have a better suggestion though. Yeah, I was torn over that too ... InnerViewportScrollbar seemed too long, but just "ViewportScrollbar" *is* ambiguous, so let's do InnerViewportScrollbar. https://codereview.chromium.org/16679011/diff/1/webkit/renderer/compositor_bi... File webkit/renderer/compositor_bindings/web_compositor_support_impl.h (right): https://codereview.chromium.org/16679011/diff/1/webkit/renderer/compositor_bi... webkit/renderer/compositor_bindings/web_compositor_support_impl.h:40: virtual WebKit::WebScrollbarLayer* createScrollbarLayer( On 2013/06/14 20:56:59, enne wrote: > I had a conversation with jamesr who suggested that cc create these scrollbar > layers itself based on layer ids passed into LayerTreeHost. That way we don't > have to plumb these special scrollbar types all the way through compositor > bindings. That seems like a better approach to me too. Yes, I will remove the compositor bindings parts, and put up a new CL.
https://codereview.chromium.org/16679011/diff/1/cc/input/viewport_scrollbar.cc File cc/input/viewport_scrollbar.cc (right): https://codereview.chromium.org/16679011/diff/1/cc/input/viewport_scrollbar.c... cc/input/viewport_scrollbar.cc:71: // TODO(wjmaclean): currently the pinch zoom overlay scrollbars are drawn as On 2013/06/17 13:44:28, wjmaclean wrote: > Then ViewportScrollbar will just have an empty implementation of PaintPart > (actually, I'd rather give cc::Scrollbar an empty default implementation if > that's ok, but I'll wait to hear from you on that). I'd prefer ViewportScrollbar having an empty implementation. I think that's more clear and it keeps cc::Scrollbar purely abstract.
PTAL, I've added the plumbing to identify the viewport layers in LTH, but stopped short (in this CL) of (1) creating the ScrollbarLayers, and (2) implementing the inner/outerViewportScrollLayer() functions (and using them).
PTAL, I've added the creation of the scrollbar layers, and plumbed the relevant layer ids to the impl tree. I think that's about as much stuff as I want to put in this patch.
https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... File cc/input/inner_viewport_scrollbar.cc (right): https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... cc/input/inner_viewport_scrollbar.cc:49: return track_rect.width() - ratio * scroll_layer_->max_scroll_offset().x(); (1) I think this math is not quite right. Can you add a unit test? In particular, if you apply a page scale, shouldn't the thumb length change? When I suggested using clip/scroll layer ids directly as a way to figure this out, more specfiically I was suggesting to use the screen space clip rect and screen space scroll layer relative to each other and not just their layer space bounds. (2) More importantly, why is this math on the main thread? These values will be frozen and pushed to the ScrollbarLayerImpl, which will then not update as the clip and scroll layers move around. https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... cc/input/inner_viewport_scrollbar.cc:58: if (orientation_ == HORIZONTAL) style nit: {}
https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... File cc/input/inner_viewport_scrollbar.cc (right): https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... cc/input/inner_viewport_scrollbar.cc:49: return track_rect.width() - ratio * scroll_layer_->max_scroll_offset().x(); On 2013/06/19 23:38:28, enne wrote: > In particular, if you apply a page scale, shouldn't the thumb length change? > When I suggested using clip/scroll layer ids directly as a way to figure this > out, more specfiically I was suggesting to use the screen space clip rect and > screen space scroll layer relative to each other and not just their layer space > bounds. Your relative layer space approach should work with regular scrollbars too. For the long term, do we still need two implementation of Scrollbar at all? https://codereview.chromium.org/16679011/diff/22002/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/16679011/diff/22002/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:89: const WebKit::WebLayer* pageScaleLayerLayer, nits: LayerLayer? https://codereview.chromium.org/16679011/diff/22002/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:93: const WebKit::WebLayer* innerViewportVerticalScrollbarLayer) OVERRIDE; I feel CC is making too much assumption about the layer structure. It looks like the main purpose of this is to allow CC to setup the scrollbar layers. From the other thread I know that you and jamesr@ made the consensus. Just wondering why? Is it really messy to setup the scrollbar layers from Blink-side? Does that imply we want to setup scrollbar layers in CC for frames and overflow:scroll as well?
Please see attached comments, ... I'll hold off on putting up the revised CL until everyone is done commenting on this first iteration. https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... File cc/input/inner_viewport_scrollbar.cc (right): https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... cc/input/inner_viewport_scrollbar.cc:49: return track_rect.width() - ratio * scroll_layer_->max_scroll_offset().x(); On 2013/06/20 01:16:08, trchen wrote: > On 2013/06/19 23:38:28, enne wrote: > > In particular, if you apply a page scale, shouldn't the thumb length change? > > When I suggested using clip/scroll layer ids directly as a way to figure this > > out, more specfiically I was suggesting to use the screen space clip rect and > > screen space scroll layer relative to each other and not just their layer > space > > bounds. > > Your relative layer space approach should work with regular scrollbars too. For > the long term, do we still need two implementation of Scrollbar at all? We still need something that will work without a backing WebCore::Scrollbar, which cc::Scrollbar still requires. This class is not expected to ever work with non-overlay/non-innerviewport scrollbars. https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... cc/input/inner_viewport_scrollbar.cc:49: return track_rect.width() - ratio * scroll_layer_->max_scroll_offset().x(); On 2013/06/19 23:38:28, enne wrote: > (1) I think this math is not quite right. Can you add a unit test? Yes, I'll add a unit test. > In particular, if you apply a page scale, shouldn't the thumb length change? > When I suggested using clip/scroll layer ids directly as a way to figure this > out, more specfiically I was suggesting to use the screen space clip rect and > screen space scroll layer relative to each other and not just their layer space > bounds. Yes, you're right ... I'll adjust this so the thumb takes this into account. > (2) More importantly, why is this math on the main thread? These values will be > frozen and pushed to the ScrollbarLayerImpl, which will then not update as the > clip and scroll layers move around. I guess we could just provide dummy values here, and only go with the impl side values. (I had thought we would want to have valid 'initial' values, but we could likely do without.) https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... cc/input/inner_viewport_scrollbar.cc:58: if (orientation_ == HORIZONTAL) On 2013/06/19 23:38:28, enne wrote: > style nit: {} Done. https://codereview.chromium.org/16679011/diff/22002/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/16679011/diff/22002/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:89: const WebKit::WebLayer* pageScaleLayerLayer, On 2013/06/20 01:16:08, trchen wrote: > nits: LayerLayer? Done. https://codereview.chromium.org/16679011/diff/22002/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:93: const WebKit::WebLayer* innerViewportVerticalScrollbarLayer) OVERRIDE; On 2013/06/20 01:16:08, trchen wrote: > I feel CC is making too much assumption about the layer structure. > > It looks like the main purpose of this is to allow CC to setup the scrollbar > layers. From the other thread I know that you and jamesr@ made the consensus. > Just wondering why? Is it really messy to setup the scrollbar layers from > Blink-side? Does that imply we want to setup scrollbar layers in CC for frames > and overflow:scroll as well? This is a special case for scrollbars that (1) Blink never needs to interact with, and (2) have no associated WebCore::Scrollbar associated with them. In particular, if we instantiate on the Blink side (as I originally proposed), then (2) means we need additional plumbing though the compositor bindings, since we need another pathway to create scrollbars when we don't have a backing WebCore::Scrollbar.
https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... File cc/input/inner_viewport_scrollbar.cc (right): https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... cc/input/inner_viewport_scrollbar.cc:49: return track_rect.width() - ratio * scroll_layer_->max_scroll_offset().x(); On 2013/06/21 18:20:53, wjmaclean wrote: > On 2013/06/19 23:38:28, enne wrote: > > (2) More importantly, why is this math on the main thread? These values will > be > > frozen and pushed to the ScrollbarLayerImpl, which will then not update as the > > clip and scroll layers move around. > > I guess we could just provide dummy values here, and only go with the impl side > values. (I had thought we would want to have valid 'initial' values, but we > could likely do without.) I think we can do without dummy values. Most of the time dummy accessors are only added to make LayerTreeHostCommon templates happy. (e.g. scrollDelta) How are you going to implement this though? Likely you'll need to update the LayerImpl raw pointers or layer IDs during tree synchronization. Are you going to make a twin class for the main-thread counterpart? https://codereview.chromium.org/16679011/diff/22002/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/16679011/diff/22002/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:93: const WebKit::WebLayer* innerViewportVerticalScrollbarLayer) OVERRIDE; On 2013/06/21 18:20:53, wjmaclean wrote: > On 2013/06/20 01:16:08, trchen wrote: > > I feel CC is making too much assumption about the layer structure. > > > > It looks like the main purpose of this is to allow CC to setup the scrollbar > > layers. From the other thread I know that you and jamesr@ made the consensus. > > Just wondering why? Is it really messy to setup the scrollbar layers from > > Blink-side? Does that imply we want to setup scrollbar layers in CC for frames > > and overflow:scroll as well? > > This is a special case for scrollbars that (1) Blink never needs to interact > with, and (2) have no associated WebCore::Scrollbar associated with them. In > particular, if we instantiate on the Blink side (as I originally proposed), then > (2) means we need additional plumbing though the compositor bindings, since we > need another pathway to create scrollbars when we don't have a backing > WebCore::Scrollbar. Thanks for the explaination!
On 2013/06/24 21:38:04, trchen wrote: > https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... > File cc/input/inner_viewport_scrollbar.cc (right): > > https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... > cc/input/inner_viewport_scrollbar.cc:49: return track_rect.width() - ratio * > scroll_layer_->max_scroll_offset().x(); > On 2013/06/21 18:20:53, wjmaclean wrote: > > On 2013/06/19 23:38:28, enne wrote: > > > (2) More importantly, why is this math on the main thread? These values will > > be > > > frozen and pushed to the ScrollbarLayerImpl, which will then not update as > the > > > clip and scroll layers move around. > > > > I guess we could just provide dummy values here, and only go with the impl > side > > values. (I had thought we would want to have valid 'initial' values, but we > > could likely do without.) > > I think we can do without dummy values. Most of the time dummy accessors are > only added to make LayerTreeHostCommon templates happy. (e.g. scrollDelta) > > How are you going to implement this though? Likely you'll need to update the > LayerImpl raw pointers or layer IDs during tree synchronization. Yes, that is the plan. This CL is just about creating the layers, and does not claim to fully wire them up ... that's going to happen in (probably) 2 follow on CLs. > Are you going to make a twin class for the main-thread counterpart? I guess I had figure this *was* the main thread counterpart (since it's required for the creation of the ScrollbarLayer, and not the impl-layer). Since it pulls the values from the corresponding main-thread layers it will be as-up-to-date as any main-thread scrollbar I think. The values will get pushed back to the main-thread side when we make the necessary adjustments to ApplyScrollAndScale to handle the inner & outer scroll layers. But the impl-side scrollbar layers will be sync'd to the right scroll layers, and will be directly updated from there, allowing them to always be current. Since the inner-viewport scrollbars will always be solid-color, this means we can fully update them on the impl-side.
On 2013/06/25 16:52:22, wjmaclean wrote: > > > Are you going to make a twin class for the main-thread counterpart? > > I guess I had figure this *was* the main thread counterpart (since it's required > for the creation of the ScrollbarLayer, and not the impl-layer). Since it pulls > the values from the corresponding main-thread layers it will be as-up-to-date as > any main-thread scrollbar I think. The values will get pushed back to the > main-thread side when we make the necessary adjustments to ApplyScrollAndScale > to handle the inner & outer scroll layers. Sure, but why does the main thread need these values at all? It just needs to know that there's a solid color scrollbar there. All the scrollbar math you put there could just be dummy functions and it wouldn't affect anything. I see can see how the existing code has led you to write this patch within the existing system, but an end result that has useless code says to me that the existing system has some serious design smell. I haven't had quite enough time to sit and think about what the right answer is here, so I'm just going to talk out loud. It seems to me like solid color scrollbars are a bit of a different beast than main thread-painted scrollbars. They update their thumb size. They don't actually need any resources from the main thread. They more or less calculate all of their scrollbar geometry on the compositor thread. Maybe we just need to enshrine that distinction and create a SolidColorScrollbarLayer(Impl) so that you don't have to try to wrangle these inner viewport scrollbars into a box that they don't fit in. I'm not totally sure how the end result of that would look, but it's one suggestion to investigate. Anybody else have thoughts here?
cc::Scrollbar::ThumbLength()/etc shouldn't be called at all for a solid color scrollbar. I don't think you should implement them for scrollbars that are intended to always be used for solid color scrollbar layers. For solid color scrollbars, we only need the Orientation() / IsOverlay() bits. It might be useful to have a separate interface/type for solid color scrollbar layers to make this more explicit.
https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... File cc/input/inner_viewport_scrollbar.cc (right): https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... cc/input/inner_viewport_scrollbar.cc:45: gfx::Rect track_rect = TrackRect(); this is dead code https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... cc/input/inner_viewport_scrollbar.cc:58: if (orientation_ == HORIZONTAL) I'm pretty sure this is completely dead code.
On 2013/07/03 21:12:14, jamesr wrote: > https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... > File cc/input/inner_viewport_scrollbar.cc (right): > > https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... > cc/input/inner_viewport_scrollbar.cc:45: gfx::Rect track_rect = TrackRect(); > this is dead code > > https://codereview.chromium.org/16679011/diff/22002/cc/input/inner_viewport_s... > cc/input/inner_viewport_scrollbar.cc:58: if (orientation_ == HORIZONTAL) > I'm pretty sure this is completely dead code. Closing this review, transferring to https://codereview.chromium.org/23922006/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
