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

[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 71808: patch
https://bugs.webkit.org/attachment.cgi?id=71808&action=review

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

One thing not mentioned in the bug or ChangeLog is that there is a race
condition here - the application may be in an entirely different state by the
time it gets to re-process the Command-key combo. We should be on lookout for
user-visible problems caused by that.

Looks good to me. Please address the comments, and Qt build problems.

> WebKit2/UIProcess/WebPageProxy.h:289
> +    // Keyboard handling
> +    void interpretKeyEvent(uint32_t, String&);

These arguments should have names. It's not at all clear what they are.

> WebKit2/UIProcess/API/mac/PageClientImpl.mm:237
> +	   [m_wkView _resentEvent:nativeEvent];

The pattern is that selector name is a verb:

resent (v) feel bitter or indignant about

> WebKit2/UIProcess/API/mac/WKView.mm:90
> +    NSEvent *_resentKeyDownEvent; // We keep here the event when resending
it to the application.

Maybe _keyDownEventBeingResent? With that name, the comment seems superfluous,
but perhaps you could explain why it needs to be stored for future readers of
the code.

> WebKit2/UIProcess/API/mac/WKView.mm:91
> +    NSString* _currentEditingCommand;

Ditto, an explanation of why we need to store this could be helpful. I'm not
100% sure if I understand why this is one command, and not an array.

> WebKit2/UIProcess/API/mac/WKView.mm:314
> +- (BOOL)_handleStyleKeyEquivalent:(NSEvent *)event

You could add a comment about why we handle italic and bold, but not underline
here. See <https://bugs.webkit.org/show_bug.cgi?id=24943#c8> and later comments
for some discussion, but I think that boils down to "historic reasons".