Refactor DocumentMarkerController

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

+// 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 DocumentMarkerController;
+
+class DocumentMarkerList : public GarbageCollected<DocumentMarkerList> {
+  WTF_MAKE_NONCOPYABLE(DocumentMarkerList);
+
+ public:
+  explicit DocumentMarkerList(DocumentMarkerController*);
+  using iterator = Member<DocumentMarker>*;
+  using const_iterator = const Member<DocumentMarker>*;
+
+  virtual DocumentMarker::MarkerType allowedMarkerType() const = 0;
+
+  virtual size_t size() const = 0;
+  virtual bool empty() const = 0;
+  virtual DocumentMarker* at(size_t index) = 0;
+
+  virtual void clear() = 0;
+
+  virtual iterator begin() = 0;
+  virtual iterator end() = 0;
+  virtual const_iterator begin() const = 0;
+  virtual const_iterator end() const = 0;
+

[Xiaocheng, 2017/03/17 23:56:33]

I think we should store |m_markers| directly in DocumentMarkerList, so that the
accessor functions don't need to be virtual.

Unless you want different subclasses store their markers with different
structures.

[rlanday, 2017/03/18 01:12:27]

I think one of the ideas behind the refactor was that we might potentially want
to have different data structures, although it's not currently necessary, so we
might just want to do what you're suggesting

+  virtual void appendMarkersToInputList(DocumentMarkerVector* list) const = 0;
+  virtual bool copyMarkers(unsigned startOffset,
+                           int length,
+                           Node* dstNode,
+                           int delta) const = 0;
+  virtual void removeMarkers(unsigned startOffset,
+                             int length,
+                             bool shouldRemovePartiallyOverlappingMarkers,
+                             bool* didRemoveMarker) = 0;
+  virtual bool shiftMarkers(unsigned offset,
+                            unsigned oldLength,
+                            unsigned newLength) = 0;
+
+  DECLARE_VIRTUAL_TRACE();
+
+ protected:
+  struct ShiftMarkerResult {
+    unsigned newStartOffset;
+    unsigned newEndOffset;
+    bool shouldRemoveMarker;
+  };
+
+  static bool startsAfter(const Member<DocumentMarker>&, size_t startOffset);

[Xiaocheng, 2017/03/17 23:56:33]

This function is defined but not used.

+  static bool endsBefore(size_t startOffset, const Member<DocumentMarker>& rhv);

[Xiaocheng, 2017/03/17 23:56:33]

If we store |m_markers| directly in DML, we can also hide |endsBefore| as a
local function in DML.cpp, and expose a member function of DML to return
|std::upper_bound(m_markers.begin(), m_markers.end(), ..., endsBefore)|

+  static bool compareByStart(const Member<DocumentMarker>& lhv,

[Xiaocheng, 2017/03/17 23:56:33]

This should be a local function in EditingMarkerList.cpp.

+                             const Member<DocumentMarker>& rhv);
+
+  ShiftMarkerResult getShiftedMarkerPosition(const DocumentMarker&,

[Xiaocheng, 2017/03/17 23:56:33]

This should be a member function of DocumentMarker

+                                             unsigned offset,
+                                             unsigned oldLength,
+                                             unsigned newLength);
+
+  Member<DocumentMarkerController> m_documentMarkerController;
+};
+
+}  // namespace blink
+
+#endif  // DocumentMarkerList_h