Bug 46518 – need access info from WebInputEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

KeyboardEvent doesn't include all info we need.

Created attachment 68763 [details]
obtain WebKeyboardEvent from saved current version

Created attachment 68782 [details]
added missing header file

[Brett Wilson (Google)]

This looks fine to me, but I'm not a reviewer. I've pinged fishd.

I was originally thinking we could call currentInputEvent in the Pepper event code, but this change is probably more robust.

[Darin Fisher (:fishd, Google)]

Comment on attachment 68782 [details]
added missing header file

View in context: https://bugs.webkit.org/attachment.cgi?id=68782&action=review

I think the plan, which I originally suggested to Brett, is flawed.  It is not
sufficient to rely on currentInputEvent like this.

> WebKit/chromium/src/WebPluginContainerImpl.cpp:449
> +        static_cast<const WebKeyboardEvent*>(WebViewImpl::currentInputEvent());

I think this will crash if JavaScript on the page programmatically generates a KeyboardEvent.

[Darin Fisher (:fishd, Google)]

If currentInputEvent() is null, then we should just use the WebKeyboardEvent generated from the WebCore::KeyboardEvent.  Otherwise, we should still use the generated WebKeyboardEvent but merge in any additional modifiers from currentInputEvent().

Created attachment 69253 [details]
resotre info from currentInputEvent only when it exists

WebKeyboardEvent is restored to be created from KeyboardEvent. Key state (of lock keys) is retrieved from currentInputEvent only when it exists.

[Darin Fisher (:fishd, Google)]

Comment on attachment 69253 [details]
resotre info from currentInputEvent only when it exists

View in context: https://bugs.webkit.org/attachment.cgi?id=69253&action=review

> WebKit/chromium/ChangeLog:5
> +        Need access info direct from WebInputEvent.

please insert a link to this bug report here.

also, please revise the description of this change to better reflect what is
being done in the code.

> WebKit/chromium/src/WebPluginContainerImpl.cpp:450
> +        static_cast<const WebKeyboardEvent*>(WebViewImpl::currentInputEvent());

how do you know that this is a WebKeyboardEvent?

> WebKit/chromium/src/WebPluginContainerImpl.cpp:453
> +    if (currentInputEvent) {

Should you also validate that the currentInputEvent is a WebKeyboardEvent?  During
the dispatch of another type of event, JS might dispatch a synthetic KeyboardEvent.

in the future, please remember to set the '?' flag on the "review" field when
attaching your patch.  Otherwise, people won't know to look at your patch for
review.

Created attachment 69399 [details]
retrieving lock key state only when stashed event is keyboard event; update Changelog;

Actually, the type checking of stashed event is not really needed since modifiers' capsLock and numLock bits will not be set for events except keyboardEvent. I just add it for safety.

[WebKit Review Bot]

Attachment 69399 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebPluginContainerImpl.cpp:452:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 69408 [details]
 retrieving lock key state only when stashed event is keyboard event; update Changelog; fix webkit type checking;

[Darin Fisher (:fishd, Google)]

Comment on attachment 69408 [details]
 retrieving lock key state only when stashed event is keyboard event; update Changelog; fix webkit type checking;

R=me

[WebKit Commit Bot]

Comment on attachment 69408 [details]
 retrieving lock key state only when stashed event is keyboard event; update Changelog; fix webkit type checking;

Rejecting patch 69408 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
l.
Files=14, Tests=304,  1 wallclock secs ( 0.63 cusr +  0.14 csys =  0.77 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/abarth/git/webkit-queue/LayoutTests
Testing 21431 test cases.
http/tests/navigation/ping-same-origin.html -> failed

Exiting early after 1 failures. 20749 tests run.
425.18s total testing time

20748 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
33 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4229037

[Darin Fisher (:fishd, Google)]

Comment on attachment 69408 [details]
 retrieving lock key state only when stashed event is keyboard event; update Changelog; fix webkit type checking;

Let's try the commit queue once more.  That test is flaky.

[WebKit Commit Bot]

Comment on attachment 69408 [details]
 retrieving lock key state only when stashed event is keyboard event; update Changelog; fix webkit type checking;

Rejecting patch 69408 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 69408]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=69408&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=46518&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 69408 from bug 46518.
Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4241009

[Eric Seidel (no email)]

Patch failed to apply.  working on fixing the output in those cases now.

[Eric Seidel (no email)]

This needs an updated patch which applies to tip of tree.

Created attachment 70774 [details]
update the patch to tip of tree

Created attachment 70780 [details]
retrieve modifiers after ctrl-c is handled

I found this problem when updating the patch to tip of the tree. Please review again. Thanks!

[WebKit Review Bot]

Attachment 70780 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebPluginContainerImpl.cpp:475:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 70784 [details]
update to tip of tree, retrieve modifiers after ctrl-c is handled, fix webkit style checking

[WebKit Commit Bot]

Comment on attachment 70784 [details]
update to tip of tree, retrieve modifiers after ctrl-c is handled, fix webkit style checking

Clearing flags on attachment: 70784

Committed r69840: <http://trac.webkit.org/changeset/69840>

[Eric Seidel (no email)]

Please use webkit-patch upload to upload patches so that old patches get obsoleted when you upload new ones.