Bug 93724

Created attachment 157742 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

@rniwa: Would you be willing to review this patch?

@olilan: Is this related to upstreaming the chromium-android port?

Yes, this is part of chromium-Android upstreaming, though this implementation isn't in downstream Chrome for Android. These methods are re-implemented here in response to a suggestion from rniwa in a chromium review.

Cool, thanks.

Comment on attachment 157742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157742&action=review WebKit API changes seem fine. Please have rniwa@ review the editor changes and usage. > Source/WebKit/chromium/src/WebViewImpl.cpp:2234 > +void WebViewImpl:: deleteSurroundingText(int before, int after) nit: space after "::"

This patch needs to be reviewed by Alexey.

Can you explain what functionality is enabled by this? Terminology here is fairly confusing (setComposingRegion sounds like rendering code in WebKit).

Context from http://codereview.chromium.org/10836053/ Add IPCs/methods for additional IME actions. This adds IPCs and support in RenderViewImpl for the following: ReplaceAll, Unselect, SetEditableSelectionOffsets, SetComposingRegion, DeleteSurroundingText. These are used to implement IME actions in the Android port, providing functions as required by the Android InputConnection interface.

Comment on attachment 157742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157742&action=review > Source/WebCore/editing/Editor.cpp:2370 > +bool Editor::setComposingRegion(int compositionStart, int compositionEnd, const Vector<CompositionUnderline>& underlines) I would rename it to setCompositionWithoutInsertingText. > Source/WebCore/editing/Editor.cpp:2382 > + Element* rootEditableElement = m_frame->selection()->rootEditableElement(); > + if (!rootEditableElement) > + return false; It's strange that we check the editability here. The caller should probably check this. > Source/WebCore/editing/Editor.cpp:2412 > + unsigned startOffset = range->startOffset(); > + unsigned endOffset = range->endOffset(); > + m_compositionNode = toText(startNode); > + m_compositionStart = startOffset; > + m_compositionEnd = endOffset; > + m_customCompositionUnderlines = underlines; > + size_t numUnderlines = m_customCompositionUnderlines.size(); > + for (size_t i = 0; i < numUnderlines; ++i) { > + m_customCompositionUnderlines[i].startOffset += startOffset; > + m_customCompositionUnderlines[i].endOffset += endOffset; > + } > + if (startNode->renderer()) > + startNode->renderer()->repaint(); This is very unusual things to do. Why do we need to do this? > Source/WebKit/chromium/src/WebViewImpl.cpp:2221 > +bool WebViewImpl::setComposingRegion(int compositionStart, int compositionEnd, const WebVector<WebCompositionUnderline>& underlines) Ditto about the function name. > Source/WebKit/chromium/src/WebViewImpl.cpp:2228 > + if (!editor || !editor->canEdit()) In fact, you're checking the editability here!

Comment on attachment 157742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157742&action=review >> Source/WebCore/editing/Editor.cpp:2412 >> + startNode->renderer()->repaint(); > > This is very unusual things to do. Why do we need to do this? Do you mean the repaint, or all of the above? This was intended to be similar to what is done in the setComposition method (lines 1461-1471), including the repaint.

Comment on attachment 157742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157742&action=review >>> Source/WebCore/editing/Editor.cpp:2412 >>> + startNode->renderer()->repaint(); >> >> This is very unusual things to do. Why do we need to do this? > > Do you mean the repaint, or all of the above? This was intended to be similar to what is done in the setComposition method (lines 1461-1471), including the repaint. No, why are we manually setting composition underlines?

> > +bool Editor::setComposingRegion(int compositionStart, int compositionEnd, const Vector<CompositionUnderline>& underlines) > I would rename it to setCompositionWithoutInsertingText. So, it converts existing text into a composition (similar to what Kotoeri "Reverse Conversion" does on Mac)?

Created attachment 157950 [details] Patch

Comment on attachment 157742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157742&action=review >> Source/WebCore/editing/Editor.cpp:2370 >> +bool Editor::setComposingRegion(int compositionStart, int compositionEnd, const Vector<CompositionUnderline>& underlines) > > I would rename it to setCompositionWithoutInsertingText. Done. >> Source/WebCore/editing/Editor.cpp:2382 >> + return false; > > It's strange that we check the editability here. The caller should probably check this. OK, check removed. >>>> Source/WebCore/editing/Editor.cpp:2412 >>>> + startNode->renderer()->repaint(); >>> >>> This is very unusual things to do. Why do we need to do this? >> >> Do you mean the repaint, or all of the above? This was intended to be similar to what is done in the setComposition method (lines 1461-1471), including the repaint. > > No, why are we manually setting composition underlines? This sets the underlines in the same way that the existing method setComposition(const String& text, const Vector<CompositionUnderline>& underlines, unsigned selectionStart, unsigned selectionEnd) does. Underlines can be provided by callers (as they are currently in WebViewImpl::setComposition) and this just adjusts them to have the correct start offset for the new composition. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2221 >> +bool WebViewImpl::setComposingRegion(int compositionStart, int compositionEnd, const WebVector<WebCompositionUnderline>& underlines) > > Ditto about the function name. Done. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2234 >> +void WebViewImpl:: deleteSurroundingText(int before, int after) > > nit: space after "::" Fixed.

