MostVisitedSites: simplify prefs

components/ntp_tiles/most_visited_sites.cc

 // Copyright 2013 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_tiles/most_visited_sites.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
@@ skipping 59 matching lines @@
   if (cmd_line->HasSwitch(ntp_tiles::switches::kDisableNTPPopularSites))
     return false;
   if (cmd_line->HasSwitch(ntp_tiles::switches::kEnableNTPPopularSites))
     return true;
   return base::StartsWith(group_name, "Enabled",
                           base::CompareCase::INSENSITIVE_ASCII);
 }
 
 // Determine whether we need any popular suggestions to fill up a grid of
 // |num_tiles| tiles.
-bool NeedPopularSites(const PrefService* prefs, size_t num_tiles) {
+bool NeedPopularSites(const PrefService* prefs, int num_tiles) {
+  if (num_tiles <= prefs->GetInteger(prefs::kNumPersonalSuggestions))
+    return false;
+
+  // TODO(treib): Remove after M55.
   const base::ListValue* source_list =
-      prefs->GetList(ntp_tiles::prefs::kNTPSuggestionsIsPersonal);
+      prefs->GetList(ntp_tiles::prefs::kDeprecatedNTPSuggestionsIsPersonal);

[sfiera, 2016/07/08 09:59:15]

How important is this to preserve? Seems like a user would hit popular sites
once after upgrading, then never again, using maybe 1kB of unnecessary data and
delaying startup after first launch by a few ms.

There's nothing wrong with keeping it, though.

[Marc Treib, 2016/07/08 10:02:17]

Well, accumulated, it'd still be a few hundred million unnecessary requests.
Since we have some other "remove after M55" code around, I see no reason not to
keep this as well.

   // If there aren't enough previous suggestions to fill the grid, we need
   // popular suggestions.
-  if (source_list->GetSize() < num_tiles)
+  if (static_cast<int>(source_list->GetSize()) < num_tiles)
     return true;
   // Otherwise, if any of the previous suggestions is not personal, then also
   // get popular suggestions.
-  for (size_t i = 0; i < num_tiles; ++i) {
+  for (int i = 0; i < num_tiles; ++i) {
     bool is_personal = false;
     if (source_list->GetBoolean(i, &is_personal) && !is_personal)
       return true;
   }
   // The whole grid is already filled with personal suggestions, no point in
   // bothering with popular ones.
   return false;
 }
 
 bool AreURLsEquivalent(const GURL& url1, const GURL& url2) {
@@ skipping 164 matching lines @@
   LogHistogramEvent(histogram, tile_type, NUM_TILE_TYPES);
 }
 
 void MostVisitedSites::OnBlockedSitesChanged() {
   BuildCurrentSuggestions();
 }
 
 // static
 void MostVisitedSites::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
-  // TODO(treib): Remove this, it's unused. Do we need migration code to clean
-  // up existing entries?
-  registry->RegisterListPref(ntp_tiles::prefs::kNTPSuggestionsURL);
-  // TODO(treib): Remove this. It's only used to determine if we need
-  // PopularSites at all. Find a way to do that without prefs, or failing that,
-  // replace this list pref by a simple bool.
-  registry->RegisterListPref(ntp_tiles::prefs::kNTPSuggestionsIsPersonal);
+  registry->RegisterIntegerPref(prefs::kNumPersonalSuggestions, 0);
+  // TODO(treib): Remove after M55.
+  registry->RegisterListPref(prefs::kDeprecatedNTPSuggestionsURL);
+  registry->RegisterListPref(prefs::kDeprecatedNTPSuggestionsIsPersonal);
 }
 
 void MostVisitedSites::BuildCurrentSuggestions() {
   // Get the current suggestions from cache. If the cache is empty, this will
   // fall back to TopSites.
   OnSuggestionsProfileAvailable(
       suggestions_service_->GetSuggestionsDataFromCache());
 }
 
 void MostVisitedSites::InitiateTopSitesQuery() {
@@ skipping 173 matching lines @@
   size_t num_actual_tiles = personal_suggestions.size() +
                             whitelist_suggestions.size() +
                             popular_sites_suggestions.size();
   DCHECK_LE(num_actual_tiles, static_cast<size_t>(num_sites_));
 
   current_suggestions_ = MergeSuggestions(std::move(personal_suggestions),
                                           std::move(whitelist_suggestions),
                                           std::move(popular_sites_suggestions));
   DCHECK_EQ(num_actual_tiles, current_suggestions_.size());
 
-  if (received_popular_sites_)
-    SaveCurrentSuggestionsToPrefs();
+  int num_personal_suggestions = 0;
+  for (const auto& suggestion : current_suggestions_) {
+    if (suggestion.source != POPULAR)
+      num_personal_suggestions++;
+  }
+  prefs_->SetInteger(prefs::kNumPersonalSuggestions, num_personal_suggestions);
+  // TODO(treib): Remove after M55.
+  prefs_->ClearPref(prefs::kDeprecatedNTPSuggestionsIsPersonal);
+  prefs_->ClearPref(prefs::kDeprecatedNTPSuggestionsURL);
 }
 
 // static
 MostVisitedSites::SuggestionsVector MostVisitedSites::MergeSuggestions(
     SuggestionsVector personal_suggestions,
     SuggestionsVector whitelist_suggestions,
     SuggestionsVector popular_suggestions) {
   SuggestionsVector merged_suggestions;
   AppendSuggestions(std::move(personal_suggestions), &merged_suggestions);
   AppendSuggestions(std::move(whitelist_suggestions), &merged_suggestions);
   AppendSuggestions(std::move(popular_suggestions), &merged_suggestions);
   return merged_suggestions;
 }
 
-void MostVisitedSites::SaveCurrentSuggestionsToPrefs() {
-  base::ListValue url_list;
-  base::ListValue source_list;
-  for (const auto& suggestion : current_suggestions_) {
-    url_list.AppendString(suggestion.url.spec());
-    source_list.AppendBoolean(suggestion.source != POPULAR);
-  }
-  prefs_->Set(ntp_tiles::prefs::kNTPSuggestionsIsPersonal, source_list);
-  prefs_->Set(ntp_tiles::prefs::kNTPSuggestionsURL, url_list);
-}
-
 void MostVisitedSites::NotifyMostVisitedURLsObserver() {
   if (received_most_visited_sites_ && received_popular_sites_ &&
       !recorded_uma_) {
     RecordImpressionUMAMetrics();
     UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.NumberOfTiles",
                                 current_suggestions_.size());
     recorded_uma_ = true;
   }
 
   if (!observer_)
@@ skipping 31 matching lines @@
 
 void MostVisitedSites::TopSitesChanged(TopSites* top_sites,
                                        ChangeReason change_reason) {
   if (mv_source_ == TOP_SITES) {
     // The displayed suggestions are invalidated.
     InitiateTopSitesQuery();
   }
 }
 
 }  // namespace ntp_tiles