[NTP Snippets] Persist remote categories in prefs

components/ntp_snippets/remote/ntp_snippets_service.cc

Index: components/ntp_snippets/remote/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/remote/ntp_snippets_service.cc b/components/ntp_snippets/remote/ntp_snippets_service.cc
index 78aea3a04b60edba453d0f2562906971ae05a88e..610b2ecc68d521740e175b6f1851ede2301f9ac8 100644
--- a/components/ntp_snippets/remote/ntp_snippets_service.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_service.cc
@@ -193,12 +193,19 @@ NTPSnippetsService::NTPSnippetsService(
       thumbnail_requests_throttler_(
           pref_service,
           RequestThrottler::RequestType::CONTENT_SUGGESTION_THUMBNAIL) {
-  // Articles category always exists; others will be added as needed.
-  categories_[articles_category_] = CategoryContent();
-  categories_[articles_category_].localized_title =
-      l10n_util::GetStringUTF16(IDS_NTP_ARTICLE_SUGGESTIONS_SECTION_HEADER);
-  observer->OnCategoryStatusChanged(this, articles_category_,
-                                    categories_[articles_category_].status);
+  RestoreCategoriesFromPrefs();
+  // The articles category always exists. Add it if we didn't get it from prefs.
+  // TODO(treib): Rethink this.
+  if (!base::ContainsKey(categories_, articles_category_)) {
+    categories_[articles_category_] = CategoryContent();
+    categories_[articles_category_].localized_title =
+        l10n_util::GetStringUTF16(IDS_NTP_ARTICLE_SUGGESTIONS_SECTION_HEADER);
+  }
+  // Tell the observer about all the categories.
+  for (const auto& entry : categories_) {
+    observer->OnCategoryStatusChanged(this, entry.first, entry.second.status);
+  }
+
   if (database_->IsErrorState()) {
     EnterState(State::ERROR_OCCURRED);
     UpdateAllCategoryStatus(CategoryStatus::LOADING_ERROR);
@@ -218,7 +225,10 @@ NTPSnippetsService::~NTPSnippetsService() = default;
 
 // static
 void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
-  registry->RegisterListPref(prefs::kSnippetHosts); //TODO remove
+  // TODO(treib): Add cleanup logic for prefs::kSnippetHosts, then remove it
+  // completely after M56.
+  registry->RegisterListPref(prefs::kSnippetHosts);
+  registry->RegisterListPref(prefs::kRemoteSuggestionCategories);
   registry->RegisterInt64Pref(prefs::kSnippetBackgroundFetchingIntervalWifi, 0);
   registry->RegisterInt64Pref(prefs::kSnippetBackgroundFetchingIntervalFallback,
                               0);
@@ -299,12 +309,12 @@ void NTPSnippetsService::RescheduleFetching(bool force) {
 }
 
 CategoryStatus NTPSnippetsService::GetCategoryStatus(Category category) {
-  DCHECK(categories_.find(category) != categories_.end());
+  DCHECK(base::ContainsKey(categories_, category));
   return categories_[category].status;
 }
 
 CategoryInfo NTPSnippetsService::GetCategoryInfo(Category category) {
-  DCHECK(categories_.find(category) != categories_.end());
+  DCHECK(base::ContainsKey(categories_, category));
   const CategoryContent& content = categories_[category];
   return CategoryInfo(content.localized_title,
                       ContentSuggestionsCardLayout::FULL_CARD,
@@ -363,7 +373,7 @@ void NTPSnippetsService::ClearCachedSuggestions(Category category) {
   if (!initialized())
     return;
 
-  if (categories_.find(category) == categories_.end())
+  if (!base::ContainsKey(categories_, category))
     return;
   CategoryContent* content = &categories_[category];
   if (content->snippets.empty())
@@ -381,7 +391,7 @@ void NTPSnippetsService::ClearCachedSuggestions(Category category) {
 void NTPSnippetsService::GetDismissedSuggestionsForDebugging(
     Category category,
     const DismissedSuggestionsCallback& callback) {
-  DCHECK(categories_.find(category) != categories_.end());
+  DCHECK(base::ContainsKey(categories_, category));
 
   std::vector<ContentSuggestion> result;
   const CategoryContent& content = categories_[category];
@@ -404,7 +414,7 @@ void NTPSnippetsService::GetDismissedSuggestionsForDebugging(
 
 void NTPSnippetsService::ClearDismissedSuggestionsForDebugging(
     Category category) {
-  DCHECK(categories_.find(category) != categories_.end());
+  DCHECK(base::ContainsKey(categories_, category));
 
   if (!initialized())
     return;
@@ -430,7 +440,7 @@ int NTPSnippetsService::GetMaxSnippetCountForTesting() {
 
 GURL NTPSnippetsService::FindSnippetImageUrl(
     const ContentSuggestion::ID& suggestion_id) const {
-  DCHECK(categories_.find(suggestion_id.category()) != categories_.end());
+  DCHECK(base::ContainsKey(categories_, suggestion_id.category()));
 
   const CategoryContent& content = categories_.at(suggestion_id.category());
   const NTPSnippet* snippet =
@@ -467,10 +477,9 @@ void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
   if (state_ == State::ERROR_OCCURRED)
     return;
   DCHECK(state_ == State::NOT_INITED);
-  DCHECK_EQ(1u, categories_.size());  // Only articles category, so far.
-  DCHECK(categories_.find(articles_category_) != categories_.end());
+  DCHECK(base::ContainsKey(categories_, articles_category_));
 
-  // TODO(sfiera): support non-article categories in database.
+  // TODO(treib): Support non-article categories in database. crbug.com/653476
   CategoryContent* content = &categories_[articles_category_];
   for (std::unique_ptr<NTPSnippet>& snippet : snippets) {
     if (snippet->is_dismissed())
@@ -503,6 +512,8 @@ void NTPSnippetsService::OnFetchFinished(
   if (!ready())
     return;
 
+  // Mark all categories as not provided by the server in the latest fetch. The
+  // ones we got will be marked again below.
   for (auto& item : categories_) {
     CategoryContent* content = &item.second;
     content->provided_by_server = false;
@@ -514,6 +525,8 @@ void NTPSnippetsService::OnFetchFinished(
   // If snippets were fetched successfully, update our |categories_| from each
   // category provided by the server.
   if (fetched_categories) {
+    // TODO(treib): Reorder |categories_| to match the order we received from
+    // the server. crbug.com/653816
     // TODO(jkrcal): A bit hard to understand with so many variables called
     // "*categor*". Isn't here some room for simplification?
     for (NTPSnippetsFetcher::FetchedCategory& fetched_category :
@@ -545,6 +558,10 @@ void NTPSnippetsService::OnFetchFinished(
     }
   }
 
+  // We might have gotten new categories (or updated the titles of existing
+  // ones), so update the pref.
+  StoreCategoriesToPrefs();
+
   for (const auto& item : categories_) {
     Category category = item.first;
     UpdateCategoryStatus(category, CategoryStatus::AVAILABLE);
@@ -574,7 +591,7 @@ void NTPSnippetsService::ArchiveSnippets(Category category,
                                          NTPSnippet::PtrVector* to_archive) {
   CategoryContent* content = &categories_[category];
 
-  // TODO(sfiera): handle DB for non-articles too.
+  // TODO(treib): Handle DB for non-articles too. crbug.com/653476
   if (category == articles_category_) {
     database_->DeleteSnippets(GetSnippetIDVector(*to_archive));
     // Do not delete the thumbnail images as they are still handy on open NTPs.
@@ -692,6 +709,8 @@ void NTPSnippetsService::ClearExpiredDismissedSnippets() {
     UpdateCategoryStatus(category, CategoryStatus::NOT_PROVIDED);
     categories_.erase(category);
   }
+
+  StoreCategoriesToPrefs();
 }
 
 void NTPSnippetsService::ClearOrphanedImages() {
@@ -726,6 +745,8 @@ void NTPSnippetsService::NukeAllSnippets() {
   for (Category category : categories_to_erase) {
     categories_.erase(category);
   }
+
+  StoreCategoriesToPrefs();
 }
 
 void NTPSnippetsService::OnSnippetImageFetchedFromDatabase(
@@ -762,7 +783,7 @@ void NTPSnippetsService::OnSnippetImageDecodedFromDatabase(
 void NTPSnippetsService::FetchSnippetImageFromNetwork(
     const ContentSuggestion::ID& suggestion_id,
     const ImageFetchedCallback& callback) {
-  if (categories_.find(suggestion_id.category()) == categories_.end()) {
+  if (!base::ContainsKey(categories_, suggestion_id.category())) {
     OnSnippetImageDecodedFromNetwork(
         callback, suggestion_id.id_within_category(), gfx::Image());
     return;
@@ -967,7 +988,7 @@ void NTPSnippetsService::NotifyNewSuggestions() {
 
 void NTPSnippetsService::UpdateCategoryStatus(Category category,
                                               CategoryStatus status) {
-  DCHECK(categories_.find(category) != categories_.end());
+  DCHECK(base::ContainsKey(categories_, category));
   CategoryContent& content = categories_[category];
   if (status == content.status)
     return;
@@ -1007,6 +1028,81 @@ const NTPSnippet* NTPSnippetsService::CategoryContent::FindSnippet(
   return nullptr;
 }
 
+namespace {
+
+const char kCategoryContentId[] = "id";

[Bernhard Bauer, 2016/10/07 16:21:27]

Hm... Shouldn't the anonymous namespace go to the top of the file?

[Marc Treib, 2016/10/10 08:33:43]

That's certainly the common thing, though the style guide doesn't actually
enforce it as far as I can tell. In fact, from
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts:
"Note that if there is more than one class in the .cc file, it can aid clarity
to define your file-scope helpers in an anonymous namespace just above the class
that uses them, instead of at the top of the file."
(Which of course doesn't actually apply here, since it's not a separate
class...)
Oh well, moved it to the top. Consistency!

+const char kCategoryContentTitle[] = "title";
+const char kCategoryContentProvidedByServer[] = "provided_by_server";
+
+}  // namespace
+
+void NTPSnippetsService::RestoreCategoriesFromPrefs() {
+  // This must only be called at startup, before there are any categories.
+  DCHECK(categories_.empty());
+
+  const base::ListValue* list =
+      pref_service_->GetList(prefs::kRemoteSuggestionCategories);
+  for (const std::unique_ptr<base::Value>& entry : *list) {
+    const base::DictionaryValue* dict = nullptr;
+    if (!entry->GetAsDictionary(&dict)) {
+      DLOG(WARNING) << "Invalid category pref value: " << *entry;
+      continue;
+    }
+    int id = 0;
+    if (!dict->GetInteger(kCategoryContentId, &id)) {
+      DLOG(WARNING) << "Invalid category pref value, missing '"
+                    << kCategoryContentId << "': " << *entry;
+      continue;
+    }
+    base::string16 title;
+    if (!dict->GetString(kCategoryContentTitle, &title)) {
+      DLOG(WARNING) << "Invalid category pref value, missing '"
+                    << kCategoryContentTitle << "': " << *entry;
+      continue;
+    }
+    bool provided_by_server = false;
+    if (!dict->GetBoolean(kCategoryContentProvidedByServer,
+                          &provided_by_server)) {
+      DLOG(WARNING) << "Invalid category pref value, missing '"
+                    << kCategoryContentProvidedByServer << "': " << *entry;
+      continue;
+    }
+
+    Category category = category_factory()->FromIDValue(id);
+    categories_[category] = CategoryContent();
+    categories_[category].localized_title = title;
+    categories_[category].provided_by_server = provided_by_server;
+  }
+}
+
+void NTPSnippetsService::StoreCategoriesToPrefs() {
+  // Collect all the CategoryContents.
+  std::vector<std::pair<Category, const CategoryContent*>> to_store;
+  for (const auto& entry : categories_)
+    to_store.emplace_back(entry.first, &entry.second);
+  // Sort them into the proper category order.
+  std::sort(to_store.begin(), to_store.end(),
+            [this](const std::pair<Category, const CategoryContent*>& left,
+                   const std::pair<Category, const CategoryContent*>& right) {
+              return category_factory()->CompareCategories(left.first,
+                                                           right.first);
+            });
+  // Convert the relevant info into a base::ListValue for storage.
+  base::ListValue list;
+  for (const auto& entry : to_store) {
+    Category category = entry.first;
+    const base::string16& title = entry.second->localized_title;
+    bool provided_by_server = entry.second->provided_by_server;
+    auto dict = base::MakeUnique<base::DictionaryValue>();
+    dict->SetInteger(kCategoryContentId, category.id());
+    dict->SetString(kCategoryContentTitle, title);
+    dict->SetBoolean(kCategoryContentProvidedByServer, provided_by_server);
+    list.Append(std::move(dict));
+  }
+  // Finally, store the result in the pref service.
+  pref_service_->Set(prefs::kRemoteSuggestionCategories, list);
+}
+
 NTPSnippetsService::CategoryContent::CategoryContent() = default;
 NTPSnippetsService::CategoryContent::CategoryContent(CategoryContent&&) =
     default;