Bug 72632 - [Chromium] Make GestureRecognizer Parameters Configurable (Incomplete - Seeking Feedback)
Summary: [Chromium] Make GestureRecognizer Parameters Configurable (Incomplete - Seeki...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eugene Girard
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-17 10:31 PST by Eugene Girard
Modified: 2012-02-24 05:48 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.51 KB, patch)
2011-11-17 10:39 PST, Eugene Girard
no flags Details | Formatted Diff | Diff
Patch (15.39 KB, patch)
2011-12-06 15:28 PST, Eugene Girard
no flags Details | Formatted Diff | Diff
Patch (14.92 KB, patch)
2011-12-06 16:15 PST, Eugene Girard
no flags Details | Formatted Diff | Diff
Patch (18.71 KB, patch)
2012-01-06 12:29 PST, Eugene Girard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eugene Girard 2011-11-17 10:31:40 PST
The GestureRecognizer currently uses several (5) hard-coded parameters.  We would like to make these configurable.
Comment 1 Eugene Girard 2011-11-17 10:39:32 PST
Created attachment 115619 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-17 10:41:43 PST
Attachment 115619 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/PlatformGestureRec..." exit_code: 1

Source/WebKit/chromium/public/WebSettings.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/public/WebSettings.h:138:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:50:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:52:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:91:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:93:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/chromium/GestureRecognizerChromium.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/chromium/GestureRecognizerChromium.h:62:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/PlatformGestureRecognizer.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/PlatformGestureRecognizer.h:62:  Missing space after ,  [whitespace/comma] [3]
Source/WebKit/chromium/src/WebSettingsImpl.h:129:  Missing space after ,  [whitespace/comma] [3]
Source/WebKit/chromium/src/WebSettingsImpl.cpp:479:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 12 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 WebKit Review Bot 2011-11-17 10:42:01 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Robert Kroeger 2011-11-17 12:59:19 PST
Comment on attachment 115619 [details]
Patch

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

I have some quibbles. Am looking forward to seeing the Chrome side. Do you need a unit test?

>> Source/WebCore/platform/PlatformGestureRecognizer.h:38
>> +#include <map>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

