[webkit-reviews] review denied: [Bug 54417] [chromium] Add a basic gesture recognizer to the Chromium platform : [Attachment 92841] patch2

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Robert Kroeger
<rjkroege at chromium.org>'s request for review:
Bug 54417: [chromium] Add a basic gesture recognizer to the Chromium platform
https://bugs.webkit.org/show_bug.cgi?id=54417

Attachment 92841: patch2
https://bugs.webkit.org/attachment.cgi?id=92841&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92841&action=review

I don't really understand gestures, so the below are just general nits.  Also,
none of this code seems to be Chromium specific.  Perhaps it shouldn't be
Chromium specific?  As an example, WebKit has image decodes that are used by
some (but not all) platforms.  This seems like a general platform-agnostic
implementation that some ports might want to replace with native gesture
managers.  As such, it shouldn't be in Chromium-specific files.

> Source/WebCore/platform/PlatformGestureRecognizer.cpp:39
> +#if !PLATFORM(CHROMIUM)

We probably should exclude this file from the Chromium build instead of adding
platform-specific ifdefs to a platform agnostic file.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:40
> +// Edge functions.

I'd remove this comment.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:49
> +// Maximum hold down (s) for a touch to be treated as a click.
> +static const double maxClickDownTime = 0.8;

I'd rename this constant so that the comment is not needed.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:52
> +// Minimum hold down (s) for a touch to be treated as a click.
> +static const double minClickDownTime = 0.01;

ditto

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:56
> +// Maximum manhattan displacement for touch motion to
> +// still be considered as a click.
> +static const int maxManhattanDistance = 20;

ditto

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:66
> +// TODO(rjkroege): write this function

WebKit uses FIXME comments rather than TODO comments.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:173
> +//
> +// Edge functions
> +//

No need for these kinds of comments.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:176
> +    gm->setState(GestureRecognizerChromium::PendingSyntheticClick);

gm => gestureRecongizer

WebKit prefers variable names that are made of complete english words.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:180
> +bool reset(GestureRecognizerChromium* gm, const PlatformTouchPoint& p)

Unused parameters should have their names omitted (otherwise they won't build
on PLATFORM(MAC))

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:48
> +// A GestureRecognizerChromium detects gestures occurring in the touch
event.
> +// In response to a given touch event, the GestureRecognizerChromium,
updates
> +// its internal state and optionally dispatches synthetic events to the
> +// invoking WebViewImpl.

WebKit doesn't usually have class-level comments.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:52
> +    // The states of the gesture manager state machine.

This comment is useless.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:64
> +    // Adds a new edge function to the GestureRecognizerChromium.

Almost all of these comment don't add any information and should be removed.