ALL-IN-ONE: Refactor ComputeInlineBoxPosition()

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

Index: third_party/WebKit/Source/core/editing/VisibleUnits.cpp
diff --git a/third_party/WebKit/Source/core/editing/VisibleUnits.cpp b/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
index 8bf72e4bb23fdd0318d0ae852edb090b2a798f10..b9c70ccd23c3f09a9e39706d2d157ce810d3b37e 100644
--- a/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
+++ b/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
@@ -882,90 +882,127 @@ static bool IsStartOfDifferentDirection(const InlineBox* inline_box) {
   return prev_box->BidiLevel() > inline_box->BidiLevel();
 }
 
+// TODO(yosin): We have a forward declaration here to reduce patch size. We'll
+// replica this with an implementation once review finished.
+static InlineBoxPosition ComputeInlineBoxPositionInternal(LayoutObject*,
+                                                          InlineBox*,
+                                                          int,
+                                                          TextDirection);
+
+// TODO(yosin): We have a forward declaration here to reduce patch size. We'll
+// replica this with an implementation once review finished.
+static InlineBoxPosition ComputeInlineBoxPositionForTextNode(LayoutText*,
+                                                             int,
+                                                             TextAffinity,
+                                                             TextDirection);
+
 template <typename Strategy>
 static InlineBoxPosition ComputeInlineBoxPositionTemplate(
     const PositionTemplate<Strategy>& position,
     TextAffinity affinity,
     TextDirection primary_direction) {
-  InlineBox* inline_box = nullptr;
-  int caret_offset = position.ComputeEditingOffset();
+  const int caret_offset = position.ComputeEditingOffset();
   Node* const anchor_node = position.AnchorNode();
-  LayoutObject* layout_object =
+  LayoutObject* const layout_object =
       anchor_node->IsShadowRoot()
           ? ToShadowRoot(anchor_node)->host().GetLayoutObject()
           : anchor_node->GetLayoutObject();
 
   DCHECK(layout_object) << position;
 
-  if (!layout_object->IsText()) {
-    inline_box = 0;
-    if (CanHaveChildrenForEditing(anchor_node) &&
-        layout_object->IsLayoutBlockFlow() &&
-        HasRenderedNonAnonymousDescendantsWithHeight(layout_object)) {
-      // Try a visually equivalent position with possibly opposite
-      // editability. This helps in case |this| is in an editable block
-      // but surrounded by non-editable positions. It acts to negate the
-      // logic at the beginning of
-      // |LayoutObject::createPositionWithAffinity()|.
-      PositionTemplate<Strategy> equivalent =
-          DownstreamIgnoringEditingBoundaries(position);
-      if (equivalent == position) {
-        equivalent = UpstreamIgnoringEditingBoundaries(position);
-        if (equivalent == position ||
-            DownstreamIgnoringEditingBoundaries(equivalent) == position)
-          return InlineBoxPosition(inline_box, caret_offset);
-      }
-
-      return ComputeInlineBoxPosition(equivalent, TextAffinity::kUpstream,
-                                      primary_direction);
-    }
-    if (layout_object->IsBox()) {
-      inline_box = ToLayoutBox(layout_object)->InlineBoxWrapper();
-      if (!inline_box || (caret_offset > inline_box->CaretMinOffset() &&
-                          caret_offset < inline_box->CaretMaxOffset()))
-        return InlineBoxPosition(inline_box, caret_offset);
+  if (layout_object->IsText()) {
+    return ComputeInlineBoxPositionForTextNode(
+        ToLayoutText(layout_object), caret_offset, affinity, primary_direction);
+  }
+  if (CanHaveChildrenForEditing(anchor_node) &&
+      layout_object->IsLayoutBlockFlow() &&
+      HasRenderedNonAnonymousDescendantsWithHeight(layout_object)) {
+    // Try a visually equivalent position with possibly opposite
+    // editability. This helps in case |this| is in an editable block
+    // but surrounded by non-editable positions. It acts to negate the
+    // logic at the beginning of
+    // |LayoutObject::createPositionWithAffinity()|.
+    const PositionTemplate<Strategy>& downstream_equivalent =
+        DownstreamIgnoringEditingBoundaries(position);
+    if (downstream_equivalent != position) {
+      return ComputeInlineBoxPosition(
+          downstream_equivalent, TextAffinity::kUpstream, primary_direction);
     }
-  } else {
-    LayoutText* text_layout_object = ToLayoutText(layout_object);
-
-    InlineTextBox* candidate = nullptr;
-
-    for (InlineTextBox* box : InlineTextBoxesOf(*text_layout_object)) {
-      int caret_min_offset = box->CaretMinOffset();
-      int caret_max_offset = box->CaretMaxOffset();
+    const PositionTemplate<Strategy>& upstream_equivalent =
+        UpstreamIgnoringEditingBoundaries(position);
+    if (upstream_equivalent == position ||
+        DownstreamIgnoringEditingBoundaries(upstream_equivalent) == position)
+      return InlineBoxPosition();
+    return ComputeInlineBoxPosition(upstream_equivalent,
+                                    TextAffinity::kUpstream, primary_direction);
+  }
+  if (!layout_object->IsBox())
+    return InlineBoxPosition();
+  InlineBox* const box = ToLayoutBox(layout_object)->InlineBoxWrapper();
+  if (!box)
+    return InlineBoxPosition();
+  if (caret_offset > box->CaretMinOffset() &&
+      caret_offset < box->CaretMaxOffset()) {
+    return InlineBoxPosition(box, caret_offset);
+  }
+  return ComputeInlineBoxPositionInternal(layout_object, box, caret_offset,
+                                          primary_direction);
+}
 
-      if (caret_offset < caret_min_offset || caret_offset > caret_max_offset ||
-          (caret_offset == caret_max_offset && box->IsLineBreak()))
-        continue;
+static InlineBoxPosition ComputeInlineBoxPositionForTextNode(
+    LayoutText* text_layout_object,
+    int caret_offset,
+    TextAffinity affinity,
+    TextDirection primary_direction) {
+  InlineTextBox* inline_box = nullptr;
+  InlineTextBox* candidate = nullptr;
+  for (InlineTextBox* box : InlineTextBoxesOf(*text_layout_object)) {
+    const int caret_min_offset = box->CaretMinOffset();
+    const int caret_max_offset = box->CaretMaxOffset();
 
-      if (caret_offset > caret_min_offset && caret_offset < caret_max_offset)
-        return InlineBoxPosition(box, caret_offset);
+    if (caret_offset < caret_min_offset || caret_offset > caret_max_offset ||
+        (caret_offset == caret_max_offset && box->IsLineBreak()))
+      continue;
 
-      if (((caret_offset == caret_max_offset) ^
-           (affinity == TextAffinity::kDownstream)) ||
-          ((caret_offset == caret_min_offset) ^
-           (affinity == TextAffinity::kUpstream)) ||
-          (caret_offset == caret_max_offset && box->NextLeafChild() &&
-           box->NextLeafChild()->IsLineBreak())) {
-        inline_box = box;
-        break;
-      }
+    if (caret_offset > caret_min_offset && caret_offset < caret_max_offset)
+      return InlineBoxPosition(box, caret_offset);
 
-      candidate = box;
+    if (((caret_offset == caret_max_offset) ^
+         (affinity == TextAffinity::kDownstream)) ||
+        ((caret_offset == caret_min_offset) ^
+         (affinity == TextAffinity::kUpstream)) ||
+        (caret_offset == caret_max_offset && box->NextLeafChild() &&
+         box->NextLeafChild()->IsLineBreak())) {
+      inline_box = box;
+      break;
     }
-    if (candidate && candidate == text_layout_object->LastTextBox() &&
-        affinity == TextAffinity::kDownstream) {
-      inline_box = SearchAheadForBetterMatch(text_layout_object);

[Xiaocheng, 2017/06/08 21:34:12]

I don't understand this part.

It seems to be setting the value of |inline_box| repeatedly in the loop without
returning. Why it can be changed to an early return?

[yosin_UTC9, 2017/06/09 01:55:55]

Setting |inline_box| is one in the loop at L976. After setting |iline_box|, we
break loop.

This loop is can be represents as:

(is_early_return, inline_box, candidate) = this_loop(...);
if (is_eraly_return)
  return InlineBoxPosition(inline_box, caret_offset); // as L968
...

-      if (inline_box)
-        caret_offset = inline_box->CaretMinOffset();
+
+    candidate = box;
+  }
+  if (candidate && candidate == text_layout_object->LastTextBox() &&
+      affinity == TextAffinity::kDownstream) {
+    if (InlineBox* found = SearchAheadForBetterMatch(text_layout_object)) {
+      return ComputeInlineBoxPositionInternal(text_layout_object, found,
+                                              found->CaretMinOffset(),
+                                              primary_direction);
     }
-    if (!inline_box)
-      inline_box = candidate;
   }
+  if (inline_box) {
+    return ComputeInlineBoxPositionInternal(text_layout_object, inline_box,
+                                            caret_offset, primary_direction);
+  }
+  if (candidate) {
+    return ComputeInlineBoxPositionInternal(text_layout_object, candidate,
+                                            caret_offset, primary_direction);
+  }
+  return InlineBoxPosition();
+}
 
-  if (!inline_box)
-    return InlineBoxPosition(inline_box, caret_offset);
-
+static InlineBoxPosition ComputeInlineBoxPositionInternal(
+    LayoutObject* layout_object,
+    InlineBox* inline_box,
+    int caret_offset,
+    TextDirection primary_direction) {
   unsigned char level = inline_box->BidiLevel();
 
   if (inline_box->Direction() == primary_direction) {
@@ -1040,8 +1077,7 @@ static InlineBoxPosition ComputeInlineBoxPositionTemplate(
     return InlineBoxPosition(inline_box, caret_offset);
   }
 
-  if (layout_object &&
-      layout_object->Style()->GetUnicodeBidi() == UnicodeBidi::kPlaintext) {
+  if (layout_object->Style()->GetUnicodeBidi() == UnicodeBidi::kPlaintext) {
     if (inline_box->BidiLevel() < level)
       return InlineBoxPosition(inline_box, inline_box->CaretLeftmostOffset());
     return InlineBoxPosition(inline_box, inline_box->CaretRightmostOffset());
@@ -1128,12 +1164,14 @@ LayoutRect LocalCaretRectOfPositionTemplate(
   InlineBoxPosition box_position =
       ComputeInlineBoxPosition(position.GetPosition(), position.Affinity());
 
-  if (box_position.inline_box)
-    layout_object = LineLayoutAPIShim::LayoutObjectFrom(
-        box_position.inline_box->GetLineLayoutItem());
+  if (box_position.inline_box) {
+    return LineLayoutAPIShim::LayoutObjectFrom(
+               box_position.inline_box->GetLineLayoutItem())
+        ->LocalCaretRect(box_position.inline_box, box_position.offset_in_box);
+  }
 
-  return layout_object->LocalCaretRect(box_position.inline_box,
-                                       box_position.offset_in_box);
+  return layout_object->LocalCaretRect(

[yosin_UTC9, 2017/06/08 10:19:24]

Since ComputeInlineBoxPosition() returns offset zero when there are no inline
box for
position, we should compute offset from position.

+      nullptr, position.GetPosition().ComputeEditingOffset());
 }
 
 // This function was added because the caret rect that is calculated by