Refactor OfflinePageSuggestionsProvider dismissed ID handling

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 42 matching lines @@
                               ? CategoryStatus::AVAILABLE_LOADING
                               : CategoryStatus::NOT_PROVIDED),
       downloads_status_(downloads_enabled ? CategoryStatus::AVAILABLE_LOADING
                                           : CategoryStatus::NOT_PROVIDED),
       offline_page_model_(offline_page_model),
       recent_tabs_category_(
           category_factory->FromKnownCategory(KnownCategories::RECENT_TABS)),
       downloads_category_(
           category_factory->FromKnownCategory(KnownCategories::DOWNLOADS)),
       pref_service_(pref_service),
-      dismissed_recent_tab_ids_(ReadDismissedIDsFromPrefs(
-          prefs::kDismissedRecentOfflineTabSuggestions)),
-      dismissed_download_ids_(
-          ReadDismissedIDsFromPrefs(prefs::kDismissedDownloadSuggestions)),
       download_manager_ui_enabled_(download_manager_ui_enabled) {
   DCHECK(recent_tabs_enabled || downloads_enabled);
   offline_page_model_->AddObserver(this);
   FetchOfflinePages();
 }
 
 OfflinePageSuggestionsProvider::~OfflinePageSuggestionsProvider() {
   offline_page_model_->RemoveObserver(this);
 }
 
