DocumentMarkerList refactor as an interface

third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h

Index: third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h
diff --git a/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h
new file mode 100644
index 0000000000000000000000000000000000000000..8de4090de5d15a1b4ff497d60f7f6f04cde97cc6
--- /dev/null
+++ b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h
@@ -0,0 +1,55 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef DocumentMarkerList_h
+#define DocumentMarkerList_h
+
+#include "core/editing/markers/DocumentMarker.h"
+#include "platform/heap/Handle.h"
+
+namespace blink {
+
+class CORE_EXPORT DocumentMarkerList
+    : public GarbageCollectedFinalized<DocumentMarkerList> {
+ public:
+  DocumentMarkerList();
+  using iterator = Member<DocumentMarker>*;

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

These iterator types are unused.

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

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

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.

[yosin_UTC9, 2017/04/11 01:39:37]

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

+  using const_iterator = const Member<DocumentMarker>*;
+
+  virtual ~DocumentMarkerList();
+
+  virtual DocumentMarker::MarkerType allowedMarkerType() const = 0;

[yosin_UTC9, 2017/04/11 01:39:37]

Do we really need this?
We should avoid dynamic type checking of list.

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

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

+
+  virtual bool empty() const = 0;
+  virtual DocumentMarker* at(size_t index) = 0;

[yosin_UTC9, 2017/04/11 01:39:37]

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

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

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

+
+  virtual void add(DocumentMarker*) = 0;

[yosin_UTC9, 2017/04/11 01:39:37]

Should we call |addInternal()| or another to indicate this API is not for
generic use.

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

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

+  virtual void clear() = 0;
+
+  virtual void appendMarkersToInputList(DocumentMarkerVector* list) const = 0;

[yosin_UTC9, 2017/04/11 01:39:37]

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.

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

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.

+
+  enum class DidCopyMarkerOrNot { kDidNotCopyMarker, kDidCopyMarker };
+  virtual DidCopyMarkerOrNot copyMarkers(unsigned startOffset,

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

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.

[yosin_UTC9, 2017/04/11 01:39:37]

It is OK to use |bool| return value. Since we should have describe about |bool|
value within API member description.

+                                         int length,
+                                         DocumentMarkerList* dstList,
+                                         int delta) = 0;
+
+  enum class DidRemoveMarkerOrNot { kDidNotRemoveMarker, kDidRemoveMarker };
+  virtual DidRemoveMarkerOrNot removeMarkers(

[yosin_UTC9, 2017/04/11 01:39:37]

It is OK to use |bool| return value. Since we should have describe about |bool|
value within API member description.

+      unsigned startOffset,
+      int length,
+      bool shouldRemovePartiallyOverlappingMarkers) = 0;

[yosin_UTC9, 2017/04/11 01:39:37]

Please use |enum class| instead of |bool|.

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

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

+
+  enum class DidShiftMarkerOrNot { kDidNotShiftMarker, kDidShiftMarker };
+  virtual DidShiftMarkerOrNot shiftMarkers(unsigned offset,
+                                           unsigned oldLength,
+                                           unsigned newLength) = 0;
+
+  DEFINE_INLINE_VIRTUAL_TRACE() {}
+  DISALLOW_COPY_AND_ASSIGN(DocumentMarkerList);
+};
+
+}  // namespace blink
+
+#endif  // DocumentMarkerList_h