[webkit-reviews] review granted: [Bug 48271] Support Appkit key bindings and custom key bindings in WebKit2 : [Attachment 72109] Patch4

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 48271: Support Appkit key bindings and custom key bindings in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=48271

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72109&action=review

> WebCore/dom/KeyboardEvent.h:36
> +	   KeypressCommand() { }

I don't immediately see why a default constructor is now needed. Could you add
a comment?

> WebCore/dom/KeyboardEvent.h:38
> +	   KeypressCommand(const String& commandName, const String& text) :
commandName(commandName), text(text) { ASSERT(commandName == "insertText:" ||
commandName == "insertText"); }

This is surprising and fragile. How hard would it be to ensure that only one
form is seen by KeypressCommand? Unless I'm mistaken, that would be a one byte
change.

> WebKit2/UIProcess/WebPageProxy.h:304
> +#if PLATFORM(MAC)
> +    // Keyboard handling
> +    void interpretKeyEvent(uint32_t eventType,
Vector<WebCore::KeypressCommand>&);
> +#endif

How do we need to send the event back to application on other platforms for
menu shortcut processing?

> WebKit2/UIProcess/API/mac/WKView.mm:324
> +    // This should not be changed, since it could break some Mac
applications that
> +    // rely on this inherent behavior.
> +    // See https://bugs.webkit.org/show_bug.cgi?id=24943

That's a stronger comment than the one made by Darin in bug 24943. I'm not sure
which is more accurate.