@@ skipping 43 matching lines @@
   NOTREACHED() << "Unknown category " << category.id();
   return CategoryInfo(base::string16(),
                       ContentSuggestionsCardLayout::MINIMAL_CARD,
                       /* has_more_button */ false);
 }
 
 void OfflinePageSuggestionsProvider::DismissSuggestion(
     const std::string& suggestion_id) {
   Category category = GetCategoryFromUniqueID(suggestion_id);
   std::string offline_page_id = GetWithinCategoryIDFromUniqueID(suggestion_id);
-  if (category == recent_tabs_category_) {
-    DCHECK_NE(CategoryStatus::NOT_PROVIDED, recent_tabs_status_);
-    dismissed_recent_tab_ids_.insert(offline_page_id);
-    StoreDismissedIDsToPrefs(prefs::kDismissedRecentOfflineTabSuggestions,
-                             dismissed_recent_tab_ids_);
-  } else if (category == downloads_category_) {
-    DCHECK_NE(CategoryStatus::NOT_PROVIDED, downloads_status_);
-    dismissed_download_ids_.insert(offline_page_id);
-    StoreDismissedIDsToPrefs(prefs::kDismissedDownloadSuggestions,
-                             dismissed_download_ids_);
-  } else {
-    NOTREACHED() << "Unknown category " << category.id();
-  }
+  std::set<std::string> dismissed_ids = ReadDismissedIDsFromPrefs(category);
+  dismissed_ids.insert(offline_page_id);
+  StoreDismissedIDsToPrefs(category, dismissed_ids);
 }
 
 void OfflinePageSuggestionsProvider::FetchSuggestionImage(
     const std::string& suggestion_id,
     const ImageFetchedCallback& callback) {
   // TODO(pke): Fetch proper thumbnail from OfflinePageModel once it's available
   // 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;
-  const std::set<std::string>* dismissed_ids = nullptr;
-  if (category == recent_tabs_category_) {
-    DCHECK_NE(CategoryStatus::NOT_PROVIDED, recent_tabs_status_);
-    dismissed_ids = &dismissed_recent_tab_ids_;
-  } else if (category == downloads_category_) {
-    DCHECK_NE(CategoryStatus::NOT_PROVIDED, downloads_status_);
-    dismissed_ids = &dismissed_download_ids_;
-  } else {
-    NOTREACHED() << "Unknown category " << category.id();
-    return suggestions;
-  }
-
-  for (const std::string& dismissed_id : *dismissed_ids) {
+  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::ClearDismissedSuggestionsForDebugging(
     Category category) {
-  if (category == recent_tabs_category_) {
-    DCHECK_NE(CategoryStatus::NOT_PROVIDED, recent_tabs_status_);
-    dismissed_recent_tab_ids_.clear();
-    StoreDismissedIDsToPrefs(prefs::kDismissedRecentOfflineTabSuggestions,
-                             dismissed_recent_tab_ids_);
-  } else if (category == downloads_category_) {
-    DCHECK_NE(CategoryStatus::NOT_PROVIDED, downloads_status_);
-    dismissed_download_ids_.clear();
-    StoreDismissedIDsToPrefs(prefs::kDismissedDownloadSuggestions,
-                             dismissed_download_ids_);
-  } else {
-    NOTREACHED() << "Unknown category " << category.id();
-  }
+  StoreDismissedIDsToPrefs(category, std::set<std::string>());
   FetchOfflinePages();
 }
 
 void OfflinePageSuggestionsProvider::OfflinePageModelLoaded(
     OfflinePageModel* model) {
   DCHECK_EQ(offline_page_model_, model);
 }
 
 void OfflinePageSuggestionsProvider::OfflinePageModelChanged(
     OfflinePageModel* model) {
   DCHECK_EQ(offline_page_model_, model);
   FetchOfflinePages();
 }
 
 void OfflinePageSuggestionsProvider::OfflinePageDeleted(
     int64_t offline_id,
     const offline_pages::ClientId& client_id) {
-  std::string offline_page_id = base::IntToString(offline_id);
   if (recent_tabs_status_ != CategoryStatus::NOT_PROVIDED &&
       client_id.name_space == offline_pages::kLastNNamespace) {
-    auto it = std::find(dismissed_recent_tab_ids_.begin(),
-                        dismissed_recent_tab_ids_.end(), offline_page_id);
-    if (it == dismissed_recent_tab_ids_.end()) {
-      observer()->OnSuggestionInvalidated(
-          this, recent_tabs_category_,
-          MakeUniqueID(recent_tabs_category_, offline_page_id));
-    } else {
-      dismissed_recent_tab_ids_.erase(it);
-      StoreDismissedIDsToPrefs(prefs::kDismissedRecentOfflineTabSuggestions,
-                               dismissed_recent_tab_ids_);
-    }
+    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)) {
-    auto it = std::find(dismissed_download_ids_.begin(),
-                        dismissed_download_ids_.end(), offline_page_id);
-    if (it == dismissed_download_ids_.end()) {
-      observer()->OnSuggestionInvalidated(
-          this, downloads_category_,
-          MakeUniqueID(downloads_category_, offline_page_id));
-    } else {
-      dismissed_download_ids_.erase(it);
-      StoreDismissedIDsToPrefs(prefs::kDismissedDownloadSuggestions,
-                               dismissed_download_ids_);
-    }
+    InvalidateSuggestion(downloads_category_, offline_id);
   }
 }
 
 void OfflinePageSuggestionsProvider::FetchOfflinePages() {
   offline_page_model_->GetAllPages(
       base::Bind(&OfflinePageSuggestionsProvider::OnOfflinePagesLoaded,
                  base::Unretained(this)));
 }
 
 void OfflinePageSuggestionsProvider::OnOfflinePagesLoaded(
     const MultipleOfflinePageItemResult& result) {
   bool need_recent_tabs = recent_tabs_status_ != CategoryStatus::NOT_PROVIDED;
   bool need_downloads = downloads_status_ != CategoryStatus::NOT_PROVIDED;
   if (need_recent_tabs)
     NotifyStatusChanged(recent_tabs_category_, CategoryStatus::AVAILABLE);
   if (need_downloads)
     NotifyStatusChanged(downloads_category_, CategoryStatus::AVAILABLE);
 
+  std::set<std::string> dismissed_recent_tab_ids =
+      ReadDismissedIDsFromPrefs(recent_tabs_category_);
+  std::set<std::string> dismissed_download_ids =
+      ReadDismissedIDsFromPrefs(downloads_category_);
+  std::set<std::string> cleaned_recent_tab_ids;
+  std::set<std::string> cleaned_download_ids;
   std::vector<const OfflinePageItem*> recent_tab_items;
   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 &&
-        !dismissed_recent_tab_ids_.count(base::IntToString(item.offline_id))) {
-      recent_tab_items.push_back(&item);
+        item.client_id.name_space == offline_pages::kLastNNamespace) {
+      if (dismissed_recent_tab_ids.count(offline_page_id)) {

[Marc Treib, 2016/08/16 11:48:37]

nit: no braces required

[Philipp Keck, 2016/08/16 12:04:02]

Done.

+        cleaned_recent_tab_ids.insert(offline_page_id);

[Marc Treib, 2016/08/16 11:48:37]

Hm. This depends on us getting *all* offline pages here, which is true for now,
but will probably change soon. Maybe at least worth a comment?

[Philipp Keck, 2016/08/16 12:04:02]

Added comment in FetchOfflinePages, so that Justin's CL gets a merge conflict.

+      } 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.
     if (need_downloads &&
         item.client_id.name_space == offline_pages::kAsyncNamespace &&
-        base::IsValidGUID(item.client_id.id) &&
-        !dismissed_download_ids_.count(base::IntToString(item.offline_id))) {
-      download_items.push_back(&item);
+        base::IsValidGUID(item.client_id.id)) {
+      if (dismissed_download_ids.count(offline_page_id)) {

[Marc Treib, 2016/08/16 11:48:37]

also here, no braces required

[Philipp Keck, 2016/08/16 12:04:02]

Done.

+        cleaned_download_ids.insert(offline_page_id);
+      } else {
+        download_items.push_back(&item);
+      }
     }
   }
 
   // TODO(pke): Once we have our OfflinePageModel getter and that doesn't do it
   // already, filter out duplicate URLs for recent tabs here. Duplicates for
   // downloads are fine.
 
   if (need_recent_tabs) {
     observer()->OnNewSuggestions(
         this, recent_tabs_category_,
         GetMostRecentlyVisited(recent_tabs_category_,
                                std::move(recent_tab_items)));
+    if (cleaned_recent_tab_ids.size() != dismissed_recent_tab_ids.size()) {

[Marc Treib, 2016/08/16 11:48:37]

and here

[Philipp Keck, 2016/08/16 12:04:01]

Done.

+      StoreDismissedIDsToPrefs(recent_tabs_category_, cleaned_recent_tab_ids);
+    }
   }
   if (need_downloads) {
     observer()->OnNewSuggestions(
         this, downloads_category_,
         GetMostRecentlyVisited(downloads_category_, std::move(download_items)));
+    if (cleaned_download_ids.size() != dismissed_download_ids.size()) {

[Marc Treib, 2016/08/16 11:48:37]

and here :)

[Philipp Keck, 2016/08/16 12:04:02]

Done.

+      StoreDismissedIDsToPrefs(downloads_category_, cleaned_download_ids);
+    }
   }
 }
 
 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;
