Bug 96112 – [chromium] Add a method didHandleGestureEvent to WebViewClient, called from WebViewImpl::handleGestureEvent.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oli Lan]

[chromium] Allow WebViewClients to handle tap and longpress gesture events.

[Oli Lan]

Created attachment 162779 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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.

[Adam Barth]

I spoke with olilan by chat and he's going to investigate whether Android can use the existing mouse event simulation code.

[Oli Lan]

Created attachment 164415 [details]
Patch

[Oli Lan]

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.

[Adam Barth]

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?

[Oli Lan]

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.

[Oli Lan]

Created attachment 164536 [details]
Patch

[Adam Barth]

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?

[Adam Barth]

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?

[Adam Barth]

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.

[Oli Lan]

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.

[Oli Lan]

Created attachment 164723 [details]
Patch

[Adam Barth]

Comment on attachment 164723 [details]
Patch

Thanks for iterating on this patch.

[WebKit Review Bot]

Comment on attachment 164723 [details]
Patch

Clearing flags on attachment: 164723

Committed r129029: <http://trac.webkit.org/changeset/129029>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.