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..0bb32549d51c773575f856c8bbbcfa0ee1391905 100644
--- a/components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
+++ b/components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
@@ -38,6 +38,17 @@ struct OrderByMostRecentlyVisited {
   }
 };
 
+bool IsRecentTab(const offline_pages::ClientId& client_id) {
+  return client_id.name_space == offline_pages::kLastNNamespace;
+}
+
+bool IsDownload(const offline_pages::ClientId& client_id) {
+  // TODO(pke): Use kDownloadNamespace once the OfflinePageModel uses that.
+  // The current logic is taken from DownloadUIAdapter::IsVisibleInUI.
+  return client_id.name_space == offline_pages::kAsyncNamespace &&
+         base::IsValidGUID(client_id.id);
+}
+
 }  // namespace
 
 OfflinePageSuggestionsProvider::OfflinePageSuggestionsProvider(
@@ -142,21 +153,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(
@@ -183,11 +186,10 @@ void OfflinePageSuggestionsProvider::OfflinePageDeleted(
   // UI containing an invalidated suggestion unless the status is something
   // other than NOT_PROVIDED, so only notify invalidation in that case.
   if (recent_tabs_status_ != CategoryStatus::NOT_PROVIDED &&
-      client_id.name_space == offline_pages::kLastNNamespace) {
+      IsRecentTab(client_id)) {
     InvalidateSuggestion(recent_tabs_category_, offline_id);
   } else if (downloads_status_ != CategoryStatus::NOT_PROVIDED &&
-             client_id.name_space == offline_pages::kAsyncNamespace &&
-             base::IsValidGUID(client_id.id)) {
+             IsDownload(client_id)) {
     InvalidateSuggestion(downloads_category_, offline_id);
   }
 }
@@ -220,20 +222,14 @@ void OfflinePageSuggestionsProvider::OnOfflinePagesLoaded(
   std::vector<const OfflinePageItem*> download_items;
   for (const OfflinePageItem& item : result) {
     std::string offline_page_id = base::IntToString(item.offline_id);
-    if (need_recent_tabs &&
-        item.client_id.name_space == offline_pages::kLastNNamespace) {
+    if (need_recent_tabs && IsRecentTab(item.client_id)) {
       if (dismissed_recent_tab_ids.count(offline_page_id))
         cleaned_recent_tab_ids.insert(offline_page_id);
       else
         recent_tab_items.push_back(&item);
     }
 
-    // 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.
-    if (need_downloads &&
-        item.client_id.name_space == offline_pages::kAsyncNamespace &&
-        base::IsValidGUID(item.client_id.id)) {
+    if (need_downloads && IsDownload(item.client_id)) {
       if (dismissed_download_ids.count(offline_page_id))
         cleaned_download_ids.insert(offline_page_id);
       else
@@ -262,6 +258,24 @@ void OfflinePageSuggestionsProvider::OnOfflinePagesLoaded(
   }
 }
 
+void OfflinePageSuggestionsProvider::OnOfflinePagesLoadedForDismissedDebugging(
+    Category category,
+    const DismissedSuggestionsCallback& callback,
+    const offline_pages::MultipleOfflinePageItemResult& result) {
+  std::set<std::string> dismissed_ids = ReadDismissedIDsFromPrefs(category);
+  bool (*matches_category)(const offline_pages::ClientId&) =
+      (category == recent_tabs_category_) ? &IsRecentTab : &IsDownload;

[Marc Treib, 2016/08/19 12:36:15]

This is quite hard to read. How about something like
if ((category == recent_tabs && IsRecentTab(..)) || (same for downloads))
? Much easier to understand, and not much longer overall.

[Philipp Keck, 2016/08/19 14:20:40]

The reason I introduced it is because the structure of the if would be:
if (((A&B) | (C&D)) & E)
where E is the dismissed-check.

If the | is done here, the if becomes if (X & E), which is much nicer.

Maybe I'm missing something?, but I wouldn't find the other solution easier to
read. This solution needs you to read some stars and a function pointer, but I
think the logical flow is more obvious here.

That being said, I'm fine with either solution.

[Marc Treib, 2016/08/19 15:18:30]

If you're concerned that the condition in the if becomes to long, then pull part
of it out into a separate variable. Raw function pointers are IMO almost never a
good idea in C++.

[Bernhard Bauer, 2016/08/19 16:22:38]

Alternatively / in addition, you could split the check up by `continue`-ing if
one of the conditions fails. And if you want to keep the category match check in
a single expression, you could try using a lambda instead of a function pointer
(which at least looks nicer) or using a helper function that calls out to
IsRecentTab() or IsDownload() based on the category.

[Philipp Keck, 2016/08/19 16:52:36]

The "continue"-solution is a good idea, I implemented that. No need for a single
condition, that was just to save indentation. But with "continue", that problem
is gone.

Would a lambda get rid of the function pointer?

+
+  std::vector<ContentSuggestion> suggestions;
+  for (const OfflinePageItem& item : result) {
+    if ((*matches_category)(item.client_id) &&
+        dismissed_ids.count(base::IntToString(item.offline_id))) {
+      suggestions.push_back(ConvertOfflinePage(category, item));
+    }
+  }
+  callback.Run(category, std::move(suggestions));
+}
+
 void OfflinePageSuggestionsProvider::NotifyStatusChanged(
     Category category,
     CategoryStatus new_status) {