Move ComputeTextRects before Range::BoundingBox()

Move ComputeTextRects before Range::BoundingBox()
Currently textQuads and ComputeTextRects algorithm is
mostly same. The difference of behaviour of these two functions
can be handled efficiently by using templates.
    
This patch is the first step to achieve above behaviour.
This patch removes the TODO comment and forward declaration of
computeTextRects.
    
BUG=691198
Review-Url: https://codereview.chromium.org/2827603006
Cr-Commit-Position: refs/heads/master@{#466607}
Committed: https://chromium.googlesource.com/chromium/src/+/0c2398f2a9fe6a2432bf86861da2e31480b2cc8d

[tanvir, 2017-04-19 05:36:18]

The CQ bit was checked by tanvir.rizvi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 05:36:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 05:36:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 05:36:35]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[tanvir, 2017-04-19 05:37:33]

Description was changed from

==========
In-place change of Range::textQuads() to use EphemeralRange.

In-place change of Range::textQuads() to use EphemeralRange.
This patch is first step to use EphemeralRange
when calculating textQuads from LayoutObject.

BUG=691198
==========

to

==========
In-place change of Range::textQuads() to use EphemeralRange.

In-place change of Range::textQuads() to use EphemeralRange.
This patch is first step to use EphemeralRange
when calculating textQuads from LayoutObject.

BUG=691198
==========

[tanvir, 2017-04-19 05:37:33]

tanvir.rizvi@samsung.com changed reviewers:
+ shanmuga.m@samsung.com, srirama.m@samsung.com

[Srirama, 2017-04-19 05:39:21]

The CQ bit was checked by srirama.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 05:39:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tanvir, 2017-04-19 06:01:24]

tanvir.rizvi@samsung.com changed reviewers:
+ xiaochengh@chromium.org, yosin@chromium.org

[Srirama, 2017-04-19 06:43:41]

https://codereview.chromium.org/2827603006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2827603006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1502: LayoutText* layoutText =
ToLayoutText(layoutObject);
Align with new coding guidelines.
s/layoutText/layout_text

https://codereview.chromium.org/2827603006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1503: unsigned startOffset =
s/startOffset/start_offset and everywhere else

[tanvir, 2017-04-19 07:23:09]

https://codereview.chromium.org/2827603006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2827603006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1502: LayoutText* layoutText =
ToLayoutText(layoutObject);
On 2017/04/19 06:43:41, Srirama wrote:
> Align with new coding guidelines.
> s/layoutText/layout_text

Done.

https://codereview.chromium.org/2827603006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1503: unsigned startOffset =
On 2017/04/19 06:43:41, Srirama wrote:
> s/startOffset/start_offset and everywhere else

Done.

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

https://codereview.chromium.org/2827603006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2827603006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1485: quads =
ComputeTextQuads(EphemeralRange(this), use_selection_height);
Could you verify if this is copy assignment or move assignment?

We don't want copy assignment.

https://codereview.chromium.org/2827603006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1488: static Vector<FloatQuad>
ComputeTextQuads(const EphemeralRange& range,
Do we want to templatize this function for flat tree?

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

https://codereview.chromium.org/2827603006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1504: node == start_container ?
start_position.OffsetInContainerNode() : 0;
|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.

https://codereview.chromium.org/2827603006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1508:
layout_text->AbsoluteQuadsForRange(quads, start_offset, end_offset);
|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:06:00]

Review comments changes done. PTAL!!!

https://codereview.chromium.org/2827603006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2827603006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1485: quads =
ComputeTextQuads(EphemeralRange(this), use_selection_height);
On 2017/04/19 07:54:56, Xiaocheng wrote:
> Could you verify if this is copy assignment or move assignment?
> 
> We don't want copy assignment.

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

