Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

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

Index: third_party/WebKit/Source/core/editing/InputMethodController.cpp
diff --git a/third_party/WebKit/Source/core/editing/InputMethodController.cpp b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
index 50c4d95d121410f20ccfe36e4d0942ff890e6f28..6df4672b33a4ab72708c1ce10b6efe1ea67abe38 100644
--- a/third_party/WebKit/Source/core/editing/InputMethodController.cpp
+++ b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
@@ -183,6 +183,61 @@ AtomicString getInputModeAttribute(Element* element) {
   return element->fastGetAttribute(HTMLNames::inputmodeAttr).lower();
 }
 
+bool hasInvalidSurrogatePair(const UChar* text,
+                             const int start,
+                             const int end) {
+  for (int i = start; i < end; i++) {
+    if (U_IS_SURROGATE(text[i])) {
+      const bool isValidSurrogatePair =
+          U16_IS_LEAD(text[i]) && i + 1 < end && U16_IS_TRAIL(text[i + 1]);
+      if (!isValidSurrogatePair)
+        return true;
+      i++;
+    }
+  }
+  return false;
+}
+
+std::pair<int, int> invalidDeletionLength() {
+  return std::make_pair(-1, -1);
+}
+
+bool isInvalidDeletionLength(const std::pair<int, int>& lengthPair) {
+  return lengthPair.first == -1 && lengthPair.second == -1;
+}
+
+std::pair<int, int> convertDeletionLengthForDeleteSurroundingTextInCodePoints(

[Changwan Ryu, 2017/01/25 07:30:56]

The name is a bit unnecessarily lengthy. How about renaming this to
calculateDeletionLengthsInCodePoints()?

[yabinh, 2017/02/08 12:32:41]

Done.

+    const String& text,
+    const int beforeLengthInCodePoints,
+    const int afterLengthInCodePoints,
+    const int selectionStart,
+    const int selectionEnd) {
+  int deletionStart = selectionStart;
+  int deletionEnd = selectionEnd;
+  const int length = static_cast<int>(text.length());
+
+  const UChar* UText;
+  if (text.is8Bit()) {

[Changwan Ryu, 2017/01/25 07:30:56]

I really hope that we don't do the conversion here because this will only be
used for testing anyways.

Could you instead add CHECK(!text.is8Bit()); here?

Blink tests may need to use String::FromUTF8() to do the conversion into UTF-16.
Could you investigate ImeTest case? It's a bit concerning that it's being
converted to UTF-8 somewhere in the pipeline.

[yabinh, 2017/02/08 12:32:41]

Actually we convert utf16 to utf8 in RenderWidget::OnImeCommitText():
|text| is utf16, but |WebString::fromUTF16(text)| is utf8.
(WebString::fromUTF16() tries to create 8 bits if possible.)

So we need to convert it back for
ImeTest#testInputTextEvents_DeleteSurroundingTextInCodePoints.
As for another similar test
testContentEditableEvents_DeleteSurroundingTextInCodePoints, we don't have to do
that because there is already some text (utf16) in contenteditable, so when
inserting "hello"(utf8), blink will covert it to utf16 and then insert. (in
String::insert()).

[Changwan Ryu, 2017/02/09 01:06:34]

Thanks for looking into it. StringImpl::create8BitIfPossible() seems to compress
the data to 8 bit whenever possible, and not do the actual conversion, which is
very smart and does not break assumptions in other parts of the file.

Now that we understand in what cases we have UTF-8, I prefer the following
structure:

if (text.is8Bit()) {
  U8_BACK_N(text.characters8(), 0, deletionStart, beforeLengthInCodePoints);
  U8_FWD_N(text.characters8(), deletionEnd, length, afterLengthInCodePoints);
} else {
  U16_BACK_N(...);
  // Required by Android's InputConnection spec.
  if (hasInvalidSurrogatePair(...))
    return invalidDeletionLength();
  U16_FWD_N(...);
  // Required by Android's InputConnection spec.
  if (hasInvalidSurrogatePair(...))
    return invalidDeletionLength();
}

...

This way we can avoid unnecessary conversion and still rest assured that both
cases are covered by tests.

[yabinh, 2017/02/09 05:21:37]

Done.

+    const String& text16 = String::make16BitFrom8BitSource(
+        text.characters8(), static_cast<size_t>(length));
+    UText = text16.characters16();
+  } else {
+    UText = text.characters16();
+  }
+
+  U16_BACK_N(UText, 0, deletionStart, beforeLengthInCodePoints);
+  if (hasInvalidSurrogatePair(UText, deletionStart, selectionStart))
+    return invalidDeletionLength();
+
+  U16_FWD_N(UText, deletionEnd, length, afterLengthInCodePoints);
+  if (hasInvalidSurrogatePair(UText, selectionEnd, deletionEnd))
+    return invalidDeletionLength();
+
+  const int beforeLength = selectionStart - deletionStart;
+  const int afterLength = deletionEnd - selectionEnd;

[Changwan Ryu, 2017/01/25 07:30:56]

could you add the following?
DCHECK_GE(beforeLength, 0);
DCHECK_GE(afterLength, 0);

[yabinh, 2017/02/08 12:32:42]

Done.

+  return std::make_pair(beforeLength, afterLength);
+}
+
 }  // anonymous namespace
 
 InputMethodController* InputMethodController::create(LocalFrame& frame) {
@@ -853,6 +908,35 @@ void InputMethodController::deleteSurroundingText(int before, int after) {
   setSelectionOffsets(PlainTextRange(selectionStart, selectionEnd));
 }
 
+void InputMethodController::deleteSurroundingTextInCodePoints(int before,
+                                                              int after) {
+  DCHECK_GE(before, 0);
+  DCHECK_GE(after, 0);
+  if (!editor().canEdit())
+    return;
+  const PlainTextRange selectionOffsets(getSelectionOffsets());
+  if (selectionOffsets.isNull())
+    return;
+  Element* const rootEditableElement =
+      frame().selection().rootEditableElement();
+  if (!rootEditableElement)
+    return;
+
+  const int selectionStart = static_cast<int>(selectionOffsets.start());
+  const int selectionEnd = static_cast<int>(selectionOffsets.end());
+  const String& text =
+      plainText(EphemeralRange::rangeOfContents(*rootEditableElement),
+                TextIteratorEmitsObjectReplacementCharacter);
+  std::pair<int, int> deletionLengthPair =
+      convertDeletionLengthForDeleteSurroundingTextInCodePoints(
+          text, before, after, selectionStart, selectionEnd);
+  if (isInvalidDeletionLength(deletionLengthPair))
+    return;
+
+  return deleteSurroundingText(deletionLengthPair.first,
+                               deletionLengthPair.second);
+}
+
 WebTextInputInfo InputMethodController::textInputInfo() const {
   WebTextInputInfo info;
   if (!isAvailable())