Greetings Oli, Thank you for your interesting change. Just out of curiosity, I'm personally interested in the usage scenarios of the Editor::setCompositionWithoutInsertingText function as Alexey wrote in his comment. (If this function has usage scenarios also useful for other platforms, we like to replace other platform-specific code to use it.) Would it be possible to give us usage scenarios of this function, i.e. when Android IMEs calls this function? For example, reconverting text, etc. (I'm not a WebKit reviewer and it is good to ignore my comment, though.) Regards, Hironori Bono (In reply to comment #14) > So, it converts existing text into a composition (similar to what Kotoeri "Reverse Conversion" does on Mac)?

Android IMEs utilise the InputConnection interface which provides a method setComposingRegion that does precisely what setCompositionWithoutInsertingText does - it creates a composition between the two provided offsets in the existing text being edited. IMEs can use this method however they wish. An example is the third-party IME SwiftKey which uses it to set the composition whenever the cursor is moved. For example, if the text is "hello" and the cursor is moved to be after the "e", SwiftKey calls setComposingRegion to make "he" the composition.

So, sounds like this is exactly what Kotoeri does, so adding code to WebCore should be unnecessary.

Do you know how Kotoeri achieves this? From what I can see, the only way currently to create a new composition is to use Editor::setComposition(text, undelines, selectionStart, selectionEnd), which inserts new text. It's possible to achieve the same effect as the new method by deleting text and calling setComposition to reinsert it, passing in the existing selection offsets, but it seems cleaner to be able to do this without having to delete and reinsert.

> It's possible to achieve the same effect as the new method by deleting text and calling setComposition to reinsert it I don't know, but this seems very likely.

Comment on attachment 157950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157950&action=review > Source/WebCore/editing/Editor.cpp:2376 > + if (compositionNode && compositionNode->renderer()) > + compositionNode->renderer()->repaint(); This call to repaint() is superfluous. cancelComposition must have updated it already. > Source/WebCore/editing/Editor.cpp:2390 > + It appears that we can just select the range (without changing composition node, etc...) and call setComposition at this point.

Comment on attachment 157950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157950&action=review >> Source/WebCore/editing/Editor.cpp:2376 >> + compositionNode->renderer()->repaint(); > > This call to repaint() is superfluous. cancelComposition must have updated it already. Without this, the node isn't repainted - cancelComposition calls setComposition(emptyString(), CancelComposition) which doesn't call repaint. In my testing I found that without this, calling setCompositionWithoutInsertingText with 0,0 would leave the previous underlines visible, even though the composition was cancelled. >> Source/WebCore/editing/Editor.cpp:2390 >> + > > It appears that we can just select the range (without changing composition node, etc...) and call setComposition at this point. No, that won't work because setComposition selects the existing composition near the start of the method (as the intention is that setComposition replaces the existing composition with the new composition). To use setComposition here, we'd have to do the following: - cancel the existing composition - retrieve the current selection offsets - select the range - retrieve the text of the range - delete the range - call setComposition with the text and previous selection offsets

Greetings Oli, To look into the reversion conversion of Kotoeri, it seems WebKit retrieves the selection text, passes it to Kotoeri, and calls Editor::setComposition() with the new text sent from Kotoeri. (This new text may differ from the selection text when using Kotoeri.) I like this method because it ensures the composition text is always in one text node. (For other platforms, the selection text used for reverse conversion is not always in one node. The Editor::setCompositionWithoutInsertingText function seems to return false without creating a composition range when a selection text is not in one node and I wonder if this change can handle this case.) Regards, Hironori Bono (In reply to comment #20) > Do you know how Kotoeri achieves this?

(In reply to comment #23) > (From update of attachment 157950 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157950&action=review > > >> Source/WebCore/editing/Editor.cpp:2376 > >> + compositionNode->renderer()->repaint(); > > > > This call to repaint() is superfluous. cancelComposition must have updated it already. > > Without this, the node isn't repainted - cancelComposition calls setComposition(emptyString(), CancelComposition) which doesn't call repaint. That sounds like a bug we need to fix.

(In reply to comment #24) @hbono, you're right, this solution doesn't handle the case where the new composition would span more than one node. Implementing this by calling setComposition would handle that case. @rniwa, what's your opinion on this? Is the current implementation in my patch ok, or would you prefer an implementation as I described in #23, where we would call setComposition after saving and deleting the range?

Comment on attachment 157950 [details] Patch Marking r-, please don't add new Editor code when you don't need to.

Retreiving text and setting the composition text is probably the right approach. We might even consider moving some code from Mac port's implemanation to share more code.

(In reply to comment #28) > Retreiving text and setting the composition text is probably the right approach. We might even consider moving some code from Mac port's implemanation to share more code. @rniwa: considering ap's comments, where would you suggest we do this? Should I return to my original implementation (i.e. do this in Chromium code instead of here)?

(In reply to comment #29) > (In reply to comment #28) > > Retreiving text and setting the composition text is probably the right approach. We might even consider moving some code from Mac port's implemanation to share more code. > > @rniwa: considering ap's comments, where would you suggest we do this? Should I return to my original implementation (i.e. do this in Chromium code instead of here)? This should be done in Source/chromium/WebKit. We can probably share code with Mac port. @hbono: do you know where this logic for Kotoeri is implemented?

Greetings Niwa-san, Apple WebKit implements it in <WebKit2/UIProcess/API/mac/WKView.mm>. In brief, OS X calls NSTextInputClient methods to get the selection text and start a composition as listed below when I start a reconvert of Kotoeri. 1. OS X calls [WKView selectedRange:] (*1) to get the selection range; 2. OS X calls [WKView attributedSubstringForProposedRange:] (*2) to get the selection text, and; 3. OS X calls [WKView setMarkedText:] (*3) to start a composition with new text. (*1) http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit2/UIProcess/API/mac/WKView.mm&l=1419 (*2) http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit2/UIProcess/API/mac/WKView.mm&l=1572 (*3) http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit2/UIProcess/API/mac/WKView.mm&l=1518 On Chromium, when the current selection changes, a renderer sends a ViewHostMsg_SelectionChange message to send its range and its text to a browser (*4) to avoid a sync call from a browser to a renderer. That is, a browser returns this range and text when OS X needs it. Can Android also use them? (*4) http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_host/render_view_host_impl.cc&l=1285 Regards, Hironori Bono (In reply to comment #30) > @hbono: do you know where this logic for Kotoeri is implemented?

Created attachment 159140 [details] Patch

OK, I have implemented the new setComposition method in WebViewImpl. I've renamed it to setCompositionFromExistingText as it's not quite true that it doesn't insert text. It's necessary for the method to reselect the previous selection rather than just using the selection parameters of Editor::setComposition as setComposition can only select from within the new composition.

Comment on attachment 159140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159140&action=review Oh my, this patch is much prettier. > Source/WebKit/chromium/src/WebViewImpl.cpp:2240 > + editor->performDelete(); You need to call document()->execCommand("delete", true) instead otherwise this deletion will be added to the undo stack as a separate undo step. > Source/WebKit/chromium/src/WebViewImpl.cpp:2248 > +void WebViewImpl::deleteSurroundingText(int before, int after) deleteSurroundingText sounds as if you're only deleting the surrounding text and not text in between. I would have called this extendSelectionAndDelete. > Source/WebKit/chromium/src/WebViewImpl.cpp:2267 > + editor->performDelete(); Ditto about calling document()->execCommand instead.

Created attachment 159446 [details] Patch

Comment on attachment 159140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159140&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:2240 >> + editor->performDelete(); > > You need to call document()->execCommand("delete", true) instead otherwise this deletion will be added to the undo stack as a separate undo step. OK, done. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2248 >> +void WebViewImpl::deleteSurroundingText(int before, int after) > > deleteSurroundingText sounds as if you're only deleting the surrounding text and not text in between. > I would have called this extendSelectionAndDelete. Renamed as suggested. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2267 >> + editor->performDelete(); > > Ditto about calling document()->execCommand instead. Done.

Comment on attachment 159446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159446&action=review > Source/WebKit/chromium/ChangeLog:3 > + Add WebView methods setCompositionFromExistingText and extendSelectionAndDelete. Please update this line so that it matches the bug title.

Created attachment 159470 [details] Patch

Comment on attachment 159446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159446&action=review >> Source/WebKit/chromium/ChangeLog:3 >> + Add WebView methods setCompositionFromExistingText and extendSelectionAndDelete. > > Please update this line so that it matches the bug title. Done.

Comment on attachment 159470 [details] Patch Since you're not a committer, you should ask some committer to land your patch or set cq? flag.

Comment on attachment 159470 [details] Patch Clearing flags on attachment: 159470 Committed r126200: <http://trac.webkit.org/changeset/126200>

All reviewed patches have been landed. Closing bug.