Bug 61721

plugins/mouse-events.html mentioned InChromiumBugs http://crbug.com/10351.

Created attachment 95419 [details] Patch wip

I should maybe remove the X11 fixup into another bug. Anyho ...

> Tests plugins/mouse-events.html plugins/key-events.html require event logging for example. Does this patch make them pass?

Maybe. I've given up trying to build the win port with my setup. I can build chrome windows port though, and can reliably test this change there ...

There is an exclusion line for the mouse-events.html test in the chromium test_expectations.txt file. I commented that line out and ran the chromium DRT in debug to see what the test actually outputs ... Test for bug 11517: Flash clicks/interactivity not working properly. Patched in my change and repeated the above. The output is: Test for bug 11517: Flash clicks/interactivity not working properly. No joy. Add attribute windowPlugin="false" to the <embed> tag so the test plugin is loaded in windowless mode, and retest. The output is: CONSOLE MESSAGE: line 18: PLUGIN: getFocusEvent CONSOLE MESSAGE: line 18: PLUGIN: mouseDown at (20, 20) CONSOLE MESSAGE: line 19: PLUGIN: mouseUp at (20, 20) CONSOLE MESSAGE: line 21: PLUGIN: mouseDown at (30, 30) CONSOLE MESSAGE: line 23: PLUGIN: mouseUp at (40, 30) Test for bug 11517: Flash clicks/interactivity not working properly. Conclusion: mac ports load plugins in windowless mode by default, chrome linux/win load them windowed by default. Attribute windowPlugin="false" on the <embed> forces windowless plugin mode.

OK so the keyboard-events.html might work too. Again there's an exclusion line in the chromium test_expectations.txt file. I commented that line out and ran the chromium DRT in debug _with_ my change. The output is: This test checks if a plug-in can receive keyboard events sent from eventSender. This is a test for Bug 34936. No joy. Add attribute windowPlugin="false" to the <embed> as before and retest. The output is: CONSOLE MESSAGE: line 18: PLUGIN: getFocusEvent CONSOLE MESSAGE: line 18: PLUGIN: mouseDown at (20, 20) CONSOLE MESSAGE: line 19: PLUGIN: mouseUp at (20, 20) CONSOLE MESSAGE: line 22: PLUGIN: keyDown 'A' CONSOLE MESSAGE: line 22: PLUGIN: keyUp 'A' CONSOLE MESSAGE: line 23: PLUGIN: keyDown 'B' CONSOLE MESSAGE: line 23: PLUGIN: keyUp 'B' CONSOLE MESSAGE: line 24: PLUGIN: keyDown 'C' CONSOLE MESSAGE: line 24: PLUGIN: keyUp 'C' This test checks if a plug-in can receive keyboard events sent from eventSender. This is a test for Bug 34936. Windowless mode plugins are needed for these tests, and all ports should at least test for that -- windowless Flash plugins are common on the web.

Created attachment 95600 [details] Patch for review.

Comment on attachment 95600 [details] Patch for review. This looks correct to me. Events generated by eventSender probably won't get forwarded to windowed plugins since windowed plugins get events from the OS (I guess we could try to make OS events and forward them, but that's probably not worth the effort). By making it a windowless plugin, WebKit can forward the events to the plugin.

Also, I suspect this will fix the tests on webkit win as well.

Comment on attachment 95600 [details] Patch for review. Clearing flags on attachment: 95600 Committed r87855: <http://trac.webkit.org/changeset/87855>

All reviewed patches have been landed. Closing bug.

