Bug 96112

Created attachment 162779 [details] Patch

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.

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.

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

Created attachment 164415 [details] Patch

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 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 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.

Created attachment 164536 [details] Patch

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 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 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 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.

Created attachment 164723 [details] Patch

Comment on attachment 164723 [details] Patch Thanks for iterating on this patch.

Comment on attachment 164723 [details] Patch Clearing flags on attachment: 164723 Committed r129029: <http://trac.webkit.org/changeset/129029>

All reviewed patches have been landed. Closing bug.