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.
<rdar://problem/14262126>
Created attachment 206104 [details] Patch
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?
(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.
(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?
Created attachment 206360 [details] Patch
Created attachment 206363 [details] Patch
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)
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
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
(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?
(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...
(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().
The content of attachment 206360 [details] has been deleted by David Kilzer (:ddkilzer) <ddkilzer@webkit.org> who provided the following reason: Delete The token used to delete this attachment was generated at 2013-07-10 10:49:01 PST.
Created attachment 206398 [details] Patch
committed http://trac.webkit.org/changeset/152541