[webkit-reviews] review granted: [Bug 50788] Implement IME support for Mac in WebKit2 : [Attachment 76126] 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 50788: Implement IME support for Mac in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=50788

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

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

> WebCore/ChangeLog:11
> +	   (WebCore::KeypressCommand::KeypressCommand): Removed ASSERT in
constructor,
> +	   since it is now used for more than one command.

This patch expands the set of messages encoded in KeypressCommand, changing it
into a strange "Objective-C invocation with one string argument". Losing the
semantic meaning of the class is not great.

On the other hand, I can't even easily describe its previous semantic meaning
anyway. Something that can be sent when handling a key, which is any message
without arguments or insertText, but nothing else.

> WebKit2/ChangeLog:11
> +	   to the WebProcess. These calls all have a timeout of 1 second.

Unfortunately, attributedSubstringFromRange can legitimately take a lot of time
if the client asks for the whole document. AppKit has been known to do that in
some cases.

So, please test behavior with large documents, especially when adding
attributedSubstringFromRange.

> WebKit2/ChangeLog:44
> +	   (-[WKView conversationIdentifier]): Added.

Actually, moved.

> WebKit2/UIProcess/API/mac/PageClientImpl.mm:231
> +    commandsList = [m_wkView _interceptKeyEvent:event.nativeEvent()
> +				    hasComposition:hasComposition 
> +				    selectionStart:selectionStart
> +				      selectionEnd:selectionEnd
> +					underlines:underlines];

Can these be combined in a single argument with a helpful name, something like
StateForTextInput? The method doesn't really need any of these, so passing them
into it can confuse the reader.

> WebKit2/UIProcess/API/mac/WKView.mm:476
> +    _data-> _hasMarkedText = hasComposition;

Extra space here.