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 08b2e64320847b16760d3d3654e6b97e170044b2..6563c8407b0fc3162d4b4450ea501e46547e5f05 100644
--- a/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
+++ b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
@@ -39,27 +39,22 @@
 
 namespace blink {
 
+class CompositionMarkerList;
+class GrammarMarkerList;
 class Node;
-class RenderedDocumentMarker;
-class Text;
-
-class MarkerRemoverPredicate final {
- public:
-  explicit MarkerRemoverPredicate(const Vector<String>& words);
-  bool operator()(const DocumentMarker&, const Text&) const;
-
- private:
-  Vector<String> m_words;
-};
+class RenderedTextMatchMarker;
+class SpellingMarkerList;
+class TextMatchMarkerList;
 
 class CORE_EXPORT DocumentMarkerController final
-    : public GarbageCollected<DocumentMarkerController> {
+    : public GarbageCollectedFinalized<DocumentMarkerController> {
   WTF_MAKE_NONCOPYABLE(DocumentMarkerController);
 
  public:
   explicit DocumentMarkerController(const Document&);
 
   void clear();
+  void addMarker(Node*, DocumentMarker*);
   void addMarker(const Position& start,
                  const Position& end,
                  DocumentMarker::MarkerType,
@@ -91,7 +86,7 @@ class CORE_EXPORT DocumentMarkerController final
                      RemovePartiallyOverlappingMarkerOrNot =
                          DoNotRemovePartiallyOverlappingMarker);

[rlanday, 2017/02/28 00:53:51]

It appears that the RemovePartiallyOverlappingMarker param is only ever set for
spellcheck markers:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/s...

So we may be able to remove this param, at the cost of either re-implementing
removeMarkers() in SpellcheckMarkerList, or hard-coding a check based on the
marker type

[Xiaocheng, 2017/03/01 22:24:48]

It's good if we can get rid of such a parameter.

In fact, the removal of stale spelling markers (namely,
SpellChecker::updateMarkersForWordsBlablabla) has been a performance bottleneck
for a long time.

If seems that we can get rid of that function, and simply maintain existing
markers as a SynchronousMutationObserver...

   void removeMarkers(Node*,
-                     unsigned startOffset,
+                     int startOffset,

[rlanday, 2017/02/28 00:53:51]

I thought about switching all the instances of unsigned ints in
DocumentMarker-related code over to signed ints (just because I think unsigned
ints are generally a bad idea, they don't enforce that values passed in are
non-negative, they just cause the values to overflow instead), but I ended up
punting on that; this change got left in though, how do we feel about this?

[Xiaocheng, 2017/03/01 22:24:48]

Let's discuss in crbug.com/696831

                      int length,
                      DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers(),
                      RemovePartiallyOverlappingMarkerOrNot =
@@ -102,16 +97,19 @@ class CORE_EXPORT DocumentMarkerController final
   void removeMarkers(
       Node*,
       DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers());
-  void removeMarkers(const MarkerRemoverPredicate& shouldRemoveMarker);
+  void removeSpellingMarkersForWords(const Vector<String>& words);
   void repaintMarkers(
       DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers());
   void shiftMarkers(Node*, unsigned startOffset, int delta);
   // Returns true if markers within a range are found.
-  bool setMarkersActive(const EphemeralRange&, bool);
+  bool setTextMatchMarkersActive(const EphemeralRange&, bool);
   // 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 setTextMatchMarkersActive(Node*,
+                                 unsigned startOffset,
+                                 unsigned endOffset,
+                                 bool);
+  bool hasMarkers(Node*) const;
 
   DocumentMarkerVector markersFor(
       Node*,
@@ -119,10 +117,11 @@ class CORE_EXPORT DocumentMarkerController final
   DocumentMarkerVector markersInRange(const EphemeralRange&,
                                       DocumentMarker::MarkerTypes);
   DocumentMarkerVector markers();
-  Vector<IntRect> renderedRectsForMarkers(DocumentMarker::MarkerType);
-  void updateMarkerRenderedRectIfNeeded(const Node&, RenderedDocumentMarker&);
-  void invalidateRectsForAllMarkers();
-  void invalidateRectsForMarkersInNode(const Node&);
+  Vector<IntRect> renderedRectsForTextMatchMarkers();
+  void updateTextMatchMarkerRenderedRectIfNeeded(const Node&,
+                                                 RenderedTextMatchMarker&);
+  void invalidateRectsForAllTextMatchMarkers();
+  void invalidateRectsForTextMatchMarkersInNode(Node&);
 
   DECLARE_TRACE();
 
@@ -131,23 +130,18 @@ class CORE_EXPORT DocumentMarkerController final
 #endif
 
  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>>;
+
+  MarkerMap<SpellingMarkerList> m_spelling;
+  MarkerMap<GrammarMarkerList> m_grammar;
+  MarkerMap<TextMatchMarkerList> m_textMatches;
+  MarkerMap<CompositionMarkerList> m_compositions;
+
   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;
   const Member<const Document> m_document;
 };