[webkit-reviews] review granted: [Bug 90316] [Chromium]Add DoubleTap gesture : [Attachment 166100] Patch

[bugzilla-daemon at webkit.org]

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

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

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


This looks fine.  In general, we prefer for tests to use the API whenever
possible so that our tests don't impose constraints on how WebCore evolves.

I'm deferring to aelias for the actual subject matter of the patch.  Most of my
review comments have been about the structure of the code.

> Source/WebKit/chromium/src/WebViewImpl.cpp:751
> +	   eventSwallowed = true;

I guess the model here is that we're always going to swallow the double-tap
gesture and never pass it along.  The other option is always pass it along and
then call animateZoomAroundPoint if no one else swallows the event.  It's not
entirely clear to me which is better. I guess we can try this for now and then
starting passing the event along later.

> Source/WebKit/chromium/src/WebViewImpl.h:570
> +    // Exposed for testing purposes.

This comment isn't really needed anymore.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:329
> +    webViewImpl->mainFrameImpl()->frameView()->layout();

How does this differ from WebView::layout?  I'm not sur eI understand why we
need to pierce the API here.