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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72632





--- Comment #4 from Robert Kroeger <rjkroege at chromium.org>  2011-11-17 12:59:19 PST ---
(From update of attachment 115619)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.