Adjust common prefix/suffix correctly for InsertIncrementalTextCommand

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

 // Copyright (c) 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/editing/commands/InsertIncrementalTextCommand.h"
 
 #include "core/dom/Document.h"
 #include "core/dom/Element.h"
 #include "core/dom/Text.h"
 #include "core/editing/EditingUtilities.h"
@@ skipping 33 matching lines @@
   const Position& adjustedPosition = previousPositionOf(
       nextPositionOf(position, PositionMoveType::GraphemeCluster),
       PositionMoveType::GraphemeCluster);
   DCHECK_EQ(position.anchorNode(), adjustedPosition.anchorNode());
   DCHECK_GE(position.computeOffsetInContainerNode(),
             adjustedPosition.computeOffsetInContainerNode());
   return static_cast<size_t>(position.computeOffsetInContainerNode() -
                              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();
   const size_t diff = computeDistanceToLeftGraphemeBoundary(position);

[yabinh, 2017/03/22 02:38:32]

We don't need replace the DCHECK in [prefix] case.

   DCHECK_GE(commonPrefixLength, diff);
   return commonPrefixLength - diff;
 }
 
 // If current position is at grapheme boundary, return 0; otherwise, return the
 // distance to its nearest right grapheme boundary.
 size_t computeDistanceToRightGraphemeBoundary(const Position& position) {
   const Position& adjustedPosition = nextPositionOf(
       previousPositionOf(position, PositionMoveType::GraphemeCluster),
       PositionMoveType::GraphemeCluster);
   DCHECK_EQ(position.anchorNode(), adjustedPosition.anchorNode());
   DCHECK_GE(adjustedPosition.computeOffsetInContainerNode(),
             position.computeOffsetInContainerNode());
   return static_cast<size_t>(adjustedPosition.computeOffsetInContainerNode() -
                              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;
 }
 
 const String computeTextForInsertion(const String& newText,
                                      const size_t commonPrefixLength,
                                      const size_t commonSuffixLength) {
   return newText.substring(
       commonPrefixLength,
       newText.length() - commonPrefixLength - commonSuffixLength);
 }
@@ skipping 34 matching lines @@
 
 void InsertIncrementalTextCommand::doApply(EditingState* editingState) {
   const Element* element = endingSelection().rootEditableElement();
   DCHECK(element);
 
   const EphemeralRange selectionRange(endingSelection().start(),
                                       endingSelection().end());
   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 =
       computeTextForInsertion(m_text, commonPrefixLength, commonSuffixLength);
 
   const int offset = static_cast<int>(commonPrefixLength);
   const int length =
       static_cast<int>(oldTextLength - commonPrefixLength - commonSuffixLength);
   const VisibleSelection& selectionForInsertion = computeSelectionForInsertion(
       selectionRange, offset, length, endingSelection().isDirectional());
 
   setEndingSelectionWithoutValidation(selectionForInsertion.start(),
                                       selectionForInsertion.end());
 
   InsertTextCommand::doApply(editingState);
 }
 
 }  // namespace blink