Reland: Fix setComposingText with empty text when newCursorPosition != 1

third_party/WebKit/Source/web/WebViewImpl.cpp

Index: third_party/WebKit/Source/web/WebViewImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebViewImpl.cpp b/third_party/WebKit/Source/web/WebViewImpl.cpp
index 3e92e32a2f867341300bb60c550f5729a13a1c55..c44a9fa4d494b3c77d3d0a21143e712114936821 100644
--- a/third_party/WebKit/Source/web/WebViewImpl.cpp
+++ b/third_party/WebKit/Source/web/WebViewImpl.cpp
@@ -2314,6 +2314,29 @@ void WebViewImpl::setFocus(bool enable)
     }
 }
 
+std::pair<int, int> WebViewImpl::adjustSelectionForBoundary(int selectionStart, int selectionEnd, const int textLength)
+{

[yabinh, 2016/07/06 11:53:55]

textInputInfo() is quite an expensive function, but it seems that we have no
choice.
See the discussion here:
https://codereview.chromium.org/1995333002/#msg9

[Changwan Ryu, 2016/07/07 09:09:33]

You should avoid calling textInputInfo(). textInputInfo() can take a up to 100ms
or more. See crbug.com/532519 for more details.

At the minimum, I think you can call an excerpt from textInputInfo():

EphemeralRange range =
focusedFrame->inputMethodController().compositionEphemeralRange();
if (range.isNotNull()) {
...
}

However, I don't think that you should be doing this here.

The current structure is as follows:

1. Do a predictive boundary check in WebViewImpl::adjustSelectionForBoundary().

But internally it's calling InputMethodController::compositionEphemeralRange()
because
selection boundary check has a dependency on the previous composition and the
new composition. The reason is that the new cursor position is relative to the
new composition region, not the old cursor position.

(See 
https://developer.android.com/reference/android/view/inputmethod/InputConnect...,
int) for more details.)

2. Then call InputMethodController with a valid parameter.


We assumed that parameter check doesn't depend on the internal state of IMC, but
it does. Also, it could have been much simpler if we do the boundary check after
the composition has already been made.

I'd say that selectionStart / selectionEnd are not absolute values you can
simply check, but they are complex relative values that can only be checked in
the middle of the process.

In this sense, I prefer the previous approach where you did the check inside
setComposingText / commitText().

yosin@, what do you think?

+    WebTextInputInfo textInputInfo = this->textInputInfo();
+
+    int compositionLength = textInputInfo.compositionEnd - textInputInfo.compositionStart;
+    int beforeCompositionLength;
+    if (compositionLength != 0)
+        beforeCompositionLength = textInputInfo.compositionStart;
+    else
+        beforeCompositionLength = textInputInfo.selectionStart;
+    int afterCompositionLength = static_cast<int>(textInputInfo.value.length()) - compositionLength - beforeCompositionLength;
+
+    // In case of exceeding the left boundary.
+    selectionStart = std::max(selectionStart, -beforeCompositionLength);
+    selectionEnd = std::max(selectionEnd, selectionStart);
+
+    // In case of exceeding the right boundary.
+    selectionEnd = std::min(selectionEnd, textLength + afterCompositionLength);
+    selectionStart = std::min(selectionStart, selectionEnd);
+
+    return std::make_pair(selectionStart, selectionEnd);
+}
+
 bool WebViewImpl::setComposition(
     const WebString& text,
     const WebVector<WebCompositionUnderline>& underlines,
@@ -2355,13 +2378,15 @@ bool WebViewImpl::setComposition(
 
     UserGestureIndicator gestureIndicator(DefinitelyProcessingNewUserGesture);
 

[yabinh, 2016/07/06 11:53:55]

adjustSelectionForBoundary is moved here, thus it can be reused by
WebViewImpl::confirmComposition. It will save a lot of trouble.
See the cl I am working on:
https://codereview.chromium.org/1995333002/

+    std::pair<int, int> selectionPair = adjustSelectionForBoundary(selectionStart, selectionEnd, text.length());
+
     // When the range of composition underlines overlap with the range between
     // selectionStart and selectionEnd, WebKit somehow won't paint the selection
     // at all (see InlineTextBox::paint() function in InlineTextBox.cpp).
     // But the selection range actually takes effect.
     inputMethodController.setComposition(String(text),
                            CompositionUnderlineVectorBuilder(underlines),
-                           selectionStart, selectionEnd);
+                           selectionPair.first, selectionPair.second);
 
     return text.isEmpty() || inputMethodController.hasComposition();
 }