Bug 50788 - Implement IME support for Mac in WebKit2
Summary: Implement IME support for Mac in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-12-09 15:11 PST by Enrica Casucci
Modified: 2010-12-10 16:08 PST (History)
3 users (show)

See Also:


Attachments
Patch (49.34 KB, patch)
2010-12-09 15:25 PST, Enrica Casucci
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2010-12-09 15:11:53 PST
Tracking the work needed to support Input methods on Mac.
Comment 1 Enrica Casucci 2010-12-09 15:25:11 PST
Created attachment 76126 [details]
Patch
Comment 2 Alexey Proskuryakov 2010-12-09 16:40:46 PST
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.
Comment 3 Build Bot 2010-12-09 18:44:03 PST
Attachment 76126 [details] did not build on win:
Build output: http://queues.webkit.org/results/6996019
Comment 4 Enrica Casucci 2010-12-09 19:20:21 PST
> > 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.
Comment 5 Enrica Casucci 2010-12-09 19:21:41 PST
I've noticed the windows build is broken by this patch. I'll fix it before landing it.
Comment 6 Enrica Casucci 2010-12-10 13:25:06 PST
Committed revision 73796.
Comment 7 Eric Seidel (no email) 2010-12-10 16:07:13 PST
I wonder if this broke editing/execCommand/4920488.html?
Comment 8 Eric Seidel (no email) 2010-12-10 16:08:05 PST
Or it might have been bug 50710.