The GestureRecognizer currently uses several (5) hard-coded parameters. We would like to make these configurable.
Created attachment 115619 [details] Patch
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.
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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 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?
(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?
(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?
(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?
Created attachment 118124 [details] Patch
The previous patch contains one typo/merge mangling.
Comment on attachment 118124 [details] Patch Attachment 118124 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10746071
Created attachment 118134 [details] Patch
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.
(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 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.
(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 on attachment 118134 [details] Patch Not up for review yet it seems.
Created attachment 121469 [details] Patch
Comment on attachment 121469 [details] Patch I believe the GestureRecognizer is gone from WebKit. Clearing review? flag.
Agreed. This functionality has been removed from WebKit. Closing WONTFIX.