[chromium] Make WebViewImpl depend on WebLayerTreeView instead of CCLayerTreeHost
Created attachment 121196 [details] Patch
Patch depends on https://bugs.webkit.org/show_bug.cgi?id=75577 - this diff is against the tree with that patch applied.
Also note that this patch has WebKit API changes so it'll require a chromium side change. Once https://bugs.webkit.org/show_bug.cgi?id=75577 lands I'll update this patch with transitional macros. Antoine - can you check over the WebLayerTreeView initialize() call and see if it looks good to you?
Comment on attachment 121196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121196&action=review I assume you have a chrome-side change that goes with this? > Source/WebKit/chromium/public/platform/WebLayerTreeView.h:135 > + WEBKIT_EXPORT void setNeedsRedraw(); Should we provide a rect here, so that we can do a partial redraw if we have partial damage? > Source/WebKit/chromium/public/platform/WebLayerTreeView.h:141 > + WEBKIT_EXPORT void setPageScaleFactorLimits(float minimum, float maximum); I'm not sure I understand what these 2 functions do, and whether they belong to this class... Is this something that could be handled by adding a transform to the root layer?
(In reply to comment #4) > (From update of attachment 121196 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121196&action=review > > I assume you have a chrome-side change that goes with this? Yup, this won't compile without chromium-side changes. I'll put those up tomorrow. > > > Source/WebKit/chromium/public/platform/WebLayerTreeView.h:135 > > + WEBKIT_EXPORT void setNeedsRedraw(); > > Should we provide a rect here, so that we can do a partial redraw if we have partial damage? We could, but we don't currently have any callers that provide partial damage on the WebKit side. Currently it's called when the entire front buffer is invalid or gone. I'd prefer to add support for rects once we have a caller. > > > Source/WebKit/chromium/public/platform/WebLayerTreeView.h:141 > > + WEBKIT_EXPORT void setPageScaleFactorLimits(float minimum, float maximum); > > I'm not sure I understand what these 2 functions do, and whether they belong to this class... > Is this something that could be handled by adding a transform to the root layer? These are mostly to do with thread-initiated zoom gestures (pinch zoom, etc). The scroll bounds are set by per-layer properties but the page scale limits are global. The scale itself can't really be hidden in the root layer's transform since the combination of the specified page scale and any thread-driven page scale delta has to respect these limits.
Comment on attachment 121196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121196&action=review >>> Source/WebKit/chromium/public/platform/WebLayerTreeView.h:141 >>> + WEBKIT_EXPORT void setPageScaleFactorLimits(float minimum, float maximum); >> >> I'm not sure I understand what these 2 functions do, and whether they belong to this class... >> Is this something that could be handled by adding a transform to the root layer? > > These are mostly to do with thread-initiated zoom gestures (pinch zoom, etc). The scroll bounds are set by per-layer properties but the page scale limits are global. The scale itself can't really be hidden in the root layer's transform since the combination of the specified page scale and any thread-driven page scale delta has to respect these limits. Would it make sense to move these, as well as maybe setHaveWheelEventHandlers to a separate interface (that this one would reference), so that this class only deals with rendering, and out-of-thread input handling would be delegated to a separate class? I'm worried that this public interface ends up being a big soup of random methods. There's a bunch of behaviors that we can imagine handling on the compositor thread, which will all need their own tunables & constraints. Even animation-related methods could be delegated to that separate class (or another one), so that this one stays purely about rendering (as the name suggests). In any case, I would probably rename those 2 as s/Page/View/ or something, since we don't generally deal with pages here.
(In reply to comment #6) > (From update of attachment 121196 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121196&action=review > > >>> Source/WebKit/chromium/public/platform/WebLayerTreeView.h:141 > >>> + WEBKIT_EXPORT void setPageScaleFactorLimits(float minimum, float maximum); > >> > >> I'm not sure I understand what these 2 functions do, and whether they belong to this class... > >> Is this something that could be handled by adding a transform to the root layer? > > > > These are mostly to do with thread-initiated zoom gestures (pinch zoom, etc). The scroll bounds are set by per-layer properties but the page scale limits are global. The scale itself can't really be hidden in the root layer's transform since the combination of the specified page scale and any thread-driven page scale delta has to respect these limits. > > Would it make sense to move these, as well as maybe setHaveWheelEventHandlers to a separate interface (that this one would reference), so that this class only deals with rendering, and out-of-thread input handling would be delegated to a separate class? I'm worried that this public interface ends up being a big soup of random methods. There's a bunch of behaviors that we can imagine handling on the compositor thread, which will all need their own tunables & constraints. > Even animation-related methods could be delegated to that separate class (or another one), so that this one stays purely about rendering (as the name suggests). That's not a bad idea. Any ideas for a name for this class? > > In any case, I would probably rename those 2 as s/Page/View/ or something, since we don't generally deal with pages here. The property is pageScale so I'd like to preserve that.
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 121196 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=121196&action=review > > > > >>> Source/WebKit/chromium/public/platform/WebLayerTreeView.h:141 > > >>> + WEBKIT_EXPORT void setPageScaleFactorLimits(float minimum, float maximum); > > >> > > >> I'm not sure I understand what these 2 functions do, and whether they belong to this class... > > >> Is this something that could be handled by adding a transform to the root layer? > > > > > > These are mostly to do with thread-initiated zoom gestures (pinch zoom, etc). The scroll bounds are set by per-layer properties but the page scale limits are global. The scale itself can't really be hidden in the root layer's transform since the combination of the specified page scale and any thread-driven page scale delta has to respect these limits. > > > > Would it make sense to move these, as well as maybe setHaveWheelEventHandlers to a separate interface (that this one would reference), so that this class only deals with rendering, and out-of-thread input handling would be delegated to a separate class? I'm worried that this public interface ends up being a big soup of random methods. There's a bunch of behaviors that we can imagine handling on the compositor thread, which will all need their own tunables & constraints. > > Even animation-related methods could be delegated to that separate class (or another one), so that this one stays purely about rendering (as the name suggests). > > That's not a bad idea. Any ideas for a name for this class? How about WebLayerTreeAnimationController or something along these lines? > > > > > In any case, I would probably rename those 2 as s/Page/View/ or something, since we don't generally deal with pages here. > > The property is pageScale so I'd like to preserve that.
Created attachment 121377 [details] split the input related things into a separate interface
I've split setHaveWheel..() and setPageScale*() into a separate interface. Is this what you had in mind? I don't think these are properly called WLTAnimationController since they aren't about animations, they are about how inputs are handled. WLTInputParams is probably not a great name either. This version churns refcount a lot too, but that's fairly easy to fix.
(In reply to comment #10) > I've split setHaveWheel..() and setPageScale*() into a separate interface. Is this what you had in mind? I don't think these are properly called WLTAnimationController since they aren't about animations, they are about how inputs are handled. WLTInputParams is probably not a great name either. This version churns refcount a lot too, but that's fairly easy to fix. WLTAnimationController was a suggestion if we put animation there too. I guess we're not. We could name it WLTInputController or something like that, since it's a bit more than a bag of properties, and will probably grow to contain active functions. I'm not really worried about refcount churn. I'm fine with this, modulo the naming bikeshed.
Just to recap something I said to James on IM, we had long modeled LayerTreeView/Host around the concept of a View that you see in other systems. E.g. NSView, or GtkFrame. Those "base interfaces" really do grow to be a pile of stuff, and personally, I dont think thats a bad thing. There are lots of ways to combat this from a code clarity point of view --- in this case, the technique that WebViewImpl uses, where we group functionality with comment blocks, seems particularly of note.
I actually think it will just stay a property bag, the actual input handling is on different interfaces (since it's called from different threads). Chromium side up here: http://codereview.chromium.org/9122020/
Comment on attachment 121377 [details] split the input related things into a separate interface Darin, WDYT?
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 121377 [details] split the input related things into a separate interface View in context: https://bugs.webkit.org/attachment.cgi?id=121377&action=review > Source/WebKit/chromium/public/platform/WebLayerTreeInputParams.h:59 > + WebPrivatePtr<WebCore::CCLayerTreeHost> m_private; It is interesting that WebLayerTreeView is also just a ref-count wrapper for CCLayerTreeHost. Might it make sense to also create a CCLayerTreeInputParams class? Normally, the WebKit API makes the most sense when API classes just wrap WebCore/lower-level concepts. Maybe this is all just transitional anyways?
Comment on attachment 121377 [details] split the input related things into a separate interface After thinking about this a while I think splitting the input stuff into a separate interface is not an improvement and makes lifetime/ownership really complicated. I'll break things up by comments + blocks in WLTV.h instead and see how that goes. New patch coming up...
Comment on attachment 121377 [details] split the input related things into a separate interface View in context: https://bugs.webkit.org/attachment.cgi?id=121377&action=review > Source/WebKit/chromium/public/platform/WebLayerTreeInputParams.h:40 > +class WebLayerTreeInputParams { /me wonders why these aren't just methods on the wrapper around CCLayerTreeHost. I dont see a value add. > Source/WebKit/chromium/src/WebLayerTreeView.cpp:133 > +WebLayerTreeInputParams WebLayerTreeView::inputParams() Is this supposed to be returning a reference? I'm a little confused...
Created attachment 123036 [details] Patch
(In reply to comment #18) > (From update of attachment 121377 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121377&action=review > > > Source/WebKit/chromium/public/platform/WebLayerTreeInputParams.h:40 > > +class WebLayerTreeInputParams { > > /me wonders why these aren't just methods on the wrapper around CCLayerTreeHost. I dont see a value add. See the comment immediately above yours.
Chromium side updated at http://codereview.chromium.org/9122020/ , that side will have to land first.
(In reply to comment #20) > See the comment immediately above yours. Ahah, sorry! ~failhat~
ping?
Created attachment 126179 [details] Patch
Rebased to ToT. Works fine, modulo a few bits of alpha on scrollbars on linux in some pixel results.
Comment on attachment 126179 [details] Patch Antoine provided feedback offline that this is adding too many event-related things to WLTV. I'm going to attempt to cut some of this out with some other refactorings and then return to this.
Created attachment 128602 [details] Patch
I've moved the wheel event handling stuff out to layers but I think the rest of the interfaces here have to stay. Page scale factor, in particular is really a property of a view and not a property of any specific layer. Alexandre - can you double-check if I've correctly characterized what startPageScaleAnimation does?
Yes, the comment for startPageScaleAnimation is accurate.
Comment on attachment 128602 [details] Patch Will need to update for http://trac.webkit.org/changeset/108829
Created attachment 128803 [details] Patch
Comment on attachment 128803 [details] Patch Maybe enne could unofficially review?
That would be great, although Enne's gardening right now and I want to respect their time. I'm also OK with waiting for Antoine to get back since he had comments on this in the past - I don't have any patches that are blocked on this currently.
Comment on attachment 128803 [details] Patch LGTM. This all seems pretty mechanical and straightforward.
Comment on attachment 128803 [details] Patch rs=me
Committed r108883: <http://trac.webkit.org/changeset/108883>