RESOLVED WONTFIX 72632
[Chromium] Make GestureRecognizer Parameters Configurable (Incomplete - Seeking Feedback)
https://bugs.webkit.org/show_bug.cgi?id=72632
Summary [Chromium] Make GestureRecognizer Parameters Configurable (Incomplete - Seeki...
Eugene Girard
Reported 2011-11-17 10:31:40 PST
The GestureRecognizer currently uses several (5) hard-coded parameters. We would like to make these configurable.
Attachments
Patch (6.51 KB, patch)
2011-11-17 10:39 PST, Eugene Girard
no flags
Patch (15.39 KB, patch)
2011-12-06 15:28 PST, Eugene Girard
no flags
Patch (14.92 KB, patch)
2011-12-06 16:15 PST, Eugene Girard
no flags
Patch (18.71 KB, patch)
2012-01-06 12:29 PST, Eugene Girard
no flags
Eugene Girard
Comment 1 2011-11-17 10:39:32 PST
WebKit Review Bot
Comment 2 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.
WebKit Review Bot
Comment 3 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.
Robert Kroeger
Comment 4 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.
Darin Fisher (:fishd, Google)
Comment 5 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?
Eugene Girard
Comment 6 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?
Darin Fisher (:fishd, Google)
Comment 7 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?
Robert Kroeger
Comment 8 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?
Eugene Girard
Comment 9 2011-12-06 15:28:54 PST
Eugene Girard
Comment 10 2011-12-06 15:36:45 PST
The previous patch contains one typo/merge mangling.
WebKit Review Bot
Comment 11 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
Eugene Girard
Comment 12 2011-12-06 16:15:53 PST
Robert Kroeger
Comment 13 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.
Fady Samuel
Comment 14 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
Eric Seidel (no email)
Comment 15 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.
Robert Kroeger
Comment 16 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.
Eric Seidel (no email)
Comment 17 2011-12-21 11:39:17 PST
Comment on attachment 118134 [details] Patch Not up for review yet it seems.
Eugene Girard
Comment 18 2012-01-06 12:29:24 PST
James Robinson
Comment 19 2012-02-21 20:48:18 PST
Comment on attachment 121469 [details] Patch I believe the GestureRecognizer is gone from WebKit. Clearing review? flag.
Eugene Girard
Comment 20 2012-02-24 05:48:01 PST
Agreed. This functionality has been removed from WebKit. Closing WONTFIX.
Note You need to log in before you can comment on or make changes to this bug.