[webkit-reviews] review denied: [Bug 90316] [Chromium]Add DoubleTap gesture : [Attachment 159213] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied yusufo at chromium.org's request for
review:
Bug 90316: [Chromium]Add DoubleTap gesture
https://bugs.webkit.org/show_bug.cgi?id=90316

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

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


Sounds like we'll need to iterate a bit on this patch.

> Source/WebKit/chromium/src/WebViewImpl.cpp:698
> +    case WebInputEvent::GestureDoubleTap: {
> +	   animateZoomAroundPoint(WebPoint(event.x, event.y), DoubleTap);
> +	   return true;
> +    }

Why don't we want to give WebCore a chance to handle this gesture first via
handleGestureEvent ?  I would expect us to call animateZoomAroundPoint as the
default event handler after WebCore has processed it.  Maybe I don't understand
how gesture events work in enough detail...

> Source/WebKit/chromium/src/WebViewImpl.cpp:1061
> +	   // be visible). TODO(johnme): Revisit this if it isn't always true.

TODO(johnme): -> FIXME:

(WebKit uses FIXME rather than TODO and doesn't list user names in the
comments.)

> Source/WebKit/chromium/src/WebViewImpl.cpp:1143
> +	 scroll = clampOffsetAtScale(scroll, scale);

Four-space indent.