Bug 89769 – [chromium] Notify the document if a plugin accepts touch input events

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

[Sadrul Habib Chowdhury]

[chromium] Notify the document if a plugin accepts touch input events

[Sadrul Habib Chowdhury]

Created attachment 149057 [details]
Patch

[Sadrul Habib Chowdhury]

The corresponding chromium CL is http://codereview.chromium.org/10628020/

[Adam Barth]

Comment on attachment 149057 [details]
Patch

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

> Source/WebKit/chromium/public/WebPluginContainer.h:111
> +    // Notifies when the plugin starts/stops accepting touch events.
> +    virtual void setAcceptingTouchEvents(bool) = 0;

setAcceptingTouchEvents -> setIsAcceptingTouchEvents or setAcceptsTouchEvents ?

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:601
> +    if (m_acceptingTouchEvents && m_element->document())
> +        m_element->document()->didRemoveTouchEventHandler();

How can m_element->document() be null?  In the case where it becomes null, do we need to call didRemoveTouchEventHandler?

[Adam Barth]

Is there a way to test this change?

[Sadrul Habib Chowdhury]

Created attachment 149093 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 149057 [details]
Patch

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

>> Source/WebKit/chromium/public/WebPluginContainer.h:111
>> +    virtual void setAcceptingTouchEvents(bool) = 0;
> 
> setAcceptingTouchEvents -> setIsAcceptingTouchEvents or setAcceptsTouchEvents ?

Done.

>> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:601
>> +        m_element->document()->didRemoveTouchEventHandler();
> 
> How can m_element->document() be null?  In the case where it becomes null, do we need to call didRemoveTouchEventHandler?

I wasn't sure if it could be NULL so I added the check. I looked at the relevant code and did some testing, and it looks like it cannot be null. So I have removed the null-check here.

[Sadrul Habib Chowdhury]

(In reply to comment #4)
> Is there a way to test this change?

I am working on writing a browser-test for this in the chrome side.

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

[Adam Barth]

Comment on attachment 149093 [details]
Patch

Ok.  A browser_test isn't ideal since we won't see it fail until after we roll WebKit into Chromium, but I can understand it's hard to make a LayoutTest for this case.  Thanks for the patch.

[Sadrul Habib Chowdhury]

Comment on attachment 149093 [details]
Patch

Indeed. Thanks for the quick review!

[Adam Barth]

Comment on attachment 149093 [details]
Patch

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

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:594
> +    , m_acceptingTouchEvents(false)

nit: This should be named m_isAcceptingTouchEvents also.  The idea is that you should be able to read these boolean flags as a sentence "If this object isAcceptingTouchEvents ..."

[Sadrul Habib Chowdhury]

Created attachment 149117 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 149093 [details]
Patch

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

>> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:594
>> +    , m_acceptingTouchEvents(false)
> 
> nit: This should be named m_isAcceptingTouchEvents also.  The idea is that you should be able to read these boolean flags as a sentence "If this object isAcceptingTouchEvents ..."

Ah, good catch. Done. Thanks

[WebKit Review Bot]

Comment on attachment 149117 [details]
Patch

Clearing flags on attachment: 149117

Committed r121070: <http://trac.webkit.org/changeset/121070>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.