Introduce LocalCaretRect for refactoring Local{Caret,Selection}RectPosition()

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 9c1e4f97f47ba8c88cc1897b6d94bf203f0e6628..21814ab948bda1681641ed562a1e24d033ba0190 100644
--- a/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
+++ b/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
@@ -1263,90 +1263,89 @@ InlineBoxPosition ComputeInlineBoxPosition(const PositionInFlatTree& position,
       position, affinity, primary_direction);
 }
 
-template <typename Strategy>
-LayoutRect LocalCaretRectOfPositionTemplate(
-    const PositionWithAffinityTemplate<Strategy>& position,
-    LayoutObject*& layout_object) {
-  if (position.IsNull()) {
-    layout_object = nullptr;
-    return LayoutRect();
-  }
-  Node* node = position.AnchorNode();
+// TODO(editing-dev): Once we mark |LayoutObject::LocalCaretRect()| |const|,
+// we should make this function to take |const LayoutObject&|.
+static LocalCaretRect ComputeLocalCaretRect(
+    LayoutObject* layout_object,
+    const InlineBoxPosition box_position) {
+  return LocalCaretRect(
+      layout_object, layout_object->LocalCaretRect(box_position.inline_box,
+                                                   box_position.offset_in_box));
+}
 
-  layout_object = node->GetLayoutObject();
-  if (!layout_object)
-    return LayoutRect();
+template <typename Strategy>
+LocalCaretRect LocalCaretRectOfPositionTemplate(
+    const PositionWithAffinityTemplate<Strategy>& position) {
+  if (position.IsNull())
+    return LocalCaretRect();
+  Node* const node = position.AnchorNode();
+  if (!node->GetLayoutObject())

[Xiaocheng, 2017/06/08 21:00:42]

We should store

LayoutObject* const layout_object = node->GetLayoutObject();

here to avoid repeated calls of Node::GetLayoutObject().

[yosin_UTC9, 2017/06/09 06:13:29]

Done.

+    return LocalCaretRect();
 
-  InlineBoxPosition box_position =
+  const 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 ComputeLocalCaretRect(node->GetLayoutObject(), box_position);

[Xiaocheng, 2017/06/08 21:00:41]

We can use early return style without switching the if and else branches.

Let's do that to be git log friendly.

[yosin_UTC9, 2017/06/09 06:13:29]

Done.

 
-  return layout_object->LocalCaretRect(box_position.inline_box,
-                                       box_position.offset_in_box);
+  return ComputeLocalCaretRect(
+      LineLayoutAPIShim::LayoutObjectFrom(
+          box_position.inline_box->GetLineLayoutItem()),
+      box_position);
 }
 
 // This function was added because the caret rect that is calculated by
 // using the line top value instead of the selection top.
 template <typename Strategy>
-LayoutRect LocalSelectionRectOfPositionTemplate(
-    const PositionWithAffinityTemplate<Strategy>& position,
-    LayoutObject*& layout_object) {
-  if (position.IsNull()) {
-    layout_object = nullptr;
-    return LayoutRect();
-  }
-  Node* node = position.AnchorNode();
-  layout_object = node->GetLayoutObject();
-  if (!layout_object)
-    return LayoutRect();
+LocalCaretRect LocalSelectionRectOfPositionTemplate(
+    const PositionWithAffinityTemplate<Strategy>& position) {
+  if (position.IsNull())
+    return LocalCaretRect();
+  Node* const node = position.AnchorNode();
+  if (!node->GetLayoutObject())
+    return LocalCaretRect();
 
-  InlineBoxPosition box_position =
+  const InlineBoxPosition& box_position =
       ComputeInlineBoxPosition(position.GetPosition(), position.Affinity());
 
   if (!box_position.inline_box)
-    return LayoutRect();
+    return LocalCaretRect();

[Xiaocheng, 2017/06/08 21:00:42]

The original code returns a non-null LayoutObject.

Let's not change that.

[yosin_UTC9, 2017/06/09 06:13:29]

I create another patch to return nullptr for layout object.
http://crrev.com/2925323004

 
-  layout_object = LineLayoutAPIShim::LayoutObjectFrom(
+  LayoutObject* const layout_object = LineLayoutAPIShim::LayoutObjectFrom(
       box_position.inline_box->GetLineLayoutItem());
 
-  LayoutRect rect = layout_object->LocalCaretRect(box_position.inline_box,
-                                                  box_position.offset_in_box);
+  const LayoutRect& rect = layout_object->LocalCaretRect(
+      box_position.inline_box, box_position.offset_in_box);
 
   if (rect.IsEmpty())
-    return rect;
+    return LocalCaretRect();

[Xiaocheng, 2017/06/08 21:00:42]

Ditto.

[yosin_UTC9, 2017/06/09 06:13:29]

I create another patch to return nullptr for layout object.
http://crrev.com/2925323004

 
   InlineBox* const box = box_position.inline_box;
   if (layout_object->Style()->IsHorizontalWritingMode()) {
-    rect.SetY(box->Root().SelectionTop());
-    rect.SetHeight(box->Root().SelectionHeight());
-    return rect;
+    return LocalCaretRect(
+        layout_object,
+        LayoutRect(LayoutPoint(rect.X(), box->Root().SelectionTop()),
+                   LayoutSize(rect.Width(), box->Root().SelectionHeight())));
   }
 
-  rect.SetX(box->Root().SelectionTop());
-  rect.SetWidth(box->Root().SelectionHeight());
-  return rect;
+  return LocalCaretRect(
+      layout_object,
+      LayoutRect(LayoutPoint(box->Root().SelectionTop(), rect.Y()),
+                 LayoutSize(box->Root().SelectionHeight(), rect.Height())));
 }
 
-LayoutRect LocalCaretRectOfPosition(const PositionWithAffinity& position,
-                                    LayoutObject*& layout_object) {
-  return LocalCaretRectOfPositionTemplate<EditingStrategy>(position,
-                                                           layout_object);
+LocalCaretRect LocalCaretRectOfPosition(const PositionWithAffinity& position) {
+  return LocalCaretRectOfPositionTemplate<EditingStrategy>(position);
 }
 
-LayoutRect LocalSelectionRectOfPosition(const PositionWithAffinity& position,
-                                        LayoutObject*& layout_object) {
-  return LocalSelectionRectOfPositionTemplate<EditingStrategy>(position,
-                                                               layout_object);
+static LocalCaretRect LocalSelectionRectOfPosition(
+    const PositionWithAffinity& position) {
+  return LocalSelectionRectOfPositionTemplate<EditingStrategy>(position);
 }
 
-LayoutRect LocalCaretRectOfPosition(
-    const PositionInFlatTreeWithAffinity& position,
-    LayoutObject*& layout_object) {
-  return LocalCaretRectOfPositionTemplate<EditingInFlatTreeStrategy>(
-      position, layout_object);
+LocalCaretRect LocalCaretRectOfPosition(
+    const PositionInFlatTreeWithAffinity& position) {
+  return LocalCaretRectOfPositionTemplate<EditingInFlatTreeStrategy>(position);
 }
 
 static LayoutUnit BoundingBoxLogicalHeight(LayoutObject* o,
@@ -1487,20 +1486,21 @@ static bool InRenderedText(const PositionTemplate<Strategy>& position) {
   return false;
 }
 
+FloatQuad LocalCaretRect::LocalToAbsoluteQuad() const {
+  return layout_object->LocalToAbsoluteQuad(FloatRect(rect));
+}
+
 bool RendersInDifferentPosition(const Position& position1,
                                 const Position& position2) {
   if (position1.IsNull() || position2.IsNull())
     return false;
-  LayoutObject* layout_object1;
-  const LayoutRect& rect1 =
-      LocalCaretRectOfPosition(PositionWithAffinity(position1), layout_object1);
-  LayoutObject* layout_object2;
-  const LayoutRect& rect2 =
-      LocalCaretRectOfPosition(PositionWithAffinity(position2), layout_object2);
-  if (!layout_object1 || !layout_object2)
-    return layout_object1 != layout_object2;
-  return layout_object1->LocalToAbsoluteQuad(FloatRect(rect1)) !=
-         layout_object2->LocalToAbsoluteQuad(FloatRect(rect2));
+  const LocalCaretRect& caret_rect1 =
+      LocalCaretRectOfPosition(PositionWithAffinity(position1));
+  const LocalCaretRect& caret_rect2 =
+      LocalCaretRectOfPosition(PositionWithAffinity(position2));
+  if (!caret_rect1.layout_object || !caret_rect2.layout_object)
+    return caret_rect1.layout_object != caret_rect2.layout_object;
+  return caret_rect1.LocalToAbsoluteQuad() != caret_rect2.LocalToAbsoluteQuad();
 }
 
 static bool IsVisuallyEmpty(const LayoutObject* layout) {
@@ -2014,14 +2014,11 @@ template <typename Strategy>
 static IntRect AbsoluteCaretBoundsOfAlgorithm(
     const VisiblePositionTemplate<Strategy>& visible_position) {
   DCHECK(visible_position.IsValid()) << visible_position;
-  LayoutObject* layout_object;
-  LayoutRect local_rect = LocalCaretRectOfPosition(
-      visible_position.ToPositionWithAffinity(), layout_object);
-  if (local_rect.IsEmpty() || !layout_object)
+  const LocalCaretRect& caret_rect =
+      LocalCaretRectOfPosition(visible_position.ToPositionWithAffinity());
+  if (caret_rect.IsEmpty())
     return IntRect();
-
-  return layout_object->LocalToAbsoluteQuad(FloatRect(local_rect))
-      .EnclosingBoundingBox();
+  return caret_rect.LocalToAbsoluteQuad().EnclosingBoundingBox();
 }
 
 IntRect AbsoluteCaretBoundsOf(const VisiblePosition& visible_position) {
@@ -2032,14 +2029,11 @@ template <typename Strategy>
 static IntRect AbsoluteSelectionBoundsOfAlgorithm(
     const VisiblePositionTemplate<Strategy>& visible_position) {
   DCHECK(visible_position.IsValid()) << visible_position;
-  LayoutObject* layout_object;
-  LayoutRect local_rect = LocalSelectionRectOfPosition(
-      visible_position.ToPositionWithAffinity(), layout_object);
-  if (local_rect.IsEmpty() || !layout_object)
+  const LocalCaretRect& caret_rect =
+      LocalSelectionRectOfPosition(visible_position.ToPositionWithAffinity());
+  if (caret_rect.IsEmpty())
     return IntRect();
-
-  return layout_object->LocalToAbsoluteQuad(FloatRect(local_rect))
-      .EnclosingBoundingBox();
+  return caret_rect.LocalToAbsoluteQuad().EnclosingBoundingBox();
 }
 
 IntRect AbsoluteSelectionBoundsOf(const VisiblePosition& visible_position) {