@@ skipping 40 matching lines @@
             OrderByMostRecentlyVisited());
   std::vector<ContentSuggestion> suggestions;
   for (const OfflinePageItem* offline_page_item : offline_page_items) {
     suggestions.push_back(ConvertOfflinePage(category, *offline_page_item));
     if (suggestions.size() == kMaxSuggestionsCount)
       break;
   }
   return suggestions;
 }
 
+void OfflinePageSuggestionsProvider::InvalidateSuggestion(Category category,
+                                                          int64_t offline_id) {
+  std::string offline_page_id = base::IntToString(offline_id);
+  observer()->OnSuggestionInvalidated(this, category,
+                                      MakeUniqueID(category, offline_page_id));
+
+  std::set<std::string> dismissed_ids = ReadDismissedIDsFromPrefs(category);
+  auto it =
+      std::find(dismissed_ids.begin(), dismissed_ids.end(), offline_page_id);
+  if (it != dismissed_ids.end()) {
+    dismissed_ids.erase(it);
+    StoreDismissedIDsToPrefs(category, dismissed_ids);
+  }
+}
+
+std::string OfflinePageSuggestionsProvider::GetDismissedPref(
+    Category category) const {
+  if (category == recent_tabs_category_) {
+    return prefs::kDismissedRecentOfflineTabSuggestions;
+  } else if (category == downloads_category_) {
+    return prefs::kDismissedDownloadSuggestions;
+  } else {
+    NOTREACHED() << "Unknown category " << category.id();
+    return std::string();
+  }
+}
+
 std::set<std::string> OfflinePageSuggestionsProvider::ReadDismissedIDsFromPrefs(
-    const std::string& pref_name) const {
+    Category category) const {
   std::set<std::string> dismissed_ids;
-  const base::ListValue* list = pref_service_->GetList(pref_name);
+  const base::ListValue* list =
+      pref_service_->GetList(GetDismissedPref(category));
   for (const std::unique_ptr<base::Value>& value : *list) {
     std::string dismissed_id;
     bool success = value->GetAsString(&dismissed_id);
     DCHECK(success) << "Failed to parse dismissed offline page ID from prefs";
     dismissed_ids.insert(dismissed_id);
   }
   return dismissed_ids;
 }
 
 void OfflinePageSuggestionsProvider::StoreDismissedIDsToPrefs(
-    const std::string& pref_name,
+    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(pref_name, list);
+  pref_service_->Set(GetDismissedPref(category), list);
 }
 
 }  // namespace ntp_snippets