Bug 104427 – WebViewClient::didHandleGestureEvent needs to distinguish the case if the event is processed or swallowed

• Component: New Bugs
• Status: RESOLVED FIXED

[Tien-Ren Chen]

WebViewClient::didHandleGestureEvent needs to distinguish the case if the event is processed or swallowed

[Tien-Ren Chen]

Created attachment 178324 [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.

[Tien-Ren Chen]

Created attachment 178325 [details]
Patch

[WebKit Review Bot]

Comment on attachment 178325 [details]
Patch

Attachment 178325 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15183654

New failing tests:
WebViewTest.ClientTapHandling

[Darin Fisher (:fishd, Google)]

Comment on attachment 178325 [details]
Patch

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

> Source/WebKit/chromium/public/WebViewClient.h:148
> +    enum EventStatus {

nit: It feels like this enum would be better at the top-level.  WebViewClient is already a big interface.

> Source/WebKit/chromium/public/WebViewClient.h:149
> +        EventStatusBubbledUp,

nit: Another name for EventStatusBubbledUp would be EventStatusUnprocessed or EventStatusNotProcessed, right?
Such a name might be easier to understand.  It was not obvious to me that "bubbled up" meant that WebKit
didn't handle the event.

Q: Are there cases where the event might get handled, but still propagate to another handler that would
then cancel further event propagation?

[Tien-Ren Chen]

(In reply to comment #5)
> (From update of attachment 178325 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178325&action=review
> 
> > Source/WebKit/chromium/public/WebViewClient.h:148
> > +    enum EventStatus {
> 
> nit: It feels like this enum would be better at the top-level.  WebViewClient is already a big interface.

Do you mean move to the top-level and also to a separate header?
My thought is that put it under WebViewClient is good for namespace management. WebKit is probably a bigger interface than WebKit::WebViewClient right? :)

> > Source/WebKit/chromium/public/WebViewClient.h:149
> > +        EventStatusBubbledUp,
> 
> nit: Another name for EventStatusBubbledUp would be EventStatusUnprocessed or EventStatusNotProcessed, right?
> Such a name might be easier to understand.  It was not obvious to me that "bubbled up" meant that WebKit
> didn't handle the event.

Got it. Will change.

> Q: Are there cases where the event might get handled, but still propagate to another handler that would
> then cancel further event propagation?

I don't think so. If that's the case then we also need to move the didHandleGestureEvent hook point.
Personally I don't like the hook approach. A cleaner way would be looking at the return value from WebViewImpl::handleInputEvent. However I don't know what will be the side-effects if I change it that way.

[Tien-Ren Chen]

Created attachment 178664 [details]
Patch

[Tien-Ren Chen]

Ping. Do we need improvements other than mentioned above?

[James Robinson]

IMO it really doesn't make a lot of sense to have this on WebView-anything since input event handling is a WebWidget concept, not WebView.

[Tien-Ren Chen]

Created attachment 181814 [details]
Patch

[Tien-Ren Chen]

(In reply to comment #9)
> IMO it really doesn't make a lot of sense to have this on WebView-anything since input event handling is a WebWidget concept, not WebView.

It makes sense to move didHandleGestureEvent to WebWidgetClient. This new patch does it and I'm uploading a chromium patch to move corresponding handler from RenderViewImpl to RenderWidget.

I think we still need to handle passed-in events in WebViewImpl. After all we need someone to forward the events to the WebCore. Even if we can refactor the code to do the otherwise, it is out of the scope of this patch.

[Adam Barth]

Comment on attachment 181814 [details]
Patch

LGTM, but I'm not sure if jamesr or fishd have more comments.  Please give them a chance to comment before landing.

[James Robinson]

Comment on attachment 181814 [details]
Patch

Me likey!

[WebKit Review Bot]

Comment on attachment 181814 [details]
Patch

Clearing flags on attachment: 181814

Committed r139532: <http://trac.webkit.org/changeset/139532>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.