Bug 96112

Summary: [chromium] Add a method didHandleGestureEvent to WebViewClient, called from WebViewImpl::handleGestureEvent.
Product: WebKit Reporter: Oli Lan <olilan>
Component: New BugsAssignee: Oli Lan <olilan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, fishd, jamesr, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66687    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Oli Lan 2012-09-07 08:16:22 PDT
[chromium] Allow WebViewClients to handle tap and longpress gesture events.
Comment 1 Oli Lan 2012-09-07 08:19:39 PDT
Created attachment 162779 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-07 08:23:31 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adam Barth 2012-09-14 10:03:16 PDT
I suspect we're going to want fishd to review this patch.

I've seen this in the diff for a while, but I don't fully understand why it is necessary.  For example, why isn't WebKit responsible for simulating mouse events from touch events?  My understanding is that we already have code to do that.
Comment 4 Adam Barth 2012-09-14 10:38:06 PDT
I spoke with olilan by chat and he's going to investigate whether Android can use the existing mouse event simulation code.
Comment 5 Oli Lan 2012-09-17 10:41:51 PDT
Created attachment 164415 [details]
Patch
Comment 6 Oli Lan 2012-09-17 10:43:38 PDT
Thanks Adam. I am changing our implementation in Android to use the existing tap/longpress handling as discussed. We still need some client-specific behaviour, so Patch 2 now adds methods didHandleTapEvent and didHandleLongPressEvent, which are called when the events are handled but do not replace the standard handling.
Comment 7 Adam Barth 2012-09-17 11:42:20 PDT
Comment on attachment 164415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164415&action=review

Thanks.  This is much better.  A couple small points below.

> Source/WebKit/chromium/public/WebViewClient.h:147
> +    // Called when GestureTap or GestureLongPress are handled.
> +    virtual void didHandleTapEvent(int x, int y) { }
> +    virtual void didHandleLongPressEvent(int x, int y) { }

Should we just pass the whole WebGestureEvent?  The client might be interested in more fields in the future and then we won't need to rev the API

> Source/WebKit/chromium/src/WebViewImpl.cpp:736
> +        m_client->didHandleTapEvent(event.x, event.y);
> +
>          return gestureHandled;

Do we want to call this API when gestureHandled is both true and when it is false?  It seems a bit odd to tell the client that we handled this event when gestureHandled is false...  Maybe we should pass this bool to the client?
Comment 8 Oli Lan 2012-09-18 05:16:31 PDT
Comment on attachment 164415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164415&action=review

>> Source/WebKit/chromium/public/WebViewClient.h:147
>> +    virtual void didHandleLongPressEvent(int x, int y) { }
> 
> Should we just pass the whole WebGestureEvent?  The client might be interested in more fields in the future and then we won't need to rev the API

Yes, that makes sense. Once we're passing the gesture event though, it doesn't seem right to have separate methods for tap and long press as we'll be passing the event type in the WebGestureEvent. So I'll change this to add just a didHandleGestureEvent method and call it for any gesture event.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:736
>>          return gestureHandled;
> 
> Do we want to call this API when gestureHandled is both true and when it is false?  It seems a bit odd to tell the client that we handled this event when gestureHandled is false...  Maybe we should pass this bool to the client?

Good point; passing the bool makes sense.
Comment 9 Oli Lan 2012-09-18 05:21:27 PDT
Created attachment 164536 [details]
Patch
Comment 10 Adam Barth 2012-09-18 13:05:45 PDT
Comment on attachment 164536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164536&action=review

> Source/WebKit/chromium/public/WebViewClient.h:146
> +    virtual void didHandleGestureEvent(const WebGestureEvent& event, bool handled) { }

The handled parameter here still seems a bit odd.  What does it mean for didHandleGestureEvent to be called when handled is false?
Comment 11 Adam Barth 2012-09-18 13:15:01 PDT
Comment on attachment 164536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164536&action=review

>> Source/WebKit/chromium/public/WebViewClient.h:146
>> +    virtual void didHandleGestureEvent(const WebGestureEvent& event, bool handled) { }
> 
> The handled parameter here still seems a bit odd.  What does it mean for didHandleGestureEvent to be called when handled is false?

This might just be a naming issue.  Would defaultWasPrevented be an appropriate name?
Comment 12 Adam Barth 2012-09-18 13:18:27 PDT
Comment on attachment 164536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164536&action=review

>>> Source/WebKit/chromium/public/WebViewClient.h:146
>>> +    virtual void didHandleGestureEvent(const WebGestureEvent& event, bool handled) { }
>> 
>> The handled parameter here still seems a bit odd.  What does it mean for didHandleGestureEvent to be called when handled is false?
> 
> This might just be a naming issue.  Would defaultWasPrevented be an appropriate name?

Looking around in the code, "eventSwallowed" looks like it might be an appropriate name.
Comment 13 Oli Lan 2012-09-19 05:57:41 PDT
Comment on attachment 164536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164536&action=review

>>>> Source/WebKit/chromium/public/WebViewClient.h:146
>>>> +    virtual void didHandleGestureEvent(const WebGestureEvent& event, bool handled) { }
>>> 
>>> The handled parameter here still seems a bit odd.  What does it mean for didHandleGestureEvent to be called when handled is false?
>> 
>> This might just be a naming issue.  Would defaultWasPrevented be an appropriate name?
> 
> Looking around in the code, "eventSwallowed" looks like it might be an appropriate name.

OK, changing to eventSwallowed.
Comment 14 Oli Lan 2012-09-19 05:58:59 PDT
Created attachment 164723 [details]
Patch
Comment 15 Adam Barth 2012-09-19 11:46:21 PDT
Comment on attachment 164723 [details]
Patch

Thanks for iterating on this patch.
Comment 16 WebKit Review Bot 2012-09-19 12:07:15 PDT
Comment on attachment 164723 [details]
Patch

Clearing flags on attachment: 164723

Committed r129029: <http://trac.webkit.org/changeset/129029>
Comment 17 WebKit Review Bot 2012-09-19 12:07:19 PDT
All reviewed patches have been landed.  Closing bug.