Make RenderedRectsForMarkers() to ignore disconnected nodes.

Make RenderedRectsForMarkers() to ignore disconnected nodes.

This patch changes DocumentMarkerController::RenderedRectsForMarkers()
to ignore disconnected nodes.
and it is first step to use EphemeralRange in updateMarkerRenderedRect().

we will progress three steps.
[1/3] Make RenderedRectsForMarkers() to ignore disconnected nodes.
[2/3] In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
[3/3] Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
Review-Url: https://codereview.chromium.org/2776103002
Cr-Commit-Position: refs/heads/master@{#464368}
Committed: https://chromium.googlesource.com/chromium/src/+/d2680c72ae7adfa4a857432179666cb6b03e0781

[Hwanseung Lee, 2017-03-27 06:24:54]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 06:25:09]

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-03-27 06:34:12]

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

[commit-bot: I haz the power, 2017-03-27 06:34:13]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Hwanseung Lee, 2017-03-28 10:52:47]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 10:53:04]

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-03-28 11:26:16]

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

[commit-bot: I haz the power, 2017-03-28 11:26:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-03-28 14:07:18]

Description was changed from

==========
[WIP] updateMarkerRenderedRect() should use EphemeralRange

BUG=691202

Signed-off-by: Hwanseung Lee <hs1217.lee@samsung.com>
==========

to

==========
[WIP] updateMarkerRenderedRect() should use EphemeralRange

BUG=691202
==========

[Hwanseung Lee, 2017-03-28 14:07:26]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 14:07:51]

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-03-28 15:18:58]

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

[commit-bot: I haz the power, 2017-03-28 15:18:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hwanseung Lee, 2017-03-28 15:49:03]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 15:49:51]

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-03-28 17:47:16]

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

[commit-bot: I haz the power, 2017-03-28 17:47:16]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Hwanseung Lee, 2017-03-29 14:28:48]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 14:29:12]

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

[Hwanseung Lee, 2017-03-29 14:57:32]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 14:57:47]

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

[Hwanseung Lee, 2017-03-29 14:59:10]

Description was changed from

==========
[WIP] updateMarkerRenderedRect() should use EphemeralRange

BUG=691202
==========

to

==========
updateMarkerRenderedRect() should use EphemeralRange

implement an EphemeralRange::boundingBox(), 
which should be an equivalent to Range::boundingBox.

BUG=691202
==========

[Hwanseung Lee, 2017-03-29 14:59:10]

hs1217.lee@samsung.com changed reviewers:
+ yosin@chromium.org

[Hwanseung Lee, 2017-03-29 15:02:01]

hs1217.lee@samsung.com changed reviewers:
+ xiaochengh@chromium.org, yoichio@chromium.org

[Hwanseung Lee, 2017-03-29 15:06:34]

yosin,  xiaochengh, yoichio@

PTAL, thank you.

[commit-bot: I haz the power, 2017-03-29 16:54:16]

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

[commit-bot: I haz the power, 2017-03-29 16:54:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xiaocheng, 2017-03-29 19:51:08]

Thanks for the patch!

https://codereview.chromium.org/2776103002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EphemeralRange.h (right):

https://codereview.chromium.org/2776103002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EphemeralRange.h:108: void
textRects(Vector<IntRect>&, bool useSelectionHeight = false) const;
This function is just implementation detail and shouldn't appear in a header
file. Besides, |userSelectionHeight| is never used.

https://codereview.chromium.org/2776103002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EphemeralRange.h:109: IntRect
boundingBox() const;
According to the discussion in https://codereview.chromium.org/2775663008, we
should not make EphemeralRange depend on layout.

Could you introduce new functions in VisibleUnits:

IntRect boundingBox(const EphemeralRange&);
IntRect boundingBox(const EphemeralRangeInFlatTree&);

https://codereview.chromium.org/2776103002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EphemeralRangeTest.cpp (right):

https://codereview.chromium.org/2776103002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EphemeralRangeTest.cpp:224:
TEST_F(EphemeralRangeTest, boundingBox) {
Please move the test case to VisibleUnitsTest, and introduce another test case
for EphemeralRangeInFlatTree.

https://codereview.chromium.org/2776103002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EphemeralRangeTest.cpp:241:
EXPECT_EQ(range->boundingBox().toString(),
Please inspect the return value directly. No need to compare it to
Range::boundingBox.

[Hwanseung Lee, 2017-03-29 23:23:01]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 23:23:46]

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

[Hwanseung Lee, 2017-03-30 00:20:52]

Description was changed from

==========
updateMarkerRenderedRect() should use EphemeralRange

implement an EphemeralRange::boundingBox(), 
which should be an equivalent to Range::boundingBox.

BUG=691202
==========

to

==========
updateMarkerRenderedRect() should use EphemeralRange

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

BUG=691202
==========

[commit-bot: I haz the power, 2017-03-30 00:46:36]

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

[commit-bot: I haz the power, 2017-03-30 00:46:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-03-30 01:23:18]

https://codereview.chromium.org/2776103002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2776103002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:218:
marker.setRenderedRect(LayoutRect(boundingBox(EphemeralRange(range))));
Note: Intention of this TODO is using |EphemeralRange| through out this function

instead of just here.

Anyway, this patch is good start point, I recommend to split this patch into two
patches:

1. In-place change of Range::textRects() and boundingBox() to EphemeralRange
version in "Range.cpp"
In-place change with some unit tests for sanity.
  (in-place change) Rename/Change Range::textRects() to static Vector<IntRect>
computeTextRects(const EphemeralRange&)
  (in-place change) Rename/Change Range::boundingBox() to InRect
computeBoundingBox(const EphmeralRange&)
  Range::boundinBox() { return computeBondingBox(EphemeralRange(this)); }

Since Range::textRects() is private member of Range and it is used only in
Range::boundingBox(), we don't need to expose computeTextRects().

2. Rewrite  updateMarkerRenderedRect() to use EphemeralRange instead of Range

Note: We would like to have Flat Tree version of computeBoundingBox() too for
using it in TextFinder.

[Hwanseung Lee, 2017-03-30 13:40:02]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 13:40:27]

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-03-30 14:07:48]

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

