Split OfflinePageSuggestions into two categories

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 897bc5b45e0356fc4062cd8cd7940c2943a38bb4..6521a2d4c73ea4612d6b8ed17c02378a0adc7061 100644
--- a/components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
+++ b/components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
@@ -4,6 +4,8 @@
 
 #include "components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h"
 
+#include <algorithm>
+
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/strings/string_number_conversions.h"
@@ -11,8 +13,11 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "components/ntp_snippets/pref_names.h"
+#include "components/offline_pages/client_namespace_constants.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "grit/components_strings.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/image/image.h"
 
 using offline_pages::MultipleOfflinePageItemResult;
@@ -25,21 +30,40 @@ namespace {
 
 const int kMaxSuggestionsCount = 5;
 
+struct {
+  bool operator()(const OfflinePageItem* left,
+                  const OfflinePageItem* right) const {
+    return left->last_access_time > right->last_access_time;
+  }
+} OrderByMostRecentlyVisited;

[Marc Treib, 2016/08/12 10:12:53]

This instantiates one global instance of the (unnamed) comparator struct. Is
that on purpose?
(I don't think there's anything wrong with this as such, but it is uncommon...)

[Philipp Keck, 2016/08/12 10:22:55]

This is from example code of std::sort. Otherwise, we'd have to create a new
instance every time, right? So even if it's uncommon, it seems like a reasonable
choice here. Creating an instance of this doesn't really have a "meaning", it's
just a dummy.

[Bernhard Bauer, 2016/08/12 10:28:31]

Is that going to create a static initializer? If so, it's disallowed.

(It would probably be fine to name the struct OrderByMostRecentlyVisited and
construct an instance when needed for sorting.)

[Philipp Keck, 2016/08/12 11:02:25]

Done.

+
 }  // namespace
 
 OfflinePageSuggestionsProvider::OfflinePageSuggestionsProvider(
+    bool recent_tabs_enabled,
+    bool downloads_enabled,
     ContentSuggestionsProvider::Observer* observer,
     CategoryFactory* category_factory,
     OfflinePageModel* offline_page_model,
     PrefService* pref_service)
     : ContentSuggestionsProvider(observer, category_factory),
-      category_status_(CategoryStatus::AVAILABLE_LOADING),
+      recent_tabs_status_(recent_tabs_enabled
+                              ? CategoryStatus::AVAILABLE_LOADING
+                              : CategoryStatus::NOT_PROVIDED),
+      downloads_status_(downloads_enabled ? CategoryStatus::AVAILABLE_LOADING
+                                          : CategoryStatus::NOT_PROVIDED),
       offline_page_model_(offline_page_model),
-      provided_category_(
-          category_factory->FromKnownCategory(KnownCategories::OFFLINE_PAGES)),
-      pref_service_(pref_service) {
+      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)) {
+  DCHECK(recent_tabs_enabled || downloads_enabled);
   offline_page_model_->AddObserver(this);
-  ReadDismissedIDsFromPrefs();
   FetchOfflinePages();
 }
 
