Bug 100675 – [chromium] Provide WebKit API interface for platform gesture curves

• Component: UI Events
• Status: RESOLVED FIXED

[Robert Kroeger]

Currently there is code in Platform to animate selected inertial fling initiating gestures. One gesture curve does not fit all platforms. Add interfaces that permit the Chromium platform to provide its fling animation implementation from outside of WebKit.

[Robert Kroeger]

Created attachment 171461 [details]
Patch

[Robert Kroeger]

Comment on attachment 171461 [details]
Patch

jamesr@: Please take a look.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 171461 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171461&action=review

> Source/Platform/chromium/public/WebGestureCurve.h:41
> +    // Returns a name of the curve for use in debugging.
> +    virtual const char* debugName() const = 0;

Should we make this #ifndef NDEBUG?

> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:27
> +

No need for this blank line.

> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:31
> +#include "public/WebGestureCurve.h"
> +#include "public/WebGestureCurveTarget.h"

We've been using the <public/WebGestureCurve.h> style when including these sorts of headers.

> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:33
> +#if PLATFORM(CHROMIUM)

No need for PLAFORM(CHROMIUM) ifdefs in the chromium directory.

> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:51
> +#if PLATFORM(CHROMIUM)

ditto

> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:69
> +    , m_waitingForFirstTick(false)

Its odd that you initialize this to true above and false here.

[James Robinson]

Comment on attachment 171461 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171461&action=review

This looks very similar to WebFlingAnimator.  Did you consider consolidating?

> Source/Platform/chromium/public/WebGestureCurveTarget.h:35
> +    // FIXME: add interfaces for scroll(), etc.

I don't understand this FIXME. What other interfaces would you need?

