WebViewClient::didHandleGestureEvent needs to distinguish the case if the event is processed or swallowed
Created attachment 178324 [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.
Created attachment 178325 [details] Patch
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
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?
(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.
Created attachment 178664 [details] Patch
Ping. Do we need improvements other than mentioned above?
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.
Created attachment 181814 [details] Patch
(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.
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.
Comment on attachment 181814 [details] Patch Me likey!
Comment on attachment 181814 [details] Patch Clearing flags on attachment: 181814 Committed r139532: <http://trac.webkit.org/changeset/139532>
All reviewed patches have been landed. Closing bug.