Comment/test for DocumentMarkerList::MarkersIntersectingRange() with collapsed range

Comment/test for DocumentMarkerList::MarkersIntersectingRange() with collapsed range

When I added this method, I didn't realize it had useful behavior for the case
where a collapsed range is passed in. It turns out it does (it returns markers that
contain the start/end position in their interior), so I'm adding a comment
explaining this behavior and a test case testing it.

BUG=707867
Review-Url: https://codereview.chromium.org/2953183004
Cr-Commit-Position: refs/heads/master@{#489192}
Committed: https://chromium.googlesource.com/chromium/src/+/bfc3b0fc01351c264824c22da6afbe6e64f46c47

[rlanday, 2017-06-23 19:38:28]

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

[rlanday, 2017-06-23 19:38:37]

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

[commit-bot: I haz the power, 2017-06-23 19:38:44]

Dry run: CQ is trying da patch.

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

[Xiaocheng, 2017-06-23 21:11:39]

Can we use less ambiguous terms other than "empty"? "Empty" reminds me of the
empty set.

For a range with start == end, a better term is "collapsed".

So the comment above the function can be changed to "Return markers whose
interiors have non-empty overlap with the range [start_offset, end_offset]. Note
that the range can be collapsed, in which case markers containing the offset in
their interiors are returned."

(should we use singular or plural forms of "interior" and "have"?)

https://codereview.chromium.org/2953183004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditorTest.cpp
(right):

https://codereview.chromium.org/2953183004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditorTest.cpp:455:
TEST_F(DocumentMarkerListEditorTest, MarkersIntersectingRange_EmptyRange) {
s/Empty/Collapsed/

[rlanday, 2017-06-23 21:16:51]

On 2017/06/23 at 21:11:39, xiaochengh wrote:
> Can we use less ambiguous terms other than "empty"? "Empty" reminds me of the
empty set.
> For a range with start == end, a better term is "collapsed".

I guess you're correct that the interval's not really "empty," it does contain a
single point. It looks like the mathematical term for such an interval is
"degenerate," I'm happy to use "collapsed" though :)


> So the comment above the function can be changed to "Return markers whose
interiors have non-empty overlap with the range [start_offset, end_offset]. Note
that the range can be collapsed, in which case markers containing the offset in
their interiors are returned."
> 
> (should we use singular or plural forms of "interior" and "have"?)

I believe it's correct as you phrased it.

[rlanday, 2017-06-23 21:21:20]

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

[rlanday, 2017-06-23 21:21:32]

On 2017/06/23 at 21:16:51, rlanday wrote:
> On 2017/06/23 at 21:11:39, xiaochengh wrote:
> > Can we use less ambiguous terms other than "empty"? "Empty" reminds me of
the empty set.
> > For a range with start == end, a better term is "collapsed".
> 
> I guess you're correct that the interval's not really "empty," it does contain
a single point. It looks like the mathematical term for such an interval is
"degenerate," I'm happy to use "collapsed" though :)
> 
> 
> > So the comment above the function can be changed to "Return markers whose
interiors have non-empty overlap with the range [start_offset, end_offset]. Note
that the range can be collapsed, in which case markers containing the offset in
their interiors are returned."
> > 
> > (should we use singular or plural forms of "interior" and "have"?)
> 
> I believe it's correct as you phrased it.

Updated

[commit-bot: I haz the power, 2017-06-23 21:21:38]

Dry run: CQ is trying da patch.

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

[rlanday, 2017-06-23 21:23:05]

Description was changed from

==========
Comment/test for DocumentMarkerList::MarkersIntersectingRange() with empty range

When I added this method, I didn't realize it had useful behavior for the case
where an empty range is passed in. It turns out it does (it returns markers that
contain the start/end position in their interior), so I'm adding a comment
explaining this behavior and a test case testing it.

BUG=707867
==========

to

==========
Comment/test for DocumentMarkerList::MarkersIntersectingRange() with collapsed
range

When I added this method, I didn't realize it had useful behavior for the case
where a collapsed range is passed in. It turns out it does (it returns markers
that
contain the start/end position in their interior), so I'm adding a comment
explaining this behavior and a test case testing it.

BUG=707867
==========

[Xiaocheng, 2017-06-23 21:40:32]

lgtm

https://codereview.chromium.org/2953183004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h
(right):

https://codereview.chromium.org/2953183004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:36: //
Return markers whose interiors have non-empty overlap with the range
nit: s/Return/Returns/

Sorry for my typo.

[rlanday, 2017-07-24 23:33:46]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-07-24 23:33:47]

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/2953183004/#ps60001
(title: "Rebase")

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

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-07-25 02:29:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1500939226810810,
"parent_rev": "5ff24448da1ae5e61aa6fb830f04a498fbbea52f", "commit_rev":
"2199a66f493ca7b2559fce9f8f128d13824e67bd"}

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

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1500939226810810,
"parent_rev": "683d610375dbbf8b425948e401a5f7608fa0c2bc", "commit_rev":
"bfc3b0fc01351c264824c22da6afbe6e64f46c47"}

[commit-bot: I haz the power, 2017-07-25 02:30:30]

Description was changed from

==========
Comment/test for DocumentMarkerList::MarkersIntersectingRange() with collapsed
range

When I added this method, I didn't realize it had useful behavior for the case
where a collapsed range is passed in. It turns out it does (it returns markers
that
contain the start/end position in their interior), so I'm adding a comment
explaining this behavior and a test case testing it.

BUG=707867
==========

to

==========
Comment/test for DocumentMarkerList::MarkersIntersectingRange() with collapsed
range

When I added this method, I didn't realize it had useful behavior for the case
where a collapsed range is passed in. It turns out it does (it returns markers
that
contain the start/end position in their interior), so I'm adding a comment
explaining this behavior and a test case testing it.

BUG=707867

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

[commit-bot: I haz the power, 2017-07-25 02:30:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/bfc3b0fc01351c264824c22da6af...