[webkit-reviews] review granted: [Bug 86694] Implement DOM_KEY_LOCATION_LEFT and RIGHT of KeyboardEvent's location property : [Attachment 142419] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 86694: Implement DOM_KEY_LOCATION_LEFT and RIGHT of KeyboardEvent's
location property
https://bugs.webkit.org/show_bug.cgi?id=86694

Attachment 142419: Patch
https://bugs.webkit.org/attachment.cgi?id=142419&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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?