Bug 72632 – [Chromium] Make GestureRecognizer Parameters Configurable (Incomplete - Seeking Feedback)

• Status: RESOLVED
• Resolution: WONTFIX
• Component: UI Events

[Eugene Girard]

The GestureRecognizer currently uses several (5) hard-coded parameters.  We would like to make these configurable.

[Eugene Girard]

Created attachment 115619 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Robert Kroeger]

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.

[Darin Fisher (:fishd, Google)]

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?

[Eugene Girard]

(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?

[Darin Fisher (:fishd, Google)]

(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?

[Robert Kroeger]

(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?

[Eugene Girard]

Created attachment 118124 [details]
Patch

[Eugene Girard]

The previous patch contains one typo/merge mangling.

[WebKit Review Bot]

Comment on attachment 118124 [details]
Patch

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

[Eugene Girard]

Created attachment 118134 [details]
Patch

[Robert Kroeger]

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.

[Fady Samuel]

(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

[Eric Seidel (no email)]

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.

[Robert Kroeger]

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

[Eric Seidel (no email)]

Comment on attachment 118134 [details]
Patch

Not up for review yet it seems.

[Eugene Girard]

Created attachment 121469 [details]
Patch

[James Robinson]

Comment on attachment 121469 [details]
Patch

I believe the GestureRecognizer is gone from WebKit. Clearing review? flag.

[Eugene Girard]

Agreed.  This functionality has been removed from WebKit.  Closing WONTFIX.