Handle newCursorPosition correctly for Android's commitText()

third_party/WebKit/Source/core/editing/InputMethodController.h

 /*
  * Copyright (C) 2006, 2007, 2008 Apple 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:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 39 matching lines @@
         KeepSelection,
     };
 
     static InputMethodController* create(LocalFrame&);
     DECLARE_TRACE();
 
     // international text input composition
     bool hasComposition() const;
     void setComposition(const String&, const Vector<CompositionUnderline>&, int selectionStart, int selectionEnd);
     void setCompositionFromExistingText(const Vector<CompositionUnderline>&, unsigned compositionStart, unsigned compositionEnd);
+
     // Inserts the text that is being composed as a regular text and returns true
     // if composition exists.
-    bool confirmComposition();
+    bool replaceComposition();

[aelias_OOO_until_Jul13, 2016/08/25 02:50:57]

I think the old name "confirmComposition" is better for this one.  It doesn't
have text passed in for replacement, it just confirms the ongoing.

[yabinh, 2016/08/25 10:27:08]

Done.

     // Inserts the given text string in the place of the existing composition
     // and returns true.
-    bool confirmComposition(const String& text, ConfirmCompositionBehavior confirmBehavior = KeepSelection);
+    bool replaceComposition(const String& text, ConfirmCompositionBehavior confirmBehavior = KeepSelection);

[aelias_OOO_until_Jul13, 2016/08/25 02:50:57]

This is the one I wanted to make private.  AFAIK, it has no outside callers. 
(If there's one, maybe it can be cleaned up?  Particularly if it's only in a
test, as we shouldn't have public APIs purely for tests.)

Secondly, please remove the default argument here.

[yabinh, 2016/08/25 10:27:08]

Default argument is removed.

This one is called in WebLocalFrameImpl::insertText(), and we can't simply
replace it with the above one (replaceComposition()) or confirmComposition(const
String& text). So maybe we should also keep it public?

[aelias_OOO_until_Jul13, 2016/08/25 22:43:15]

The if/else logic in WebLocalFrameImpl::insertText() seems really
similar/redundant with confirmCompositionOrInsertText(), so please try to delete
it and replace with a single call to that.  (The behavior is a little different
but likely in the direction that the InputMethodController variant has fewer
bugs.)

[yabinh, 2016/08/26 01:23:20]

When |text| is empty, replaceComposition(const String& text,
ConfirmCompositionBehavior confirmBehavior)  will delete the composing text, but
confirmComposition (or the old confirmCompositionOrInsertText) will insert the
composing text. We can't ignore the difference.

Yes, we can move some code from replaceComposition into
WebLocalFrameImpl::insertText to do the deletion stuff, but it will be kind of
ugly.

[aelias_OOO_until_Jul13, 2016/08/26 02:08:59]

Looks like WebLocalFrame::insertText actually is dead code with no callers so
any difference is *extremely* ignorable :).  Please simply delete
WebLocalFrame::insertText().

[yabinh, 2016/08/26 02:46:23]

Yes. We can simply delete it. Perfect!

+
     // Inserts the text that is being composed or specified non-empty text and
-    // returns true.
-    bool confirmCompositionOrInsertText(const String& text, ConfirmCompositionBehavior);
+    // returns true. This will not change the selection.
+    bool confirmComposition(const String& text);
+    // Inserts the text that is being composed or specified non-empty text and
+    // returns true. This will change the selection according to
+    // relativeCursorPosition.
+    bool confirmCompositionWithCursor(const String& text, int relativeCursorPosition);
+

[yabinh, 2016/08/26 02:46:23]

On a second thought, I think it's better to name them
confirmCompositionOrInsertText and confirmCompositionOrInsertTextWithCursor,
because they not only confirm composition, but also insert text if no
composition. Plus, since we have confirmComposition() to confirm composition
only, it will be confusing.

What do you think, aelias@?

[aelias_OOO_until_Jul13, 2016/08/26 05:58:59]

Dunno, I find that too long and I think that method names with words like "or",
"try" or "maybe" obscure more than they clarify.  I'd suggest it be renamed
"commitComposition" to have less implication that an existing composition exists
to confirm.  But I don't care that much and maybe yosin@ would overrule me on
the name anyway, so still lgtm regardless of whatever name you choose.

[yabinh, 2016/08/26 07:18:32]

OK. I'll use commitComposition for now and ask yosin@'s opinion.

     // Deletes the existing composition text.
     void cancelComposition();
     void cancelCompositionIfSelectionIsInvalid();
     EphemeralRange compositionEphemeralRange() const;
     Range* compositionRange() const;
 
     void clear();
     void documentDetached();
 
     PlainTextRange getSelectionOffsets() const;
@@ skipping 30 matching lines @@
     }
 
     String composingText() const;
     void selectComposition() const;
     bool setSelectionOffsets(const PlainTextRange&, FrameSelection::SetSelectionOptions = FrameSelection::CloseTyping);
 };
 
 } // namespace blink
 
 #endif // InputMethodController_h