[commit-bot: I haz the power, 2017-03-30 14:07:48]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Hwanseung Lee, 2017-03-30 14:14:44]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 14:15:06]

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-03-30 14:28:54]

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

[commit-bot: I haz the power, 2017-03-30 14:28:55]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Hwanseung Lee, 2017-03-30 15:32:19]

On 2017/03/30 01:23:18, yosin_UTC9 wrote:
>
https://codereview.chromium.org/2776103002/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
> (right):
> 
>
https://codereview.chromium.org/2776103002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:218:
> marker.setRenderedRect(LayoutRect(boundingBox(EphemeralRange(range))));
> Note: Intention of this TODO is using |EphemeralRange| through out this
function
> 
> instead of just here.
> 
> Anyway, this patch is good start point, I recommend to split this patch into
two
> patches:
> 
> 1. In-place change of Range::textRects() and boundingBox() to EphemeralRange
> version in "Range.cpp"
> In-place change with some unit tests for sanity.
>   (in-place change) Rename/Change Range::textRects() to static Vector<IntRect>
> computeTextRects(const EphemeralRange&)
>   (in-place change) Rename/Change Range::boundingBox() to InRect
> computeBoundingBox(const EphmeralRange&)
>   Range::boundinBox() { return computeBondingBox(EphemeralRange(this)); }
> 
> Since Range::textRects() is private member of Range and it is used only in
> Range::boundingBox(), we don't need to expose computeTextRects().
> 
> 2. Rewrite  updateMarkerRenderedRect() to use EphemeralRange instead of Range
> 
> Note: We would like to have Flat Tree version of computeBoundingBox() too for
> using it in TextFinder.

yosin@
i try to split patch into two patchs.
patchSet9 is first step to split patch.
when it run tests, it faced many crash.
because when Range object changed to EphemeralRange, it is check whether Range
is isConnected() or not in constructor.
if isConnected() were false in Range::boundingBox(), what can we return?
if we should check isConnected() before called Range::boundingBox(), we should
touch code at updateMarkerRenderedRect() in step1.
how do you think?

[yoichio, 2017-03-31 01:56:37]

As you probably know that tanvir.rizvi@samsung.com is working around
Range::textQuads.

textQuads and textRects are very similar except calling
layoutText->absoluteQuadsForRange
 or  layoutText->absoluteRectsForRange.
Why are you of 2 going to impl in different way?

I recommend you first to reafactor textQuads and textRects sharing code.

[Hwanseung Lee, 2017-03-31 13:01:06]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 13:01:26]

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-03-31 13:39:22]

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

[commit-bot: I haz the power, 2017-03-31 13:39:23]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Hwanseung Lee, 2017-03-31 14:46:40]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 14:47:06]

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-03-31 15:16:40]

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

[commit-bot: I haz the power, 2017-03-31 15:16:41]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Hwanseung Lee, 2017-04-01 00:43:44]

Patchset #11 (id:200001) has been deleted

[Hwanseung Lee, 2017-04-01 00:43:57]

Patchset #10 (id:180001) has been deleted

[Hwanseung Lee, 2017-04-01 00:44:15]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-01 00:44:40]

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-01 01:38:37]

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

[commit-bot: I haz the power, 2017-04-01 01:38:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-04-01 05:31:49]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-01 05:31:54]

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-01 07:18:05]

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

[commit-bot: I haz the power, 2017-04-01 07:18:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-04-01 16:57:12]

On 2017/03/31 01:56:37, yoichio wrote:
> As you probably know that mailto:tanvir.rizvi@samsung.com is working around
> Range::textQuads.
> 
> textQuads and textRects are very similar except calling
> layoutText->absoluteQuadsForRange
>  or  layoutText->absoluteRectsForRange.
> Why are you of 2 going to impl in different way?
> 
> I recommend you first to reafactor textQuads and textRects sharing code.

i didn't know his working.
and his patch is not merged yet. so if i will refactor textQauds and textRects,
it makes conflict.
so i will refactor next patch.

