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..175e38542fad00a9e804fceb032a8cef43da6cb8
--- /dev/null
+++ b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h
@@ -0,0 +1,64 @@
+// 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> {

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

Should interfaces be on-heap?

I guess not.

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

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.

+ public:
+  DocumentMarkerList();

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

Since the class is purely virtual, can we get rid of its ctor, dtor and .cpp
file?

[rlanday, 2017/04/07 18:22:55]

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.

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

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

allowedMarkerType() and isFooMarkerList() are kind-of duplicate.

allowedMarkerType() itself should be enough.

[rlanday, 2017/04/07 18:22:55]

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.

+  virtual bool isCompositionMarkerList() const;

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

Having type predicate indicates that the design of API isn't good.

+  virtual bool isSpellCheckMarkerList() const;
+  virtual bool isTextMatchMarkerList() const;
+
+  virtual size_t size() const = 0;

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

Do we really need to expose |size()|?

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

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 18:22:56]

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

+  virtual bool empty() const = 0;
+  virtual DocumentMarker* at(size_t index) = 0;
+
+  virtual void add(DocumentMarker*) = 0;
+  virtual void clear() = 0;
+
+  virtual iterator begin() = 0;

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

We should avoid to expose iterator.

+  virtual iterator end() = 0;
+  virtual const_iterator begin() const = 0;
+  virtual const_iterator end() const = 0;
+
+  virtual void appendMarkersToInputList(DocumentMarkerVector* list) const = 0;
+
+  enum class DidCopyMarkerOrNot { DidNotCopyMarker, DidCopyMarker };
+  virtual DidCopyMarkerOrNot copyMarkers(unsigned startOffset,
+                                         int length,
+                                         DocumentMarkerList* dstList,
+                                         int delta) = 0;
+
+  enum class DidRemoveMarkerOrNot { DidNotRemoveMarker, DidRemoveMarker };
+  virtual DidRemoveMarkerOrNot removeMarkers(
+      unsigned startOffset,
+      int length,
+      bool shouldRemovePartiallyOverlappingMarkers) = 0;
+
+  enum class DidShiftMarkerOrNot { DidNotShiftMarker, DidShiftMarker };
+  virtual DidShiftMarkerOrNot shiftMarkers(unsigned offset,
+                                           unsigned oldLength,
+                                           unsigned newLength) = 0;
+
+  DEFINE_INLINE_VIRTUAL_TRACE() {}
+  DISALLOW_COPY_AND_ASSIGN(DocumentMarkerList);
+};
+
+}  // namespace blink
+
+#endif  // DocumentMarkerList_h