Bug 50788 – Implement IME support for Mac in WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Enrica Casucci]

Tracking the work needed to support Input methods on Mac.

[Enrica Casucci]

Created attachment 76126 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 76126 [details]
Patch

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.

[Build Bot]

Attachment 76126 [details] did not build on win:
Build output: http://queues.webkit.org/results/6996019

[Enrica Casucci]

> > 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.
I will, I can change the timeout for every call, and I can increase it for this particular one when implemented.
 
> > 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.

I agree, the number of parameters grew while I was implementing and it is confusing now. I'll change it.
 
> > WebKit2/UIProcess/API/mac/WKView.mm:476
> > +    _data-> _hasMarkedText = hasComposition;
> 
> Extra space here.
I'll fix it.
Thanks for the review.

[Enrica Casucci]

I've noticed the windows build is broken by this patch. I'll fix it before landing it.

[Enrica Casucci]

Committed revision 73796.

[Eric Seidel (no email)]

I wonder if this broke editing/execCommand/4920488.html?

[Eric Seidel (no email)]

Or it might have been bug 50710.