Bug 46518 - need access info from WebInputEvent
Summary: need access info from WebInputEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-24 14:48 PDT by wjia
Modified: 2010-12-20 22:41 PST (History)
6 users (show)

See Also:


Attachments
obtain WebKeyboardEvent from saved current version (2.05 KB, patch)
2010-09-24 15:03 PDT, wjia
no flags Details | Formatted Diff | Diff
added missing header file (2.24 KB, patch)
2010-09-24 16:13 PDT, wjia
fishd: review-
Details | Formatted Diff | Diff
resotre info from currentInputEvent only when it exists (1.58 KB, patch)
2010-09-29 14:53 PDT, wjia
fishd: review-
Details | Formatted Diff | Diff
retrieving lock key state only when stashed event is keyboard event; update Changelog; (1.88 KB, patch)
2010-09-30 17:08 PDT, wjia
no flags Details | Formatted Diff | Diff
retrieving lock key state only when stashed event is keyboard event; update Changelog; fix webkit type checking; (1.88 KB, patch)
2010-09-30 18:12 PDT, wjia
fishd: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
update the patch to tip of tree (1.68 KB, patch)
2010-10-14 13:51 PDT, wjia
no flags Details | Formatted Diff | Diff
retrieve modifiers after ctrl-c is handled (1.71 KB, patch)
2010-10-14 14:47 PDT, wjia
no flags Details | Formatted Diff | Diff
update to tip of tree, retrieve modifiers after ctrl-c is handled, fix webkit style checking (1.71 KB, patch)
2010-10-14 15:02 PDT, wjia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description wjia 2010-09-24 14:48:12 PDT
KeyboardEvent doesn't include all info we need.
Comment 1 wjia 2010-09-24 15:03:09 PDT
Created attachment 68763 [details]
obtain WebKeyboardEvent from saved current version
Comment 2 wjia 2010-09-24 16:13:22 PDT
Created attachment 68782 [details]
added missing header file
Comment 3 Brett Wilson (Google) 2010-09-27 15:45:19 PDT
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.
Comment 4 Darin Fisher (:fishd, Google) 2010-09-29 09:20:48 PDT
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.
Comment 5 Darin Fisher (:fishd, Google) 2010-09-29 09:23:35 PDT
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().
Comment 6 wjia 2010-09-29 14:53:06 PDT
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.
Comment 7 Darin Fisher (:fishd, Google) 2010-09-30 00:30:01 PDT
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.
Comment 8 wjia 2010-09-30 17:08:38 PDT
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.
Comment 9 WebKit Review Bot 2010-09-30 17:17:27 PDT
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.
Comment 10 wjia 2010-09-30 18:12:51 PDT
Created attachment 69408 [details]
 retrieving lock key state only when stashed event is keyboard event; update Changelog; fix webkit type checking;
Comment 11 Darin Fisher (:fishd, Google) 2010-09-30 23:18:43 PDT
Comment on attachment 69408 [details]
 retrieving lock key state only when stashed event is keyboard event; update Changelog; fix webkit type checking;

R=me
Comment 12 WebKit Commit Bot 2010-10-01 00:13:09 PDT
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
Comment 13 Darin Fisher (:fishd, Google) 2010-10-01 00:54:39 PDT
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.
Comment 14 WebKit Commit Bot 2010-10-01 01:07:08 PDT
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 commit-queue@webkit.org...
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
Comment 15 Eric Seidel (no email) 2010-10-01 09:04:56 PDT
Patch failed to apply.  working on fixing the output in those cases now.
Comment 16 Eric Seidel (no email) 2010-10-14 11:37:17 PDT
This needs an updated patch which applies to tip of tree.
Comment 17 wjia 2010-10-14 13:51:42 PDT
Created attachment 70774 [details]
update the patch to tip of tree
Comment 18 wjia 2010-10-14 14:47:35 PDT
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!
Comment 19 WebKit Review Bot 2010-10-14 14:50:56 PDT
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.
Comment 20 wjia 2010-10-14 15:02:31 PDT
Created attachment 70784 [details]
update to tip of tree, retrieve modifiers after ctrl-c is handled, fix webkit style checking
Comment 21 WebKit Commit Bot 2010-10-14 23:33:55 PDT
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>
Comment 22 Eric Seidel (no email) 2010-12-20 22:41:20 PST
Please use webkit-patch upload to upload patches so that old patches get obsoleted when you upload new ones.