Move ComputeTextRects before Range::BoundingBox()

third_party/WebKit/Source/core/dom/Range.cpp

Index: third_party/WebKit/Source/core/dom/Range.cpp
diff --git a/third_party/WebKit/Source/core/dom/Range.cpp b/third_party/WebKit/Source/core/dom/Range.cpp
index a34467ca556c983820440502773e6e3a72ab2688..36ee4f777c73da73b9a03c35d887d54cdc37927d 100644
--- a/third_party/WebKit/Source/core/dom/Range.cpp
+++ b/third_party/WebKit/Source/core/dom/Range.cpp
@@ -1476,27 +1476,38 @@ static Vector<IntRect> computeTextRects(const EphemeralRange& range) {
   return rects;
 }
 
+// TODO(tanvir.rizvi): We should move this implementation to VisibleUnits and
+// then this function will be removed.
+static Vector<FloatQuad> ComputeTextQuads(const EphemeralRange&, bool);
+
 void Range::TextQuads(Vector<FloatQuad>& quads,
                       bool use_selection_height) const {
-  Node* start_container = &start_.Container();
+  quads = ComputeTextQuads(EphemeralRange(this), use_selection_height);

[Xiaocheng, 2017/04/19 07:54:56]

Could you verify if this is copy assignment or move assignment?

We don't want copy assignment.

[tanvir, 2017/04/19 14:05:59]

As discussed in https://codereview.chromium.org/2775663008/ #44, it uses copy
assignment. Updated. Thanks for pointing it out. 

+}
+
+static Vector<FloatQuad> ComputeTextQuads(const EphemeralRange& range,

[Xiaocheng, 2017/04/19 07:54:56]

Do we want to templatize this function for flat tree?

Anyway, even if we do, it shouldn't be done in this patch.

[tanvir, 2017/04/19 14:05:59]

Yes , i will template the function in the further patches for this Bug.

+                                          bool use_selection_height) {
+  const Position& start_position = range.StartPosition();
+  const Position& end_position = range.EndPosition();
+  Node* start_container = start_position.ComputeContainerNode();
   DCHECK(start_container);
-  Node* end_container = &end_.Container();
+  Node* end_container = end_position.ComputeContainerNode();
   DCHECK(end_container);
 
-  Node* stop_node = PastLastNode();
-  for (Node* node = FirstNode(); node != stop_node;
-       node = NodeTraversal::Next(*node)) {
-    LayoutObject* r = node->GetLayoutObject();
-    if (!r || !r->IsText())
+  Vector<FloatQuad> quads;
+  for (Node& node : range.Nodes()) {
+    LayoutObject* const layout_object = node.GetLayoutObject();
+    if (!layout_object || !layout_object->IsText())
       continue;
-    LayoutText* layout_text = ToLayoutText(r);
-    unsigned start_offset = node == start_container ? start_.Offset() : 0;
+    LayoutText* layout_text = ToLayoutText(layout_object);
+    unsigned start_offset =
+        node == start_container ? start_position.OffsetInContainerNode() : 0;

[Xiaocheng, 2017/04/19 07:54:56]

|OffsetInContainerNode()| only works for OffsetInAnchor positions. So we should:

1) Either state and DCHECK that |start_position| and |end_position| are
OffsetInAnchor;

2) Or call |ComputeOffsetInContainerNode()| instead, at the cost of worse
performance (*).

I prefer 1) since the only call site is from Range::TextQuads(), which always
generated OffsetInAnchor positions. And there is no way to introduce new
behavior for a pure-refactoring patch like this.

(*) |ComputeOffsetInContainerNode()| claims O(1) running time for
parent-anchored positions, which is wrong -- it's still O(n) for OffsetInAnchor
if you check the implementation.

[tanvir, 2017/04/19 14:05:59]

Followed Approach 1. Thanks for explanation.

     unsigned end_offset = node == end_container
-                              ? end_.Offset()
+                              ? end_position.OffsetInContainerNode()
                               : std::numeric_limits<unsigned>::max();
-    layout_text->AbsoluteQuadsForRange(quads, start_offset, end_offset,
-                                       use_selection_height);
+    layout_text->AbsoluteQuadsForRange(quads, start_offset, end_offset);

[Xiaocheng, 2017/04/19 07:54:56]

|use_selection_height| shouldn't be dropped. 

It's risky to do refactoring and optimization in the same patch. This patch
should focus on refactoring |TextQuads()|. The removal of this redundant
parameter should be done in a dedicated patch.

[tanvir, 2017/04/19 14:05:59]

Done.

   }
+  return quads;
 }
 
 bool AreRangesEqual(const Range* a, const Range* b) {