Add TextMatchMarkerList in preparation for DocumentMarkerController refactor

Add TextMatchMarkerList in preparation for DocumentMarkerController refactor

I have this patch committed locally on top of the patch that adds
SpellCheckMarkerList:
https://codereview.chromium.org/2770413003

although I'm not sure there are any actual dependencies on that. This marker
list is a little more involved than the others because I'm introducing a
subclass of DocumentMarker called TextMatchMarker to hold the rendered-rect
information that TextMatch markers need. Currently this information is stored in
RenderedDocumentMarker, which I'm actually leaving in place in this patch since
we're not switching DocumentMarkerController over to use the new lists yet. So
that code is still creating TextMatch markers as DocumentMarkers directly (not
TextMatchMarkers) and calling RenderedDocumentMarker::create() to get a
RenderedDocumentMarker.

To make my TextMatchMarker subclass work properly, I had to make
DocumentMarker's copy constructor protected and force callers to use a new
clone() method. This required some small changes to DocumentMarkerController.

Right now we support having different data on different types of DocumentMarkers
by having different "DocumentMarkerDetails" subclasses we attach to them; we may
want to rewrite DocumentMarker at some point so the other markers are subclasses
themselves like TextMatchMarker is in this CL.

BUG=707867

[rlanday, 2017-03-29 00:11:50]

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

[rlanday, 2017-03-29 00:11:57]

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

[rlanday, 2017-03-29 00:11:57]

rlanday@chromium.org changed required reviewers:
+ yosin@chromium.org

[commit-bot: I haz the power, 2017-03-29 00:12: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-03-29 00:25:30]

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

[commit-bot: I haz the power, 2017-03-29 00:25:31]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rlanday, 2017-03-29 01:17:45]

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

[commit-bot: I haz the power, 2017-03-29 01:18:28]

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-29 03:10:11]

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

[commit-bot: I haz the power, 2017-03-29 03:10:11]

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_...)

[rlanday, 2017-03-29 22:58:09]

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

[commit-bot: I haz the power, 2017-03-29 22:58:59]

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

[Xiaocheng, 2017-03-29 23:20:57]

https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):

https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:137: virtual
DocumentMarker* clone() const;
The introduction of clone() seems pretty light weight and orthogonal to
everything else.

Let's split it into another patch, which seems landable before all other
patches.

Let's also use it in copyMarkers() so that we can get rid of the multiple
reference issue.

https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListTest.cpp
(right):

https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListTest.cpp:15:
class TextMatchMarkerListTest : public ::testing::Test {
You may inherit from EditingTestBase (in editing/EditingTestBase.h) to get rid
of standard details like page and HTML setup.

https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListTest.cpp:88: 
Do we want to inspect the actual value of the rect?

[rlanday, 2017-03-29 23:54:56]

https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListTest.cpp
(right):

https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListTest.cpp:88: 
On 2017/03/29 at 23:20:57, Xiaocheng wrote:
> Do we want to inspect the actual value of the rect?

I don't know if it's 100% predictable, the rendered rect test in
DocumentMarkerController doesn't check the exact values...we could check that
they're both non-zero at least, I think that might be helpful

[Xiaocheng, 2017-03-29 23:57:55]

On 2017/03/29 at 23:54:56, rlanday wrote:
>
https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListTest.cpp
(right):
> 
>
https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListTest.cpp:88:

> On 2017/03/29 at 23:20:57, Xiaocheng wrote:
> > Do we want to inspect the actual value of the rect?
> 
> I don't know if it's 100% predictable, the rendered rect test in
DocumentMarkerController doesn't check the exact values...we could check that
they're both non-zero at least, I think that might be helpful

sgtm.

[commit-bot: I haz the power, 2017-03-30 01:10:04]

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

[commit-bot: I haz the power, 2017-03-30 01:10:05]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-03-30 20:43:10]

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

[rlanday, 2017-03-30 20:43:22]

On 2017/03/29 at 23:57:55, xiaochengh wrote:
> On 2017/03/29 at 23:54:56, rlanday wrote:
> >
https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
> > File
third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListTest.cpp
(right):
> > 
> >
https://codereview.chromium.org/2784753002/diff/40001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListTest.cpp:88: 
> > On 2017/03/29 at 23:20:57, Xiaocheng wrote:
> > > Do we want to inspect the actual value of the rect?
> > 
> > I don't know if it's 100% predictable, the rendered rect test in
DocumentMarkerController doesn't check the exact values...we could check that
they're both non-zero at least, I think that might be helpful
> 
> sgtm.

Updated

[commit-bot: I haz the power, 2017-03-30 20:44: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-03-30 21:12:07]

Looks good with a nit.

I have an idea about further polishing the DML class, though.

https://codereview.chromium.org/2784753002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/TextMatchMarkerList.cpp
(right):

