[webkit-reviews] review denied: [Bug 52606] keyLocation for KeyboardEvent object doesn't always have correct value : [Attachment 86603] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied wez at chromium.org's request for review:
Bug 52606: keyLocation for KeyboardEvent object doesn't always have correct
value
https://bugs.webkit.org/show_bug.cgi?id=52606

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86603&action=review

I see no attempt to set these flags correctly for Qt or Mac or EFL. It would be
much better if the patch covered those too. Typically tests make clear what’s
missing on what platforms so the lack of a test here is a problem.

> Source/WebCore/ChangeLog:10
> +	   No new tests. (OOPS!)

A bug fix like this needs a test. And you can’t land something that still has
any OOPS other than the Reviewed by line which is filled in automatically.

> Source/WebCore/dom/KeyboardEvent.cpp:73
> +    , m_keyLocation(DOM_KEY_LOCATION_STANDARD)
>      , m_altGraphKey(false)
>  {
> +    if (key.isKeypad())
> +	   m_keyLocation = DOM_KEY_LOCATION_NUMPAD;
> +    else if (key.isLeftKey())
> +	   m_keyLocation = DOM_KEY_LOCATION_LEFT;
> +    else if (key.isRightKey())
> +	   m_keyLocation = DOM_KEY_LOCATION_RIGHT;

It would be more elegant to have a function to do this and set m_keyLocation
directly to the correct value instead of first setting it to standard and then
overwriting it.

> Source/WebCore/platform/PlatformKeyboardEvent.h:155
> +	   bool isLeftKey() const { return m_isLeftKey; }
> +	   bool isRightKey() const { return m_isRightKey; }

This should be an enum with three values rather than two separate booleans.
Left and right are not independent. Separate booleans is inelegant and can lead
to unnecessary ambiguity.

It would probably be best to use the same four value design as the DOM itself:
numpad, left, right, or none of the above.