Refactor DocumentMarkerController

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

Index: third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
diff --git a/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
index 060c6b305b1149c465c58af5e5a460c015493c06..634342990b91c67eb77ff92d87c47f864ab44986 100644
--- a/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
+++ b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
@@ -40,16 +40,18 @@
 
 namespace blink {
 
+class CompositionMarkerList;
+class DocumentMarkerList;
+class GrammarMarkerList;
 class Node;
-class RenderedDocumentMarker;
-class Text;
+class SpellingMarkerList;
+class TextMatchMarkerList;
 
 class MarkerRemoverPredicate final {
  public:
   explicit MarkerRemoverPredicate(const Vector<String>& words);
   bool operator()(const DocumentMarker&, const Text&) const;
 
- private:

[Xiaocheng, 2017/03/21 19:12:11]

Irrelevant change.

[rlanday, 2017/03/21 19:49:12]

This is necessary because I'm changing the implementation of removeMarkers(const
MarkerRemoverPredicate&) to pass m_words to a method on SpellCheckMarkerList

   Vector<String> m_words;
 };
 
@@ -63,6 +65,7 @@ class CORE_EXPORT DocumentMarkerController final
   explicit DocumentMarkerController(Document&);
 
   void clear();
+  void addMarker(Node*, DocumentMarker*);
   void addMarker(const Position& start,
                  const Position& end,
                  DocumentMarker::MarkerType,
@@ -113,7 +116,7 @@ class CORE_EXPORT DocumentMarkerController final
   // Returns true if markers within a range defined by a node, |startOffset| and
   // |endOffset| are found.
   bool setMarkersActive(Node*, unsigned startOffset, unsigned endOffset, bool);
-  bool hasMarkers(Node* node) const { return m_markers.contains(node); }
+  bool hasMarkers(Node*) const;
 
   DocumentMarkerVector markersFor(
       Node*,
@@ -122,9 +125,9 @@ class CORE_EXPORT DocumentMarkerController final
                                       DocumentMarker::MarkerTypes);
   DocumentMarkerVector markers();
   Vector<IntRect> renderedRectsForMarkers(DocumentMarker::MarkerType);
-  void updateMarkerRenderedRectIfNeeded(const Node&, RenderedDocumentMarker&);
+
   void invalidateRectsForAllMarkers();
-  void invalidateRectsForMarkersInNode(const Node&);
+  void invalidateRectsForMarkersInNode(Node&);
 
   DECLARE_TRACE();
 
@@ -139,23 +142,65 @@ class CORE_EXPORT DocumentMarkerController final
                               unsigned newLength) final;
 
  private:
-  void addMarker(Node*, const DocumentMarker&);
-
-  using MarkerList = HeapVector<Member<RenderedDocumentMarker>>;
-  using MarkerLists =
-      HeapVector<Member<MarkerList>, DocumentMarker::MarkerTypeIndexesCount>;
-  using MarkerMap = HeapHashMap<WeakMember<const Node>, Member<MarkerLists>>;
-  void mergeOverlapping(MarkerList*, RenderedDocumentMarker*);
-  bool possiblyHasMarkers(DocumentMarker::MarkerTypes);
-  void removeMarkersFromList(MarkerMap::iterator, DocumentMarker::MarkerTypes);
+  template <typename MarkerListType>
+  using MarkerMap = HeapHashMap<WeakMember<Node>, Member<MarkerListType>>;
+
+  template <typename MarkerListType>
+  void addIntoMarkerMap(MarkerMap<MarkerListType>* markerMap,

[Xiaocheng, 2017/03/21 19:12:11]

Hmm... I forgot that templated member functions must be inlined. And the callers
of these functions still look quite lengthy.

A better idea seems to be storing the marker lists simply as
|MarkerMap<DocumentMarkerList>|, and let polymorphism handle everything. Sketch:

MarkerMap<DocumentMarkerList> m_spelling;
MarkerMap<DocumentMarkerList> m_grammar;
...

MarkerMap<DocumentMarkerList>& DMC::markerMapFor(type) {
  switch (type) {
    case DocumentMarker::Spelling:
      return m_spelling;
    case ...
      ...
  }
  NOTREACHED();
}

DMC::addMarker(node, newMarker) {
  ...
  MarkerMap<DMList>& markerMap = markerMapFor(newMarker->type());
  auto it = markerMap.find(...)
  ...
  it->value->insert(newMarker);
}

In this way we don't need to forward declare the subclasses of
DocumentMarkerList, either.

[rlanday, 2017/03/21 19:49:12]

We can try that, but then we'll have to cast the marker lists to call certain
methods

+                        Node* node,
+                        DocumentMarker* newMarker) {
+    auto it = markerMap->find(node);
+    if (it == markerMap->end()) {
+      markerMap->insert(node, new MarkerListType(this));
+      it = markerMap->find(node);
+    }
+    it->value->insert(newMarker);
+  }
+
+  template <typename MarkerListType>
+  void shiftMarkersInNode(MarkerMap<MarkerListType>* markerMap,
+                          CharacterData* node,
+                          unsigned offset,
+                          unsigned oldLength,
+                          unsigned newLength,
+                          bool* didShiftMarker) {
+    auto it = markerMap->find(node);
+    if (it != markerMap->end()) {
+      *didShiftMarker = it->value->shiftMarkers(offset, oldLength, newLength) ||
+                        *didShiftMarker;
+    }
+  }
+
+  template <typename MarkerListType>
+  void removeMarkersInRangeFromNode(MarkerMap<MarkerListType>* markerMap,
+                                    Node* node,
+                                    unsigned startOffset,
+                                    int length,
+                                    RemovePartiallyOverlappingMarkerOrNot
+                                        shouldRemovePartiallyOverlappingMarker,
+                                    bool* docDirty) {
+    auto it = markerMap->find(node);
+    if (it != markerMap->end()) {
+      it->value->removeMarkers(startOffset, length,
+                               shouldRemovePartiallyOverlappingMarker,
+                               docDirty);
+      if (it->value->empty())
+        markerMap->remove(node);
+    }
+  }
+
+  HeapVector<Member<DocumentMarkerList>> getMarkerListsForNode(
+      Node*,
+      DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers());
   void removeMarkers(TextIterator&,
                      DocumentMarker::MarkerTypes,
                      RemovePartiallyOverlappingMarkerOrNot);
 
-  MarkerMap m_markers;
-  // Provide a quick way to determine whether a particular marker type is absent
-  // without going through the map.
-  DocumentMarker::MarkerTypes m_possiblyExistingMarkerTypes;
+  MarkerMap<SpellingMarkerList> m_spelling;
+  MarkerMap<GrammarMarkerList> m_grammar;
+  MarkerMap<TextMatchMarkerList> m_textMatches;
+  MarkerMap<CompositionMarkerList> m_compositions;
+
   const Member<const Document> m_document;
 };