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..fdf9f79b88eb14ff7cac0b6ade541220fa7992bf 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;
+
 }  // 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,68 @@ 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_;
+  } else if (category == downloads_category_) {

[Marc Treib, 2016/08/12 09:21:14]

nit: No "else" after "return"

[Philipp Keck, 2016/08/12 09:56:40]

Done. Don't really like this rule. Now the shape of this method is different
from the others (the voids), only because of its return value, but even though
they all just do different things depending on the category.

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

Well, the rule is the rule... in general, I do like the rule (reduces
indentation), though I do see how it's a bit annoying in this instance. Oh well.

If you want things to be more consistent, you could add returns in the other
methods.

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

Alternatively, use a switch statement?

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

Yeah, but that wouldn't get rid of the code duplicates. This method is okay, but
look at DismissSuggestion, for example. The whole "DCHECK_NE", "insert", "Store"
logic is there twice. On the other hand, Marc has a point: It might actually be
that we need different logic for those in the future.

+    return downloads_status_;
+  } else {
+    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"),
-                      ContentSuggestionsCardLayout::MINIMAL_CARD);
+  if (category == recent_tabs_category_) {
+    return CategoryInfo(l10n_util::GetStringUTF16(
+                            IDS_NTP_RECENT_TAB_SUGGESTIONS_SECTION_HEADER),
+                        ContentSuggestionsCardLayout::MINIMAL_CARD);
+  } else if (category == downloads_category_) {

[Marc Treib, 2016/08/12 09:21:14]

Same here

[Philipp Keck, 2016/08/12 09:56:40]

Done.

+    return CategoryInfo(
+        l10n_util::GetStringUTF16(IDS_NTP_DOWNLOAD_SUGGESTIONS_SECTION_HEADER),
+        ContentSuggestionsCardLayout::MINIMAL_CARD);
+  } else {
+    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);

[Marc Treib, 2016/08/12 09:21:14]

This is enforced in the ContentSuggestionsService?

[Philipp Keck, 2016/08/12 09:56:40]

Yes, but not only:
Firstly, if the status is NOT_PROVIDED, the mapping "category -> provider" is
deleted, so if a dismissed-call comes back, it doesn't know which provider to
call and just discards it.

In addition, the recent_tabs_category_ field can only ever have this value if it
was already set at startup (because it's a flag). So it's either NOT_PROVIDED
all the time, or never.

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

Acknowledged.

+    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_);
+  }

[Marc Treib, 2016/08/12 09:21:14]

else NOTREACHED?

[Philipp Keck, 2016/08/12 09:56:40]

Done.

 }
 
 void OfflinePageSuggestionsProvider::FetchSuggestionImage(
@@ -91,7 +149,6 @@ void OfflinePageSuggestionsProvider::FetchSuggestionImage(
 
 void OfflinePageSuggestionsProvider::ClearCachedSuggestionsForDebugging(
     Category category) {
-  DCHECK_EQ(category, provided_category_);
   // Ignored.
 }
 
@@ -100,11 +157,21 @@ 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_) {
+  std::set<std::string>* dismissed_ids = nullptr;

[Marc Treib, 2016/08/12 09:21:14]

const?

[Philipp Keck, 2016/08/12 09:56:40]

Done.

+  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 {
+    return suggestions;

[Marc Treib, 2016/08/12 09:21:14]

NOTREACHED?

[Philipp Keck, 2016/08/12 09:56:40]

Done.

+  }
+
+  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 +181,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 +222,73 @@ void OfflinePageSuggestionsProvider::FetchOfflinePages() {
 
 void OfflinePageSuggestionsProvider::OnOfflinePagesLoaded(
     const MultipleOfflinePageItemResult& result) {
-  NotifyStatusChanged(CategoryStatus::AVAILABLE);
-
-  std::vector<ContentSuggestion> suggestions;
+  bool needRecentTabs = recent_tabs_status_ != CategoryStatus::NOT_PROVIDED;

[Marc Treib, 2016/08/12 09:21:14]

need_recent_tabs, need_downloads

[Philipp Keck, 2016/08/12 09:56:40]

Done.

+  bool needDownloads = recent_tabs_status_ != CategoryStatus::NOT_PROVIDED;

[Marc Treib, 2016/08/12 09:21:14]

downloads_status_

[Philipp Keck, 2016/08/12 09:56:40]

Done. Good catch. The whole code duplication isn't an awesome solution ...

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

The alternative would be to have getters like GetStatus(category), that might
save at least some of the duplication. Though things will get kinda messy if
there are any differences between the sections (like the removing duplicates
thing). Your call :)

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

I'd refactor only after the provider has grown or changed a bit more, then it's
probably easier whether code duplication is a bad thing or necessary.

+  if (needRecentTabs)
+    NotifyStatusChanged(recent_tabs_category_, CategoryStatus::AVAILABLE);

[Marc Treib, 2016/08/12 09:21:14]

Braces for single=line bodies or not? Either is fine with me, but please be
consistent at least within each file.

[Philipp Keck, 2016/08/12 09:56:40]

This file is supposed to be without. Changed two instances above.

+  if (needDownloads)
+    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 (needRecentTabs &&
+        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 (needDownloads &&
+               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 BookmarkModel getter and that doesn't do it
+  // already, filter out duplicate URLs for recent tabs here. Duplicates for
+  // downloads are fine.
+
+  if (needRecentTabs) {
+    observer()->OnNewSuggestions(
+        this, recent_tabs_category_,
+        GetMostRecentlyVisited(recent_tabs_category_, &recent_tab_items));
+  }
+  if (needDownloads) {
+    observer()->OnNewSuggestions(
+        this, downloads_category_,
+        GetMostRecentlyVisited(downloads_category_, &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 {

[Marc Treib, 2016/08/12 09:21:14]

Hrm, I'm not a fan of this kind of mutable input argument. Maybe pass it in by
value and use std::move at the call site? Or alternatively, split this method in
two: One that only does the sorting, and one that does the rest (and gets a
const ref).

[Philipp Keck, 2016/08/12 09:56:40]

Done.

+  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) {
+  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) {
   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