You must use wtf/*

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:37
> +    double maximumTouchDownDurationInSecondsForClick = 0.8;

I would make these static member variables.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:43
> +    enum ConfigurationSetting {

You will need this enum elsewhere. In particular, you would need this down in Chrome land. At a minimum, you will want this enum in the header file. It also should not be in the anonymous name space.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:49
> +    };

need a blank line between these blocks

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:50
>> +    double readConfiguration(const std::map<int,double>& configuration, ConfigurationSetting index, double defaultValue)
> 
> Missing space after ,  [whitespace/comma] [3]

why not make this a method?

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:52
>> +        std::map<int,double>::const_iterator item = configuration.find(index);
> 
> Missing space after ,  [whitespace/comma] [3]

you are in webcore here.  WTF::HashMap

> Source/WebKit/chromium/src/WebSettingsImpl.cpp:476
> +    OwnPtr<PlatformGestureRecognizer> recognizer = PlatformGestureRecognizer::create();

you are building an instance to set what are class variables. This seems undesirable to me.
Comment 5 Darin Fisher (:fishd, Google) 2011-11-17 13:43:54 PST
Comment on attachment 115619 [details]
Patch

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

> Source/WebKit/chromium/src/WebSettingsImpl.h:128
> +    virtual void setGestureParameters(const std::map<int,double>&);

hmm.. we don't have a good way of pasing a map across the webkit API boundary.
we've thus far avoided using std::map.  in this case, a simple structure with
five named fields would seem to be OK.  why not do that?
Comment 6 Eugene Girard 2011-11-17 15:29:25 PST
(In reply to comment #5)
> (From update of attachment 115619 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115619&action=review
> 
> > Source/WebKit/chromium/src/WebSettingsImpl.h:128
> > +    virtual void setGestureParameters(const std::map<int,double>&);
> 
> hmm.. we don't have a good way of pasing a map across the webkit API boundary.
> we've thus far avoided using std::map.  in this case, a simple structure with
> five named fields would seem to be OK.  why not do that?

I was hoping to minimize the impact on the public UI (PlatformGestureRecognizer.h) since other implementations would need different parameters.  Is there support for passing a WTF structure?
Comment 7 Darin Fisher (:fishd, Google) 2011-11-18 09:54:17 PST
(In reply to comment #6)
> I was hoping to minimize the impact on the public UI (PlatformGestureRecognizer.h) since other implementations would need different parameters.  Is there support for passing a WTF structure?

There is not support for passing a WTF structure through the WebKit API.  The WebKit API exists to isolate WTF and WebCore types from Chromium.

Since we are just talking about the Chromium WebKit API, why do other implementations matter?
Comment 8 Robert Kroeger 2011-11-18 09:57:26 PST
(In reply to comment #7)
> (In reply to comment #6)
> > I was hoping to minimize the impact on the public UI (PlatformGestureRecognizer.h) since other implementations would need different parameters.  Is there support for passing a WTF structure?
> 
> There is not support for passing a WTF structure through the WebKit API.  The WebKit API exists to isolate WTF and WebCore types from Chromium.
> 
> Since we are just talking about the Chromium WebKit API, why do other implementations matter?

How about making an abstract "GestureRecognizerParameters" class with a GestureRecognizerParametersChromium subclass that contains a struct. This lets the PlatformGestureRecognizer interface stay platform-neutral and the GRPC as Chromium-only code can be safely built/modified from the Chromium WebKit API?
Comment 9 Eugene Girard 2011-12-06 15:28:54 PST
Created attachment 118124 [details]
Patch
Comment 10 Eugene Girard 2011-12-06 15:36:45 PST
The previous patch contains one typo/merge mangling.
Comment 11 WebKit Review Bot 2011-12-06 15:50:20 PST
Comment on attachment 118124 [details]
Patch

Attachment 118124 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10746071
Comment 12 Eugene Girard 2011-12-06 16:15:53 PST
Created attachment 118134 [details]
Patch
Comment 13 Robert Kroeger 2011-12-08 07:56:39 PST
Comment on attachment 118134 [details]
Patch

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

> Source/WebCore/platform/PlatformGestureRecognizer.h:59
> +    // Settings that control gesture recognition.

this change is inserting platform-dependent config into what ought to be a platform-independent api.

I still maintain that a different mechanism is needed.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:36
> +double GestureRecognizerChromium::s_maximumTouchDownDurationInSecondsForClick = 0.8;

Didn't know that s_ is the convention for this. Cool

> Source/WebKit/chromium/public/WebView.h:441
> +    // Settings that control gesture recognition.

A large part of me would really like to see this interface aggregated. An array of doubles?

Also: you can enclose all of this in a #if ENABLE(GESTURE_RECOGNIZER)?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3074
> +void WebViewImpl::setGestureMaximumTouchDownDurationInSecondsForClick(double val)

enclose entire with #if ENABLE(G...)? If not, why?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3086
> +    return 0;

I like having the trailing . on the 0 so it's easier to remember that this is a double constant.
Comment 14 Fady Samuel 2011-12-08 08:09:45 PST
(In reply to comment #13)
> (From update of attachment 118134 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118134&action=review
> 
> > Source/WebCore/platform/PlatformGestureRecognizer.h:59
> > +    // Settings that control gesture recognition.
> 
> this change is inserting platform-dependent config into what ought to be a platform-independent api.
> 
> I still maintain that a different mechanism is needed.
> 
> > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:36
> > +double GestureRecognizerChromium::s_maximumTouchDownDurationInSecondsForClick = 0.8;
> 
> Didn't know that s_ is the convention for this. Cool
> 
> > Source/WebKit/chromium/public/WebView.h:441
> > +    // Settings that control gesture recognition.
> 
> A large part of me would really like to see this interface aggregated. An array of doubles?
> 
> Also: you can enclose all of this in a #if ENABLE(GESTURE_RECOGNIZER)?
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3074
> > +void WebViewImpl::setGestureMaximumTouchDownDurationInSecondsForClick(double val)
> 
> enclose entire with #if ENABLE(G...)? If not, why?
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3086
> > +    return 0;
> 
> I like having the trailing . on the 0 so it's easier to remember that this is a double constant.

http://www.webkit.org/coding/coding-style.html
Comment 15 Eric Seidel (no email) 2011-12-19 10:35:08 PST
Comment on attachment 118134 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

How do we test this gesture recognition?  Tests are required for changes to WebCore when possible/practical.
Comment 16 Robert Kroeger 2011-12-19 10:45:00 PST
(In reply to comment #15)
> (From update of attachment 118134 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118134&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> How do we test this gesture recognition?  Tests are required for changes to WebCore when possible/practical.

I note in passing (largely as a suggestion for Eugene) that there are WebKit unit tests for the gesture recognizer -- they could be exercised with different settings. Alternatively, there are also layout tests exercising the gesture recognizer and these could executed for different parameters configured via a windows.internals interface.
Comment 17 Eric Seidel (no email) 2011-12-21 11:39:17 PST
Comment on attachment 118134 [details]
Patch

Not up for review yet it seems.
Comment 18 Eugene Girard 2012-01-06 12:29:24 PST
Created attachment 121469 [details]
Patch
Comment 19 James Robinson 2012-02-21 20:48:18 PST
Comment on attachment 121469 [details]
Patch

I believe the GestureRecognizer is gone from WebKit. Clearing review? flag.
Comment 20 Eugene Girard 2012-02-24 05:48:01 PST
Agreed.  This functionality has been removed from WebKit.  Closing WONTFIX.