Bug 75591 – [chromium] Make WebViewImpl depend on WebLayerTreeView instead of CCLayerTreeHost

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Make WebViewImpl depend on WebLayerTreeView instead of CCLayerTreeHost

[James Robinson]

Created attachment 121196 [details]
Patch

[James Robinson]

Patch depends on https://bugs.webkit.org/show_bug.cgi?id=75577 - this diff is against the tree with that patch applied.

[James Robinson]

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?

[Antoine Labour]

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?

[James Robinson]

(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.

[Antoine Labour]

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.

[James Robinson]

(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.

[Antoine Labour]

(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.

[James Robinson]

Created attachment 121377 [details]
split the input related things into a separate interface

[James Robinson]

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.

[Antoine Labour]

(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.

[Nat Duca]

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.

[James Robinson]

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/

[James Robinson]

Comment on attachment 121377 [details]
split the input related things into a separate interface

Darin, WDYT?

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

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?

[James Robinson]

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...

[Nat Duca]

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...

[James Robinson]

Created attachment 123036 [details]
Patch

[James Robinson]

(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.

[James Robinson]

Chromium side updated at http://codereview.chromium.org/9122020/ , that side will have to land first.

[Nat Duca]

(In reply to comment #20)
> See the comment immediately above yours.

Ahah, sorry! ~failhat~

[James Robinson]

ping?

[James Robinson]

Created attachment 126179 [details]
Patch

[James Robinson]

Rebased to ToT.  Works fine, modulo a few bits of alpha on scrollbars on linux in some pixel results.

[James Robinson]

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.

[James Robinson]

Created attachment 128602 [details]
Patch

[James Robinson]

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?

[Alexandre Elias]

Yes, the comment for startPageScaleAnimation is accurate.

[James Robinson]

Comment on attachment 128602 [details]
Patch

Will need to update for http://trac.webkit.org/changeset/108829

[James Robinson]

Created attachment 128803 [details]
Patch

[Kenneth Russell]

Comment on attachment 128803 [details]
Patch

Maybe enne could unofficially review?

[James Robinson]

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.

[Adrienne Walker]

Comment on attachment 128803 [details]
Patch

LGTM.  This all seems pretty mechanical and straightforward.

[Kenneth Russell]

Comment on attachment 128803 [details]
Patch

rs=me

[James Robinson]

Committed r108883: <http://trac.webkit.org/changeset/108883>