> Source/WebKit/chromium/public/platform/WebGestureCurve.h:1
> +/*

Why are you adding this header?

> Source/WebKit/chromium/public/platform/WebGestureCurveTarget.h:1
> +/*

Why this header?

> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:33
> +#if PLATFORM(CHROMIUM)

You're in a directory called .../chromium/..., the PLATFORM(CHROMIUM) check is redundant

> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:51
> +#if PLATFORM(CHROMIUM)

ditto

> Source/WebKit/chromium/src/WebActiveGestureAnimation.h:46
> +    static PassOwnPtr<WebActiveGestureAnimation> create(PassOwnPtr<WebGestureCurve>, WebGestureCurveTarget*);
> +    static PassOwnPtr<WebActiveGestureAnimation> create(PassOwnPtr<WebGestureCurve>, WebGestureCurveTarget*, double startTime);

WebKit style would be to just have one function with a default parameter instead of two

[Robert Kroeger]

Comment on attachment 171461 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171461&action=review

>> Source/Platform/chromium/public/WebGestureCurve.h:41
>> +    virtual const char* debugName() const = 0;
> 
> Should we make this #ifndef NDEBUG?

This method is used to help populate TRACE_EVENT_ASYNC_BEGIN1 calls in users of WebGestureCurve instances so perhaps not. The function name is misleading. I have renamed in p2.

>> Source/Platform/chromium/public/WebGestureCurveTarget.h:35
>> +    // FIXME: add interfaces for scroll(), etc.
> 
> I don't understand this FIXME. What other interfaces would you need?

oversight. Left over from sed-ing over PlatformGestureCurveTarget.h

>> Source/WebKit/chromium/public/platform/WebGestureCurve.h:1
>> +/*
> 
> Why are you adding this header?

Was unclear on its necessity. Removed in p2

>> Source/WebKit/chromium/public/platform/WebGestureCurveTarget.h:1
>> +/*
> 
> Why this header?

Was unclear on its necessity. Removed in p2

>> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:27
>> +
> 
> No need for this blank line.

done, p2

>> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:31
>> +#include "public/WebGestureCurveTarget.h"
> 
> We've been using the <public/WebGestureCurve.h> style when including these sorts of headers.

done, p2

>>> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:33
>>> +#if PLATFORM(CHROMIUM)
>> 
>> No need for PLAFORM(CHROMIUM) ifdefs in the chromium directory.
> 
> You're in a directory called .../chromium/..., the PLATFORM(CHROMIUM) check is redundant

done, p2

>>> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:51
>>> +#if PLATFORM(CHROMIUM)
>> 
>> ditto
> 
> ditto

done p2

>> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:69
>> +    , m_waitingForFirstTick(false)
> 
> Its odd that you initialize this to true above and false here.

All curves (conceptually) begin in the m_waitingForFirstTick  = true state as they run their time from 0 and have never ticked. A curve with a non-0 startTime would imply that we're replicating a curve that was already being animated on impl and is being re-created on main so it has already ticked. 

I think that I can make this clearer and delete some code while I'm at it. Done in p2.

>> Source/WebKit/chromium/src/WebActiveGestureAnimation.h:46
>> +    static PassOwnPtr<WebActiveGestureAnimation> create(PassOwnPtr<WebGestureCurve>, WebGestureCurveTarget*, double startTime);
> 
> WebKit style would be to just have one function with a default parameter instead of two

I renamed the creates to make their usage clearer. Different and hopefully better in p2.

[Robert Kroeger]

(In reply to comment #5)
> (From update of attachment 171461 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171461&action=review
> 
> This looks very similar to WebFlingAnimator.  Did you consider consolidating?

AFAIK: WebFlingAnimator is only as a virtual interface via WebFlingAnimatorToGestureCurveAdapter. So, I was planning on moving a version of WebFlingAnimatorToGestureCurveAdapter to Chromium and wrapping WebFlingAnimator's implementation in Chromium with the adapter so that it would implement WebGestureCurve.

[Robert Kroeger]

Created attachment 171531 [details]
Patch

[Robert Kroeger]

jamesr@ could you please take another look?

[James Robinson]

Comment on attachment 171531 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171531&action=review

One more round and then we should be good to go.

> Source/Platform/chromium/public/WebGestureCurve.h:41
> +    // Returns a name of the curve for use in tracing.
> +    virtual const char* traceName() const = 0;

I don't think you should add this - put the tracing on the implementation side if you want to trace with additional information known only by the implementation. The lifetime of the WebGestureCurve is the same as that of the animation so you will get the same data out.  This isn't logically part of the API.

> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:1
> +/*

This file doesn't have any dependencies on things in Source/WebKit/ or Source/WebCore and is implementing a Platform API, so it should be in Source/Platform/chromium/src

> Source/WebKit/chromium/src/WebActiveGestureAnimation.h:1
> +/*

This file as well doesn't have any dependencies on things in Source/WebKit/ or Source/WebCore and is implementing a Platform API, so it should be in Source/Platform/chromium/src

[Robert Kroeger]

Comment on attachment 171531 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171531&action=review

>> Source/Platform/chromium/public/WebGestureCurve.h:41
>> +    virtual const char* traceName() const = 0;
> 
> I don't think you should add this - put the tracing on the implementation side if you want to trace with additional information known only by the implementation. The lifetime of the WebGestureCurve is the same as that of the animation so you will get the same data out.  This isn't logically part of the API.

done in p3.

>> Source/WebKit/chromium/src/WebActiveGestureAnimation.cpp:1
>> +/*
> 
> This file doesn't have any dependencies on things in Source/WebKit/ or Source/WebCore and is implementing a Platform API, so it should be in Source/Platform/chromium/src

moved elsewhere and renamed in p3 -- see next comment.

>> Source/WebKit/chromium/src/WebActiveGestureAnimation.h:1
>> +/*
> 
> This file as well doesn't have any dependencies on things in Source/WebKit/ or Source/WebCore and is implementing a Platform API, so it should be in Source/Platform/chromium/src

done in p3. 

But given that this class is effectively a helper for WebViewImpl, by analogy to PageWidgetDelegate.h, why doesn't it belong in Source/WebKit/chromium/src? Perhaps without the Web name prefix?

[Robert Kroeger]

Created attachment 171849 [details]
Patch

[Robert Kroeger]

Comment on attachment 171849 [details]
Patch

jamesr@ please take another look?

[James Robinson]

Comment on attachment 171849 [details]
Patch

R=me

Re putting it in Source/WebKit/chromium/src vs Source/Platform/chromium/src: we have two WebKit APIs, a client API (lives in WebKit/chromium/public) and a platform API (Platform/chromium/public).  The ideal is to keep dependencies a DAG.  Implementations of the client API can depend on all of WebKit since users of the client API are depending on WebKit.  The client API can't be used by anything outside of the WebKit layer (meaning WebCore / Platform can't use client API concepts).  Implementations of the platform API can't depend on anything in WebKit outside of the platform API, but anything in WebKit/WebCore/Platform can depend on them.

Since you've defined Web*Gesture* as part of the platform API, the implementation belongs in the Platform location.  This means anything in WebKit/WebCore/Platform are able to depend on it.  If the only caller is in the client API implementation (WebViewImpl) then if you want you can move the whole thing to the client API.  I think we'll want to depend on this from WebCore/platform/ScrollAnimatorNone so I'd be inclined to leave it where it is.

[WebKit Review Bot]

Comment on attachment 171849 [details]
Patch

Clearing flags on attachment: 171849

Committed r133203: <http://trac.webkit.org/changeset/133203>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Robert Kroeger]

(In reply to comment #14)
> (From update of attachment 171849 [details])
> R=me
> 
> Re putting it in Source/WebKit/chromium/src vs Source/Platform/chromium/src: we have two WebKit APIs, a client API (lives in WebKit/chromium/public) and a platform API (Platform/chromium/public).  The ideal is to keep dependencies a DAG.  Implementations of the client API can depend on all of WebKit since users of the client API are depending on WebKit.  The client API can't be used by anything outside of the WebKit layer (meaning WebCore / Platform can't use client API concepts).  Implementations of the platform API can't depend on anything in WebKit outside of the platform API, but anything in WebKit/WebCore/Platform can depend on them.
> 
> Since you've defined Web*Gesture* as part of the platform API, the implementation belongs in the Platform location.  This means anything in WebKit/WebCore/Platform are able to depend on it.  If the only caller is in the client API implementation (WebViewImpl) then if you want you can move the whole thing to the client API.  I think we'll want to depend on this from WebCore/platform/ScrollAnimatorNone so I'd be inclined to leave it where it is.

Thanks for the info. This was very helpful.