Introduce Position::IsEquivalent()

Introduce Position::IsEquivalent()

This patch introduces |Position::IsEquivalent()| to check whether two positions
are in same position regardless anchor type, e.g. |Position(node, 0)| and
|Position::FirstPositionInNode(node)|. denote a position before first child of
|node|.

This patch is a preparation of patch[1].

[1] http://crrev.com/2875933004: Utilize Position::IsEquivalent() in
{Up,Down}streamIgnoringEditingBoundaries()

BUG=716093, 719343
TEST=run_webkit_unit_tests --gtest_filter=PositionTest.IsEquivalent

Review-Url: https://codereview.chromium.org/2875253002
Cr-Commit-Position: refs/heads/master@{#471689}
Committed: https://chromium.googlesource.com/chromium/src/+/4aee85c54527cbd0857b07d6dc4699465f55a18f

[yosin_UTC9, 2017-05-12 07:46:58]

Description was changed from

==========
2017-05-12T16:40:51

BUG=719343

2017-05-12T16:29:18
==========

to

==========
2017-05-12T16:40:51

BUG=719343
==========

[yosin_UTC9, 2017-05-12 07:47:03]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 07:47:22]

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-05-12 08:25:09]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 08:25:29]

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-05-12 08:31:14]

Description was changed from

==========
2017-05-12T16:40:51

BUG=719343
==========

to

==========
Introduce Position::IsEquivalent()

This patch introduces |Position::IsEquivalent()| to check whether two positions
are in same position regardless anchor type, e.g. |Position(node, 0)| and
|Position::FirstPositionInNode(node)|. denote a position before first child of
|node|.

This patch is a preparation of patch[1].

[1] http://crrev.com/2875933004: Utilize Position::IsEquivalent() in
{Up,Down}streamIgnoringEditingBoundaries()

BUG=716093, 719343
TEST=run_webkit_unit_tests --gtest_filter=PositionTest.IsEquivalent
==========

[commit-bot: I haz the power, 2017-05-12 09:22:09]

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

[commit-bot: I haz the power, 2017-05-12 09:22:10]

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

[yosin_UTC9, 2017-05-12 09:25:24]

yosin@chromium.org changed reviewers:
+ xiaochengh@chromium.org, yoichio@chromium.org

[yosin_UTC9, 2017-05-12 09:25:25]

PTAL

This patch is a preparation of fixing Release-Blocker bug.

[hugoh_UTC2, 2017-05-12 14:12:14]

hugoh@opera.com changed reviewers:
+ hugoh@opera.com

[hugoh_UTC2, 2017-05-12 14:12:14]

https://codereview.chromium.org/2875253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/PositionTest.cpp (right):

https://codereview.chromium.org/2875253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/PositionTest.cpp:14: SetBodyContent("<a
id=sample>0<b>1</b>2</b>");
<b>2</b> ?

Missing </a> ?

[yoichio, 2017-05-15 01:43:05]

https://codereview.chromium.org/2875253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Position.cpp (right):

https://codereview.chromium.org/2875253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Position.cpp:359: return other.IsNull();
nlgtm because Nan != Nan generally.

[yosin_UTC9, 2017-05-15 04:07:17]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

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

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-05-15 04:08:15]

PTAL

https://codereview.chromium.org/2875253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Position.cpp (right):

https://codereview.chromium.org/2875253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Position.cpp:359: return other.IsNull();
On 2017/05/15 at 01:43:05, yoichio wrote:
> nlgtm because Nan != Nan generally.

IsNull() doesn't mean NaN.
Also, Position::operator==() does same thing about IsNull()

