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

third_party/WebKit/Source/web/WebViewImpl.cpp

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 2296 matching lines @@
                 focusedFrame->inputMethodController().confirmComposition();
 
                 if (autofillClient)
                     autofillClient->setIgnoreTextChanges(false);
             }
             m_imeAcceptEvents = false;
         }
     }
 }
 
+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,
     int selectionStart,
     int selectionEnd)
 {
     LocalFrame* focused = toLocalFrame(focusedCoreFrame());
     if (!focused || !m_imeAcceptEvents)
         return false;
 
@@ skipping 20 matching lines @@
 
     // A keypress event is canceled. If an ongoing composition exists, then the
     // keydown event should have arisen from a handled key (e.g., backspace).
     // In this case we ignore the cancellation and continue; otherwise (no
     // ongoing composition) we exit and signal success only for attempts to
     // clear the composition.
     if (m_suppressNextKeypressEvent && !inputMethodController.hasComposition())
         return text.isEmpty();
 
     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();
 }
 
 bool WebViewImpl::confirmComposition()
 {
     return confirmComposition(DoNotKeepSelection);
 }
 
 bool WebViewImpl::confirmComposition(ConfirmCompositionBehavior selectionBehavior)
@@ skipping 2158 matching lines @@
 {
     // TODO(oshima): Investigate if this should return the ScreenInfo's scale factor rather than
     // page's scale factor, which can be 1 in use-zoom-for-dsf mode.
     if (!page())
         return 1;
 
     return page()->deviceScaleFactor();
 }
 
 } // namespace blink