[Hwanseung Lee, 2017-04-01 17:00:10]

Description was changed from

==========
updateMarkerRenderedRect() should use EphemeralRange

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

BUG=691202
==========

to

==========
updateMarkerRenderedRect() should use EphemeralRange [1/2]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

BUG=691202
==========

[Hwanseung Lee, 2017-04-01 17:22:46]

yosin@

patchset 10 is passed, except layout_test of mac_chromium_rel_ng.
(https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...)

i investigated this test.
i think new expected results is better than actual result.
this test is find "findme" and mark in scroll.
"findme" is one, but there are two marks at scrollbar in actual result image.
(https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...).
but expected results has one mark in scroll.
and i tried test same page in nightly chrome.
result was same with actual result image.
but when i try find again "findme" using "command + f", it was changed results.
changed results is same with new expected results.
it means, present actual result is wrong.
how do you think?

[Hwanseung Lee, 2017-04-01 17:44:39]

On 2017/04/01 17:22:46, Hwanseung Lee wrote:
> yosin@
> 
> patchset 10 is passed, except layout_test of mac_chromium_rel_ng.
>
(https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...)
> 
> i investigated this test.
> i think new expected results is better than actual result.
> this test is find "findme" and mark in scroll.
> "findme" is one, but there are two marks at scrollbar in actual result image.
>
(https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...).
> but expected results has one mark in scroll.
> and i tried test same page in nightly chrome.
> result was same with actual result image.
> but when i try find again "findme" using "command + f", it was changed
results.
> changed results is same with new expected results.
> it means, present actual result is wrong.
> how do you think?

oh sorry.... i used changed word with "expected" and "actual" in upper comment.
my opinion is new actual result is better than expected result.
refer to images.
https://bugs.chromium.org/p/chromium/issues/detail?id=691202#c13
1_find_findme_before_chaged.png is before to change page.
2_changed_page.png is expected result.
3_find_again_findme_at_2.png is new actual result.

[Hwanseung Lee, 2017-04-01 17:54:58]

Description was changed from

==========
updateMarkerRenderedRect() should use EphemeralRange [1/2]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

BUG=691202
==========

to

==========
updateMarkerRenderedRect() should use EphemeralRange [1/2]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox() to EphemeralRange
version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

this patch is step1.

BUG=691202
==========

[Hwanseung Lee, 2017-04-01 17:55:17]

Description was changed from

==========
updateMarkerRenderedRect() should use EphemeralRange [1/2]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox() to EphemeralRange
version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

this patch is step1.

BUG=691202
==========

to

==========
updateMarkerRenderedRect() should use EphemeralRange [1/2]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox()
to EphemeralRange version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

this patch is step1.

BUG=691202
==========

[Hwanseung Lee, 2017-04-03 13:20:50]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 13:21:06]

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

[Hwanseung Lee, 2017-04-03 13:22:30]

yosin@
gentle ping. PTAL.

[commit-bot: I haz the power, 2017-04-03 15:20:25]

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

[commit-bot: I haz the power, 2017-04-03 15:20:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-04-03 22:42:31]

Patchset #11 (id:240001) has been deleted

[Hwanseung Lee, 2017-04-03 22:42:40]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 22:43:27]

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

[yosin_UTC9, 2017-04-04 01:20:39]

https://codereview.chromium.org/2776103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2776103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1471: return rects;
nit: s/rects/std::move(rects)/

This is subject to RVO[1]. But, it is better to specify it for disambiguation.

[1] https://en.wikipedia.org/wiki/Return_value_optimization

