Bug 63144

Created attachment 98177 [details] Patch

Before and _after_ my change: % ./Tools/Scripts/run-webkit-tests --debug LayoutTests/plugins/*.html Testing 82 test cases. plugins/application-plugin-plugins-disabled.html -> failed plugins/destroy-during-npp-new.html -> crashed plugins/embed-attributes-style.html -> failed plugins/get-url-with-blank-target.html -> failed plugins/keyboard-events.html -> failed plugins/netscape-dom-access.html -> failed plugins/plugin-initiate-popup-window.html -> failed plugins/plugin-javascript-access.html -> failed plugins/windowless_plugin_paint_test.html -> failed stderr 73 test cases (89%) succeeded 8 test cases (9%) had incorrect layout 1 test case (1%) crashed 1 test case (1%) had stderr output

plugins/keyboard-events.html will be fixed on bug 62375.

Comment on attachment 98177 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98177&action=review > Source/WebCore/ChangeLog:9 > + No new tests. Covered by existing tests, plugins/mouse-events.html and > + plugins/keyboard-events.html. Both are semi-broken on the win port due Does plugins/mouse-events.html really cover this change? If this change affects keyboard-events.html, why don't you need to update the expected results?

> Does plugins/mouse-events.html really cover this change? I did change handleMouseEvent(), and mouse-events.html depends on that code. > If this change affects keyboard-events.html, why don't you need to update the expected results? The result of keyboard-events.html did not change.

(In reply to comment #5) > > Does plugins/mouse-events.html really cover this change? > > I did change handleMouseEvent(), and mouse-events.html depends on that code. OK. It is confusing to have that change included in this bug, which is supposedly just about keypress events. It would be clearer to do it separately. > > If this change affects keyboard-events.html, why don't you need to update the expected results? > > The result of keyboard-events.html did not change. I see, because logging isn't implemented on Windows yet. You should mention that in the ChangeLog. Otherwise it is confusing to have a change that's covered by a test but doesn't affect that test's output.

(In reply to comment #6) > > > > I did change handleMouseEvent(), and mouse-events.html depends on that code. > > OK. It is confusing to have that change included in this bug, which is supposedly just about keypress events. It would be clearer to do it separately. > Maybe. It's an "ASSERT(m_plugin && !m_isWindowed)" added to handleMouseEvent() and also to handleKeyboardEvent(), 1) for symmetry and 2) to make the code clearer to me. This code feels so three-weeks ago, and that ASSERT reminded me ... "oh right, windowless plugins". Helped me, and hope it helps others, understand the preconditions needed by these routines, since that was entirely unclear to me from reading the code, prior to making my change. > > > > The result of keyboard-events.html did not change. > > I see, because logging isn't implemented on Windows yet. You should mention that in the ChangeLog. Otherwise it is confusing to have a change that's covered by a test but doesn't affect that test's output. But logging is implemented on win; I fixed that in bug 61721. I too was also confused about not having to update the expectations. Tracked it down - bug 33973 - seems Jessie noticed my plugin logging fix and updated the expectations already :)

OK. I at least think you should explain things more clearly in the ChangeLog.

(In reply to comment #8) > OK. I at least think you should explain things more clearly in the ChangeLog. OK, let's try that then.

Created attachment 98724 [details] Patch

Comment on attachment 98724 [details] Patch Does WebKit2 need this fix as well?

Comment on attachment 98724 [details] Patch Clearing flags on attachment: 98724 Committed r89823: <http://trac.webkit.org/changeset/89823>

All reviewed patches have been landed. Closing bug.

> Does WebKit2 need this fix as well? Eventually. NetscapePlugin::platformHandleKeyboardEvent() is currently notImplemented() on the windows port.