Summary: | [chromium] make trackpad gesture curves configurable externally | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robert Kroeger <rjkroege> | ||||||||||||||
Component: | Platform | Assignee: | 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
Robert Kroeger
2012-03-20 07:20:34 PDT
Created attachment 132829 [details]
Patch
Early feedback welcome. Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. 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 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. (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? (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. Created attachment 132853 [details]
Patch
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 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 Created attachment 132896 [details]
Patch
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 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 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 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 Created attachment 137162 [details]
Patch
jamesr@: please look again as you see fit. 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 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 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 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) Created attachment 138873 [details]
Patch
Comment on attachment 138873 [details] Patch Attachment 138873 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12518744 Created attachment 139015 [details]
Patch
fixed build issue from previous version. jamesr@ please take another look. Comment on attachment 139015 [details]
Patch
Are you still interested in having this patch reviewed?
During an offline discussion with jamesr@, we decided to not proceed with this until necessary. My apologies for failing to update. 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). |