https://codereview.chromium.org/2776103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1474: IntRect
Range::computeBoundingBox(const EphemeralRange& range) const {
We don't need to make |computeBoundingBox()| as member of Range.
Move outside Range as global function in Range.h.

Another patch will move |computeBoundingBox()| to "VisibleUnits.{cpp,h}"

https://codereview.chromium.org/2776103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2776103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:216:
if (!exceptionState.hadException() && range->isConnected()) {
This change should not be in this patch.
If we need to satisfy this condition, we should check it call sites,
|node.isConnection()|, actually
there is only one call site
|DocumentMarkerController::renderedRectsForMarkers()|, and should have
|DCEHCK(node.isConnected()| in this function.

Example:
static void updateMarkerRenderedRect(const Node& node, ...) {
  DCHECK(node.isConnected()) << node;
  ...

Vector<IntRect> DocumentMarkerController::renderedRectsForMarkers(...) {
   ...
  for (MarkerMap::iterator nodeIterator = m_markers.begin();
       nodeIterator != end; ++nodeIterator) {
    // inner loop; process each marker in this node
    const Node& node = *nodeIterator->key;
    if (!node.isConnected())
      continue;
  ...
}

[commit-bot: I haz the power, 2017-04-04 02:50:29]

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

[commit-bot: I haz the power, 2017-04-04 02:50:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Hwanseung Lee, 2017-04-04 15:46:06]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 15:46:23]

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-04 18:48:51]

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

[commit-bot: I haz the power, 2017-04-04 18:48:52]

Dry run: This issue passed the CQ dry run.

[Hwanseung Lee, 2017-04-05 15:17:48]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 15:18:17]

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

[Hwanseung Lee, 2017-04-05 15:21:31]

yosin@
PTAL. thank you.

https://codereview.chromium.org/2776103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2776103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1471: return rects;
On 2017/04/04 01:20:39, yosin_UTC9 wrote:
> nit: s/rects/std::move(rects)/
> 
> This is subject to RVO[1]. But, it is better to specify it for disambiguation.
> 
> [1] https://en.wikipedia.org/wiki/Return_value_optimization

Done.

https://codereview.chromium.org/2776103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1474: IntRect
Range::computeBoundingBox(const EphemeralRange& range) const {
On 2017/04/04 01:20:39, yosin_UTC9 wrote:
> We don't need to make |computeBoundingBox()| as member of Range.
> Move outside Range as global function in Range.h.
> 
> Another patch will move |computeBoundingBox()| to "VisibleUnits.{cpp,h}"

Done.

https://codereview.chromium.org/2776103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2776103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:216:
if (!exceptionState.hadException() && range->isConnected()) {
On 2017/04/04 01:20:39, yosin_UTC9 wrote:
> This change should not be in this patch.
> If we need to satisfy this condition, we should check it call sites,
> |node.isConnection()|, actually
> there is only one call site
> |DocumentMarkerController::renderedRectsForMarkers()|, and should have
> |DCEHCK(node.isConnected()| in this function.
> 
> Example:
> static void updateMarkerRenderedRect(const Node& node, ...) {
>   DCHECK(node.isConnected()) << node;
>   ...
> 
> Vector<IntRect> DocumentMarkerController::renderedRectsForMarkers(...) {
>    ...
>   for (MarkerMap::iterator nodeIterator = m_markers.begin();
>        nodeIterator != end; ++nodeIterator) {
>     // inner loop; process each marker in this node
>     const Node& node = *nodeIterator->key;
>     if (!node.isConnected())
>       continue;
>   ...
> }

Done.

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

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

[commit-bot: I haz the power, 2017-04-05 16:39:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hwanseung Lee, 2017-04-06 00:08:07]

Patchset #13 (id:300001) has been deleted

[Hwanseung Lee, 2017-04-06 00:08:16]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 00:09:00]

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-06 01:13:32]

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

[commit-bot: I haz the power, 2017-04-06 01:13:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-04-06 02:05:41]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 02:06:17]

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-06 03:07:48]

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

[commit-bot: I haz the power, 2017-04-06 03:07:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-04-06 03:36:04]

Patchset #13 (id:320001) has been deleted

[yosin_UTC9, 2017-04-06 04:26:05]

https://codereview.chromium.org/2776103002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2776103002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4052: Vector<IntRect>
computeTextRects(
In this patch, please do this change in "Range.cpp" rather than moving to
"VisibleUnits.cpp" for
ease of tracking change of Range to EphemeralRange.

In other words, this patch should be change Range.cpp only. Following patches
will
 1. Move computeTextRects() to "VisibleUnits.cpp"
 2. Change call sites to use computeTextRects()

Also, we don't expect behavior changes by this effort, since this is just
re-factoring.

[Hwanseung Lee, 2017-04-06 14:22:39]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 14:23:03]

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-06 16:47:33]

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

[commit-bot: I haz the power, 2017-04-06 16:47:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-04-06 22:24:47]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[Hwanseung Lee, 2017-04-06 22:24:51]

Patchset #13 (id:340001) has been deleted

[commit-bot: I haz the power, 2017-04-06 22:25:59]

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-07 00:35:30]

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

[commit-bot: I haz the power, 2017-04-07 00:35:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-04-07 00:55:42]

Patchset #13 (id:360001) has been deleted

[Hwanseung Lee, 2017-04-07 00:55:56]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 00:56:32]

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

[Hwanseung Lee, 2017-04-07 02:26:18]

PTAL. thank you.

[commit-bot: I haz the power, 2017-04-07 04:01:23]

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

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

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-04-07 04:48:11]

Why does this patch changes layout text expectations?
Since this patch is re-factoring, we don't expect to change behavior.

Is EphemeralRange::node() and Range::firstNode() + Range::pastlastNode() +
NodeTraversal::next() different?

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Range.cpp (left):

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1453: Vector<IntRect> rects;
Just change these two lines:

const Vector<IntRect> rects = computeTextRects(EphemeralRange(this));

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1455: template <typename Strategy>
We don't need to use |template <Strategy>|, |computeTextRects()| works only for
DOM tree.

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1456: static Vector<IntRect>
computeTextRects(
It is OK to adding forward declaration before |Range::boundingBox()| with TODO
explains that we'll move impl and remove forward decl of computeTextRects()
here.

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1457: const
EphemeralRangeTemplate<Strategy>& range) {
nit: const EphemeralRange&

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1458: const
PositionTemplate<Strategy> startPosition = range.startPosition();
nit const Position&

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Range.h (right):

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.h:145: IntRect computeBoundingBox(const
EphemeralRange&) const;
No need to introduce |computeBondingBox()| in this patch.
This patch is just replace |textRects| to |computeTextRects(const
EphemeralRange&)|.

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:207:
DCHECK(node.isConnected()) << node;
Please move this change into another patch.

[Hwanseung Lee, 2017-04-07 15:01:28]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 15:01:49]

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

