Handle newCursorPosition correctly for Android's commitText()

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 2350 matching lines @@
     // 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);
 
     return text.isEmpty() || inputMethodController.hasComposition();
 }
 
-bool WebViewImpl::confirmComposition()
+bool WebViewImpl::confirmComposition(int newCursorPosition)
 {
-    return confirmComposition(DoNotKeepSelection);
+    return confirmComposition(DoNotKeepSelection, newCursorPosition);
 }
 
-bool WebViewImpl::confirmComposition(ConfirmCompositionBehavior selectionBehavior)
+bool WebViewImpl::confirmComposition(ConfirmCompositionBehavior selectionBehavior, int newCursorPosition)
 {
-    return confirmComposition(WebString(), selectionBehavior);
+    return confirmComposition(WebString(), selectionBehavior, newCursorPosition);
 }
 
-bool WebViewImpl::confirmComposition(const WebString& text)
+bool WebViewImpl::confirmComposition(const WebString& text, int newCursorPosition)
 {
     UserGestureIndicator gestureIndicator(DefinitelyProcessingNewUserGesture);
-    return confirmComposition(text, DoNotKeepSelection);
+    return confirmComposition(text, DoNotKeepSelection, newCursorPosition);
 }
 
-bool WebViewImpl::confirmComposition(const WebString& text, ConfirmCompositionBehavior selectionBehavior)
+bool WebViewImpl::confirmComposition(const WebString& text, ConfirmCompositionBehavior selectionBehavior, int newCursorPosition)
 {
     LocalFrame* focused = toLocalFrame(focusedCoreFrame());
     if (!focused || !m_imeAcceptEvents)
         return false;
 
+    InputMethodController& inputMethodController = focused->inputMethodController();
+    if (inputMethodController.hasComposition()) {
+        if (newCursorPosition < 1) {
+            newCursorPosition = newCursorPosition + textInputInfo().compositionStart;

[Changwan Ryu, 2016/05/24 08:35:50]

1. Don't call textInputInfo() repeatedly, as it is quite an expensive function.
2. InputMethodController has access to m_compositionRange. Why do you have to
call textInputInfo().compositionStart? Is it because of
WebPlugin::confirmComposition()? How did you test it?

[yabinh, 2016/07/06 11:44:30]

InputMethodController::m_compositionRange doesn't work for multiple nodes,
because the offset in InputMethodController::m_compositionRange is based on its
anchor node, not rootEditableElement.

textInputInfo() works for multiple nodes.

BTW, I'll upload a new patch once another cl is committed:
https://codereview.chromium.org/2020973002/

+        } else {
+            newCursorPosition = newCursorPosition + textInputInfo().compositionEnd - 1;
+        }
+    } else {
+        if (newCursorPosition < 1) {
+            newCursorPosition = newCursorPosition + textInputInfo().selectionStart;
+        } else {
+            newCursorPosition = newCursorPosition + textInputInfo().selectionStart + text.length() - 1;
+        }
+    }
+
     if (WebPlugin* plugin = focusedPluginIfInputMethodSupported(focused))
-        return plugin->confirmComposition(text, selectionBehavior);
+        return plugin->confirmComposition(text, selectionBehavior, newCursorPosition);
 
-    return focused->inputMethodController().confirmCompositionOrInsertText(text, selectionBehavior == KeepSelection ? InputMethodController::KeepSelection : InputMethodController::DoNotKeepSelection);
+    if (selectionBehavior == KeepSelection)

[Changwan Ryu, 2016/05/24 08:35:50]

Why couldn't you pass selectionBehavior to confirmCompositionOrInsertText()? I
think it's always better to put the logic in one place than spread it around.

+        return inputMethodController.confirmCompositionOrInsertText(text, InputMethodController::KeepSelection);
+
+    return inputMethodController.confirmCompositionOrInsertText(text, InputMethodController::DoNotKeepSelection)
+        && inputMethodController.setEditableSelectionOffsetsWithBoundaryCheck(newCursorPosition, newCursorPosition);
 }
 
 bool WebViewImpl::compositionRange(size_t* location, size_t* length)
 {
     // FIXME: Long term, the focused frame should be a local frame. For now,
     // return early to avoid crashes.
     Frame* frame = focusedCoreFrame();
     if (!frame || frame->isRemoteFrame())
         return false;
 
@@ skipping 2127 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