Bug 63144 - Send keypress events to windowless plugins on the windows port.
Summary: Send keypress events to windowless plugins on the windows port.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: noel gordon
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-22 08:41 PDT by noel gordon
Modified: 2011-06-27 19:36 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.05 KB, patch)
2011-06-22 08:55 PDT, noel gordon
no flags Details | Formatted Diff | Diff
Patch (3.47 KB, patch)
2011-06-27 07:47 PDT, noel gordon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description noel gordon 2011-06-22 08:41:06 PDT
Map keypress to WM_CHAR in NPEvent.event.
Comment 1 noel gordon 2011-06-22 08:55:08 PDT
Created attachment 98177 [details]
Patch
Comment 2 noel gordon 2011-06-22 08:56:17 PDT
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
Comment 3 noel gordon 2011-06-22 08:58:02 PDT
plugins/keyboard-events.html will be fixed on bug 62375.
Comment 4 Adam Roben (:aroben) 2011-06-22 12:33:49 PDT
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?
Comment 5 noel gordon 2011-06-22 20:12:07 PDT
> 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.
Comment 6 Adam Roben (:aroben) 2011-06-23 06:30:20 PDT
(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.
Comment 7 noel gordon 2011-06-23 08:43:00 PDT
(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 :)
Comment 8 Adam Roben (:aroben) 2011-06-23 09:09:44 PDT
OK. I at least think you should explain things more clearly in the ChangeLog.
Comment 9 noel gordon 2011-06-27 07:47:04 PDT
(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.
Comment 10 noel gordon 2011-06-27 07:47:27 PDT
Created attachment 98724 [details]
Patch
Comment 11 Adam Roben (:aroben) 2011-06-27 07:56:07 PDT
Comment on attachment 98724 [details]
Patch

Does WebKit2 need this fix as well?
Comment 12 WebKit Review Bot 2011-06-27 08:37:30 PDT
Comment on attachment 98724 [details]
Patch

Clearing flags on attachment: 98724

Committed r89823: <http://trac.webkit.org/changeset/89823>
Comment 13 WebKit Review Bot 2011-06-27 08:37:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 noel gordon 2011-06-27 19:36:29 PDT
> Does WebKit2 need this fix as well?

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