template <typename Strategy>
bool operator==(const PositionTemplate<Strategy>& a,
                const PositionTemplate<Strategy>& b) {
  if (a.IsNull())
    return b.IsNull();

https://codereview.chromium.org/2875253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/PositionTest.cpp (right):

https://codereview.chromium.org/2875253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/PositionTest.cpp:14: SetBodyContent("<a
id=sample>0<b>1</b>2</b>");
On 2017/05/12 at 14:12:14, hugoh_UTC2 wrote:
> <b>2</b> ?
> 
> Missing </a> ?

Good catch. It should be "</a>"

[yoichio, 2017-05-15 04:17:29]

> > nlgtm because Nan != Nan generally.
> 
> IsNull() doesn't mean NaN.
> Also, Position::operator==() does same thing about IsNull()
> 
> template <typename Strategy>
> bool operator==(const PositionTemplate<Strategy>& a,
>                 const PositionTemplate<Strategy>& b) {
>   if (a.IsNull())
>     return b.IsNull();
Then we should fix it first because |ComparePositions|
 assumes Positions are not null.
Since we order Positions, Null Position should be settled outside of it.

[yosin_UTC9, 2017-05-15 05:10:22]

On 2017/05/15 at 04:17:29, yoichio wrote:
> > > nlgtm because Nan != Nan generally.
> > 
> > IsNull() doesn't mean NaN.
> > Also, Position::operator==() does same thing about IsNull()
> > 
> > template <typename Strategy>
> > bool operator==(const PositionTemplate<Strategy>& a,
> >                 const PositionTemplate<Strategy>& b) {
> >   if (a.IsNull())
> >     return b.IsNull();
> Then we should fix it first because |ComparePositions|
>  assumes Positions are not null.
> Since we order Positions, Null Position should be settled outside of it.

Talk offline. I understand people feel checking equality of null-Position is
strange, and
some people doesn't think so.

But, it seems to avoid using null position improves readability, we'll get rid
of null
position and use std::optional<Position> in later.

For this patch, Position::IsEquivalent() follows Position::operator==() to
minimize patch.
We'll do eliminating null position in later with std::optional<Position>

[yoichio, 2017-05-15 05:16:41]

lgtm

[yosin_UTC9, 2017-05-15 05:27:50]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-05-15 05:27:52]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-05-15 05:28:11]

CQ is trying da patch.

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

[yoichio, 2017-05-15 05:35:14]

I think types introducing |compare()| should live in the sanctuary.
Since generally |==| operator is used by |compare|, I don't want to 
use |==| w/o the assumptions that |compare| dchecks.

[yosin_UTC9, 2017-05-15 06:45:55]

On 2017/05/15 at 05:35:14, yoichio wrote:
> I think types introducing |compare()| should live in the sanctuary.
> Since generally |==| operator is used by |compare|, I don't want to 
> use |==| w/o the assumptions that |compare| dchecks.

Anyway, please file a bug to avoid null position for equality/equivalent.

Just FYI, number of call sites of operator==() is 73, operator!=() is 26.

Some call sites does null-checks
 if (extent_position.IsNull() || selection.VisibleBase().DeepEquivalent() ==
extent_position.DeepEquivalent())

[commit-bot: I haz the power, 2017-05-15 07:35:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494826072506140,
"parent_rev": "3f5b99e8adc4118ac88dbbb1964144c70097f4cf", "commit_rev":
"4aee85c54527cbd0857b07d6dc4699465f55a18f"}

[commit-bot: I haz the power, 2017-05-15 07:35:18]

Description was changed from

==========
Introduce Position::IsEquivalent()

This patch introduces |Position::IsEquivalent()| to check whether two positions
are in same position regardless anchor type, e.g. |Position(node, 0)| and
|Position::FirstPositionInNode(node)|. denote a position before first child of
|node|.

This patch is a preparation of patch[1].

[1] http://crrev.com/2875933004: Utilize Position::IsEquivalent() in
{Up,Down}streamIgnoringEditingBoundaries()

BUG=716093, 719343
TEST=run_webkit_unit_tests --gtest_filter=PositionTest.IsEquivalent
==========

to

==========
Introduce Position::IsEquivalent()

This patch introduces |Position::IsEquivalent()| to check whether two positions
are in same position regardless anchor type, e.g. |Position(node, 0)| and
|Position::FirstPositionInNode(node)|. denote a position before first child of
|node|.

This patch is a preparation of patch[1].

[1] http://crrev.com/2875933004: Utilize Position::IsEquivalent() in
{Up,Down}streamIgnoringEditingBoundaries()

BUG=716093, 719343
TEST=run_webkit_unit_tests --gtest_filter=PositionTest.IsEquivalent

Review-Url: https://codereview.chromium.org/2875253002
Cr-Commit-Position: refs/heads/master@{#471689}
Committed:
https://chromium.googlesource.com/chromium/src/+/4aee85c54527cbd0857b07d6dc46...
==========

[commit-bot: I haz the power, 2017-05-15 07:35:24]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/4aee85c54527cbd0857b07d6dc46...

[yoichio, 2017-05-15 09:09:24]

Filed:
https://bugs.chromium.org/p/chromium/issues/detail?id=722234