Bug 95756 – [chromium] Enable different fling behaviour for touchscreen and touchpad

• Component: UI Events
• Status: RESOLVED FIXED

[Robert Kroeger]

It is desirable to provide different fling animation curves for touchscreen and touchpad initiated flings. Differentiate the GestureFlingStart events in these two cases and use this difference to select different curves via a factory.

[Robert Kroeger]

Created attachment 162068 [details]
Patch

[Robert Kroeger]

jamesr@: Could you take a look please.

[James Robinson]

Comment on attachment 162068 [details]
Patch

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

> Source/WebCore/platform/TouchpadFlingPlatformGestureCurve.cpp:56
> +PassOwnPtr<PlatformGestureCurve> TouchpadFlingPlatformGestureCurve::create(const FloatPoint& velocity, const IntPoint& cumulativeScroll)

Why did you change this?

IntPoint is 2 ints - same size as a reference on 64 bit systems

> Source/WebCore/platform/chromium/support/PlatformGestureCurveFactory.h:40
>      PassOwnPtr<WebCore::PlatformGestureCurve> createCurve(const WebCore::FloatPoint&, const WebCore::IntRect& range);
> +    PassOwnPtr<WebCore::PlatformGestureCurve> createCurve(int eventModifiers, const WebCore::FloatPoint&, const WebCore::IntPoint& cumulativeScroll = WebCore::IntPoint());

could these names describe what they are for? they both create curves, obviously, but beyond that I can't tell when one or the other should be used

> Source/WebKit/chromium/public/WebInputEvent.h:156
> +        // Modifier for gesture events. Set if the event comes from a touchscreen.
> +        TouchScreenSourced = 1 << 11,

Hmm, are you sure this makes sense as a modifier on all inputs? So far the only thing we want it for is GestureScrollBegin. What about putting it on WebGestureEvent's flingStart instead?

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

[Robert Kroeger]

Comment on attachment 162068 [details]
Patch

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

>> Source/WebCore/platform/TouchpadFlingPlatformGestureCurve.cpp:56
>> +PassOwnPtr<PlatformGestureCurve> TouchpadFlingPlatformGestureCurve::create(const FloatPoint& velocity, const IntPoint& cumulativeScroll)
> 
> Why did you change this?
> 
> IntPoint is 2 ints - same size as a reference on 64 bit systems

To match the constructor. But as you point out, completely unnecessary. Removed in p2.

>> Source/WebCore/platform/chromium/support/PlatformGestureCurveFactory.h:40
>> +    PassOwnPtr<WebCore::PlatformGestureCurve> createCurve(int eventModifiers, const WebCore::FloatPoint&, const WebCore::IntPoint& cumulativeScroll = WebCore::IntPoint());
> 
> could these names describe what they are for? they both create curves, obviously, but beyond that I can't tell when one or the other should be used

I have merged into one in p2. I think this change is more of a step towards aligning CrOS/Android fling implementations.

>> Source/WebKit/chromium/public/WebInputEvent.h:156
>> +        TouchScreenSourced = 1 << 11,
> 
> Hmm, are you sure this makes sense as a modifier on all inputs? So far the only thing we want it for is GestureScrollBegin. What about putting it on WebGestureEvent's flingStart instead?

Not entirely but it was simple and didn't seem too much of a stretch given that some other modifiers (e.g. mouse buttons) only apply to specific event types. I've coded it up as a modifier on GestureFlingStart in p2.

[Robert Kroeger]

Created attachment 162242 [details]
Patch

[James Robinson]

Comment on attachment 162242 [details]
Patch

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

> Source/WebKit/chromium/public/WebInputEvent.h:406
> +        int sourceDevice;

Why not just bool or an 2-state enum? Bitfield seems unnecessarily big and extra unwieldy

[James Robinson]

Comment on attachment 162242 [details]
Patch

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

> Source/WebKit/chromium/public/WebActiveWheelFlingParameters.h:42
> +    int sourceDevice;

If a fling is from a touchscreen, you won't need to transfer it so you'll never need to use this path.

[Robert Kroeger]

Comment on attachment 162242 [details]
Patch

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

>> Source/WebKit/chromium/public/WebActiveWheelFlingParameters.h:42
>> +    int sourceDevice;
> 
> If a fling is from a touchscreen, you won't need to transfer it so you'll never need to use this path.

I agree that this is the intent. And it continues to be my intent to write a patch that makes it so. But it's not true at present: fling is still via WebCompositorInputHandlerImpl::scrollBy by way of manufactured WheelEvents. And my preference would be to do that change separate from this change. Are you amenable to this?

>> Source/WebKit/chromium/public/WebInputEvent.h:406
>> +        int sourceDevice;
> 
> Why not just bool or an 2-state enum? Bitfield seems unnecessarily big and extra unwieldy

enum seems cleaner. Done in p3

[Robert Kroeger]

Created attachment 162304 [details]
Patch

[James Robinson]

Comment on attachment 162304 [details]
Patch

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

> Source/WebCore/platform/chromium/support/PlatformGestureCurveFactory.cpp:51
> +    if (!flingAnimator && deviceSource == WebGestureEvent::Touchscreen)

early return would read better and be shorter - i.e. if the flingAnimator is non-null at this point, construct + return a WebFlingAni..Adapter. Then you can write the rest of the function without the !flingAnimator checks

> Source/WebKit/chromium/public/WebActiveWheelFlingParameters.h:42
> +    int sourceDevice;

wrong type - this should be a WebGestureEvent::SourceDevice, right?

> Source/WebKit/chromium/public/WebInputEvent.h:406
> +        int sourceDevice;

int != enum - use the more specific type

[Robert Kroeger]

Comment on attachment 162304 [details]
Patch

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

>> Source/WebCore/platform/chromium/support/PlatformGestureCurveFactory.cpp:51
>> +    if (!flingAnimator && deviceSource == WebGestureEvent::Touchscreen)
> 
> early return would read better and be shorter - i.e. if the flingAnimator is non-null at this point, construct + return a WebFlingAni..Adapter. Then you can write the rest of the function without the !flingAnimator checks

Much nicer with your suggestion. Thanks.

>> Source/WebKit/chromium/public/WebActiveWheelFlingParameters.h:42
>> +    int sourceDevice;
> 
> wrong type - this should be a WebGestureEvent::SourceDevice, right?

Definitely, done p4

>> Source/WebKit/chromium/public/WebInputEvent.h:406
>> +        int sourceDevice;
> 
> int != enum - use the more specific type

Done in p4

[Robert Kroeger]

Created attachment 162321 [details]
Patch

[WebKit Review Bot]

Comment on attachment 162321 [details]
Patch

Rejecting attachment 162321 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
nputHandlerImpl.cpp
Hunk #2 FAILED at 267.
1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp.rej
patching file Source/WebKit/chromium/src/WebViewImpl.cpp
Hunk #3 FAILED at 693.
1 out of 4 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/WebViewImpl.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13774175

[Robert Kroeger]

Created attachment 162510 [details]
Patch: fixed merge conflict.

[WebKit Review Bot]

Comment on attachment 162510 [details]
Patch: fixed merge conflict.

Clearing flags on attachment: 162510

Committed r127767: <http://trac.webkit.org/changeset/127767>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.