Adjust common prefix/suffix correctly for InsertIncrementalTextCommand

third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp

Index: third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp
diff --git a/third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp b/third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp
index 4759d76a93b4d40bd5da1fd2f4e0904419c9d984..5d01df334bae6e8115465cbcbf88449539a4fcf4 100644
--- a/third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp
+++ b/third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp
@@ -51,17 +51,18 @@ size_t computeDistanceToLeftGraphemeBoundary(const Position& position) {
                              adjustedPosition.computeOffsetInContainerNode());
 }
 
-size_t computeCommonGraphemeClusterPrefixLength(
-    const int selectionStart,
-    const String& oldText,
-    const String& newText,
-    const Element* rootEditableElement) {
+size_t computeCommonGraphemeClusterPrefixLength(const Position& selectionStart,
+                                                const String& oldText,
+                                                const String& newText) {
   const size_t commonPrefixLength = computeCommonPrefixLength(oldText, newText);
+  const int selectionOffset = selectionStart.computeOffsetInContainerNode();
+  const ContainerNode* selectionNode =
+      selectionStart.computeContainerNode()->parentNode();

[Changwan Ryu, 2017/03/23 21:20:17]

Is it safe not to have a null check here? I mean, both for containernode and
parent node. BTW, it seems that selectionNode here just means rootEditable, not
the selection itself.

 
   // For grapheme cluster, we should adjust it for grapheme boundary.
   const EphemeralRange& range =
-      PlainTextRange(0, selectionStart + commonPrefixLength)
-          .createRange(*rootEditableElement);
+      PlainTextRange(0, selectionOffset + commonPrefixLength)
+          .createRange(*selectionNode);
   if (range.isNull())
     return 0;
   const Position& position = range.endPosition();
@@ -83,22 +84,24 @@ size_t computeDistanceToRightGraphemeBoundary(const Position& position) {
                              position.computeOffsetInContainerNode());
 }
 
-size_t computeCommonGraphemeClusterSuffixLength(
-    const int selectionStart,
-    const String& oldText,
-    const String& newText,
-    const Element* rootEditableElement) {
+size_t computeCommonGraphemeClusterSuffixLength(const Position& selectionStart,
+                                                const String& oldText,
+                                                const String& newText) {
   const size_t commonSuffixLength = computeCommonSuffixLength(oldText, newText);
+  const int selectionOffset = selectionStart.computeOffsetInContainerNode();
+  const ContainerNode* selectionNode =
+      selectionStart.computeContainerNode()->parentNode();

[Changwan Ryu, 2017/03/23 21:20:17]

Same comment applies here.

 
   // For grapheme cluster, we should adjust it for grapheme boundary.
   const EphemeralRange& range =
-      PlainTextRange(0, selectionStart + oldText.length() - commonSuffixLength)
-          .createRange(*rootEditableElement);
+      PlainTextRange(0, selectionOffset + oldText.length() - commonSuffixLength)
+          .createRange(*selectionNode);
   if (range.isNull())
     return 0;
   const Position& position = range.endPosition();
   const size_t diff = computeDistanceToRightGraphemeBoundary(position);

[yabinh, 2017/03/21 10:13:44]

BTW, I don't think we need worry about the DCHECK in
computeCommonGraphemeCluster[Prefix]Length. I couldn't find a case where it
fails. So let's keep it as it is.

[yosin_UTC9, 2017/03/22 02:22:39]

I'm confused.

Do you want to keep "newly added" if-statement instead of DCHECK_GE()?

[yabinh, 2017/03/22 02:35:34]

Sorry for the confusion. I want to replace DCHECK in [suffix] case, and keep it
in [prefix] case.

-  DCHECK_GE(commonSuffixLength, diff);
+  if (diff > commonSuffixLength)
+    return 0;
   return commonSuffixLength - diff;
 }
 
@@ -153,16 +156,15 @@ void InsertIncrementalTextCommand::doApply(EditingState* editingState) {
   const String oldText = plainText(selectionRange);
   const String& newText = m_text;
 
-  const int selectionStart =
-      endingSelection().start().computeOffsetInContainerNode();
+  const Position& selectionStart = endingSelection().start();
   const size_t newTextLength = newText.length();
   const size_t oldTextLength = oldText.length();
   const size_t commonPrefixLength = computeCommonGraphemeClusterPrefixLength(
-      selectionStart, oldText, newText, element);
+      selectionStart, oldText, newText);
   // We should ignore common prefix when finding common suffix.
   const size_t commonSuffixLength = computeCommonGraphemeClusterSuffixLength(
       selectionStart, oldText.right(oldTextLength - commonPrefixLength),
-      newText.right(newTextLength - commonPrefixLength), element);
+      newText.right(newTextLength - commonPrefixLength));
   DCHECK_GE(oldTextLength, commonPrefixLength + commonSuffixLength);
 
   m_text =