[webkit-reviews] review granted: [Bug 40529] eventSender.keyDown("delete") incorrectly sends a backspace on some platforms : [Attachment 58682] proposed fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 40529: eventSender.keyDown("delete") incorrectly sends a backspace on some
platforms
https://bugs.webkit.org/show_bug.cgi?id=40529

Attachment 58682: proposed fix
https://bugs.webkit.org/attachment.cgi?id=58682&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   * platform/mac/KeyEventMac.mm:
(WebCore::PlatformKeyboardEvent::PlatformKeyboardEvent):
> +	   Use virtual key code to force correct character code for clarity.
Also, reworded comment,
> +	   since saying that "backspace needs to always be 8" misleadingly
implied that it could
> +	   "sometimes" be such without this code.

I don’t think that the use of 0x33 here improves clarity. Where does that 0x33
value come from? AppKit has constants named NSDeleteCharacter and
NSDeleteFunctionKey, but I don’t see any constant for the 0x33 value. At least
within this source file, I’d like to see a named constant used for the 0x33
value. Or maybe the comment can say what the number means.

Do we have a guarantee that this new code behaves the same in all cases as the
old code? Even in applications that construct their own NSEvent objects?

>      // The adjustments below are only needed in backward compatibility mode,
but we cannot tell what mode we are in from here.

Is this comment still correct? What is "backward compatibility mode"?

> +	   * DumpRenderTree/mac/EventSendingController.mm:
> +	   (-[EventSendingController keyDown:withModifiers:withLocation:]): We
were sending a broken
> +	   event for "delete" - it had virtual key code from forward delete,
and text from backspace.
> +	   Fixed "delete" to mean forward delete.

I don't think that was our intention when making the word "delete" be one of
the values taken by the keyDown function in eventSender.

Both keys are labeled "delete" on my keyboard, and the Unicode character for
U+007F is named DELETE. Because of all these overlapping uses of the word, I
think it’s confusing to use "delete" to mean the forward delete button. Maybe
we should change eventSender.keyDown to work with the names "backspace" and
"forward delete" and eliminate the "delete" name entirely?

r=me on this change as is, since it at least makes things consistent, but
please consider my comments anyway