Bug 81663 - [chromium] make trackpad gesture curves configurable externally
: [chromium] make trackpad gesture curves configurable externally
Status: RESOLVED WONTFIX
: WebKit
Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-03-20 07:20 PST by
Modified: 2012-07-27 07:19 PST (History)


Attachments
Patch (6.35 KB, patch)
2012-03-20 08:25 PST, Robert Kroeger
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.27 KB, patch)
2012-03-20 11:01 PST, Robert Kroeger
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.69 KB, patch)
2012-03-20 14:22 PST, Robert Kroeger
no flags Review Patch | Details | Formatted Diff | Diff
Patch (23.34 KB, patch)
2012-04-13 15:22 PST, Robert Kroeger
no flags Review Patch | Details | Formatted Diff | Diff
Patch (25.35 KB, patch)
2012-04-25 14:28 PST, Robert Kroeger
no flags Review Patch | Details | Formatted Diff | Diff
Patch (25.36 KB, patch)
2012-04-26 09:10 PST, Robert Kroeger
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-20 07:20:34 PST
Provide a mechanism so that the embedder can configure a trackpad gesture curve.
------- Comment #1 From 2012-03-20 08:25:57 PST -------
Created an attachment (id=132829) [details]
Patch
------- Comment #2 From 2012-03-20 08:27:31 PST -------
Early feedback welcome.
------- Comment #3 From 2012-03-20 08:28:47 PST -------
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
------- Comment #4 From 2012-03-20 09:08:55 PST -------
(From update of attachment 132829 [details])
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 From 2012-03-20 09:19:13 PST -------
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 From 2012-03-20 09:42:21 PST -------
(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 From 2012-03-20 10:01:25 PST -------
(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 From 2012-03-20 11:01:30 PST -------
Created an attachment (id=132853) [details]
Patch
------- Comment #9 From 2012-03-20 11:47:20 PST -------
(From update of attachment 132853 [details])
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 From 2012-03-20 13:55:20 PST -------
(From update of attachment 132853 [details])
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 From 2012-03-20 14:22:05 PST -------
Created an attachment (id=132896) [details]
Patch
------- Comment #12 From 2012-03-20 14:24:36 PST -------
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 From 2012-03-20 16:37:47 PST -------
(From update of attachment 132896 [details])
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 From 2012-03-20 18:54:07 PST -------
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 From 2012-03-21 09:06:05 PST -------
(From update of attachment 132896 [details])
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 From 2012-04-13 15:22:14 PST -------
Created an attachment (id=137162) [details]
Patch
------- Comment #17 From 2012-04-13 15:23:32 PST -------
jamesr@: please look again as you see fit.
------- Comment #18 From 2012-04-13 15:41:17 PST -------
(From update of attachment 137162 [details])
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 From 2012-04-16 08:55:56 PST -------
(From update of attachment 137162 [details])
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 From 2012-04-16 12:58:21 PST -------
(From update of attachment 137162 [details])
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 From 2012-04-25 14:24:37 PST -------
(From update of attachment 137162 [details])
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 From 2012-04-25 14:28:51 PST -------
Created an attachment (id=138873) [details]
Patch
------- Comment #23 From 2012-04-25 19:38:59 PST -------
(From update of attachment 138873 [details])
Attachment 138873 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12518744
------- Comment #24 From 2012-04-26 09:10:40 PST -------
Created an attachment (id=139015) [details]
Patch
------- Comment #25 From 2012-04-26 09:11:38 PST -------
fixed build issue from previous version.
------- Comment #26 From 2012-04-27 07:21:15 PST -------
jamesr@ please take another look.
------- Comment #27 From 2012-07-27 01:56:06 PST -------
(From update of attachment 139015 [details])
Are you still interested in having this patch reviewed?
------- Comment #28 From 2012-07-27 06:57:17 PST -------
During an offline discussion with jamesr@, we decided to not proceed with this until necessary. My apologies for failing to update.
------- Comment #29 From 2012-07-27 07:19:34 PST -------
(From update of attachment 139015 [details])
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).