DocumentMarkerList refactor as an interface

DocumentMarkerList refactor as an interface

This is a sketch of what the DocumentMarkerList hierarchy looks like if we pull
all the shared logic out of DocumentMarkerList into a helper class
(DocumentMarkerListUtils). I haven't actually implemented most of the helper
functions but this should give us some idea of what the code will look like if
we implement it this way.

BUG=707867

[rlanday, 2017-04-06 22:58:06]

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

[rlanday, 2017-04-06 23:03:14]

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

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

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

[Xiaocheng, 2017-04-07 00:06:33]

Didn't go through all details. The sketch looks good.

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h
(right):

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:14: : public
GarbageCollectedFinalized<DocumentMarkerList> {
Should interfaces be on-heap?

I guess not.

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:16:
DocumentMarkerList();
Since the class is purely virtual, can we get rid of its ctor, dtor and .cpp
file?

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:22: virtual
DocumentMarker::MarkerType allowedMarkerType() const = 0;
allowedMarkerType() and isFooMarkerList() are kind-of duplicate.

allowedMarkerType() itself should be enough.

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListUtils.h
(right):

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListUtils.h:13:
using MarkerList = HeapVector<Member<DocumentMarker>>;
This is the same as DocumentMarkerVector.

[commit-bot: I haz the power, 2017-04-07 00:54:18]

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

[commit-bot: I haz the power, 2017-04-07 00:54:19]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-04-07 03:16:22]

Let's start with simple interface having minimum members.
The purpose of DMList interface is provide implementation of DMC API.
Thus DMList starts with

interface DMList {
  void clear();
  void addMarker(...);
  void copyMarkres(...); // we may split this member before designing DMList?
  void removeMarkers(...);
  void shiftMarkers(...);
  bool hasMakers(...);
  Vecotr<DM*> computeMakresFor(Node*);
  ... whatever DMC public member need to be redirect to DMList impl.
};

// Two options for DMListImpl
// 1. Containment
// 2. Editor class

// For containment pattern
class GenericDMList {
  public:
    // Functions of DMList
    void removeMarker(...) { ... copy from DMC ... }
  private:
   HeapVector<Member<DM>> markers_;
};

class SpllingMarkerList : public DMList {
 public:
  // Impl of DMList
  void removeMarker(...) { list_->removeMarker(); } // should be in .cpp
  ...
 private:
  Member<GenericDMList> list_;
};

// For Editor pattern
class DMListEditor {
 public:
    // Functions of DMList
    void removeMarker(Vector<Member>* DM, ...) { ... copy from DMC ... }
  
};

class SpllingMarkerList : public DMList {
 public:
  // Impl of DMList
  void removeMarker(...) {
    DMListEditor::removeMarker(list_, ...);
  }
  ...
 private:
  Member<GenericDMList> list_;
};

Both ways have PROS/CONS.

Containment:
PROS: Easy to start
CONS: Hard to share code, e.g. copyMarkers() is high level operation, it can be
implement with add/remove/split functionality