@@ -50,34 +74,66 @@ OfflinePageSuggestionsProvider::~OfflinePageSuggestionsProvider() {
 // static
 void OfflinePageSuggestionsProvider::RegisterProfilePrefs(
     PrefRegistrySimple* registry) {
-  registry->RegisterListPref(prefs::kDismissedOfflinePageSuggestions);
+  registry->RegisterListPref(prefs::kDismissedRecentOfflineTabSuggestions);
+  registry->RegisterListPref(prefs::kDismissedDownloadSuggestions);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
 std::vector<Category> OfflinePageSuggestionsProvider::GetProvidedCategories() {
-  return std::vector<Category>({provided_category_});
+  std::vector<Category> provided_categories;
+  if (recent_tabs_status_ != CategoryStatus::NOT_PROVIDED)
+    provided_categories.push_back(recent_tabs_category_);
+  if (downloads_status_ != CategoryStatus::NOT_PROVIDED)
+    provided_categories.push_back(downloads_category_);
+  return provided_categories;
 }
 
 CategoryStatus OfflinePageSuggestionsProvider::GetCategoryStatus(
     Category category) {
-  DCHECK_EQ(category, provided_category_);
-  return category_status_;
+  if (category == recent_tabs_category_)
+    return recent_tabs_status_;
+  if (category == downloads_category_)
+    return downloads_status_;
+  NOTREACHED() << "Unknown category " << category.id();
+  return CategoryStatus::NOT_PROVIDED;
 }
 
 CategoryInfo OfflinePageSuggestionsProvider::GetCategoryInfo(
     Category category) {
-  // TODO(pke): Use the proper string once it's agreed on.
-  return CategoryInfo(base::ASCIIToUTF16("Offline pages"),
+  if (category == recent_tabs_category_) {
+    return CategoryInfo(l10n_util::GetStringUTF16(
+                            IDS_NTP_RECENT_TAB_SUGGESTIONS_SECTION_HEADER),
+                        ContentSuggestionsCardLayout::MINIMAL_CARD);
+  }
+  if (category == downloads_category_) {
+    return CategoryInfo(
+        l10n_util::GetStringUTF16(IDS_NTP_DOWNLOAD_SUGGESTIONS_SECTION_HEADER),
+        ContentSuggestionsCardLayout::MINIMAL_CARD);
+  }
+  NOTREACHED() << "Unknown category " << category.id();
+  return CategoryInfo(base::string16(),
                       ContentSuggestionsCardLayout::MINIMAL_CARD);
 }
 
 void OfflinePageSuggestionsProvider::DismissSuggestion(
     const std::string& suggestion_id) {
+  Category category = GetCategoryFromUniqueID(suggestion_id);
   std::string offline_page_id = GetWithinCategoryIDFromUniqueID(suggestion_id);
-  dismissed_ids_.insert(offline_page_id);
-  StoreDismissedIDsToPrefs();
+  if (category == recent_tabs_category_) {
+    DCHECK_NE(recent_tabs_status_, CategoryStatus::NOT_PROVIDED);

[Bernhard Bauer, 2016/08/12 10:28:31]

The (not 😉) expected value should go first for nicer error messages in case of
failure.

[Philipp Keck, 2016/08/12 11:02:25]

Done.

+    dismissed_recent_tab_ids_.insert(offline_page_id);
+    StoreDismissedIDsToPrefs(prefs::kDismissedRecentOfflineTabSuggestions,
+                             dismissed_recent_tab_ids_);
+  } else if (category == downloads_category_) {
+    DCHECK_NE(downloads_status_, CategoryStatus::NOT_PROVIDED);
+    dismissed_download_ids_.insert(offline_page_id);
+    StoreDismissedIDsToPrefs(prefs::kDismissedDownloadSuggestions,
+                             dismissed_download_ids_);
+  } else {
+    NOTREACHED() << "Unknown category " << category.id();
+  }
 }
 
 void OfflinePageSuggestionsProvider::FetchSuggestionImage(
@@ -91,7 +147,6 @@ void OfflinePageSuggestionsProvider::FetchSuggestionImage(
 
 void OfflinePageSuggestionsProvider::ClearCachedSuggestionsForDebugging(
     Category category) {
-  DCHECK_EQ(category, provided_category_);
   // Ignored.
 }
 
@@ -100,11 +155,22 @@ OfflinePageSuggestionsProvider::GetDismissedSuggestionsForDebugging(
     Category category) {
   // TODO(pke): Make GetDismissedSuggestionsForDebugging asynchronous so this
   // can return proper values.
-  DCHECK_EQ(category, provided_category_);
   std::vector<ContentSuggestion> suggestions;
-  for (const std::string& dismissed_id : dismissed_ids_) {
+  const std::set<std::string>* dismissed_ids = nullptr;
+  if (category == recent_tabs_category_) {
+    DCHECK_NE(recent_tabs_status_, CategoryStatus::NOT_PROVIDED);
+    dismissed_ids = &dismissed_recent_tab_ids_;
+  } else if (category == downloads_category_) {
+    DCHECK_NE(downloads_status_, CategoryStatus::NOT_PROVIDED);
+    dismissed_ids = &dismissed_download_ids_;
+  } else {
+    NOTREACHED() << "Unknown category " << category.id();
+    return suggestions;
+  }
+
+  for (const std::string& dismissed_id : *dismissed_ids) {
     ContentSuggestion suggestion(
-        MakeUniqueID(provided_category_, dismissed_id),
+        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));
@@ -114,9 +180,19 @@ OfflinePageSuggestionsProvider::GetDismissedSuggestionsForDebugging(
 
 void OfflinePageSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
     Category category) {
-  DCHECK_EQ(category, provided_category_);
-  dismissed_ids_.clear();
-  StoreDismissedIDsToPrefs();
+  if (category == recent_tabs_category_) {
+    DCHECK_NE(recent_tabs_status_, CategoryStatus::NOT_PROVIDED);
+    dismissed_recent_tab_ids_.clear();
+    StoreDismissedIDsToPrefs(prefs::kDismissedRecentOfflineTabSuggestions,
+                             dismissed_recent_tab_ids_);
+  } else if (category == downloads_category_) {
+    DCHECK_NE(downloads_status_, CategoryStatus::NOT_PROVIDED);
+    dismissed_download_ids_.clear();
+    StoreDismissedIDsToPrefs(prefs::kDismissedDownloadSuggestions,
+                             dismissed_download_ids_);
+  } else {
+    NOTREACHED() << "Unknown category " << category.id();
+  }
   FetchOfflinePages();
 }
 
@@ -145,42 +221,74 @@ void OfflinePageSuggestionsProvider::FetchOfflinePages() {
 
 void OfflinePageSuggestionsProvider::OnOfflinePagesLoaded(
     const MultipleOfflinePageItemResult& result) {
-  NotifyStatusChanged(CategoryStatus::AVAILABLE);
-
-  std::vector<ContentSuggestion> suggestions;
+  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::vector<const OfflinePageItem*> recent_tab_items;
+  std::vector<const OfflinePageItem*> download_items;
   for (const OfflinePageItem& item : result) {
-    if (dismissed_ids_.count(base::IntToString(item.offline_id)))
-      continue;
-    suggestions.push_back(ConvertOfflinePage(item));
-    if (suggestions.size() == kMaxSuggestionsCount)
-      break;
+    if (need_recent_tabs &&
+        item.client_id.name_space == offline_pages::kLastNNamespace) {
+      if (!dismissed_recent_tab_ids_.count(base::IntToString(item.offline_id)))
+        recent_tab_items.push_back(&item);
+    } else if (need_downloads &&
+               item.client_id.name_space == offline_pages::kDownloadNamespace) {
+      if (!dismissed_download_ids_.count(base::IntToString(item.offline_id)))
+        download_items.push_back(&item);
+    }
   }
 
-  observer()->OnNewSuggestions(this, provided_category_,
-                               std::move(suggestions));
+  // 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 (need_downloads) {
+    observer()->OnNewSuggestions(
+        this, downloads_category_,
+        GetMostRecentlyVisited(downloads_category_, std::move(download_items)));
+  }
 }
 
 void OfflinePageSuggestionsProvider::NotifyStatusChanged(
+    Category category,
     CategoryStatus new_status) {
-  if (category_status_ == new_status)
-    return;
-  category_status_ = new_status;
-
-  observer()->OnCategoryStatusChanged(this, provided_category_, new_status);
+  if (category == recent_tabs_category_) {
+    DCHECK_NE(recent_tabs_status_, CategoryStatus::NOT_PROVIDED);
+    if (recent_tabs_status_ == new_status)
+      return;
+    recent_tabs_status_ = new_status;
+    observer()->OnCategoryStatusChanged(this, category, new_status);
+  } else if (category == downloads_category_) {
+    DCHECK_NE(downloads_status_, CategoryStatus::NOT_PROVIDED);
+    if (downloads_status_ == new_status)
+      return;
+    downloads_status_ = new_status;
+    observer()->OnCategoryStatusChanged(this, category, new_status);
+  } else {
+    NOTREACHED() << "Unknown category " << category.id();
+  }
 }
 
 ContentSuggestion OfflinePageSuggestionsProvider::ConvertOfflinePage(
+    Category category,
     const OfflinePageItem& offline_page) const {
   // TODO(pke): Make sure the URL is actually opened as an offline URL.
   // Currently, the browser opens the offline URL and then immediately
   // redirects to the online URL if the device is online.
   ContentSuggestion suggestion(
-      MakeUniqueID(provided_category_,
-                   base::IntToString(offline_page.offline_id)),
+      MakeUniqueID(category, base::IntToString(offline_page.offline_id)),
       offline_page.GetOfflineURL());
 
-  // TODO(pke): Sort by most recently visited and only keep the top one of
-  // multiple entries for the same URL.
   // TODO(pke): Get more reasonable data from the OfflinePageModel here.
   suggestion.set_title(base::UTF8ToUTF16(offline_page.url.spec()));
   suggestion.set_snippet_text(base::string16());
@@ -189,23 +297,41 @@ ContentSuggestion OfflinePageSuggestionsProvider::ConvertOfflinePage(
   return suggestion;
 }
 
-void OfflinePageSuggestionsProvider::ReadDismissedIDsFromPrefs() {
-  dismissed_ids_.clear();
-  const base::ListValue* list =
-      pref_service_->GetList(prefs::kDismissedOfflinePageSuggestions);
+std::vector<ContentSuggestion>
+OfflinePageSuggestionsProvider::GetMostRecentlyVisited(
+    Category category,
+    std::vector<const OfflinePageItem*> offline_page_items) const {
+  std::sort(offline_page_items.begin(), offline_page_items.end(),
+            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;
+}
+
+std::set<std::string> OfflinePageSuggestionsProvider::ReadDismissedIDsFromPrefs(
+    const std::string& pref_name) const {
+  std::set<std::string> dismissed_ids;
+  const base::ListValue* list = pref_service_->GetList(pref_name);
   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(std::move(dismissed_id));
+    dismissed_ids.insert(dismissed_id);
   }
+  return dismissed_ids;
 }
 
-void OfflinePageSuggestionsProvider::StoreDismissedIDsToPrefs() {
+void OfflinePageSuggestionsProvider::StoreDismissedIDsToPrefs(
+    const std::string& pref_name,
+    const std::set<std::string>& dismissed_ids) const {
   base::ListValue list;
-  for (const std::string& dismissed_id : dismissed_ids_)
+  for (const std::string& dismissed_id : dismissed_ids)
     list.AppendString(dismissed_id);
-  pref_service_->Set(prefs::kDismissedOfflinePageSuggestions, list);
+  pref_service_->Set(pref_name, list);
 }
 
 }  // namespace ntp_snippets