[Webkit-unassigned] [Bug 52606] keyLocation for KeyboardEvent object doesn't always have correct value

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #86603|review?                     |review-
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2011-03-23 09:40:45 PST ---
(From update of attachment 86603)
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.

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