(In reply to comment #9) > This looks correct to me. > > Events generated by eventSender probably won't get forwarded to windowed plugins since windowed > plugins get events from the OS (I guess we could try to make OS events and forward them, but > that's probably not worth the effort). Agree + there are windowed plugin (unit) tests in src/webkit/plugins/npapi/test for chromium and in Tools/DumpRenderTree/TestNetscapePlugIn/win for the webkit win port. > By making it a windowless plugin, WebKit can forward the events to the plugin. Yes. Note I made handleEventWin() return 1. handleEventX11() and handleEventCarbon() return 0, handleEventCocoa() returns 1. These become the return value of NPP_HandleEvent() and the plugin container has to handle it -- return values all over the place, like real plugins :) Begs the question: should plugin containers trust the NPP_HandleEvent() return value for mouse and keyboard events? Nope in general and chromium mentions why: webplugin_delegate_impl_mac.mm http://goo.gl/WuCtv webplugin_delegate_impl_win.cc http://goo.gl/BUQ67 webplugin_delegate_impl_gtk.cc http://goo.gl/yBmQ8 They ignore the return value, and say the mouse/keyboard event was handled. That should be enough to close bug 58924 I believe, could you possibly/please take a look?

> Also, I suspect this will fix the tests on webkit win as well. Concur.

> Concur. Well, maybe for WebKit2, https://bugs.webkit.org/attachment.cgi?id=88749 where NPP_HandleEvent() returns have started to be ignored (bug 58108), and the same in the WebKit2 win netscape plugin code from my reading. https://bugs.webkit.org/show_bug.cgi?id=58108#c2 asked a good question - well what about WebKit1, are the events ignored there?

I got a clean VS2005, setup the all bits & pieces to build webkit win, and built DRT win. My results: 1) focus events are not sent through to the windowless plugin - there are no "CONSOLE MESSAGE: line 18: PLUGIN: getFocusEvent" in the DRT test results. 2) the expected mouse events are logged to the DRT result _only_ if I return 0 from the NPP_HandleEvent() call. So focus is not working, but why is a return 0 needed? The keyboard handling http://trac.webkit.org/browser/trunk/WebCore/plugins/win/PluginViewWin.cpp?rev=67854#L656 and the mouse event handling http://trac.webkit.org/browser/trunk/WebCore/plugins/win/PluginViewWin.cpp?rev=67854#L724 read the result of dispatchNPEvent(), which is the return the result of NPP_HandleEvent(), and so I see why ... if (!dispatchNPEvent(npEvent)) event->setDefaultHandled(); The event is marked handled only if the plugin returned 0 from NPP_HandleEvent(). Since that is the expectation, and since chromium ignores the event return, this suggests a new patch to get mouse event logging working on webkit win ...

Created attachment 96257 [details] Patch Part #2

FYI chromium DRT plugins/{mouse-events.html,key-events.html} still pass with this change.

(In reply to comment #18) Meant plugins/{mouse-events.html,keyboard-events.html}.

Build webkit win at tip just now, and ran the mouse-events.html test CONSOLE MESSAGE: line 0: PLUGIN: mouseDown at (20, 20) CONSOLE MESSAGE: line 0: PLUGIN: mouseUp at (20, 20) CONSOLE MESSAGE: line 0: PLUGIN: mouseDown at (30, 30) CONSOLE MESSAGE: line 0: PLUGIN: mouseUp at (40, 30) Test for bug 11517: Flash clicks/interactivity not working properly. Ok, mouse event logged at last.

.. and keyboard-events.html is looking better also. CONSOLE MESSAGE: line 0: PLUGIN: mouseDown at (20, 20) CONSOLE MESSAGE: line 0: PLUGIN: mouseUp at (20, 20) CONSOLE MESSAGE: line 0: PLUGIN: keyDown 'A' CONSOLE MESSAGE: line 0: PLUGIN: keyUp 'A' CONSOLE MESSAGE: line 0: PLUGIN: keyDown 'B' CONSOLE MESSAGE: line 0: PLUGIN: keyUp 'B' CONSOLE MESSAGE: line 0: PLUGIN: keyDown 'C' CONSOLE MESSAGE: line 0: PLUGIN: keyUp 'C' This test checks if a plug-in can receive keyboard events sent from eventSender. This is a test for Bug 34936.(In reply to

Created attachment 96429 [details] Patch Part #2.1 Add focus fix. Layout test failures noted in the win port DRT, in fast/css/* for example. But I'm not touching CSS here, so maybe that's normal? Not sure.

Noel: I would make a new bug for the fixes to the Win port. Good people to CC for a review or feedback are jhoneycutt@apple.com and andersca@apple.com.

(In reply to comment #23) > Noel: I would make a new bug for the fixes to the Win port. > > Good people to CC for a review or feedback are jhoneycutt@apple.com and andersca@apple.com. Right thx, will split the Win port change out in a new bug.

Patch Part #2 will do for the current bug therefore, r? cq?

Filed bug 62375 for the fixes to the Win port.

Comment on attachment 96257 [details] Patch Part #2 I guess this change doesn't have tests because they are covered by the fixes in 62375? That seems OK to me.

Comment on attachment 96257 [details] Patch Part #2 Clearing flags on attachment: 96257 Committed r88465: <http://trac.webkit.org/changeset/88465>

All reviewed patches have been landed. Closing bug.

> I guess this change doesn't have tests because they are covered by the fixes in 62375? That seems OK to me. Correct.