Make GetDismissedSuggestionsForDebugging asynchronous

components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc

Index: components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
diff --git a/components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc b/components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
index b98f843a638f88474b95e029e04f79264ae31d25..48101435a4a77bd3f79a4076f6f01e22c6bd4fd9 100644
--- a/components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
+++ b/components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
@@ -142,21 +142,13 @@ void OfflinePageSuggestionsProvider::ClearCachedSuggestionsForDebugging(
   // Ignored.
 }
 
-std::vector<ContentSuggestion>
-OfflinePageSuggestionsProvider::GetDismissedSuggestionsForDebugging(
-    Category category) {
-  // TODO(pke): Make GetDismissedSuggestionsForDebugging asynchronous so this
-  // can return proper values.
-  std::set<std::string> dismissed_ids = ReadDismissedIDsFromPrefs(category);
-  std::vector<ContentSuggestion> suggestions;
-  for (const std::string& dismissed_id : dismissed_ids) {
-    ContentSuggestion suggestion(
-        MakeUniqueID(category, dismissed_id),
-        GURL("http://dismissed-offline-page-" + dismissed_id));
-    suggestion.set_title(base::UTF8ToUTF16("Title not available"));
-    suggestions.push_back(std::move(suggestion));
-  }
-  return suggestions;
+void OfflinePageSuggestionsProvider::GetDismissedSuggestionsForDebugging(
+    Category category,
+    const DismissedSuggestionsCallback& callback) {
+  offline_page_model_->GetAllPages(
+      base::Bind(&OfflinePageSuggestionsProvider::
+                     OnOfflinePagesLoadedForDismissedDebugging,
+                 base::Unretained(this), category, callback));
 }
 
 void OfflinePageSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
@@ -230,7 +222,8 @@ void OfflinePageSuggestionsProvider::OnOfflinePagesLoaded(
 
     // TODO(pke): Use kDownloadNamespace once the OfflinePageModel uses that.
     // The current logic is taken from DownloadUIAdapter::IsVisibleInUI.
-    // Note: This is also copied in |OfflinePageDeleted| above.
+    // Note: This is also copied in |OfflinePageDeleted| above and in
+    // |OnOfflinePagesLoadedForDismissedDebugging| below.

[Marc Treib, 2016/08/19 10:07:51]

Maybe time to extract it into a helper function?

[Philipp Keck, 2016/08/19 12:11:54]

I thought about that, also in order to simplify the huge if-condition at line
266.
To make that simpler, I had a solution with a function pointer (not sure if
that's an anti-pattern?), which I'm putting back in now. Let me know what you
think.

     if (need_downloads &&
         item.client_id.name_space == offline_pages::kAsyncNamespace &&
         base::IsValidGUID(item.client_id.id)) {
@@ -262,6 +255,27 @@ void OfflinePageSuggestionsProvider::OnOfflinePagesLoaded(
   }
 }
 
+void OfflinePageSuggestionsProvider::OnOfflinePagesLoadedForDismissedDebugging(
+    Category category,
+    const DismissedSuggestionsCallback& callback,
+    const offline_pages::MultipleOfflinePageItemResult& result) {
+  std::set<std::string> dismissed_ids = ReadDismissedIDsFromPrefs(category);
+
+  std::vector<ContentSuggestion> suggestions;
+  for (const OfflinePageItem& item : result) {
+    if ((category == recent_tabs_category_ &&
+         item.client_id.name_space == offline_pages::kLastNNamespace) ||
+        (category == downloads_category_ &&
+         item.client_id.name_space == offline_pages::kAsyncNamespace &&
+         base::IsValidGUID(item.client_id.id))) {
+      if (dismissed_ids.count(base::IntToString(item.offline_id))) {
+        suggestions.push_back(ConvertOfflinePage(category, item));
+      }
+    }
+  }
+  return callback.Run(category, std::move(suggestions));

[Marc Treib, 2016/08/19 10:07:51]

No "return"

[Philipp Keck, 2016/08/19 12:11:54]

Done.

+}
+
 void OfflinePageSuggestionsProvider::NotifyStatusChanged(
     Category category,
     CategoryStatus new_status) {