Bug 81663

Summary: [chromium] make trackpad gesture curves configurable externally
Product: WebKit Reporter: Robert Kroeger <rjkroege>
Component: PlatformAssignee: Robert Kroeger <rjkroege>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, davemoore, dglazkov, fishd, jamesr, rjkroege, tkent, tkent+wkapi, webkit.review.bot, wjmaclean
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Robert Kroeger 2012-03-20 07:20:34 PDT
Provide a mechanism so that the embedder can configure a trackpad gesture curve.
Comment 1 Robert Kroeger 2012-03-20 08:25:57 PDT
Created attachment 132829 [details]
Patch
Comment 2 Robert Kroeger 2012-03-20 08:27:31 PDT
Early feedback welcome.
Comment 3 WebKit Review Bot 2012-03-20 08:28:47 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 WebKit Review Bot 2012-03-20 09:08:55 PDT
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
Comment 5 James Robinson 2012-03-20 09:19:13 PDT
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.
Comment 6 W. James MacLean 2012-03-20 09:42:21 PDT
(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?
Comment 7 Robert Kroeger 2012-03-20 10:01:25 PDT
(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.
Comment 8 Robert Kroeger 2012-03-20 11:01:30 PDT
Created attachment 132853 [details]
Patch
Comment 9 James Robinson 2012-03-20 11:47:20 PDT
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
Comment 10 WebKit Review Bot 2012-03-20 13:55:20 PDT
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
Comment 11 Robert Kroeger 2012-03-20 14:22:05 PDT
Created attachment 132896 [details]
Patch
Comment 12 WebKit Review Bot 2012-03-20 14:24:36 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 13 WebKit Review Bot 2012-03-20 16:37:47 PDT
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
Comment 14 Robert Kroeger 2012-03-20 18:54:07 PDT
I need to extend the approach here to add additional parameters per the changes in https://bugs.webkit.org/show_bug.cgi?id=81713
Comment 15 James Robinson 2012-03-21 09:06:05 PDT
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
Comment 16 Robert Kroeger 2012-04-13 15:22:14 PDT
Created attachment 137162 [details]
Patch
Comment 17 Robert Kroeger 2012-04-13 15:23:32 PDT
jamesr@: please look again as you see fit.
Comment 18 W. James MacLean 2012-04-13 15:41:17 PDT
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?
Comment 19 Robert Kroeger 2012-04-16 08:55:56 PDT
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).
Comment 20 James Robinson 2012-04-16 12:58:21 PDT
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?
Comment 21 Robert Kroeger 2012-04-25 14:24:37 PDT
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)
Comment 22 Robert Kroeger 2012-04-25 14:28:51 PDT
Created attachment 138873 [details]
Patch
Comment 23 WebKit Review Bot 2012-04-25 19:38:59 PDT
Comment on attachment 138873 [details]
Patch

Attachment 138873 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12518744
Comment 24 Robert Kroeger 2012-04-26 09:10:40 PDT
Created attachment 139015 [details]
Patch
Comment 25 Robert Kroeger 2012-04-26 09:11:38 PDT
fixed build issue from previous version.
Comment 26 Robert Kroeger 2012-04-27 07:21:15 PDT
jamesr@ please take another look.
Comment 27 Adam Barth 2012-07-27 01:56:06 PDT
Comment on attachment 139015 [details]
Patch

Are you still interested in having this patch reviewed?
Comment 28 Robert Kroeger 2012-07-27 06:57:17 PDT
During an offline discussion with jamesr@, we decided to not proceed with this until necessary. My apologies for failing to update.
Comment 29 WebKit Review Bot 2012-07-27 07:19:34 PDT
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).