[Webkit-unassigned] [Bug 86694] Implement DOM_KEY_LOCATION_LEFT and RIGHT of KeyboardEvent's location property

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=86694


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #142419|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #2 from Alexey Proskuryakov <ap at webkit.org>  2012-05-17 10:47:54 PST ---
(From update of attachment 142419)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.