[Hwanseung Lee, 2017-04-07 15:02:56]

Patchset #14 (id:400001) has been deleted

[commit-bot: I haz the power, 2017-04-07 15:03:46]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

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

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

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

Dry run: This issue passed the CQ dry run.

[Hwanseung Lee, 2017-04-07 15:10:40]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 15:10:58]

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-07 16:48:45]

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

[commit-bot: I haz the power, 2017-04-07 16:48:46]

Dry run: This issue passed the CQ dry run.

[Hwanseung Lee, 2017-04-07 17:55:53]

PTAL. thank you.

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1455: template <typename Strategy>
On 2017/04/07 04:48:10, yosin_UTC9 wrote:
> We don't need to use |template <Strategy>|, |computeTextRects()| works only
for
> DOM tree.

Done.

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1456: static Vector<IntRect>
computeTextRects(
On 2017/04/07 04:48:10, yosin_UTC9 wrote:
> It is OK to adding forward declaration before |Range::boundingBox()| with TODO
> explains that we'll move impl and remove forward decl of computeTextRects()
> here.

Done.

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1457: const
EphemeralRangeTemplate<Strategy>& range) {
On 2017/04/07 04:48:10, yosin_UTC9 wrote:
> nit: const EphemeralRange&

Done.

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.cpp:1458: const
PositionTemplate<Strategy> startPosition = range.startPosition();
On 2017/04/07 04:48:10, yosin_UTC9 wrote:
> nit const Position&

Done.

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Range.h (right):

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Range.h:145: IntRect computeBoundingBox(const
EphemeralRange&) const;
On 2017/04/07 04:48:10, yosin_UTC9 wrote:
> No need to introduce |computeBondingBox()| in this patch.
> This patch is just replace |textRects| to |computeTextRects(const
> EphemeralRange&)|.

Done.

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2776103002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:207:
DCHECK(node.isConnected()) << node;
On 2017/04/07 04:48:10, yosin_UTC9 wrote:
> Please move this change into another patch.

Done.

[yosin_UTC9, 2017-04-10 04:25:59]

It seems Patch 14 is as same as Patch 13.
Could you upload updated patch?

[Hwanseung Lee, 2017-04-10 04:46:09]

On 2017/04/10 04:25:59, yosin_UTC9 wrote:
> It seems Patch 14 is as same as Patch 13.
> Could you upload updated patch?

um... it is diffrent.
https://codereview.chromium.org/2776103002/diff2/380001:420001/third_party/We...
could you check it?

[yosin_UTC9, 2017-04-10 05:00:33]

On 2017/04/10 at 04:46:09, hs1217.lee wrote:
> On 2017/04/10 04:25:59, yosin_UTC9 wrote:
> > It seems Patch 14 is as same as Patch 13.
> > Could you upload updated patch?
> 
> um... it is diffrent.
>
https://codereview.chromium.org/2776103002/diff2/380001:420001/third_party/We...
> could you check it?

PS#14 still contains DocumentMarkerController.cpp and layout test changes.
Range.cpp: Range::boundingBox() is moved after |computeTextRects()|. 
It should be before |computeTextRects()| with forward declaration of
|computeTextRects()|.

You marked "Done" for comments for PS#13, but PS#14 doesn't change about above.

[Hwanseung Lee, 2017-04-11 16:09:11]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 16:09:43]

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-11 16:26:36]

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

[commit-bot: I haz the power, 2017-04-11 16:26:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hwanseung Lee, 2017-04-11 16:50:35]

Patchset #15 (id:440001) has been deleted

[Hwanseung Lee, 2017-04-11 16:50:59]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 16:52:00]

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-11 17:05:28]

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

[commit-bot: I haz the power, 2017-04-11 17:05:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hwanseung Lee, 2017-04-11 22:26:52]

Patchset #15 (id:460001) has been deleted

[Hwanseung Lee, 2017-04-11 22:27:37]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 22:28:24]

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-11 23:54:00]

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

[commit-bot: I haz the power, 2017-04-11 23:54:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hwanseung Lee, 2017-04-12 12:24:00]

Patchset #15 (id:480001) has been deleted

[Hwanseung Lee, 2017-04-12 12:24:16]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 12:24: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-12 12:27:06]

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

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

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-04-12 14:22:54]

Patchset #15 (id:500001) has been deleted

[Hwanseung Lee, 2017-04-13 00:23:19]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

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

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

[Hwanseung Lee, 2017-04-13 00:32:00]

Patchset #17 (id:560001) has been deleted

[Hwanseung Lee, 2017-04-13 00:32:54]

Patchset #16 (id:540001) has been deleted

[Hwanseung Lee, 2017-04-13 00:33:22]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 00:34:17]

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

[yoichio, 2017-04-13 01:14:17]

Looks much better. Great.
Could you rewrite patch title describing about just step-1?