https://codereview.chromium.org/2827603006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1488: static Vector<FloatQuad>
ComputeTextQuads(const EphemeralRange& range,
On 2017/04/19 07:54:56, Xiaocheng wrote:
> Do we want to templatize this function for flat tree?
> 
> Anyway, even if we do, it shouldn't be done in this patch.

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

https://codereview.chromium.org/2827603006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1504: node == start_container ?
start_position.OffsetInContainerNode() : 0;
On 2017/04/19 07:54:56, Xiaocheng wrote:
> |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.

Followed Approach 1. Thanks for explanation.

https://codereview.chromium.org/2827603006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1508:
layout_text->AbsoluteQuadsForRange(quads, start_offset, end_offset);
On 2017/04/19 07:54:56, Xiaocheng wrote:
> |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.

Done.

[Xiaocheng, 2017-04-20 00:21:52]

https://codereview.chromium.org/2827603006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2827603006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1495:
DCHECK(start_position.IsOffsetInAnchor() || start_container);
s/||/&&/

https://codereview.chromium.org/2827603006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1497:
DCHECK(end_position.IsOffsetInAnchor() || end_container);
s/||/&&/

[yosin_UTC9, 2017-04-20 01:21:02]

https://codereview.chromium.org/2827603006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2827603006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1495:
DCHECK(start_position.IsOffsetInAnchor() || start_container);
On 2017/04/20 at 00:21:52, Xiaocheng wrote:
> s/||/&&/

It is better to have two DCHECK() rather than one combined DCHECK().
When we have two DCHECK's, assertion message tells us which what assumption
broken.

BTW, |computeTextRects()| doesn't have
|DCHECK(start_position.IsOffsetInAnchor())|,
we may want to add |IsOffsetInAnchor()| in |computeTextRects()| too.

Note: |OffsetInContainerNode()| implies |IsOffsetInAnchor()|

https://codereview.chromium.org/2827603006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1497:
DCHECK(end_position.IsOffsetInAnchor() || end_container);
On 2017/04/20 at 00:21:52, Xiaocheng wrote:
> s/||/&&/

On 2017/04/20 at 00:21:52, Xiaocheng wrote:
> s/||/&&/

It is better to have two DCHECK() rather than one combined DCHECK().
When we have two DCHECK's, assertion message tells us which what assumption
broken.

[tanvir, 2017-04-20 05:39:17]

https://codereview.chromium.org/2827603006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2827603006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1495:
DCHECK(start_position.IsOffsetInAnchor() || start_container);
On 2017/04/20 01:21:01, yosin_UTC9 wrote:
> On 2017/04/20 at 00:21:52, Xiaocheng wrote:
> > s/||/&&/
> 
> It is better to have two DCHECK() rather than one combined DCHECK().
> When we have two DCHECK's, assertion message tells us which what assumption
> broken.
> 
> BTW, |computeTextRects()| doesn't have
> |DCHECK(start_position.IsOffsetInAnchor())|,
> we may want to add |IsOffsetInAnchor()| in |computeTextRects()| too.
> 
> Note: |OffsetInContainerNode()| implies |IsOffsetInAnchor()|

Done.

https://codereview.chromium.org/2827603006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1495:
DCHECK(start_position.IsOffsetInAnchor() || start_container);
On 2017/04/20 01:21:01, yosin_UTC9 wrote:
> On 2017/04/20 at 00:21:52, Xiaocheng wrote:
> > s/||/&&/
> 
> It is better to have two DCHECK() rather than one combined DCHECK().
> When we have two DCHECK's, assertion message tells us which what assumption
> broken.
> 
> BTW, |computeTextRects()| doesn't have
> |DCHECK(start_position.IsOffsetInAnchor())|,
> we may want to add |IsOffsetInAnchor()| in |computeTextRects()| too.
> 
> Note: |OffsetInContainerNode()| implies |IsOffsetInAnchor()|

Done.

https://codereview.chromium.org/2827603006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1495:
DCHECK(start_position.IsOffsetInAnchor() || start_container);
On 2017/04/20 01:21:01, yosin_UTC9 wrote:
> On 2017/04/20 at 00:21:52, Xiaocheng wrote:
> > s/||/&&/
> 
> It is better to have two DCHECK() rather than one combined DCHECK().
> When we have two DCHECK's, assertion message tells us which what assumption
> broken.
> 
> BTW, |computeTextRects()| doesn't have
> |DCHECK(start_position.IsOffsetInAnchor())|,
> we may want to add |IsOffsetInAnchor()| in |computeTextRects()| too.
> 
> Note: |OffsetInContainerNode()| implies |IsOffsetInAnchor()|

I will create a patch to add DCHECK for computeTextRects in future.

[Shanmuga Pandi, 2017-04-20 05:56:57]

The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 05:57:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-04-20 06:01:59]

