Bug 83014

Summary: [Chromium] PlatformKeyboardEvent::m_windowsVirtualKeyCode should be zero for char events
Product: WebKit Reporter: Cem Kocagil <cem.kocagil+webkit>
Component: UI EventsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: cem.kocagil+webkit, dglazkov, fishd, schenney, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 83330    
Bug Blocks:    
Attachments:
Description Flags
Patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02
none
Patch
none
Patch
cem.kocagil+webkit: review-
Patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
Rebased rniwa: review-

Description Cem Kocagil 2012-04-03 03:37:00 PDT
Chromium creates PlatformKeyboardEvents with char type that return non-zero for windowsVirtualKeyCode(). It shouldn't, as documented in PlatformKeyboardEvent.h ("Zero for Char events").
Comment 1 Cem Kocagil 2012-04-03 04:01:21 PDT
Created attachment 135304 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-03 04:42:34 PDT
Comment on attachment 135304 [details]
Patch

Attachment 135304 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12317232

New failing tests:
fullscreen/full-screen-placeholder.html
fullscreen/full-screen-zIndex-after.html
fullscreen/full-screen-twice.html
fullscreen/full-screen-css.html
fullscreen/full-screen-keyboard-disabled.html
Comment 3 WebKit Review Bot 2012-04-03 04:42:40 PDT
Created attachment 135309 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 4 Cem Kocagil 2012-04-03 09:05:41 PDT
Created attachment 135340 [details]
Patch
Comment 5 WebKit Review Bot 2012-04-03 09:08:48 PDT
Attachment 135340 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/page/EventHandler.cpp:2729:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Cem Kocagil 2012-04-03 09:11:49 PDT
Created attachment 135342 [details]
Patch
Comment 7 Cem Kocagil 2012-04-05 17:08:10 PDT
Created attachment 135940 [details]
Patch
Comment 8 WebKit Review Bot 2012-04-05 18:40:00 PDT
Comment on attachment 135940 [details]
Patch

Attachment 135940 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12339627

New failing tests:
fullscreen/full-screen-placeholder.html
fullscreen/full-screen-zIndex-after.html
fullscreen/full-screen-twice.html
fullscreen/full-screen-css.html
fullscreen/full-screen-keyboard-disabled.html
Comment 9 WebKit Review Bot 2012-04-05 18:40:06 PDT
Created attachment 135959 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 10 Cem Kocagil 2012-04-12 13:49:29 PDT
Created attachment 136963 [details]
Rebased
Comment 11 Ryosuke Niwa 2012-04-22 22:14:26 PDT
Comment on attachment 136963 [details]
Rebased

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

> Source/WebKit/chromium/ChangeLog:7
> +

Please explain why you're making this change. r- due to the lack of description.
Also, can't this be tested by a chromium unit test?
Comment 12 Cem Kocagil 2012-04-26 06:54:37 PDT
(In reply to comment #11)
> Please explain why you're making this change. r- due to the lack of description.
> Also, can't this be tested by a chromium unit test?

I explained the reason behind this change in the first comment, is it required to explain in the ChangeLog too?

You're probably right, this should be able to be tested (if there currently are unit tests around that part of the code).