Summary: | [chromium] menu key doesn't work when capslock or numslock is on | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Evan Stade <estade> | ||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Evan Stade
2010-11-09 17:21:29 PST
Created attachment 73443 [details]
try1
Attachment 73443 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chromium/public/WebInputEvent.h', u'WebKit/chromium/src/WebViewImpl.cpp']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:580: One space before end of line comments [whitespace/comments] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 73443 [details] try1 View in context: https://bugs.webkit.org/attachment.cgi?id=73443&action=review > WebKit/chromium/public/WebInputEvent.h:133 > + static const int InputModifiers = ShiftKey | ControlKey | AltKey | MetaKey; Nit: I don't like this name much. Aren't capslock and numlock also input modifiers? Should there just be a static method that returns event.modifiers & (ShiftKey | ControlKey | AltKey | MetaKey) like the other methods? Comment on attachment 73443 [details] try1 View in context: https://bugs.webkit.org/attachment.cgi?id=73443&action=review >> WebKit/chromium/public/WebInputEvent.h:133 >> + static const int InputModifiers = ShiftKey | ControlKey | AltKey | MetaKey; > > Nit: I don't like this name much. Aren't capslock and numlock also input modifiers? Should there just be a static method that returns event.modifiers & (ShiftKey | ControlKey | AltKey | MetaKey) like the other methods? I can't think of a better name (really the best name is "modifier", but the enum took that already). I got this name from the getWebInputModifiers() function in WebInputEventConversion.cpp I think it's more versatile as a bitmask but if you would rather it be an applied bitmask I can do that. Comment on attachment 73443 [details] try1 View in context: https://bugs.webkit.org/attachment.cgi?id=73443&action=review >>> WebKit/chromium/public/WebInputEvent.h:133 >>> + static const int InputModifiers = ShiftKey | ControlKey | AltKey | MetaKey; >> >> Nit: I don't like this name much. Aren't capslock and numlock also input modifiers? Should there just be a static method that returns event.modifiers & (ShiftKey | ControlKey | AltKey | MetaKey) like the other methods? > > I can't think of a better name (really the best name is "modifier", but the enum took that already). I got this name from the getWebInputModifiers() function in WebInputEventConversion.cpp > > I think it's more versatile as a bitmask but if you would rather it be an applied bitmask I can do that. Ok, if you want to keep it as a bitmask, can you change the if below to have named bools to make it easier to read? E.g.: bool isUnmodifiedMenuKey = !(event.modifiers & WebInputEvent::InputModifiers) && (event.windowsKeyCode == VKEY_APPS); bool isShiftF10 = (event.modifiers == WebInputEvent::ShiftKey) && (event.windowsKeyCode == VKEY_F10) if ((isUnmodifiedMenuKey || isShiftF10) && event.type == contextMenuTriggeringEventType) { Please fix the style complaint too. Created attachment 73521 [details]
try2
style fixed; readability suggestion implemented.
Attachment 73521 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chromium/public/WebInputEvent.h', u'WebKit/chromium/src/WebViewImpl.cpp']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:574: 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 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 73529 [details]
try3
style again
Comment on attachment 73529 [details] try3 Clearing flags on attachment: 73529 Committed r71774: <http://trac.webkit.org/changeset/71774> All reviewed patches have been landed. Closing bug. |