Extract InlineBox handling from MostForwardCaretPosition()

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

 /*
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009 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
@@ skipping 2804 matching lines @@
     if (layout_object->IsText() &&
         ToLayoutText(layout_object)->FirstTextBox()) {
       LayoutText* const text_layout_object = ToLayoutText(layout_object);
       const unsigned text_start_offset = text_layout_object->TextStartOffset();
       if (current_node != start_node) {
         DCHECK(current_pos.AtStartOfNode());
         return PositionTemplate<Strategy>(
             current_node, layout_object->CaretMinOffset() + text_start_offset);
       }
 
-      // Map offset in DOM node to offset in InlineBox.
-      DCHECK_GE(current_pos.OffsetInLeafNode(),
-                static_cast<int>(text_start_offset));
-      const unsigned text_offset =
-          current_pos.OffsetInLeafNode() - text_start_offset;
-      InlineTextBox* last_text_box = text_layout_object->LastTextBox();
-      for (InlineTextBox* box = text_layout_object->FirstTextBox(); box;
-           box = box->NextTextBox()) {
-        if (text_offset <= box->end()) {
-          if (text_offset >= box->Start())
-            return current_pos.ComputePosition();
-          continue;
-        }
-
-        if (box == last_text_box || text_offset != box->Start() + box->Len())
-          continue;
-
-        // The text continues on the next line only if the last text box is not
-        // on this line and none of the boxes on this line have a larger start
-        // offset.
-
-        bool continues_on_next_line = true;
-        InlineBox* other_box = box;
-        while (continues_on_next_line) {
-          other_box = other_box->NextLeafChild();
-          if (!other_box)
-            break;
-          if (other_box == last_text_box ||
-              (LineLayoutAPIShim::LayoutObjectFrom(
-                   other_box->GetLineLayoutItem()) == text_layout_object &&
-               ToInlineTextBox(other_box)->Start() >= text_offset))
-            continues_on_next_line = false;
-        }
-
-        other_box = box;
-        while (continues_on_next_line) {
-          other_box = other_box->PrevLeafChild();
-          if (!other_box)
-            break;
-          if (other_box == last_text_box ||
-              (LineLayoutAPIShim::LayoutObjectFrom(
-                   other_box->GetLineLayoutItem()) == text_layout_object &&
-               ToInlineTextBox(other_box)->Start() >= text_offset))
-            continues_on_next_line = false;
-        }
-
-        if (continues_on_next_line)
-          return current_pos.ComputePosition();
+      if (IsOffsetRenderedForForward(text_layout_object,
+                                     current_pos.OffsetInLeafNode())) {
+        return current_pos.ComputePosition();
       }
     }
   }
-
   return last_visible.DeprecatedComputePosition();
 }
 
+// Returns true if |offset| is rendered text of |layout_text_object|

[Xiaocheng, 2017/06/01 22:19:47]

The purpose of the function isn't clear.

An offset is a number, not text, so the proposition "|offset| is rendered text"
is ill-formed.

[yosin_UTC9, 2017/06/05 09:12:15]

Oops, sorry for confusion. |offset| should be |offset_in_node|.

As talked offline, I don't understand this part fully, I put TODO to say we need
to study more.

+static bool IsOffsetRenderedForForward(const LayoutText* text_layout_object,
+                                       int offset_in_node) {
+  const unsigned text_start_offset = text_layout_object->TextStartOffset();
+  DCHECK_GE(offset_in_node, static_cast<int>(text_start_offset));
+  const unsigned text_offset = offset_in_node - text_start_offset;
+  InlineTextBox* const last_text_box = text_layout_object->LastTextBox();
+  for (InlineTextBox* box = text_layout_object->FirstTextBox(); box;
+       box = box->NextTextBox()) {
+    if (text_offset <= box->end()) {
+      if (text_offset >= box->Start())
+        return true;

[Xiaocheng, 2017/06/01 22:19:48]

It returns true here if an adjacent character at either side of |offset_in_node|
is rendered.

[yosin_UTC9, 2017/06/05 09:12:15]

My interpretation is that if |text_offset| is in |box|, we return true.
For |return true| case, |box->Start() <= text_offset && text_offset <
box->end()|.

+      continue;
+    }
+
+    if (box == last_text_box || text_offset != box->Start() + box->Len())
+      continue;
+
+    // The text continues on the next line only if the last text box is not

[Xiaocheng, 2017/06/01 22:19:48]

nit: We can save one line here.

[yosin_UTC9, 2017/06/05 09:12:15]

Done.

+    // on this line and none of the boxes on this line have a larger start
+    // offset.
+
+    bool continues_on_next_line = true;
+    InlineBox* other_box = box;
+    while (continues_on_next_line) {
+      other_box = other_box->NextLeafChild();
+      if (!other_box)
+        break;
+      if (other_box == last_text_box ||
+          (LineLayoutAPIShim::LayoutObjectFrom(
+               other_box->GetLineLayoutItem()) == text_layout_object &&
+           ToInlineTextBox(other_box)->Start() >= text_offset))
+        continues_on_next_line = false;
+    }
+
+    other_box = box;
+    while (continues_on_next_line) {
+      other_box = other_box->PrevLeafChild();
+      if (!other_box)
+        break;
+      if (other_box == last_text_box ||
+          (LineLayoutAPIShim::LayoutObjectFrom(
+               other_box->GetLineLayoutItem()) == text_layout_object &&
+           ToInlineTextBox(other_box)->Start() >= text_offset))
+        continues_on_next_line = false;
+    }
+
+    if (continues_on_next_line)
+      return true;

[Xiaocheng, 2017/06/01 22:19:47]

I haven't understood in what situation |continues_on_next_line| remains true.

Do you have an idea?

[yosin_UTC9, 2017/06/05 09:12:15]

It seems most of case, |continues_on_next_line| remains true.
e.g. "abc$def" is a simple case, where "$" denotes line break, 

It seems |continues_on_next_line| is false, is rare.

When I change |c_on_next_line = false| to |c_on_next_line = true|,
following 5 test are failed:
  editing/selection/modify_extend/extend_by_character.html [ Failure ]
  editing/selection/modify_move/move-by-character-001.html [ Failure ]
  editing/selection/modify_move/move-by-character-003.html [ Failure ]
  editing/selection/modify_move/move-by-word-visually-multi-space.html [ Failure
]
  editing/selection/move-left-right.html [ Failure ]

One case is
assert_selection(
    '<div contenteditable>f    |oo   bar    baz</div>',
    selection => {
        for (var i = 0; i < 7; ++i)
            selection.modify('extend', 'forward', 'character');
    },
    '<div contenteditable>f    ^oo   bar |   baz</div>'),

+	 expected <div contenteditable>f    ^oo   bar |   baz</div>,
+	 but got  <div contenteditable>f    ^oo   bar|    baz</div>,
+	 sameupto <div contenteditable>f    ^oo   bar

When I set breakpoint to |c_on_next_line = false|, 32 test hits.

This is another example of |c_on_next_line = false|.
Text node: "abc $def"
|text_offset| is 3, a space before $.
|first_text_box| is "abc" (no trailing space)
|last_text_box| is "def".


This function was introduced on April 2008 by the patch[1].
Note: When I set breakpoint at |c_on_next_line| to false,
the test "move-past-trailing-space.html", added by the patch[1],
doesn't hit the breakpoint. :-<

[1] https://trac.webkit.org/changeset/32438/webkit

+  }
+  return false;
+}
+
 Position MostForwardCaretPosition(const Position& position,
                                   EditingBoundaryCrossingRule rule) {
   return MostForwardCaretPosition<EditingStrategy>(position, rule);
 }
 
 PositionInFlatTree MostForwardCaretPosition(const PositionInFlatTree& position,
                                             EditingBoundaryCrossingRule rule) {
   return MostForwardCaretPosition<EditingInFlatTreeStrategy>(position, rule);
 }
 
@@ skipping 866 matching lines @@
 
 IntRect ComputeTextRect(const EphemeralRangeInFlatTree& range) {
   return EnclosingIntRect(ComputeTextRectTemplate(range));
 }
 
 FloatRect ComputeTextFloatRect(const EphemeralRange& range) {
   return ComputeTextRectTemplate(range);
 }
 
 }  // namespace blink