lgtm.

Thanks!

[yosin_UTC9, 2017-04-20 06:59:54]

https://codereview.chromium.org/2827603006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2827603006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1488: static void
ComputeTextQuads(Vector<FloatQuad>* quads,
Could you returnVector<FloatQuad> instead of void + |Vector<FloatQuad>*| as same
as |computeTextRects()|?
See also: http://crrev.com/2810313003: In-place change of Range::testRects() and
boundingBox() to EphemeralRange.
Note: we don't need to pass |use_selection_height|

It seems |computeTextQuads()| is as same as |computeTextRects()| except for
returning |Vector<FloatRect>| or |Vector<IntRect>|.
Is it better to use template to share algorithm?

static void CollectAbsoluteBoundsForRange(Vector<IntRect> acc, ...) {
layout_text->AbsoulteRectsForRange(acc, ...); }
static void CollectAbsoluteBoundsForRange(Vector<FloatRect> acc, ...) {
layout_text->AbsoulteQuadsForRange(acc, ...); }

template <typename ElementType>
static computeTextBounds(...) {
   ...
   Vector<ElementType> result;
   for (...) {
     ...
     CollectAbosluteBoundsForRange(result, ...);
   }
}

IntRect Range::BoundingBox() const {
   const Vector<IntRect>& rects =
ComputeTextBounds<IntRect>(EphemeralRange(this));
  for (const IntRect& rect : rects)
    result.Unite(rect); 
   return results;
}

void Range::TextQuads(Vector<FloatQuad>& quads) const {
 const Vector<FloatQuad>& result =
ComputeTextBounds<FloatQuad>(EpehemralRange(this), ...);
 quads.insert(quads.end(), result.begin(), result().end());
}

Note: Since all call sites, there are two, one is RageTest.cpp and another is
LayoutObject::AbsoluteBoundingBoxRectForRange()
call TextQuad() with empty vector, we don't need to use
std::vector<T>::insert().

So, we'll change them to take result by function return value instead of output
parameter.

[tanvir, 2017-04-20 07:27:08]

On 2017/04/20 06:59:54, yosin_UTC9 wrote:
>
https://codereview.chromium.org/2827603006/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Range.cpp (right):
> 
>
https://codereview.chromium.org/2827603006/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Range.cpp:1488: static void
> ComputeTextQuads(Vector<FloatQuad>* quads,
> Could you returnVector<FloatQuad> instead of void + |Vector<FloatQuad>*| as
same
> as |computeTextRects()|?
> See also: http://crrev.com/2810313003: In-place change of Range::testRects()
and
> boundingBox() to EphemeralRange.

As per review comments#13, we don't want the copy assignment,so this approach
was taken.
Please let me know your opinion.

> Note: we don't need to pass |use_selection_height|
>
I will make this independent in a different patch. This was also discussed in
C#13.  
 
> It seems |computeTextQuads()| is as same as |computeTextRects()| except for
> returning |Vector<FloatRect>| or |Vector<IntRect>|.
> Is it better to use template to share algorithm?
> 
> static void CollectAbsoluteBoundsForRange(Vector<IntRect> acc, ...) {
> layout_text->AbsoulteRectsForRange(acc, ...); }
> static void CollectAbsoluteBoundsForRange(Vector<FloatRect> acc, ...) {
> layout_text->AbsoulteQuadsForRange(acc, ...); }
> 
> template <typename ElementType>
> static computeTextBounds(...) {
>    ...
>    Vector<ElementType> result;
>    for (...) {
>      ...
>      CollectAbosluteBoundsForRange(result, ...);
>    }
> }
> 
> IntRect Range::BoundingBox() const {
>    const Vector<IntRect>& rects =
> ComputeTextBounds<IntRect>(EphemeralRange(this));
>   for (const IntRect& rect : rects)
>     result.Unite(rect); 
>    return results;
> }
> 
> void Range::TextQuads(Vector<FloatQuad>& quads) const {
>  const Vector<FloatQuad>& result =
> ComputeTextBounds<FloatQuad>(EpehemralRange(this), ...);
>  quads.insert(quads.end(), result.begin(), result().end());
> }
> 
> Note: Since all call sites, there are two, one is RageTest.cpp and another is
> LayoutObject::AbsoluteBoundingBoxRectForRange()
> call TextQuad() with empty vector, we don't need to use
> std::vector<T>::insert().
> 
> So, we'll change them to take result by function return value instead of
output
> parameter.

Shall i do this refactoring in a separate patch after the changes for this Bug
is done, to avoid any confusion or risk.

I was thinking to do in this way.

PatchSet #1 Current patch, make Range::textQuads to use EphemralRange. 
PatchSet #2 Make LayoutObject to take EphemeralRange.
PatchSet #3 Use template for textQuads and textRects.
PatchSet #4 Move template functionality to VisibleUnits.
PatchSet #5 Avoid Passing of use_selection_height  

Please let me know your opinion.

[commit-bot: I haz the power, 2017-04-20 07:48:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 07:48:27]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-04-20 08:39:40]

On 2017/04/20 at 07:27:08, tanvir.rizvi wrote:
> On 2017/04/20 06:59:54, yosin_UTC9 wrote:
> >
https://codereview.chromium.org/2827603006/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/Range.cpp (right):
> > 
> >
https://codereview.chromium.org/2827603006/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/Range.cpp:1488: static void
> > ComputeTextQuads(Vector<FloatQuad>* quads,
> > Could you returnVector<FloatQuad> instead of void + |Vector<FloatQuad>*| as
same
> > as |computeTextRects()|?
> > See also: http://crrev.com/2810313003: In-place change of Range::testRects()
and
> > boundingBox() to EphemeralRange.
> 
> As per review comments#13, we don't want the copy assignment,so this approach
was taken.

When we write below, there is no copy.
  const Vector<FlaotRect>& rects = range->TextQuads();

> Please let me know your opinion.
> 
> > Note: we don't need to pass |use_selection_height|
> >
> I will make this independent in a different patch. This was also discussed in
C#13.  
>  
> > It seems |computeTextQuads()| is as same as |computeTextRects()| except for
> > returning |Vector<FloatRect>| or |Vector<IntRect>|.
> > Is it better to use template to share algorithm?
> > 
> > static void CollectAbsoluteBoundsForRange(Vector<IntRect> acc, ...) {
> > layout_text->AbsoulteRectsForRange(acc, ...); }
> > static void CollectAbsoluteBoundsForRange(Vector<FloatRect> acc, ...) {
> > layout_text->AbsoulteQuadsForRange(acc, ...); }
> > 
> > template <typename ElementType>
> > static computeTextBounds(...) {
> >    ...
> >    Vector<ElementType> result;
> >    for (...) {
> >      ...
> >      CollectAbosluteBoundsForRange(result, ...);
> >    }
> > }
> > 
> > IntRect Range::BoundingBox() const {
> >    const Vector<IntRect>& rects =
> > ComputeTextBounds<IntRect>(EphemeralRange(this));
> >   for (const IntRect& rect : rects)
> >     result.Unite(rect); 
> >    return results;
> > }
> > 
> > void Range::TextQuads(Vector<FloatQuad>& quads) const {
> >  const Vector<FloatQuad>& result =
> > ComputeTextBounds<FloatQuad>(EpehemralRange(this), ...);
> >  quads.insert(quads.end(), result.begin(), result().end());
> > }
> > 
> > Note: Since all call sites, there are two, one is RageTest.cpp and another
is
> > LayoutObject::AbsoluteBoundingBoxRectForRange()
> > call TextQuad() with empty vector, we don't need to use
> > std::vector<T>::insert().
> > 
> > So, we'll change them to take result by function return value instead of
output
> > parameter.
> 
> Shall i do this refactoring in a separate patch after the changes for this Bug
is done, to avoid any confusion or risk.
> 
> I was thinking to do in this way.
> 
> PatchSet #1 Current patch, make Range::textQuads to use EphemralRange. 
> PatchSet #2 Make LayoutObject to take EphemeralRange.
 Before PatchSet #2, we need to have ComputeTextQuads() in "VisibleUnits.h".
 We also wants to change a call site in RangeTest.cpp.

> PatchSet #3 Use template for textQuads and textRects.
Since three is no call sites of |Range::textQuads()|, we remove
|Range::textQuads()| here.

> PatchSet #4 Move template functionality to VisibleUnits.
> PatchSet #5 Avoid Passing of use_selection_height
Note: No call sites pass |use_selection_height|.


Current patch is copy of |computeTextRects()| with renaming of, 
 - computeTextRects => computeTextQuads
 - AbosluteRectsForRange => AbsoulteQuadsForRange
This is we've done for Range::textRects() in patch[1].

If we do re-factoring, we don't want to make copy of code.
Also, patch[1] enables templaizing via |computeTextRects()|.

We've already have refactored version of computeTextRects(), it is better to
reuse it.

1. Move ComputeTextRects before Range::BoundingBox()
2. Introduce ComputeTextBounds<T> (in-place change of ComputeTextTexts) and
change ComputeTextRects() to CTR() { retturn ComputeTextBounds<IntRect>(...); }
3. Introduce ComputeTextQuads(...) { return ComputeTextBounds<FloatRect>(...) }
and change Range::TextQuads() to use ComputeTextQudas()
4. Move ComputeTextRects() and ComputeTextQuads() with ComputeTextBounds<T> to
VisibleUnit.cpp
5. Change call sites to use ComputeTextQuads()
6. Get rid of Range::TextQuads()

Since all call sites of Range::TextQuads() don't pass |use_selection_height|,
removing |use_selection_height| can be done in Step#3.

PROS of this proposal is
 - No copy & paste (w/ modify) code.
 - Getting rid of Range::TextQuads()
 - Change of Range::TextQuads() once.

[1] http://crrev.com/2810313003 In-place change of Range::textRects() and
boundingBox() to EphemeralRange

[tanvir, 2017-04-20 14:27:42]

On 2017/04/20 08:39:40, yosin_UTC9 wrote:
> On 2017/04/20 at 07:27:08, tanvir.rizvi wrote:
> > On 2017/04/20 06:59:54, yosin_UTC9 wrote:
> > >
>
https://codereview.chromium.org/2827603006/diff/80001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/dom/Range.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/2827603006/diff/80001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/dom/Range.cpp:1488: static void
> > > ComputeTextQuads(Vector<FloatQuad>* quads,
> > > Could you returnVector<FloatQuad> instead of void + |Vector<FloatQuad>*|
as
> same
> > > as |computeTextRects()|?
> > > See also: http://crrev.com/2810313003: In-place change of
Range::testRects()
> and
> > > boundingBox() to EphemeralRange.
> > 
> > As per review comments#13, we don't want the copy assignment,so this
approach
> was taken.
> 
> When we write below, there is no copy.
>   const Vector<FlaotRect>& rects = range->TextQuads();
> 
> > Please let me know your opinion.
> > 
> > > Note: we don't need to pass |use_selection_height|
> > >
> > I will make this independent in a different patch. This was also discussed
in
> C#13.  
> >  
> > > It seems |computeTextQuads()| is as same as |computeTextRects()| except
for
> > > returning |Vector<FloatRect>| or |Vector<IntRect>|.
> > > Is it better to use template to share algorithm?
> > > 
> > > static void CollectAbsoluteBoundsForRange(Vector<IntRect> acc, ...) {
> > > layout_text->AbsoulteRectsForRange(acc, ...); }
> > > static void CollectAbsoluteBoundsForRange(Vector<FloatRect> acc, ...) {
> > > layout_text->AbsoulteQuadsForRange(acc, ...); }
> > > 
> > > template <typename ElementType>
> > > static computeTextBounds(...) {
> > >    ...
> > >    Vector<ElementType> result;
> > >    for (...) {
> > >      ...
> > >      CollectAbosluteBoundsForRange(result, ...);
> > >    }
> > > }
> > > 
> > > IntRect Range::BoundingBox() const {
> > >    const Vector<IntRect>& rects =
> > > ComputeTextBounds<IntRect>(EphemeralRange(this));
> > >   for (const IntRect& rect : rects)
> > >     result.Unite(rect); 
> > >    return results;
> > > }
> > > 
> > > void Range::TextQuads(Vector<FloatQuad>& quads) const {
> > >  const Vector<FloatQuad>& result =
> > > ComputeTextBounds<FloatQuad>(EpehemralRange(this), ...);
> > >  quads.insert(quads.end(), result.begin(), result().end());
> > > }
> > > 
> > > Note: Since all call sites, there are two, one is RageTest.cpp and another
> is
> > > LayoutObject::AbsoluteBoundingBoxRectForRange()
> > > call TextQuad() with empty vector, we don't need to use
> > > std::vector<T>::insert().
> > > 
> > > So, we'll change them to take result by function return value instead of
> output
> > > parameter.
> > 
> > Shall i do this refactoring in a separate patch after the changes for this
Bug
> is done, to avoid any confusion or risk.
> > 
> > I was thinking to do in this way.
> > 
> > PatchSet #1 Current patch, make Range::textQuads to use EphemralRange. 
> > PatchSet #2 Make LayoutObject to take EphemeralRange.
>  Before PatchSet #2, we need to have ComputeTextQuads() in "VisibleUnits.h".
>  We also wants to change a call site in RangeTest.cpp.
> 
> > PatchSet #3 Use template for textQuads and textRects.
> Since three is no call sites of |Range::textQuads()|, we remove
> |Range::textQuads()| here.
> 
> > PatchSet #4 Move template functionality to VisibleUnits.
> > PatchSet #5 Avoid Passing of use_selection_height
> Note: No call sites pass |use_selection_height|.
> 
> 
> Current patch is copy of |computeTextRects()| with renaming of, 
>  - computeTextRects => computeTextQuads
>  - AbosluteRectsForRange => AbsoulteQuadsForRange
> This is we've done for Range::textRects() in patch[1].
> 
> If we do re-factoring, we don't want to make copy of code.
> Also, patch[1] enables templaizing via |computeTextRects()|.
> 
> We've already have refactored version of computeTextRects(), it is better to
> reuse it.
> 
> 1. Move ComputeTextRects before Range::BoundingBox()
This patch will take care of step 1. 
PTAL!!!
 
> 2. Introduce ComputeTextBounds<T> (in-place change of ComputeTextTexts) and
> change ComputeTextRects() to CTR() { retturn ComputeTextBounds<IntRect>(...);
}
> 3. Introduce ComputeTextQuads(...) { return ComputeTextBounds<FloatRect>(...)
}
> and change Range::TextQuads() to use ComputeTextQudas()
> 4. Move ComputeTextRects() and ComputeTextQuads() with ComputeTextBounds<T> to
> VisibleUnit.cpp
> 5. Change call sites to use ComputeTextQuads()
> 6. Get rid of Range::TextQuads()
> 
> Since all call sites of Range::TextQuads() don't pass |use_selection_height|,
> removing |use_selection_height| can be done in Step#3.
> 
> PROS of this proposal is
>  - No copy & paste (w/ modify) code.
>  - Getting rid of Range::TextQuads()
>  - Change of Range::TextQuads() once.
> 
> [1] http://crrev.com/2810313003 In-place change of Range::textRects() and
> boundingBox() to EphemeralRange

Thanks for the explanation. I will do it in suggested way by you.

[tanvir, 2017-04-21 05:12:44]

Description was changed from

==========
In-place change of Range::textQuads() to use EphemeralRange.

In-place change of Range::textQuads() to use EphemeralRange.
This patch is first step to use EphemeralRange
when calculating textQuads from LayoutObject.

BUG=691198
==========

to

==========
Move ComputeTextRects before Range::BoundingBox()
Currently textQuads and ComputeTextRects algorithm is
mostly same. The difference of behaviour of these two functions
can be handled efficiently by using templates.
    
This patch is the first step to achieve above behaviour.
This patch moved removes the ToDO comment and forward declaration of
computeTextRects.
    
BUG=691198
==========

[tanvir, 2017-04-24 05:34:15]

PTAL!!!

[yosin_UTC9, 2017-04-24 05:55:23]

yosin@chromium.org changed reviewers:
+ tkent@chromium.org

[yosin_UTC9, 2017-04-24 05:55:24]

lgtm

+tkent@ for OWNERS review.

[tkent, 2017-04-24 06:20:14]

lgtm

CL description:
> This patch moved removes the ToDO comment and forward declaration of

moved removes the ToDO ==> removes the TODO ?

[tanvir, 2017-04-24 06:23:24]

Description was changed from

==========
Move ComputeTextRects before Range::BoundingBox()
Currently textQuads and ComputeTextRects algorithm is
mostly same. The difference of behaviour of these two functions
can be handled efficiently by using templates.
    
This patch is the first step to achieve above behaviour.
This patch moved removes the ToDO comment and forward declaration of
computeTextRects.
    
BUG=691198
==========

to

==========
Move ComputeTextRects before Range::BoundingBox()
Currently textQuads and ComputeTextRects algorithm is
mostly same. The difference of behaviour of these two functions
can be handled efficiently by using templates.
    
This patch is the first step to achieve above behaviour.
This patch moved removes the TODO comment and forward declaration of
computeTextRects.
    
BUG=691198
==========

[tanvir, 2017-04-24 07:15:09]

The CQ bit was checked by tanvir.rizvi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 07:15:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tanvir, 2017-04-24 07:15:54]

Description was changed from

==========
Move ComputeTextRects before Range::BoundingBox()
Currently textQuads and ComputeTextRects algorithm is
mostly same. The difference of behaviour of these two functions
can be handled efficiently by using templates.
    
This patch is the first step to achieve above behaviour.
This patch moved removes the TODO comment and forward declaration of
computeTextRects.
    
BUG=691198
==========

to

==========
Move ComputeTextRects before Range::BoundingBox()
Currently textQuads and ComputeTextRects algorithm is
mostly same. The difference of behaviour of these two functions
can be handled efficiently by using templates.
    
This patch is the first step to achieve above behaviour.
This patch removes the TODO comment and forward declaration of
computeTextRects.
    
BUG=691198
==========

[commit-bot: I haz the power, 2017-04-24 08:47:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 08:47:07]

Dry run: This issue passed the CQ dry run.

[tanvir, 2017-04-24 09:19:04]

The CQ bit was checked by tanvir.rizvi@samsung.com

[tanvir, 2017-04-24 09:19:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2827603006/#ps100001
(title: "Review comments Addressed.")

[commit-bot: I haz the power, 2017-04-24 09:19:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 09:22:53]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493025544839840,
"parent_rev": "517dfbd040fbab1d0175db908e38e91f96388e9f", "commit_rev":
"0c2398f2a9fe6a2432bf86861da2e31480b2cc8d"}

[commit-bot: I haz the power, 2017-04-24 09:23:03]

Description was changed from

==========
Move ComputeTextRects before Range::BoundingBox()
Currently textQuads and ComputeTextRects algorithm is
mostly same. The difference of behaviour of these two functions
can be handled efficiently by using templates.
    
This patch is the first step to achieve above behaviour.
This patch removes the TODO comment and forward declaration of
computeTextRects.
    
BUG=691198
==========

to

==========
Move ComputeTextRects before Range::BoundingBox()
Currently textQuads and ComputeTextRects algorithm is
mostly same. The difference of behaviour of these two functions
can be handled efficiently by using templates.
    
This patch is the first step to achieve above behaviour.
This patch removes the TODO comment and forward declaration of
computeTextRects.
    
BUG=691198

Review-Url: https://codereview.chromium.org/2827603006
Cr-Commit-Position: refs/heads/master@{#466607}
Committed:
https://chromium.googlesource.com/chromium/src/+/0c2398f2a9fe6a2432bf86861da2...
==========

[commit-bot: I haz the power, 2017-04-24 09:23:04]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0c2398f2a9fe6a2432bf86861da2...