Bug 81663 – [chromium] make trackpad gesture curves configurable externally

• Component: Platform
• Status: RESOLVED WONTFIX

[Robert Kroeger]

Provide a mechanism so that the embedder can configure a trackpad gesture curve.

[Robert Kroeger]

Created attachment 132829 [details]
Patch

[Robert Kroeger]

Early feedback welcome.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 132829 [details]
Patch

Attachment 132829 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12001344

New failing tests:
fast/events/touch/gesture/pad-gesture-cancel.html

[James Robinson]

Doesn't handle threaded compositor case. Can you put the parameters on the GestureFlingStart itself?  Seems reasonable that the browser would want to use different parameters for different inputs in the same WebView - say for instance if there were multiple input devices connected.

[W. James MacLean]

(In reply to comment #5)
> Doesn't handle threaded compositor case. Can you put the parameters on the GestureFlingStart itself?  Seems reasonable that the browser would want to use different parameters for different inputs in the same WebView - say for instance if there were multiple input devices connected.

If we do this (which is a good idea by the way), can we plumb though tau, and also 'alpha' to replace the hard-coded 1000 in the constructor for the time scale factor?

[Robert Kroeger]

(In reply to comment #6)
> (In reply to comment #5)
> > Doesn't handle threaded compositor case. Can you put the parameters on the GestureFlingStart itself?  Seems reasonable that the browser would want to use different parameters for different inputs in the same WebView - say for instance if there were multiple input devices connected.
> 
> If we do this (which is a good idea by the way), can we plumb though tau, and also 'alpha' to replace the hard-coded 1000 in the constructor for the time scale factor?

wjmaclean@: can we have more descriptive names than tau and alpha? And the patch already plumbs through a value that replaces the 1000.

jamesr@: sounds logical to send the params on the GFS. I'll change.

[Robert Kroeger]

Created attachment 132853 [details]
Patch

[James Robinson]

Comment on attachment 132853 [details]
Patch

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

> Source/WebKit/chromium/public/WebInputEvent.h:375
> +        , unitTimeScaleVelocityThreshold(0.0f)

I think it's worth making a dedicated type for fling, these don't make much sense for other types do they?

Otherwise the public WK changes lgtm

[WebKit Review Bot]

Comment on attachment 132853 [details]
Patch

Attachment 132853 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12050010

New failing tests:
fast/events/touch/gesture/pad-gesture-cancel.html

[Robert Kroeger]

Created attachment 132896 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 132896 [details]
Patch

Attachment 132896 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12034079

New failing tests:
fast/dom/error-to-string-stack-overflow.html

[Robert Kroeger]

I need to extend the approach here to add additional parameters per the changes in https://bugs.webkit.org/show_bug.cgi?id=81713

[James Robinson]

Comment on attachment 132896 [details]
Patch

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

Seems mostly OK. Some issues to resolve before landing.

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:43
> +    static PassOwnPtr<PlatformGestureCurve> create(const FloatPoint& velocity, float unitTimeScaleVelocityThreshold = 1000, float flingLaunchDuration = .25);

Why the default parameters? It appears these values are always set.

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:49
> +    explicit TouchFlingPlatformGestureCurve(const FloatPoint& velocity, float unitTimeScaleVelocityThreshold, float flingLaunchDuration);

no explicit for c'tors with >1 arg

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:55
> +

extra newline

[Robert Kroeger]

Created attachment 137162 [details]
Patch

[Robert Kroeger]

jamesr@: please look again as you see fit.

[W. James MacLean]

Comment on attachment 137162 [details]
Patch

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

> Source/WebCore/platform/TouchpadFlingPlatformGestureCurve.cpp:72
> +        p[i] = provided && !isnan(provided[i]) ? provided[i] : defaultEmpircalParameters[i];

This could lead to weird behaviour if one (but not all) values in provided were NaN (presumably not done on purpose) - do we want to worry about pathological cases?

[Robert Kroeger]

Comment on attachment 137162 [details]
Patch

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

>> Source/WebCore/platform/TouchpadFlingPlatformGestureCurve.cpp:72
>> +        p[i] = provided && !isnan(provided[i]) ? provided[i] : defaultEmpircalParameters[i];
> 
> This could lead to weird behaviour if one (but not all) values in provided were NaN (presumably not done on purpose) - do we want to worry about pathological cases?

My thinking was:

1. do the right thing if the browser provides no parameters -- because that's what will be happening between landing this and the browser side.
2. do something non-exception-generating if the browser provides less than the currently required parameters. 

Yes, behaviour would be weird with a partial set of parameters. The config flag is intended to facilitate debugging of curves (and presumes that one has read the comment in this file).

[James Robinson]

Comment on attachment 137162 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by James Robinson.

Leave this "Reviewed by NOBODY (OOPS!)" until you get an r+ on a patch.

> Source/WebCore/platform/TouchpadFlingPlatformGestureCurve.cpp:58
> +const float defaultEmpircalParameters[] = {1.5395e+01, 2.0466e+04, -2.9899e+04, 2.0577e+04, -5.4966e+03, 1.128445};

In this patch we have the default values here (near the end use) and lot of code leading up to this point that has to deal with the fact that a user may specify their own values or want to use these values. Seems like the whole world would be simpler if these default values were higher up the stack and always set if the user didn't specify them explicitly.

> Source/WebCore/platform/TouchpadFlingPlatformGestureCurve.cpp:71
> +    for (unsigned i = 0; i < sizeof(defaultEmpircalParameters) / sizeof(float); i++)

"sizeof(defaultEmpircalParameters) / sizeof(float)" is a mouthful to repeat - could you store it in a local? i notice that in other places in the code this is just a static number. why the pretend generality here?

> Source/WebKit/chromium/public/WebInputEvent.h:388
> +        const float NaN = 0.0f / 0.0f;

It's very weird to initialize to NaN - why would we want to do that instead of initializing to something sane (like the default curve parameters) or something like all 0s?  NaN is a footgun

> Source/WebKit/chromium/src/WebViewImpl.cpp:638
> +        if (event.size == sizeof(WebGestureFlingEvent)) {

Branching on the event.size seems pretty bizarre.  Is there no better way to do this?

[Robert Kroeger]

Comment on attachment 137162 [details]
Patch

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

>> Source/WebCore/ChangeLog:6
>> +        Reviewed by James Robinson.
> 
> Leave this "Reviewed by NOBODY (OOPS!)" until you get an r+ on a patch.

fixed in p5.

>> Source/WebCore/platform/TouchpadFlingPlatformGestureCurve.cpp:58
>> +const float defaultEmpircalParameters[] = {1.5395e+01, 2.0466e+04, -2.9899e+04, 2.0577e+04, -5.4966e+03, 1.128445};
> 
> In this patch we have the default values here (near the end use) and lot of code leading up to this point that has to deal with the fact that a user may specify their own values or want to use these values. Seems like the whole world would be simpler if these default values were higher up the stack and always set if the user didn't specify them explicitly.

Good point. 

I would like to keep the parameters here because they are specific to this kind of curve and a different kind of curve would end up having different parameters.

However, in p5 I attempted to make the rest of the stack oblivious to the value of the parameters other than how many it has to pass along.

>> Source/WebCore/platform/TouchpadFlingPlatformGestureCurve.cpp:71
>> +    for (unsigned i = 0; i < sizeof(defaultEmpircalParameters) / sizeof(float); i++)
> 
> "sizeof(defaultEmpircalParameters) / sizeof(float)" is a mouthful to repeat - could you store it in a local? i notice that in other places in the code this is just a static number. why the pretend generality here?

done in p5

>> Source/WebKit/chromium/public/WebInputEvent.h:388
>> +        const float NaN = 0.0f / 0.0f;
> 
> It's very weird to initialize to NaN - why would we want to do that instead of initializing to something sane (like the default curve parameters) or something like all 0s?  NaN is a footgun

fixed. p5.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:638
>> +        if (event.size == sizeof(WebGestureFlingEvent)) {
> 
> Branching on the event.size seems pretty bizarre.  Is there no better way to do this?

Yes: I added the number of params to GestureEvent. Seems nicer (p5)

[Robert Kroeger]

Created attachment 138873 [details]
Patch

[WebKit Review Bot]

Comment on attachment 138873 [details]
Patch

Attachment 138873 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12518744

[Robert Kroeger]

Created attachment 139015 [details]
Patch

[Robert Kroeger]

fixed build issue from previous version.

[Robert Kroeger]

jamesr@ please take another look.

[Adam Barth]

Comment on attachment 139015 [details]
Patch

Are you still interested in having this patch reviewed?

[Robert Kroeger]

During an offline discussion with jamesr@, we decided to not proceed with this until necessary. My apologies for failing to update.

[WebKit Review Bot]

Comment on attachment 139015 [details]
Patch

Cleared review? from attachment 139015 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).