Bug 86694

Created attachment 142419 [details] Patch

Comment on attachment 142419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142419&action=review Looks great. > Source/WebCore/ChangeLog:25 > + (WebCore): Unhelpful cruft like this can be removed from ChangeLogs. This is just a bug in the script - it's meant to be helpful, not authoritative. Thank you for providing helpful comments! > Source/WebCore/dom/KeyboardEvent.cpp:57 > +static inline int getWindowsVirtualCodeWithoutLocation(int keycode) In WebCore coding style, "get" prefix means that the result is returned via an out argument. So, just "windowsVirtualCodeWithoutLocation" would be right. > Source/WebCore/dom/KeyboardEvent.cpp:74 > +static inline unsigned getKeyLocation(const PlatformKeyboardEvent& key) Ditto. Also, I don't understand why this returns unsigned, and not KeyboardEvent::KeyLocationCode. > Tools/DumpRenderTree/mac/EventSendingController.mm:644 > + } else if ([character isEqualToString:@"rightMenu"]) { Perhaps "leftAlt" and "rightAlt" would be better for these. On Windows, the key that maps to VK_MENU is Alt, and on Mac, it's Option - calling that "Menu" is surprising and confusing, regardless of how the Windows API constant is named. > LayoutTests/fast/events/keydown-leftright-keys.html:51 > + testKeyEventWithLocation("rightShift", VK_SHIFT, KEY_LOCATION_RIGHT); If you put KEY_LOCATION_RIGHT in quotes, will test output become easier to read?

Created attachment 142640 [details] Patch

Thank you for reviewing. (In reply to comment #2) > (From update of attachment 142419 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142419&action=review > > Looks great. > > > Source/WebCore/ChangeLog:25 > > + (WebCore): > > Unhelpful cruft like this can be removed from ChangeLogs. This is just a bug in the script - it's meant to be helpful, not authoritative. I see. I removed. > > Thank you for providing helpful comments! > > > Source/WebCore/dom/KeyboardEvent.cpp:57 > > +static inline int getWindowsVirtualCodeWithoutLocation(int keycode) > > In WebCore coding style, "get" prefix means that the result is returned via an out argument. So, just "windowsVirtualCodeWithoutLocation" would be right. Done. > > Source/WebCore/dom/KeyboardEvent.cpp:74 > > +static inline unsigned getKeyLocation(const PlatformKeyboardEvent& key) > > Ditto. Done. > Also, I don't understand why this returns unsigned, and not KeyboardEvent::KeyLocationCode. I see. I don't have enough reason. Done. > > Tools/DumpRenderTree/mac/EventSendingController.mm:644 > > + } else if ([character isEqualToString:@"rightMenu"]) { > > Perhaps "leftAlt" and "rightAlt" would be better for these. On Windows, the key that maps to VK_MENU is Alt, and on Mac, it's Option - calling that "Menu" is surprising and confusing, regardless of how the Windows API constant is named. Done. > > > LayoutTests/fast/events/keydown-leftright-keys.html:51 > > + testKeyEventWithLocation("rightShift", VK_SHIFT, KEY_LOCATION_RIGHT); > > If you put KEY_LOCATION_RIGHT in quotes, will test output become easier to read? I agree. Done.

Comment on attachment 142640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142640&action=review > Source/WebCore/dom/KeyboardEvent.cpp:74 > +static inline int keyLocationCode(const PlatformKeyboardEvent& key) You changed the returned value to int, not to KeyboardEvent::KeyLocationCode. Is there something preventing you from using the latter?

Created attachment 142955 [details] Patch

(In reply to comment #5) > (From update of attachment 142640 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142640&action=review > > > Source/WebCore/dom/KeyboardEvent.cpp:74 > > +static inline int keyLocationCode(const PlatformKeyboardEvent& key) > > You changed the returned value to int, not to KeyboardEvent::KeyLocationCode. Is there something preventing you from using the latter? Sorry. I misunderstood your comment. I changed the code to use KeyboardEvent::KeyLocationCode type for returning value type of the function: WebCore::keyLocationCode. Best regards, Takashi Sakamoto

SInce you already got r+, you can - Rewrite "Reviewed by" line with the reviewer's name and - Upload the refined patch with no r?, but with cq+.

Created attachment 143202 [details] Patch

Comment on attachment 143202 [details] Patch As long as you are not a committer, you could have just set cq? on the previous patch.

Comment on attachment 143202 [details] Patch Clearing flags on attachment: 143202 Committed r118001: <http://trac.webkit.org/changeset/118001>

All reviewed patches have been landed. Closing bug.

You updated keyDown in DRT, but forgot about WTR: http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r118030%20(7431)/fast/events/keydown-leftright-keys-pretty-diff.html I have a fix for this, but please try to remember about WTR in the future.

Fixed WTR in http://trac.webkit.org/changeset/118064.

(In reply to comment #14) > Fixed WTR in http://trac.webkit.org/changeset/118064. Thank you for fixing WTR, Jessie. I didn't notice that I had to modify WTR. Best regards, Takashi Sakamoto

(In reply to comment #15) > (In reply to comment #14) > > Fixed WTR in http://trac.webkit.org/changeset/118064. > > Thank you for fixing WTR, Jessie. > I didn't notice that I had to modify WTR. > > Best regards, > Takashi Sakamoto Windows also needed fixing: http://trac.webkit.org/changeset/118540

It seems that on Chrome for Windows (canary channel) the location property is still always zero.

Looks like you need to fix WebInputEventFactory::keyboardEvent() in Source/WebKit/chromium/src/win/WebInputEventFactory.cpp too: WebKeyboardEvent WebInputEventFactory::keyboardEvent(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) { WebKeyboardEvent result; // TODO(pkasting): http://b/1117926 Are we guaranteed that the message that // GetMessageTime() refers to is the same one that we're passed in? Perhaps // one of the construction parameters should be the time passed by the // caller, who would know for sure. result.timeStampSeconds = GetMessageTime() / 1000.0; result.windowsKeyCode = static_cast<int>(wparam); // <--------------- HERE