Bug 63144 – Send keypress events to windowless plugins on the windows port.

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[noel gordon]

Map keypress to WM_CHAR in NPEvent.event.

[noel gordon]

Created attachment 98177 [details]
Patch

[noel gordon]

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

[noel gordon]

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

[Adam Roben (:aroben)]

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?

[noel gordon]

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

[Adam Roben (:aroben)]

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

[noel gordon]

(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 :)

[Adam Roben (:aroben)]

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

[noel gordon]

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

[noel gordon]

Created attachment 98724 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 98724 [details]
Patch

Does WebKit2 need this fix as well?

[WebKit Review Bot]

Comment on attachment 98724 [details]
Patch

Clearing flags on attachment: 98724

Committed r89823: <http://trac.webkit.org/changeset/89823>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

> Does WebKit2 need this fix as well?

Eventually. NetscapePlugin::platformHandleKeyboardEvent() is currently notImplemented() on the windows port.