[Hwanseung Lee, 2017-04-13 01:15:43]

Description was changed from

==========
updateMarkerRenderedRect() should use EphemeralRange [1/2]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox()
to EphemeralRange version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

this patch is step1.

BUG=691202
==========

to

==========
updateMarkerRenderedRect() should use EphemeralRange [step1]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox()
to EphemeralRange version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

BUG=691202
==========

[Hwanseung Lee, 2017-04-13 01:16:06]

Description was changed from

==========
updateMarkerRenderedRect() should use EphemeralRange [step1]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox()
to EphemeralRange version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

BUG=691202
==========

to

==========
[step1] updateMarkerRenderedRect() should use EphemeralRange

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox()
to EphemeralRange version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

BUG=691202
==========

[Hwanseung Lee, 2017-04-13 01:16:35]

Description was changed from

==========
[step1] updateMarkerRenderedRect() should use EphemeralRange

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox()
to EphemeralRange version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

BUG=691202
==========

to

==========
updateMarkerRenderedRect() should use EphemeralRange [step 1/2]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox()
to EphemeralRange version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

BUG=691202
==========

[commit-bot: I haz the power, 2017-04-13 03:13:26]

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

[commit-bot: I haz the power, 2017-04-13 03:13:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-04-13 03:18:01]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 03:18:34]

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

[Hwanseung Lee, 2017-04-13 03:23:28]

Description was changed from

==========
updateMarkerRenderedRect() should use EphemeralRange [step 1/2]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox()
to EphemeralRange version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

BUG=691202
==========

to

==========
updateMarkerRenderedRect() should use EphemeralRange [step-1]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox()
to EphemeralRange version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

BUG=691202
==========

[commit-bot: I haz the power, 2017-04-13 05:08:40]

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

[commit-bot: I haz the power, 2017-04-13 05:08:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hwanseung Lee, 2017-04-13 05:52:51]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 05:53:19]

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

[yosin_UTC9, 2017-04-13 05:56:59]

Please make this patch contains only Range.cpp and Range.h changes.

[Hwanseung Lee, 2017-04-13 06:16:00]

On 2017/04/13 05:56:59, yosin_UTC9 wrote:
> Please make this patch contains only Range.cpp and Range.h changes.

@yosin.
patch15 has only Range.cpp and Range.h.
but it has crash when try tests.

