Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService

components/ntp_snippets/content_suggestions_service.cc

Index: components/ntp_snippets/content_suggestions_service.cc
diff --git a/components/ntp_snippets/content_suggestions_service.cc b/components/ntp_snippets/content_suggestions_service.cc
index c1977e50c8767454c388edfb5c1b7d1a5de75e82..d8a9517c38ff6cb32b0fdd4aff9bfff60e74cb08 100644
--- a/components/ntp_snippets/content_suggestions_service.cc
+++ b/components/ntp_snippets/content_suggestions_service.cc
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <iterator>
+#include <set>
 
 #include "base/bind.h"
 #include "base/location.h"
@@ -15,8 +16,14 @@
 
 namespace ntp_snippets {
 
-ContentSuggestionsService::ContentSuggestionsService(State state)
-    : state_(state) {}
+ContentSuggestionsService::ContentSuggestionsService(
+    State state,
+    history::HistoryService* history_service)
+    : state_(state), history_service_observer_(this) {
+  // Can be null in tests.
+  if (history_service)
+    history_service_observer_.Add(history_service);
+}
 
 ContentSuggestionsService::~ContentSuggestionsService() {}
 
@@ -230,6 +237,57 @@ void ContentSuggestionsService::OnSuggestionInvalidated(
                     OnSuggestionInvalidated(category, suggestion_id));
 }
 
+// history::HistoryServiceObserver implementation.
+void ContentSuggestionsService::OnURLsDeleted(
+    history::HistoryService* history_service,
+    bool all_history,
+    bool expired,
+    const history::URLRows& deleted_rows,
+    const std::set<GURL>& favicon_urls) {
+  // We don't care about expired entries.
+  if (expired)
+    return;
+
+  // Redirect to ClearHistory().
+  base::Time begin;
+  base::Time end;
+  base::Callback<bool(const GURL& url)> filter;
+  auto return_true = [](const GURL& url) { return true; };
+  auto set_contains = [](const std::set<GURL>& set, const GURL& url) {

[Marc Treib, 2016/09/02 14:45:19]

Just inline these two definitions into the respective Binds below?
Or at least move their definitions closer to where they're used.

[vitaliii, 2016/09/02 16:40:06]

Done.

+    return set.count(url) != 0 ? true : false;

[Marc Treib, 2016/09/02 14:45:19]

remove the " ? true : false", it doesn't do anything

[vitaliii, 2016/09/02 16:40:06]

|count| returns |unsigned int|.

[Marc Treib, 2016/09/02 16:50:35]

Yes, you can keep the "!= 0" (that's nicer than the static_cast you have now),
just the arithmetic if after that was pointless.

[vitaliii, 2016/09/02 17:14:10]

Done.

+  };
+  if (all_history) {
+    begin = base::Time::UnixEpoch();
+    end = base::Time::Now();
+    filter = base::Bind(return_true);
+  } else {
+    if (deleted_rows.empty())
+      return;
+    else {

[Marc Treib, 2016/09/02 14:45:19]

No else after return.
Or, if you initialize begin/end to suitable values below, you might not need the
early exit at all.

[vitaliii, 2016/09/02 16:40:06]

Done.

+      begin = deleted_rows[0].last_visit();
+      end = deleted_rows[0].last_visit();
+      std::set<GURL> deleted_urls;
+      for (const history::URLRow& row : deleted_rows) {
+        if (row.last_visit() < begin)
+          begin = row.last_visit();
+        if (row.last_visit() > end)
+          end = row.last_visit();
+        deleted_urls.insert(row.url());
+      }
+      filter = base::Bind(set_contains, deleted_urls);
+    }
+  }
+
+  for (const auto& provider : providers_) {

[Marc Treib, 2016/09/02 14:45:19]

Just call this class's ClearHistory

[vitaliii, 2016/09/02 16:40:06]

Done.

+    provider->ClearHistory(begin, end, filter);
+  }
+}
+
+void ContentSuggestionsService::HistoryServiceBeingDeleted(
+    history::HistoryService* history_service) {
+  history_service_observer_.RemoveAll();
+}
+
 bool ContentSuggestionsService::RegisterCategoryIfRequired(
     ContentSuggestionsProvider* provider,
     Category category) {