[webkit-reviews] review canceled: [Bug 129812] [Mac] Don't perform a round-trip through WebProcess before interpreting key events : [Attachment 226044] proposed patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has canceled Alexey Proskuryakov
<ap at webkit.org>'s request for review:
Bug 129812: [Mac] Don't perform a round-trip through WebProcess before
interpreting key events
https://bugs.webkit.org/show_bug.cgi?id=129812

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

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


Will address all other comments, and fix the builds.

>> Source/WebKit2/Shared/WebEvent.h:223
>> +#endif
> 
> Should we do something with a structure instead of these ridiculously long
argument lists?

Maybe! I'm not sure if I fully understand the design here, why do we have
events and event factories?

>> Source/WebKit2/UIProcess/API/mac/WKView.mm:348
>> +	    _data->_page->setInitialFocus(direction == NSSelectingNext,
keyboardEvent != nil, NativeWebKeyboardEvent(keyboardEvent, false,
Vector<KeypressCommand>()));
> 
> This false/empty-vector stuff is a bit ugly. Should these be default argument
values instead perhaps?

I'd like to keep it ugly for now to maybe pass something more appropriate than
an event. It's not clear why setInitialFocus needs a keyboard event.

>> Source/WebKit2/UIProcess/API/mac/WKView.mm:1310
>> +	    return [super performKeyEquivalent:event];
> 
> Hard-coding a specific key string and modifiers seems a more fragile rule
than what the old code had. Is there some way to handle this without that?

I think that it's more accurate too - the Esc key is specifically what's passed
to performKeyEquivalent: from AppKit as an exception, there is no generality
here.

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:240
>>	bool handleEditingKeyboardEvent(WebCore::KeyboardEvent*);
> 
> Seems like this should take a reference, not a pointer. Next time, maybe.

Yes, there is a good amount of cleanup to do, especially once all platforms
follow suit.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:213
>> +	const Vector<KeypressCommand>& commands = event->keypressCommands();
> 
> I think this would read better with auto.

I prefer to keep the type, I like to see what my variables actually are.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:219
>>	    return false;
> 
> Really unfortunate to have this key hard-coded. Also seems strange to convert
the selector name into a command name just to do == on it.

Again, I think that this is the correct way to handle the Esc key. An
alternative would be to add a CancelOperation EditorCommand, and mark it as
text insertion command, but that would have its own downsides:

1. That would be a hack, as it's not really a text insertion command, despite
the need to dispatch keypress event.

2. We would dispatch key press for Cmd+period, and theoretically for any user
defined key bindings that generate cancelOperation:, which would be wrong.

Calling commandNameForSelectorName is silly indeed. I did this because
KeypressCommand.commandName is poorly defined, it may be either a selector or a
command name, and commandNameForSelectorName normalizes that. This should be
cleaned up separately.