as i told you, when EphemeralRange created from Range object,
it is check whether Range is isConnected() or not in constructor.
so it makes crash. so i add check logic in DocumentMarkerController.cpp.(patch
#16)
how to void crash without check logic?

and after add check logic. it do not have crash.
but actual image was changed. so it can't pass test.(patch #16 result)
i think it is effect to change to EphemeralRange from Range.
and as i told you. new actual images is better than present expected image.
(https://codereview.chromium.org/2776103002/#msg65).

[yosin_UTC9, 2017-04-13 06:31:34]

On 2017/04/13 at 06:16:00, hs1217.lee wrote:
> On 2017/04/13 05:56:59, yosin_UTC9 wrote:
> > Please make this patch contains only Range.cpp and Range.h changes.
> 
> @yosin.
> patch15 has only Range.cpp and Range.h.
> but it has crash when try tests.
> 
> as i told you, when EphemeralRange created from Range object,
> it is check whether Range is isConnected() or not in constructor.
> so it makes crash. so i add check logic in DocumentMarkerController.cpp.(patch
#16)
> how to void crash without check logic?
> 
> and after add check logic. it do not have crash.
> but actual image was changed. so it can't pass test.(patch #16 result)
> i think it is effect to change to EphemeralRange from Range.
> and as i told you. new actual images is better than present expected image.
> (https://codereview.chromium.org/2776103002/#msg65).

I see. Let's make two patches:
1. DocumentMarkerController.cpp + layout test update
2. Range.cpp + Range.h

[Hwanseung Lee, 2017-04-13 06:43:23]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 06:43:51]

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

[Hwanseung Lee, 2017-04-13 06:45:13]

Description was changed from

==========
updateMarkerRenderedRect() should use EphemeralRange [step-1]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. In-place change of Range::textRects() and boundingBox()
to EphemeralRange version in "Range.cpp"
step2. Rewrite  updateMarkerRenderedRect() to use 
EphemeralRange instead of Range

BUG=691202
==========

to

==========
updateMarkerRenderedRect() should use EphemeralRange [step-1]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. add check logic for Range is isConnected() or not
 and layout test update.
step2. In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
step3. Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
==========

[commit-bot: I haz the power, 2017-04-13 08:02:29]

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

[commit-bot: I haz the power, 2017-04-13 08:02:31]

Dry run: This issue passed the CQ dry run.

[Hwanseung Lee, 2017-04-13 08:05:27]

On 2017/04/13 06:31:34, yosin_UTC9 wrote:
> On 2017/04/13 at 06:16:00, hs1217.lee wrote:
> > On 2017/04/13 05:56:59, yosin_UTC9 wrote:
> > > Please make this patch contains only Range.cpp and Range.h changes.
> > 
> > @yosin.
> > patch15 has only Range.cpp and Range.h.
> > but it has crash when try tests.
> > 
> > as i told you, when EphemeralRange created from Range object,
> > it is check whether Range is isConnected() or not in constructor.
> > so it makes crash. so i add check logic in
DocumentMarkerController.cpp.(patch
> #16)
> > how to void crash without check logic?
> > 
> > and after add check logic. it do not have crash.
> > but actual image was changed. so it can't pass test.(patch #16 result)
> > i think it is effect to change to EphemeralRange from Range.
> > and as i told you. new actual images is better than present expected image.
> > (https://codereview.chromium.org/2776103002/#msg65).
> 
> I see. Let's make two patches:
> 1. DocumentMarkerController.cpp + layout test update
> 2. Range.cpp + Range.h

i uploaded only DocumentMarkerController.cpp + layout test update.(patch #18)
and it was passed for layout tests.

[yosin_UTC9, 2017-04-13 08:56:55]

On 2017/04/13 at 08:05:27, hs1217.lee wrote:
> On 2017/04/13 06:31:34, yosin_UTC9 wrote:
> > On 2017/04/13 at 06:16:00, hs1217.lee wrote:
> > > On 2017/04/13 05:56:59, yosin_UTC9 wrote:
> > > > Please make this patch contains only Range.cpp and Range.h changes.
> > > 
> > > @yosin.
> > > patch15 has only Range.cpp and Range.h.
> > > but it has crash when try tests.
> > > 
> > > as i told you, when EphemeralRange created from Range object,
> > > it is check whether Range is isConnected() or not in constructor.
> > > so it makes crash. so i add check logic in
DocumentMarkerController.cpp.(patch
> > #16)
> > > how to void crash without check logic?
> > > 
> > > and after add check logic. it do not have crash.
> > > but actual image was changed. so it can't pass test.(patch #16 result)
> > > i think it is effect to change to EphemeralRange from Range.
> > > and as i told you. new actual images is better than present expected
image.
> > > (https://codereview.chromium.org/2776103002/#msg65).
> > 
> > I see. Let's make two patches:
> > 1. DocumentMarkerController.cpp + layout test update
> > 2. Range.cpp + Range.h
> 
> i uploaded only DocumentMarkerController.cpp + layout test update.(patch #18)
> and it was passed for layout tests.

Thanks for uploading.
Changes are looks good.
Could you update description and summary?

e.g.
Make DocumentMarkerController::RenderedRectsForMarkers() to ignore disconnected
nodes

This patch changes DocumentMarkerController::RenderedRectsForMarkers() to ignore
disconnected nodes
since blah blah ...

[Hwanseung Lee, 2017-04-13 09:07:55]

Description was changed from

==========
updateMarkerRenderedRect() should use EphemeralRange [step-1]

implement an boundingBox() for EphemeralRange, 
which should be an equivalent to Range::boundingBox.

step1. add check logic for Range is isConnected() or not
 and layout test update.
step2. In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
step3. Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
==========

to

==========
Make RenderedRectsForMarkers() to ignore disconnected nodes [step-1]

This patch changes DocumentMarkerController::RenderedRectsForMarkers() to ignore
disconnected nodes.
and it is first step to use EphemeralRange in updateMarkerRenderedRect().

we will have three steps.
step1. Make RenderedRectsForMarkers() to ignore disconnected nodes.
step2. In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
step3. Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
==========

[Hwanseung Lee, 2017-04-13 09:09:35]

Description was changed from

==========
Make RenderedRectsForMarkers() to ignore disconnected nodes [step-1]

This patch changes DocumentMarkerController::RenderedRectsForMarkers() to ignore
disconnected nodes.
and it is first step to use EphemeralRange in updateMarkerRenderedRect().

we will have three steps.
step1. Make RenderedRectsForMarkers() to ignore disconnected nodes.
step2. In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
step3. Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
==========

to

==========
Make RenderedRectsForMarkers() to ignore disconnected nodes.

This patch changes DocumentMarkerController::RenderedRectsForMarkers() to ignore
disconnected nodes.
and it is first step to use EphemeralRange in updateMarkerRenderedRect().

we will progress three steps.
step1. Make RenderedRectsForMarkers() to ignore disconnected nodes.
step2. In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
step3. Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
==========

[Hwanseung Lee, 2017-04-13 09:10:11]

Description was changed from

==========
Make RenderedRectsForMarkers() to ignore disconnected nodes.

This patch changes DocumentMarkerController::RenderedRectsForMarkers() to ignore
disconnected nodes.
and it is first step to use EphemeralRange in updateMarkerRenderedRect().

we will progress three steps.
step1. Make RenderedRectsForMarkers() to ignore disconnected nodes.
step2. In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
step3. Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
==========

to

==========
Make RenderedRectsForMarkers() to ignore disconnected nodes.

This patch changes DocumentMarkerController::RenderedRectsForMarkers() to ignore
disconnected nodes.
and it is first step to use EphemeralRange in updateMarkerRenderedRect().

we will progress three steps.
[1/3] Make RenderedRectsForMarkers() to ignore disconnected nodes.
[2/3] In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
[3/3] Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
==========

[Hwanseung Lee, 2017-04-13 09:10:52]

Description was changed from

==========
Make RenderedRectsForMarkers() to ignore disconnected nodes.

This patch changes DocumentMarkerController::RenderedRectsForMarkers() to ignore
disconnected nodes.
and it is first step to use EphemeralRange in updateMarkerRenderedRect().

we will progress three steps.
[1/3] Make RenderedRectsForMarkers() to ignore disconnected nodes.
[2/3] In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
[3/3] Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
==========

to

==========
Make RenderedRectsForMarkers() to ignore disconnected nodes.

This patch changes DocumentMarkerController::RenderedRectsForMarkers()
to ignore disconnected nodes.
and it is first step to use EphemeralRange in updateMarkerRenderedRect().

we will progress three steps.
[1/3] Make RenderedRectsForMarkers() to ignore disconnected nodes.
[2/3] In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
[3/3] Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
==========

[Hwanseung Lee, 2017-04-13 09:13:44]

On 2017/04/13 08:56:55, yosin_UTC9 wrote:
> On 2017/04/13 at 08:05:27, hs1217.lee wrote:
> > On 2017/04/13 06:31:34, yosin_UTC9 wrote:
> > > On 2017/04/13 at 06:16:00, hs1217.lee wrote:
> > > > On 2017/04/13 05:56:59, yosin_UTC9 wrote:
> > > > > Please make this patch contains only Range.cpp and Range.h changes.
> > > > 
> > > > @yosin.
> > > > patch15 has only Range.cpp and Range.h.
> > > > but it has crash when try tests.
> > > > 
> > > > as i told you, when EphemeralRange created from Range object,
> > > > it is check whether Range is isConnected() or not in constructor.
> > > > so it makes crash. so i add check logic in
> DocumentMarkerController.cpp.(patch
> > > #16)
> > > > how to void crash without check logic?
> > > > 
> > > > and after add check logic. it do not have crash.
> > > > but actual image was changed. so it can't pass test.(patch #16 result)
> > > > i think it is effect to change to EphemeralRange from Range.
> > > > and as i told you. new actual images is better than present expected
> image.
> > > > (https://codereview.chromium.org/2776103002/#msg65).
> > > 
> > > I see. Let's make two patches:
> > > 1. DocumentMarkerController.cpp + layout test update
> > > 2. Range.cpp + Range.h
> > 
> > i uploaded only DocumentMarkerController.cpp + layout test update.(patch
#18)
> > and it was passed for layout tests.
> 
> Thanks for uploading.
> Changes are looks good.
> Could you update description and summary?
> 
> e.g.
> Make DocumentMarkerController::RenderedRectsForMarkers() to ignore
disconnected
> nodes
> 
> This patch changes DocumentMarkerController::RenderedRectsForMarkers() to
ignore
> disconnected nodes
> since blah blah ...

i updated description. is it fine?

[yosin_UTC9, 2017-04-13 09:34:22]

lgtm

Thanks for fixing this!

[Hwanseung Lee, 2017-04-13 10:21:21]

The CQ bit was checked by hs1217.lee@samsung.com

[commit-bot: I haz the power, 2017-04-13 10:21:44]

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-13 10:26:29]

CQ is committing da patch.

Bot data: {"patchset_id": 620001, "attempt_start_ts": 1492078881614510,
"parent_rev": "cbacf9f287be8eeb7d208d8abeac67e1534d1b22", "commit_rev":
"d2680c72ae7adfa4a857432179666cb6b03e0781"}

[commit-bot: I haz the power, 2017-04-13 10:27:29]

Description was changed from

==========
Make RenderedRectsForMarkers() to ignore disconnected nodes.

This patch changes DocumentMarkerController::RenderedRectsForMarkers()
to ignore disconnected nodes.
and it is first step to use EphemeralRange in updateMarkerRenderedRect().

we will progress three steps.
[1/3] Make RenderedRectsForMarkers() to ignore disconnected nodes.
[2/3] In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
[3/3] Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202
==========

to

==========
Make RenderedRectsForMarkers() to ignore disconnected nodes.

This patch changes DocumentMarkerController::RenderedRectsForMarkers()
to ignore disconnected nodes.
and it is first step to use EphemeralRange in updateMarkerRenderedRect().

we will progress three steps.
[1/3] Make RenderedRectsForMarkers() to ignore disconnected nodes.
[2/3] In-place change of Range::textRects() and boundingBox()
 to EphemeralRange version in "Range.cpp"
[3/3] Rewrite  updateMarkerRenderedRect() to use 
 EphemeralRange instead of Range

BUG=691202

Review-Url: https://codereview.chromium.org/2776103002
Cr-Commit-Position: refs/heads/master@{#464368}
Committed:
https://chromium.googlesource.com/chromium/src/+/d2680c72ae7adfa4a85743217966...
==========

[commit-bot: I haz the power, 2017-04-13 10:27:31]

Committed patchset #18 (id:620001) as
https://chromium.googlesource.com/chromium/src/+/d2680c72ae7adfa4a85743217966...