Editor:
PROS: Easy to shard code
CONS: Editor knows detail of list implementation, e.g. passing
Vector<Member<DM>>

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h
(right):

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:23: virtual
bool isCompositionMarkerList() const;
Having type predicate indicates that the design of API isn't good.

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:27: virtual
size_t size() const = 0;
Do we really need to expose |size()|?

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:34: virtual
iterator begin() = 0;
We should avoid to expose iterator.

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListUtils.h
(right):

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListUtils.h:12:
class DocumentMarkerListUtils {
Please avoid to use abbrev name, it should call |Utilities|.
Please avoid to use |Utilities|, it doesn't provide enough information.
And, it says this class is a kitchen sink.

At this time, we can call DocumentMarkerListEditor or Manipulator.

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h (right):

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:50: enum class
State { Invalid, ValidNull, ValidNotNull };
nit: Please add prefix "k" to enum members, e.g. kInvalid, kValidNull, ...

[rlanday, 2017-04-07 18:02:18]

For the containment pattern, I don't see how we can implement the operations
both for a sorted list and an unsorted list in GenericDMList (it seems like any
sort of list class should have an awareness of whether or not it's sorted,
whereas we'd instead be putting that functionality in the MarkerType-specific
classes). So I'm going to try to see if we can come up with an implementation of
the Editor pattern that we're happy with.

[rlanday, 2017-04-07 18:22:56]

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h
(right):

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:14: : public
GarbageCollectedFinalized<DocumentMarkerList> {
On 2017/04/07 at 00:06:33, Xiaocheng wrote:
> Should interfaces be on-heap?
> 
> I guess not.

The subclasses need to inherit from GarbageCollected[Finalized] somehow, so I
think if we don't do it here, we need to add another base class in addition to
the interface, which I don't think really makes sense.

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:16:
DocumentMarkerList();
On 2017/04/07 at 00:06:33, Xiaocheng wrote:
> Since the class is purely virtual, can we get rid of its ctor, dtor and .cpp
file?

The constructor is needed because DISALLOW_COPY_AND_ASSIGN deletes the copy
constructor. This causes the default constructor to not be created unless we
explicitly ask for it.

I believe pure virtual destructors must still have an empty definition.

I believe we're not supposed to define virtual methods in the header file,
therefore we need a .cpp file for the destructor.

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:22: virtual
DocumentMarker::MarkerType allowedMarkerType() const = 0;
On 2017/04/07 at 00:06:33, Xiaocheng wrote:
> allowedMarkerType() and isFooMarkerList() are kind-of duplicate.
> 
> allowedMarkerType() itself should be enough.

Ok. The reason I thought it might be nice to have the isFooMarkerList() methods
is because SpellCheckMarkerList supports multiple marker types and I can see the
cast checks potentially falling out of sync if we add more marker types.

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:27: virtual
size_t size() const = 0;
On 2017/04/07 at 03:16:22, yosin_UTC9 wrote:
> Do we really need to expose |size()|?

It looks like we don't need it in the refactored version of
DocumentMarkerController, so I guess not:
https://codereview.chromium.org/2723663002

https://codereview.chromium.org/2805553003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:27: virtual
size_t size() const = 0;
On 2017/04/07 at 03:16:22, yosin_UTC9 wrote:
> Do we really need to expose |size()|?

It looks like we don't need it in the refactored version of
DocumentMarkerController, so I guess not:
https://codereview.chromium.org/2723663002

[rlanday, 2017-04-07 21:44:35]

I responded to the comments and filled in the unimplemented methods, how does
this look now?

[rlanday, 2017-04-07 21:46:30]

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:15:
getPosOfFirstMarkerNotEndingBeforeInSortedList(
This method is only used once in this file and probably won't be needed anywhere
else...maybe I should just make it a lambda?

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:19:
static void appendMarkersToInputList(const DocumentMarkerVector& markerList,
This method isn't really an editing method...does this not belong here?

[Xiaocheng, 2017-04-07 23:42:24]

I prefer the editor pattern.

I think we don't need to spend too much time polishing the implementation
details here. Instead, we can land things in this order:

1. DMLList interface
2. DMLEditor with unit tests, with DMLEditor no longer depend on impls of DML
(in which case it might be better named DMVectorEditor...)
3. Each specific DML subclasses with unit tests

At any time, keep an all-in-one patch for reference so that we are sure that
everything landed is necessary.

The only blocker seems to be DMC::copyMarkers. We should clean it up right now
before landing the DMLList interface, so that the interface doesn't need further
cleanup.

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:17: using
iterator = Member<DocumentMarker>*;
These iterator types are unused.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:33: virtual
DidCopyMarkerOrNot copyMarkers(unsigned startOffset,
Since we don't want to keep a clone-ish method for DocumentMarker, can we clean
it up first?

That is, change it into DML::moveMarkers(start, length, dst) and use it in
SplitTextNodeCommand::doApply() and doUnapply(), before landing this interface.

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:15:
getPosOfFirstMarkerNotEndingBeforeInSortedList(
On 2017/04/07 at 21:46:29, rlanday wrote:
> This method is only used once in this file and probably won't be needed
anywhere else...maybe I should just make it a lambda?

If it's used only once and internally, making it a lambda is better.

It's always good to keep header files small.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:19:
static void appendMarkersToInputList(const DocumentMarkerVector& markerList,
On 2017/04/07 at 21:46:29, rlanday wrote:
> This method isn't really an editing method...does this not belong here?

I think this method doesn't do much. We should make the callers call
Vector::appendVector directly.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:22:
static DocumentMarkerList::DidCopyMarkerOrNot copyUnsortedMarkers(
It's better to use output to an empty DMV so that this function does not depend
on DML.

Btw, I think it's better to make it something like moveSorted/UnsortedMarkers.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:60:
static DocumentMarkerList::DidCopyMarkerOrNot copyMarkersHelper(
It doesn't make too much sense to introduce a static private function.

Just hide it in an anonymous namespace in the .cpp file.

[rlanday, 2017-04-11 01:08:59]

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:15:
getPosOfFirstMarkerNotEndingBeforeInSortedList(
On 2017/04/07 at 23:42:24, Xiaocheng wrote:
> On 2017/04/07 at 21:46:29, rlanday wrote:
> > This method is only used once in this file and probably won't be needed
anywhere else...maybe I should just make it a lambda?
> 
> If it's used only once and internally, making it a lambda is better.
> 
> It's always good to keep header files small.

I stand corrected, it's also used in
TextMatchMarkerList::SetTextMatchMarkersActive()

[rlanday, 2017-04-11 01:12:36]

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:17: using
iterator = Member<DocumentMarker>*;
On 2017/04/07 at 23:42:24, Xiaocheng wrote:
> These iterator types are unused.

They're used in some of the subclass implementations, but I suppose maybe we
shouldn't expose this implementation detail...

[Xiaocheng, 2017-04-11 01:26:02]

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:17: using
iterator = Member<DocumentMarker>*;
On 2017/04/11 at 01:12:36, rlanday wrote:
> On 2017/04/07 at 23:42:24, Xiaocheng wrote:
> > These iterator types are unused.
> 
> They're used in some of the subclass implementations, but I suppose maybe we
shouldn't expose this implementation detail...

I'm not sure if any class (DMC?) needs to know that DML and its subclasses store
the markers sequentially with a Vector. Seems better not.

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:15:
getPosOfFirstMarkerNotEndingBeforeInSortedList(
On 2017/04/11 at 01:08:59, rlanday wrote:
> On 2017/04/07 at 23:42:24, Xiaocheng wrote:
> > On 2017/04/07 at 21:46:29, rlanday wrote:
> > > This method is only used once in this file and probably won't be needed
anywhere else...maybe I should just make it a lambda?
> > 
> > If it's used only once and internally, making it a lambda is better.
> > 
> > It's always good to keep header files small.
> 
> I stand corrected, it's also used in
TextMatchMarkerList::SetTextMatchMarkersActive()

In this case, I don't have a strong preference.

I slightly prefer hiding it because of its long name.

[rlanday, 2017-04-11 01:28:32]

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

[commit-bot: I haz the power, 2017-04-11 01:29: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-11 01:39:37]

It is too early to think about implementation details, and to make patch small,
please exclude specific implementation, and just an implementation for
GenericDocumentMarkerListImpl until List API to have robust shape.

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:9:
DocumentMarkerList::DocumentMarkerList() {}
nit: s/{}/= default;/

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:11:
DocumentMarkerList::~DocumentMarkerList() {}
nit: s/{}/= default;/

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:17: using
iterator = Member<DocumentMarker>*;
On 2017/04/11 at 01:12:36, rlanday wrote:
> On 2017/04/07 at 23:42:24, Xiaocheng wrote:
> > These iterator types are unused.
> 
> They're used in some of the subclass implementations, but I suppose maybe we
shouldn't expose this implementation detail...

Yes, we should not have |iterator| in an interface declaration.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:22: virtual
DocumentMarker::MarkerType allowedMarkerType() const = 0;
Do we really need this?
We should avoid dynamic type checking of list.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:25: virtual
DocumentMarker* at(size_t index) = 0;
We don't want to expose random access of DocumentMarker.
Is it better to call DocumentMarkerSet instead of DocumentMarkerList?
It seems some people thinks List provides random access feature....

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:27: virtual
void add(DocumentMarker*) = 0;
Should we call |addInternal()| or another to indicate this API is not for
generic use.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:30: virtual
void appendMarkersToInputList(DocumentMarkerVector* list) const = 0;
We don't need this as API. We can implement this by using |add()|.
If we really need this, may be performance reason, we'll add it when need.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:33: virtual
DidCopyMarkerOrNot copyMarkers(unsigned startOffset,
It is OK to use |bool| return value. Since we should have describe about |bool|
value within API member description.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:39: virtual
DidRemoveMarkerOrNot removeMarkers(
It is OK to use |bool| return value. Since we should have describe about |bool|
value within API member description.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:42: bool
shouldRemovePartiallyOverlappingMarkers) = 0;
Please use |enum class| instead of |bool|.

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:182:
DocumentMarkerList::DidCopyMarkerOrNot didCopyMarker =
Let's add |DCHECK_GT(length, 0)|

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:189:
if (marker.endOffset() >= startOffset &&
Let's use early-continue to reduce indentation.

if (marker.endOffset() < startOffset || marker.startOffset() > endOffset)
  continue;

[rlanday, 2017-04-11 02:02:49]

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

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:22: virtual
DocumentMarker::MarkerType allowedMarkerType() const = 0;
On 2017/04/11 at 01:39:37, yosin_UTC9 wrote:
> Do we really need this?
> We should avoid dynamic type checking of list.

We can get rid of this if we're OK with the casts being unchecked (are they only
checked in ASAN/DCHECK-enabled builds?)

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:25: virtual
DocumentMarker* at(size_t index) = 0;
On 2017/04/11 at 01:39:37, yosin_UTC9 wrote:
> We don't want to expose random access of DocumentMarker.
> Is it better to call DocumentMarkerSet instead of DocumentMarkerList?
> It seems some people thinks List provides random access feature....

To me, "set" makes this sound like it's a hash set, so I think list is more
accurate

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:27: virtual
void add(DocumentMarker*) = 0;
On 2017/04/11 at 01:39:37, yosin_UTC9 wrote:
> Should we call |addInternal()| or another to indicate this API is not for
generic use.

What do you mean by "not for generic use"? All the methods on this class, add()
included, are going to be called by DocumentMarkerController.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:30: virtual
void appendMarkersToInputList(DocumentMarkerVector* list) const = 0;
On 2017/04/11 at 01:39:37, yosin_UTC9 wrote:
> We don't need this as API. We can implement this by using |add()|.
> If we really need this, may be performance reason, we'll add it when need.

This is for getting the markers out, not putting them in. The other way we could
do this is to just have a method that returns a vector of the DocumentMarkers,
but then we lose some efficiency if we want to get markers from several
DocumentMarkerLists since we have to copy the output.

https://codereview.chromium.org/2805553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:42: bool
shouldRemovePartiallyOverlappingMarkers) = 0;
On 2017/04/11 at 01:39:37, yosin_UTC9 wrote:
> Please use |enum class| instead of |bool|.

I think this is a moot point, we're getting rid of this flag in
https://codereview.chromium.org/2806683002

[rlanday, 2017-04-11 02:59:10]

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

https://codereview.chromium.org/2805553003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:170:
if (marker.StartOffset() <= end_offset) {
This is redundant, I will remove

[commit-bot: I haz the power, 2017-04-11 03:05:51]

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

[commit-bot: I haz the power, 2017-04-11 03:05:52]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-04-11 09:42:20]

I propose as following way to have small patches and keep no behavior changes.

1. Introduce DocumentMarkerListEditor which has members of
DocumentMarkerController
  in-place change of DMC
2. Move DocumentMarkerListEditor to DocumentMarkerListEditor.cpp
3. Introduce DocumentMarkerList interface and GenericDocumentMarkerListImpl
  GenericDocumentMarkerListImpl calls DocumentMakrerListEditor
  GDMLI holds markers in HeapVector<Member<DM>> as DMC.
  void GenericDocumentMarkerListImpl::removeMarkers(...) {
    DocumentMarkerListEditor::removeMakers(&m_markers, ...);
  }
4. Implement specialized DMLImpl for each DMType.

[rlanday, 2017-04-11 18:21:01]

On 2017/04/11 at 09:42:20, yosin wrote:
> I propose as following way to have small patches and keep no behavior changes.
> 
> 1. Introduce DocumentMarkerListEditor which has members of
DocumentMarkerController
>   in-place change of DMC
> 2. Move DocumentMarkerListEditor to DocumentMarkerListEditor.cpp
> 3. Introduce DocumentMarkerList interface and GenericDocumentMarkerListImpl
>   GenericDocumentMarkerListImpl calls DocumentMakrerListEditor
>   GDMLI holds markers in HeapVector<Member<DM>> as DMC.
>   void GenericDocumentMarkerListImpl::removeMarkers(...) {
>     DocumentMarkerListEditor::removeMakers(&m_markers, ...);
>   }
> 4. Implement specialized DMLImpl for each DMType.

I don't like step 1 since I already have a CL up to refactor
DocumentMarkerController on top of the new lists we're adding, and it was fairly
involved to do:
https://codereview.chromium.org/2723663002

I don't think it makes sense to implement DMC on top of DocumentMarkerListEditor
because DocumentMarkerListEditor is for implementing the DMLImpls, and won't
necessarily do exactly what we want in DMC, and also because I don't think it
makes sense to do this, only to immediately re-refactor DMC on top of the
DMLImpls.

I think we should do the following:
1. Introduce DocumentMarkerListEditor with a bunch of test cases showing that it
works properly
2. Introduce DocumentMarkerList interface and GenericDocumentMarkerListImpl
3. Implement specialized DMLImpl classes with test cases
4. Add more test cases for DocumentMarkerController verifying the existing
behavior
5. Migrate DocumentMarkerController to use the new DMLImpl classes

[rlanday, 2017-04-11 18:35:32]

On 2017/04/11 at 18:21:01, rlanday wrote:
> On 2017/04/11 at 09:42:20, yosin wrote:
> > I propose as following way to have small patches and keep no behavior
changes.
> > 
> > 1. Introduce DocumentMarkerListEditor which has members of
DocumentMarkerController
> >   in-place change of DMC
> > 2. Move DocumentMarkerListEditor to DocumentMarkerListEditor.cpp
> > 3. Introduce DocumentMarkerList interface and GenericDocumentMarkerListImpl
> >   GenericDocumentMarkerListImpl calls DocumentMakrerListEditor
> >   GDMLI holds markers in HeapVector<Member<DM>> as DMC.
> >   void GenericDocumentMarkerListImpl::removeMarkers(...) {
> >     DocumentMarkerListEditor::removeMakers(&m_markers, ...);
> >   }
> > 4. Implement specialized DMLImpl for each DMType.
> 
> I don't like step 1 since I already have a CL up to refactor
DocumentMarkerController on top of the new lists we're adding, and it was fairly
involved to do:
> https://codereview.chromium.org/2723663002
> 
> I don't think it makes sense to implement DMC on top of
DocumentMarkerListEditor because DocumentMarkerListEditor is for implementing
the DMLImpls, and won't necessarily do exactly what we want in DMC, and also
because I don't think it makes sense to do this, only to immediately re-refactor
DMC on top of the DMLImpls.
> 
> I think we should do the following:
> 1. Introduce DocumentMarkerListEditor with a bunch of test cases showing that
it works properly
> 2. Introduce DocumentMarkerList interface and GenericDocumentMarkerListImpl
> 3. Implement specialized DMLImpl classes with test cases
> 4. Add more test cases for DocumentMarkerController verifying the existing
behavior
> 5. Migrate DocumentMarkerController to use the new DMLImpl classes

Actually, the DocumentMarkerList interface is needed before we can implement
DocumentMarkerListEditor (moveMarkers() takes a pointer to a DocumentMarkerList
to move the markers to) so I think the plan should look something like this:

1. Introduce DocumentMarkerList interface
2. Introduce DocumentMarkerListEditor with a bunch of test cases showing that it
works properly
3. Introduce GenericDocumentMarkerListImpl with tests
4. Implement specialized DMLImpl classes with test cases
5. Add more test cases for DocumentMarkerController verifying the existing
behavior
6. Migrate DocumentMarkerController to use the new DMLImpl classes

[rlanday, 2017-04-11 20:47:26]

I just talked to xiaochengh, we don't really see the need for
GenericDocumentMarkerListImpl. It would introduce some asymmetry in that it
would either implement a sorted list or an unsorted list, and then the
implementations that need the opposite choice would still be overriding those
methods. Plus, I think we decided we were trying to avoid inheritance where it's
not needed. So right now my plan looks like:

1. Introduce DocumentMarkerList interface
2. Introduce DocumentMarkerListEditor with a bunch of test cases showing that it
works properly
3. Implement specialized DMLImpl classes with test cases
4. Add more test cases for DocumentMarkerController verifying the existing
behavior
5. Migrate DocumentMarkerController to use the new DMLImpl classes

[yosin_UTC9, 2017-04-12 01:43:30]

On 2017/04/11 at 20:47:26, rlanday wrote:
> I just talked to xiaochengh, we don't really see the need for
GenericDocumentMarkerListImpl. It would introduce some asymmetry in that it
would either implement a sorted list or an unsorted list, and then the
implementations that need the opposite choice would still be overriding those
methods. Plus, I think we decided we were trying to avoid inheritance where it's
not needed. So right now my plan looks like:
> 
> 1. Introduce DocumentMarkerList interface
> 2. Introduce DocumentMarkerListEditor with a bunch of test cases showing that
it works properly
> 3. Implement specialized DMLImpl classes with test cases
> 4. Add more test cases for DocumentMarkerController verifying the existing
behavior
> 5. Migrate DocumentMarkerController to use the new DMLImpl classes

I'm not fan of this approach. Background of DMList interface is unclear for
readers.
This approach is create new thing then replace current. Until step 5, this
approach makes us work on non-real world.

>It would introduce some asymmetry in that it would either implement a sorted
list or an unsorted list, and then the implementations that need the opposite
choice would still be overriding those methods
I don't understand this.

One of the objective of introducing DMList interface is isolation of changes on
each DM type.
Since we don't want to migrate all DM type lists in specialized one,
GenericDMListImpl provides
default impl.

Marker list can be implemented in three phases:

# Phase 0:
All DM type lists are GenericDMListImpl. 

# Phase 1:
When we want to have optimized version of Spelling Marker. Perpetration is:
SpellingMarkerList::AddMarker(DM* marker) { DMLEditor::AddMarker(marker); }

# Phase 2:
Then, specialize for SpellingMarkerList
SpellingMarkerList::AddMarker(DM* marker) { spelling specific implementation }

This is incrementally change current implementation and we can implement each DM
type in parallel and independent.

>moveMarkers() takes a pointer to a DocumentMarkerList to move the markers to
DMC::MoveMarkers() can implement as following pseudo code:

DMC::MoveMarkers(Node* src_node, int length, Node* dst_node) {
  MarkerListtSet& src_sets = GetMarkerListSetFor(src_node);
  MarkesrListSet& dst_sets = GetOrCreateMarkerListSetFor(dst_node);
  for (each list in src_set) {
    DM& before_maker = DMListEditor::SplitMarker(src_list, length);
    DMLEditor::AddMarker(dst_list, before_marker);
}