[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


wez at chromium.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |wez at chromium.org




--- Comment #5 from wez at chromium.org  2011-03-23 09:56:42 PST ---
(In reply to comment #4)
> (From update of attachment 86603 [details])
> 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.

Yes; I was hoping to have a quick chat with you about exactly what tests would look like.

> > 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.

Agreed.  The only problem switching to the four-value system is that it'll break source compatibility with any code that generates PlatformKeyboardEvents manually, because the m_isKeypad field and accessor will be gone.  That can be addressed easily enough, though.

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