Bug 103092 – [chromium] Allow plugins to opt-in to receive synthetic mouse events out of touch events.

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

[Sadrul Habib Chowdhury]

[chromium] Allow plugins to opt-in to receive synthetic mouse events out of touch events.

[Sadrul Habib Chowdhury]

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

[Sadrul Habib Chowdhury]

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()

[Adam Barth]

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.

[Sadrul Habib Chowdhury]

Created attachment 175809 [details]
Patch

[Sadrul Habib Chowdhury]

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)

[Adam Barth]

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.

[Adam Barth]

Other than the nit above, the API change LGTM.

[Sadrul Habib Chowdhury]

Created attachment 176032 [details]
Renamed the enum members. Thanks!

[Peter Beverloo (cr-android ews)]

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

[WebKit Review Bot]

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

[Sadrul Habib Chowdhury]

Created attachment 176057 [details]
Build fix!

[Sadrul Habib Chowdhury]

Hi Tony! Would you please review this patch? Thanks!

[Tony Chang]

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

[Sadrul Habib Chowdhury]

Created attachment 176295 [details]
Patch

[Sadrul Habib Chowdhury]

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?

[Sadrul Habib Chowdhury]

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

[Tony Chang]

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.

[Sadrul Habib Chowdhury]

Created attachment 176324 [details]
Used DEFINE_STATIC_LOCAL. Thanks!

[Sadrul Habib Chowdhury]

(In reply to comment #19)
> Created an attachment (id=176324) [details]
> Used DEFINE_STATIC_LOCAL. Thanks!

Requesting cq?

[WebKit Review Bot]

Comment on attachment 176324 [details]
Used DEFINE_STATIC_LOCAL. Thanks!

Clearing flags on attachment: 176324

Committed r136013: <http://trac.webkit.org/changeset/136013>