Summary: | need access info from WebInputEvent | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | wjia | ||||||||||||||||||
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | brettw, commit-queue, eric, fishd, webkit.review.bot, wjia | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
Description
wjia
2010-09-24 14:48:12 PDT
Created attachment 68763 [details]
obtain WebKeyboardEvent from saved current version
Created attachment 68782 [details]
added missing header file
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 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. 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.
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.
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;
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 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 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 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 Patch failed to apply. working on fixing the output in those cases now. 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!
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
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> Please use webkit-patch upload to upload patches so that old patches get obsoleted when you upload new ones. |