Make GetDismissedSuggestionsForDebugging asynchronous

components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/guid.h"
@@ skipping 124 matching lines @@
   // there.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(callback, suggestion_id, gfx::Image()));
 }
 
 void OfflinePageSuggestionsProvider::ClearCachedSuggestionsForDebugging(
     Category category) {
   // 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(
     Category category) {
   StoreDismissedIDsToPrefs(category, std::set<std::string>());
   FetchOfflinePages();
 }
 
 void OfflinePageSuggestionsProvider::OfflinePageModelLoaded(
     OfflinePageModel* model) {
@@ skipping 53 matching lines @@
     if (need_recent_tabs &&
         item.client_id.name_space == offline_pages::kLastNNamespace) {
       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.
+    // 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)) {
       if (dismissed_download_ids.count(offline_page_id))
         cleaned_download_ids.insert(offline_page_id);
       else
         download_items.push_back(&item);
     }
   }
 
@@ skipping 11 matching lines @@
   }
   if (need_downloads) {
     observer()->OnNewSuggestions(
         this, downloads_category_,
         GetMostRecentlyVisited(downloads_category_, std::move(download_items)));
     if (cleaned_download_ids.size() != dismissed_download_ids.size())
       StoreDismissedIDsToPrefs(downloads_category_, cleaned_download_ids);
   }
 }
 
+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) {
   if (category == recent_tabs_category_) {
     DCHECK_NE(CategoryStatus::NOT_PROVIDED, recent_tabs_status_);
     if (recent_tabs_status_ == new_status)
       return;
     recent_tabs_status_ = new_status;
     observer()->OnCategoryStatusChanged(this, category, new_status);
   } else if (category == downloads_category_) {
@@ skipping 86 matching lines @@
 void OfflinePageSuggestionsProvider::StoreDismissedIDsToPrefs(
     Category category,
     const std::set<std::string>& dismissed_ids) {
   base::ListValue list;
   for (const std::string& dismissed_id : dismissed_ids)
     list.AppendString(dismissed_id);
   pref_service_->Set(GetDismissedPref(category), list);
 }
 
 }  // namespace ntp_snippets