[webkit-reviews] review denied: [Bug 23156] cant alignt text to the right using right shift and right ctrl : [Attachment 28061] a proposed fix (WebKit part)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Hironori Bono <hbono at chromium.org>'s
request for review:
Bug 23156: cant alignt text to the right using right shift and right ctrl
https://bugs.webkit.org/show_bug.cgi?id=23156

Attachment 28061: a proposed fix (WebKit part)
https://bugs.webkit.org/attachment.cgi?id=28061&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Should tbe right keys be treated as entirely separate from the non-right ones?
What about the code already in WebCore that calls shiftKey()? If we really are
going to have separate booleans like this, then I think it need to be
shiftKey(), leftShiftKey(), and rightShiftKey() functions, with shiftKey()
returning m_shiftKey | m_rightShiftKey. Or maybe you intend m_shiftKey to be
true if either the left or right key is down?

If no platform-independent code is going to be calling these new functions,
then I think the Chromium code should match the other platforms and allow you
to get to a lower level platform event object. This class is supposed to define
a platform-independent interface to keys for use in platform-independent code.
If this is an interface defined only for Chromium and used only in Chromium,
then I think you should consider doing it differently.

But maybe we do have a cross-platform need to distinguish right and left
modifiers. I'm not sure. Given the title of the bug it seems that really this
is a Chromium-specific bug about needing to set the shift key boolean even when
the right shift key is the one held down, and doesn't require any change to the
cross-platform code at all, but I really can't tell.

If we're going to add booleans for the right-key versions of these modifiers,
then the patch needs to also modify the implementation on other platforms so
that those new booleans are set on those platforms too. It's bad manners to add
these for one platform and have them all be false on all the other platforms.

I'm going to say review- for now given these concerns?