[chromium] Allow plugins to opt-in to receive synthetic mouse events out of touch events.
Created attachment 175712 [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.
Hi! Does this approach look reasonable? I can add tests if so. The corresponding chromium change is: https://codereview.chromium.org/11418134/ A subsequent webkit CL will remove the deprecated WebPluginContainer::setIsAcceptingTouchEvents()
Comment on attachment 175712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175712&action=review > Source/WebKit/chromium/public/WebPluginContainer.h:53 > + TOUCH_EVENT_REQUEST_NONE, > + TOUCH_EVENT_REQUEST_RAW, > + TOUCH_EVENT_REQUEST_SYNTHESIZED_MOUSE This is the wrong style for enums. Take a look at how we do other enums in the API.
Created attachment 175809 [details] Patch
Comment on attachment 175712 [details] Patch Added a new test for the synthesized mouse events. View in context: https://bugs.webkit.org/attachment.cgi?id=175712&action=review >> Source/WebKit/chromium/public/WebPluginContainer.h:53 >> + TOUCH_EVENT_REQUEST_SYNTHESIZED_MOUSE > > This is the wrong style for enums. Take a look at how we do other enums in the API. Whoops, my bad. Sorry. Fixed. (I almost expected webkit-patch/style EWS bot to catch this)
Comment on attachment 175809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175809&action=review > Source/WebKit/chromium/public/WebPluginContainer.h:53 > + TouchEventRequestNone, > + TouchEventRequestRaw, > + TouchEventRequestSynthesizedMouse, TouchEventRequestNone -> TouchEventRequestTypeNone We reiterate the name of the enum in the values. See, for example, MovieLoadType.
Other than the nit above, the API change LGTM.
Created attachment 176032 [details] Renamed the enum members. Thanks!
Comment on attachment 176032 [details] Renamed the enum members. Thanks! Attachment 176032 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14987796
Comment on attachment 176032 [details] Renamed the enum members. Thanks! Attachment 176032 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14989738
Created attachment 176057 [details] Build fix!
Hi Tony! Would you please review this patch? Thanks!
Comment on attachment 176057 [details] Build fix! View in context: https://bugs.webkit.org/attachment.cgi?id=176057&action=review > Source/WebKit/chromium/public/WebPluginContainer.h:121 > + // Notifies when the plugin starts/stops accepting touch events. This is deprecated, use requestTouchEventType instead. > virtual void setIsAcceptingTouchEvents(bool) = 0; The plan is to remove this after updating all the callers on the Chromium side, right? > Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:527 > void WebPluginContainerImpl::setIsAcceptingTouchEvents(bool acceptingTouchEvents) > { > - if (m_isAcceptingTouchEvents == acceptingTouchEvents) > + requestTouchEventType(acceptingTouchEvents ? TouchEventRequestTypeRaw : TouchEventRequestTypeNone); > +} Do tests still call this? Could you replace the body with a NOT_REACHED()? > Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:124 > + static const WebString kPrimitiveTrue = WebString::fromUTF8("true"); > + static const WebString kPrimitiveSynthetic = WebString::fromUTF8("synthetic"); I would update the old tests to use "raw" instead of "true". It's confusing that the valid values for the attribute are "true" or "synthetic". Also, this should probably just be const char* to avoid initialization (operator== is defined for String, const char* so you can still use == below).
Created attachment 176295 [details] Patch
Comment on attachment 176057 [details] Build fix! View in context: https://bugs.webkit.org/attachment.cgi?id=176057&action=review >> Source/WebKit/chromium/public/WebPluginContainer.h:121 >> virtual void setIsAcceptingTouchEvents(bool) = 0; > > The plan is to remove this after updating all the callers on the Chromium side, right? Correct. Once this change is gardened for chromium, and the chromium CL can land, and once the chromium CL is gardened for webkit, I will put up the CL to remove this function. >> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:527 >> +} > > Do tests still call this? Could you replace the body with a NOT_REACHED()? This is still called by chromium. So when this patch is gardened in for chromium, replacing this function with NOT_REACHED will cause crash/misbehaviour. >> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:124 >> + static const WebString kPrimitiveSynthetic = WebString::fromUTF8("synthetic"); > > I would update the old tests to use "raw" instead of "true". It's confusing that the valid values for the attribute are "true" or "synthetic". > > Also, this should probably just be const char* to avoid initialization (operator== is defined for String, const char* so you can still use == below). I tried doing that. Looks like == is overloaded for String, but not for WebString. I also tried converting the WebString into a WTF::String first, but the WebString -> WTF::String conversion doesn't seem to be available for the test. So I have kept this unchanged (this is also the pattern used in other such parsing functions in this file). Does that sound reasonable?
Comment on attachment 176057 [details] Build fix! View in context: https://bugs.webkit.org/attachment.cgi?id=176057&action=review >>> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:124 >>> + static const WebString kPrimitiveSynthetic = WebString::fromUTF8("synthetic"); >> >> I would update the old tests to use "raw" instead of "true". It's confusing that the valid values for the attribute are "true" or "synthetic". >> >> Also, this should probably just be const char* to avoid initialization (operator== is defined for String, const char* so you can still use == below). > > I tried doing that. Looks like == is overloaded for String, but not for WebString. I also tried converting the WebString into a WTF::String first, but the WebString -> WTF::String conversion doesn't seem to be available for the test. So I have kept this unchanged (this is also the pattern used in other such parsing functions in this file). Does that sound reasonable? Oh, and I have changed "true" to "raw" for this. Thanks
Comment on attachment 176295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176295&action=review > Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:124 > + static const WebString kPrimitiveRaw = WebString::fromUTF8("raw"); > + static const WebString kPrimitiveSynthetic = WebString::fromUTF8("synthetic"); Please use DEFINE_STATIC_LOCAL or use a const char* and convert it to a WebString during the if condition. I'm pretty sure clang will complain about this as is.
Created attachment 176324 [details] Used DEFINE_STATIC_LOCAL. Thanks!
(In reply to comment #19) > Created an attachment (id=176324) [details] > Used DEFINE_STATIC_LOCAL. Thanks! Requesting cq?
Comment on attachment 176324 [details] Used DEFINE_STATIC_LOCAL. Thanks! Clearing flags on attachment: 176324 Committed r136013: <http://trac.webkit.org/changeset/136013>