Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(826)

Unified Diff: components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc

Issue 2245583002: Split OfflinePageSuggestions into two categories (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@offlinedismissed
Patch Set: Marc's comments Created 4 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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 (unna
Philipp Keck 2016/08/12 10:22:55 This is from example code of std::sort. Otherwise,
Bernhard Bauer 2016/08/12 10:28:31 Is that going to create a static initializer? If s
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 nic
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

Powered by Google App Engine
This is Rietveld 408576698