[Webkit-unassigned] [Bug 54417] [chromium] Add a basic gesture recognizer to the Chromium platform

[bugzilla-daemon at webkit.org]

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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #92841|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #4 from Adam Barth <abarth at webkit.org>  2011-05-09 14:01:47 PST ---
(From update of attachment 92841)
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.

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