https://codereview.chromium.org/2784753002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/TextMatchMarkerList.cpp:48: for
(auto it = getPosOfFirstMarkerNotEndingBefore(startOffset);
I'm still not quite a fan of |getPosOf...|, but didn't have a good idea to
improve it.

Now I think I got it. How about something like:

class DML {

  class MarkerRange {
    iterator begin();
    iterator end();
    iterator m_begin;
    iterator m_end;
  };

  MarkerRange markersCoveredBy(startOffset, endOffset) {
    if (!markersAreSorted())
      return MarkerRange(m_markers.begin(), m_markers.end());
    return MarkerRange(lower_bound(...), upper_bound(...)); // With whatever
optimization you like
  }

  MarkerRange markersOverlapping(startOffset, endOffset) { // maybe a third
|bool| param for overlapping only at endpoints?
    ...
  }

};

TextMatchMarkerList::setTextMatchMarkersActive(...) {
  for (marker : markersOverlapping(startOffset, endOffset)) {
    marker.setActiveMatch(active);
    docDirty = true;
  }
}

It's also going to make it easier for DML member functions to utilize the sorted
order.

I think we still have (plenty of?) time polishing the prerequisite patches.
WDYT?

https://codereview.chromium.org/2784753002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/TextMatchMarkerList.h
(right):

https://codereview.chromium.org/2784753002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/TextMatchMarkerList.h:30: bool
setTextMatchMarkersActive(unsigned startOffset,
nit: please use |enum class| return type for better clarity.

[rlanday, 2017-03-30 23:05:03]

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

[commit-bot: I haz the power, 2017-03-30 23:05:21]

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 00:56:31]

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

[commit-bot: I haz the power, 2017-03-31 00:56:32]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-03-31 17:19:13]

On 2017/03/30 at 21:12:07, xiaochengh wrote:
> Looks good with a nit.
> 
> I have an idea about further polishing the DML class, though.
> 
>
https://codereview.chromium.org/2784753002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/markers/TextMatchMarkerList.cpp
(right):
> 
>
https://codereview.chromium.org/2784753002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/markers/TextMatchMarkerList.cpp:48: for
(auto it = getPosOfFirstMarkerNotEndingBefore(startOffset);
> I'm still not quite a fan of |getPosOf...|, but didn't have a good idea to
improve it.
> 
> Now I think I got it. How about something like:
> 
> class DML {
> 
>   class MarkerRange {
>     iterator begin();
>     iterator end();
>     iterator m_begin;
>     iterator m_end;
>   };
> 
>   MarkerRange markersCoveredBy(startOffset, endOffset) {
>     if (!markersAreSorted())
>       return MarkerRange(m_markers.begin(), m_markers.end());
>     return MarkerRange(lower_bound(...), upper_bound(...)); // With whatever
optimization you like
>   }
> 
>   MarkerRange markersOverlapping(startOffset, endOffset) { // maybe a third
|bool| param for overlapping only at endpoints?
>     ...
>   }
> 
> };
> 
> TextMatchMarkerList::setTextMatchMarkersActive(...) {
>   for (marker : markersOverlapping(startOffset, endOffset)) {
>     marker.setActiveMatch(active);
>     docDirty = true;
>   }
> }
> 
> It's also going to make it easier for DML member functions to utilize the
sorted order.
> 
> I think we still have (plenty of?) time polishing the prerequisite patches.
WDYT?

It looks like we could use it for copyMarkers(). The current implementations of
removeMarkers() and shiftMarkers() work by copying the whole list, so they can't
really benefit from these helper methods unless we decide to rewrite them for
even better performance. So I think we should punt on this for now.

[Xiaocheng, 2017-03-31 18:11:26]

lgtm

[rlanday, 2017-04-03 18:55:58]

Description was changed from

==========
Add TextMatchMarkerList in preparation for DocumentMarkerController refactor

I have this patch committed locally on top of the patch that adds
SpellCheckMarkerList:
https://codereview.chromium.org/2770413003

although I'm not sure there are any actual dependencies on that. This marker
list is a little more involved than the others because I'm introducing a
subclass of DocumentMarker called TextMatchMarker to hold the rendered-rect
information that TextMatch markers need. Currently this information is stored in
RenderedDocumentMarker, which I'm actually leaving in place in this patch since
we're not switching DocumentMarkerController over to use the new lists yet. So
that code is still creating TextMatch markers as DocumentMarkers directly (not
TextMatchMarkers) and calling RenderedDocumentMarker::create() to get a
RenderedDocumentMarker.

To make my TextMatchMarker subclass work properly, I had to make
DocumentMarker's copy constructor protected and force callers to use a new
clone() method. This required some small changes to DocumentMarkerController.

Right now we support having different data on different types of DocumentMarkers
by having different "DocumentMarkerDetails" subclasses we attach to them; we may
want to rewrite DocumentMarker at some point so the other markers are subclasses
themselves like TextMatchMarker is in this CL.
==========

to

==========
Add TextMatchMarkerList in preparation for DocumentMarkerController refactor

I have this patch committed locally on top of the patch that adds
SpellCheckMarkerList:
https://codereview.chromium.org/2770413003

although I'm not sure there are any actual dependencies on that. This marker
list is a little more involved than the others because I'm introducing a
subclass of DocumentMarker called TextMatchMarker to hold the rendered-rect
information that TextMatch markers need. Currently this information is stored in
RenderedDocumentMarker, which I'm actually leaving in place in this patch since
we're not switching DocumentMarkerController over to use the new lists yet. So
that code is still creating TextMatch markers as DocumentMarkers directly (not
TextMatchMarkers) and calling RenderedDocumentMarker::create() to get a
RenderedDocumentMarker.

To make my TextMatchMarker subclass work properly, I had to make
DocumentMarker's copy constructor protected and force callers to use a new
clone() method. This required some small changes to DocumentMarkerController.

Right now we support having different data on different types of DocumentMarkers
by having different "DocumentMarkerDetails" subclasses we attach to them; we may
want to rewrite DocumentMarker at some point so the other markers are subclasses
themselves like TextMatchMarker is in this CL.

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