Adding a word to dictionary should remove spelling markers

Source/core/dom/DocumentMarkerController.cpp

Index: Source/core/dom/DocumentMarkerController.cpp
diff --git a/Source/core/dom/DocumentMarkerController.cpp b/Source/core/dom/DocumentMarkerController.cpp
index 5a7cc9b47afb89e7f4f28bc2d60120995ec16109..4da677e25e2b3c3273dff5400ec0ccd3f41cac26 100644
--- a/Source/core/dom/DocumentMarkerController.cpp
+++ b/Source/core/dom/DocumentMarkerController.cpp
@@ -31,6 +31,7 @@
 #include "core/dom/NodeTraversal.h"
 #include "core/dom/Range.h"
 #include "core/dom/RenderedDocumentMarker.h"
+#include "core/dom/Text.h"
 #include "core/editing/TextIterator.h"
 #include "core/rendering/RenderObject.h"
 
@@ -40,6 +41,20 @@
 
 namespace blink {
 
+MarkerRemoverPredicate::MarkerRemoverPredicate(const Vector<String>& words)
+    : m_words(words)
+{
+}
+
+bool MarkerRemoverPredicate::operator()(const DocumentMarker& documentMarker,
+    const Text& textNode) const {

[please use gerrit instead, 2014/08/15 19:08:07]

Please fix the indentation of parameters on line 49 and 50 to be consistent with
the rest of the code in this file:

bool class::method(const type& arg1, const type& arg2) {

[Klemen Forstnerič, 2014/08/16 15:45:50]

Done.

+    unsigned start  = documentMarker.startOffset();
+    unsigned length = documentMarker.endOffset() - documentMarker.startOffset();
+
+    String markerText = textNode.data().substring(start, length);
+    return m_words.contains(markerText);
+}
+
 namespace {
 
 DocumentMarker::MarkerTypeIndex MarkerTypeToMarkerIndex(DocumentMarker::MarkerType type)
@@ -494,6 +509,25 @@ void DocumentMarkerController::removeMarkers(Node* node, DocumentMarker::MarkerT
         removeMarkersFromList(iterator, markerTypes);
 }
 
+void DocumentMarkerController::removeMarkers(const MarkerRemoverPredicate& shouldRemoveMarker)
+{
+    for (MarkerMap::iterator i = m_markers.begin(); i != m_markers.end(); ++i) {
+        MarkerLists* markers = i->value.get();
+        for (size_t markerListIndex = 0; markerListIndex < DocumentMarker::MarkerTypeIndexesCount; ++markerListIndex) {
+            OwnPtrWillBeMember<MarkerList>& list = (*markers)[markerListIndex];
+
+            WillBeHeapVector<RawPtrWillBeMember<RenderedDocumentMarker> > markersToBeRemoved;
+            for (size_t j = 0; list.get() && j < list->size(); ++j) {
+                if (i->key->isTextNode() && shouldRemoveMarker(*list->at(j).get(), static_cast<const Text&>(*i->key)))
+                    markersToBeRemoved.append(list->at(j).get());
+            }
+
+            for (size_t j = 0; j < markersToBeRemoved.size(); ++j)
+                list->remove(list->find(markersToBeRemoved[j].get()));

[please use gerrit instead, 2014/08/15 19:08:07]

Running list->find for every element in markersToBeRemoved may be slow (on the
order of size of |list| times size of |markersToBeRemoved|). Instead of keeping
track of makers to be removed, I recommend keeping track of markers to keep and
then swapping that with |list|. That is:

-------------------------------

WillBeHeapVector<RawPtrWillBeMember<RenderedDocumentMarker> > markersToKeep;
for (size_t j = 0; list.get() && j < list->size(); ++j) {
    if (!i->key->isTextNode() || !shouldRemoveMarker(*list->at(j).get(),
static_cast<const Text&>(*i->key)))
        markersToKeep.append(list->at(j).get());
}

swap(*list, markersToKeep);

-------------------------------

I hope that compiles ;-).

[Klemen Forstnerič, 2014/08/16 15:45:50]

I was able to make the code compile by making markersToKeep of type MarkerList
and then transfering ownership of the markers I wanted to keep to that
MarkerList. Unfortunately though, the code crashes the tab process. This is the
code I had:

MarkerList markersToKeep;
for (size_t j = 0; list.get() && j < list->size(); ++j) {
    if (!i->key->isTextNode() || !shouldRemoveMarker(*list->at(j).get(),
static_cast<const Text&>(*i->key)))
        markersToKeep.append(list->at(j).release());
}

swap(*list, markersToKeep);

Thoughts? :-)

[please use gerrit instead, 2014/08/16 19:54:36]

As discussed in IRC, you can discard my suggestion here, because the increased
complexity of memory management may not be worth the performance improvement.

If you're still interested in investigating the crash in your patch, then use
gdb and/or address sanitizer
(http://www.chromium.org/developers/testing/addresssanitizer).

+        }
+    }
+}
+
 void DocumentMarkerController::removeMarkers(DocumentMarker::MarkerTypes markerTypes)
 {
     if (!possiblyHasMarkers(markerTypes))