Bug 118398 – HTMLPluginElement should set display to playing before firing mouse click

• Component: Plug-ins
• Status: RESOLVED FIXED

[Roger Fong]

HTMLPluginElement's eventHandler exits early if the display state is not set to playing. 
Therefore, we should be setting the display to playing before dispatching the mouse click.

[Roger Fong]

<rdar://problem/14262126>

[Roger Fong]

Created attachment 206104 [details]
Patch

[Jon Lee]

Comment on attachment 206104 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        The defaultEventHandler returns early of the state is not Playing.

Can we add some explanation as to why we return early? basically, we don't want to send events to plugins that are not full-out playing, since it could result in a change in the page's state, and it best mimics what would happen if the plugin was fully playing to begin with, and then the user clicked the plugin. Setting the state after dispatching the simulated click meant that the click was therefore never sent to the plugin. Setting the state beforehand lets the plugin process the simulated click.

Also, is to possible to add a layout test for this?

[Roger Fong]

(In reply to comment #3)
> (From update of attachment 206104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206104&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        The defaultEventHandler returns early of the state is not Playing.
> 
> Can we add some explanation as to why we return early? basically, we don't want to send events to plugins that are not full-out playing, since it could result in a change in the page's state, and it best mimics what would happen if the plugin was fully playing to begin with, and then the user clicked the plugin. Setting the state after dispatching the simulated click meant that the click was therefore never sent to the plugin. Setting the state beforehand lets the plugin process the simulated click.
> 
Yup..

> Also, is to possible to add a layout test for this?
Yup I think the "paused" attribute is true for both paused and stopped? So i can just make a test that clicks on a youtube and checks to see that "paused" is false.

[Jon Lee]

(In reply to comment #4)
> (In reply to comment #3)
> > Also, is to possible to add a layout test for this?
> Yup I think the "paused" attribute is true for both paused and stopped? So i can just make a test that clicks on a youtube and checks to see that "paused" is false.

You shouldn't make a layout test that relies of live web content. There might be a way to use or extend the existing plugins we have available in LayoutTests to check to see that the simulated click gets dispatched. Maybe Dean can provide some guidance on how to do this?

[Roger Fong]

Created attachment 206360 [details]
Patch

[Roger Fong]

Created attachment 206363 [details]
Patch

[Jon Lee]

Comment on attachment 206363 [details]
Patch

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

> LayoutTests/plugins/snapshotting/plugin-receives-click-event.html:9
> +	embed.addEventListener("click", function () {div.innerHTML = "PASS, plugin did receive mouse click event."});

Nit. Is "function ()" proper style, instead of "function()"? And I think the body of the function should be on its own line like in the rest of the test?

Also, if the click event listener is called, you should immediately stop the test by calling notifyDone().

> LayoutTests/plugins/snapshotting/plugin-receives-click-event.html:20
> +    }, 500);

Why is the body of the test within an enclosed timeout? I think it is unnecessary (the setup of test runner below could happen earlier as part of initialization)

[Build Bot]

Comment on attachment 206363 [details]
Patch

Attachment 206363 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/917339

New failing tests:
media/video-zoom.html

[Build Bot]

Created attachment 206370 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Roger Fong]

(In reply to comment #8)
> (From update of attachment 206363 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206363&action=review
> 
> > LayoutTests/plugins/snapshotting/plugin-receives-click-event.html:9
> > +	embed.addEventListener("click", function () {div.innerHTML = "PASS, plugin did receive mouse click event."});
> 
> Nit. Is "function ()" proper style, instead of "function()"? And I think the body of the function should be on its own line like in the rest of the test?

Based on a random sampling of tests not in that folder, it would seem that function() is the right way.

> 
> Also, if the click event listener is called, you should immediately stop the test by calling notifyDone().
> 
Ok

> > LayoutTests/plugins/snapshotting/plugin-receives-click-event.html:20
> > +    }, 500);
> 
> Why is the body of the test within an enclosed timeout? I think it is unnecessary (the setup of test runner below could happen earlier as part of initialization)

I was following a template of some other tests test (see LayoutTests/plugins/snapshotting/restart.html) for example. My assumption was that it was to give the plugin some time at first to initialize, snapshot, and all. I believe Dean wrote that test, maybe he can confirm?

[Roger Fong]

(In reply to comment #9)
> (From update of attachment 206363 [details])
> Attachment 206363 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/917339
> 
> New failing tests:
> media/video-zoom.html

Flake I think (as the media/video tests are known to do a lot).
The video test has nothing to do with plugins...

[Jon Lee]

(In reply to comment #11)
> (In reply to comment #8)
> > (From update of attachment 206363 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=206363&action=review
> > 
> > > LayoutTests/plugins/snapshotting/plugin-receives-click-event.html:9
> > > +	embed.addEventListener("click", function () {div.innerHTML = "PASS, plugin did receive mouse click event."});
> > 
> > Nit. Is "function ()" proper style, instead of "function()"? And I think the body of the function should be on its own line like in the rest of the test?
> 
> Based on a random sampling of tests not in that folder, it would seem that function() is the right way.
> 
> > 
> > Also, if the click event listener is called, you should immediately stop the test by calling notifyDone().
> > 
> Ok
> 
> > > LayoutTests/plugins/snapshotting/plugin-receives-click-event.html:20
> > > +    }, 500);
> > 
> > Why is the body of the test within an enclosed timeout? I think it is unnecessary (the setup of test runner below could happen earlier as part of initialization)
> 
> I was following a template of some other tests test (see LayoutTests/plugins/snapshotting/restart.html) for example. My assumption was that it was to give the plugin some time at first to initialize, snapshot, and all. I believe Dean wrote that test, maybe he can confirm?

Ah I see. That makes sense. I retract my earlier suggestion then. But the click event should still immediately call notifyDone().

[David Kilzer (:ddkilzer)]

The content of attachment 206360 [details] has been deleted by
    David Kilzer (:ddkilzer) <[e-mail]>
who provided the following reason:

Delete

The token used to delete this attachment was generated at 2013-07-10 10:49:01 PST.

[Roger Fong]

Created attachment 206398 [details]
Patch

[Roger Fong]

committed http://trac.webkit.org/changeset/152541