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 538a611263d2e62f88249952a03bd8489e6cfa7f..25469887b98ce086ec24b4f45dc878a147927d5d 100644
--- a/third_party/WebKit/Source/core/editing/InputMethodController.cpp
+++ b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
@@ -200,6 +200,63 @@ AtomicString getInputModeAttribute(Element* element) {
   return element->fastGetAttribute(HTMLNames::inputmodeAttr).lower();
 }
 
+bool hasInvalidSurrogatePair(const UChar* text,
+                             const int start,
+                             const int end) {

[yosin_UTC9, 2017/02/09 07:19:27]

Let's have following DCHECK's

DCHECK_GE(start, 0);
DCHECK_LE(start, end);

[yabinh, 2017/02/09 10:37:03]

Done.

+  for (int i = start; i < end; i++) {

[yosin_UTC9, 2017/02/09 07:19:27]

nit: s/i++/++i/
[1] allows to use both pre and post but so far we use pre-increment in editing/


[1]
https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

[yabinh, 2017/02/09 10:37:03]

Done.

+    if (U_IS_SURROGATE(text[i])) {
+      const bool isValidSurrogatePair =
+          U16_IS_LEAD(text[i]) && i + 1 < end && U16_IS_TRAIL(text[i + 1]);

[yosin_UTC9, 2017/02/09 07:19:27]

Let's loop over |end - 1| to avoid checking |i| here.

[yabinh, 2017/02/09 10:37:03]

We shouldn't do that because it will fail when text[end-1] is a high surrogate.

+      if (!isValidSurrogatePair)
+        return true;
+      i++;

[yosin_UTC9, 2017/02/09 07:19:27]

nit: s/i++/++i/
[1] allows to use both pre and post but so far we use pre-increment in editing/


[1]
https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

[yabinh, 2017/02/09 10:37:03]

Done.

+    }
+  }
+  return false;
+}
+
+std::pair<int, int> invalidDeletionLength() {

[yosin_UTC9, 2017/02/09 07:19:27]

Try to mark |constexpr|.

[yabinh, 2017/02/09 10:37:03]

Do you mean simply adding a mark, like this:

constexpr bool isInvalidDeletionLength(const std::pair<int, int>& lengthPair) {
  return lengthPair.first == -1 && lengthPair.second == -1;
}

It won't compile. I couldn't figure out why.

[yosin_UTC9, 2017/02/10 04:54:11]

I mean

constexpr std::pair<int, int> invalidDeletionLength() {
  return std::make_pair(-1, -1);
}

It works on MSVC, so clang must support this.

[yabinh, 2017/02/10 05:46:07]

Sorry, I was referring to the above lines. It can work in other places (I tried
several online compilers),but here I got the error:

../../third_party/WebKit/Source/core/editing/InputMethodController.cpp: In
function 'constexpr std::__ndk1::pair<int, int>
blink::{anonymous}::invalidDeletionLength()':
../../third_party/WebKit/Source/core/editing/InputMethodController.cpp:221:31:
error: call to non-constexpr function 'std::__ndk1::pair<typename
std::__ndk1::__make_pair_return<_Tp>::type, typename
std::__ndk1::__make_pair_return<_T2>::type> std::__ndk1::make_pair(_T1&&, _T2&&)
[with _T1 = int; _T2 = int; typename std::__ndk1::__make_pair_return<_T2>::type
= int; typename std::__ndk1::__make_pair_return<_Tp>::type = int]'
   return std::make_pair(-1, -1);
                               ^

+  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> calculateDeletionLengthsInCodePoints(
+    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());
+
+  if (text.is8Bit()) {
+    const LChar* LText = text.characters8();

[yosin_UTC9, 2017/02/09 07:19:27]

s/LText/character8s/

[yabinh, 2017/02/09 10:37:03]

Done.

+    U8_BACK_N(LText, 0, deletionStart, beforeLengthInCodePoints);

[yosin_UTC9, 2017/02/09 07:19:27]

I think LChar, Latin-1 Character/ doesn't have surrogate pair.
So, we don't need to use ICU macro.

[yabinh, 2017/02/09 10:37:03]

Yes. That's why we don't need to check whether it has invalid surrogate pair
like utf16. But here we still need to compute |deletionStart| (in code unit)
based on |beforeLengthInCodePoints| (in code points).

[yosin_UTC9, 2017/02/10 04:54:11]

U8_XXX_N() is for UTF-8, but LChar contains Latin-1 characters.
It is wrong to traversing Latin-1 characters as UTF-8.

[yabinh, 2017/02/10 07:27:26]

You are right.
Actually utf8 is converted in WebString::fromUTF16()
(See comment: https://codereview.chromium.org/2617443002/#msg39).
It converts the utf16 text into utf8 if the text only has Latin-1 characters.
Otherwise, the text will still be in utf16. So we can write like this:

 if (text.is8Bit()) {
    return std::make_pair(beforeLengthInCodePoints, afterLengthInCodePoints);
 } else {
...
 }

What do you think?

+    U8_FWD_N(LText, deletionEnd, length, afterLengthInCodePoints);
+  } else {
+    const UChar* UText = text.characters16();

[yosin_UTC9, 2017/02/09 07:19:27]

s/LText/character16s/

[yabinh, 2017/02/09 10:37:03]

Done.

+    U16_BACK_N(UText, 0, deletionStart, beforeLengthInCodePoints);

[yosin_UTC9, 2017/02/09 07:19:27]

nona@, do we have utility function to walk around UTF-16 string with code point?
I would like to avoid ICU's macros.

[Seigo Nonaka, 2017/02/09 07:29:21]

We only have base/i18n/char_iterator.h, but it can't go backward.

+    // Required by Android's InputConnection spec.
+    if (hasInvalidSurrogatePair(UText, deletionStart, selectionStart))
+      return invalidDeletionLength();
+
+    U16_FWD_N(UText, deletionEnd, length, afterLengthInCodePoints);
+    // Required by Android's InputConnection spec.
+    if (hasInvalidSurrogatePair(UText, selectionEnd, deletionEnd))
+      return invalidDeletionLength();
+  }
+
+  const int beforeLength = selectionStart - deletionStart;
+  const int afterLength = deletionEnd - selectionEnd;
+  DCHECK_GE(beforeLength, 0);
+  DCHECK_GE(afterLength, 0);
+  return std::make_pair(beforeLength, afterLength);
+}
+
 }  // anonymous namespace
 
 InputMethodController* InputMethodController::create(LocalFrame& frame) {
@@ -891,6 +948,37 @@ 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 TextIteratorBehavior& behavior =
+      TextIteratorBehavior::Builder()
+          .setEmitsObjectReplacementCharacter(true)
+          .build();
+  const String& text = plainText(
+      EphemeralRange::rangeOfContents(*rootEditableElement), behavior);
+  std::pair<int, int> deletionLengthPair = calculateDeletionLengthsInCodePoints(
+      text, before, after, selectionStart, selectionEnd);
+  if (isInvalidDeletionLength(deletionLengthPair))
+    return;
+
+  return deleteSurroundingText(deletionLengthPair.first,
+                               deletionLengthPair.second);
+}
+
 WebTextInputInfo InputMethodController::textInputInfo() const {
   WebTextInputInfo info;
   if (!isAvailable())