[iOS] Migrate OverflowMenu ranking to new pref and prevent wiping it

There was a bug where the DestinationUsageHistory would wipe and re-
write the kOverflowMenuDestinationUsageHistory pref without filling
in the ranking, and the OverflowMenuOrderer might not get a chance to
add the ranking list back in.

This solves the issue in two ways. First, DestinationUsageHistory
carries over any stored ranking in the pref when it's writing prefs
back out. Second, OverflowMenuOrderer migrates the ranking to a new
separate pref key.

(cherry picked from commit cc42a3d)

Bug: 1448271
Change-Id: Ib811a3c96771e6d0a1d39c4b7699458d6fe1fc05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4557210
Reviewed-by: Benjamin Williams <bwwilliams@google.com>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Robbie Gibson <rkgibson@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1151333}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4594118
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5790@{#508}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}

ios/chrome/browser/shared/model/prefs/browser_prefs.mm

@@ -195,6 +195,7 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
                                    PrefRegistry::LOSSY_PREF);
   registry->RegisterListPref(prefs::kOverflowMenuNewDestinations,
                              PrefRegistry::LOSSY_PREF);
+  registry->RegisterListPref(prefs::kOverflowMenuDestinationsOrder);
 
   // Preferences related to Enterprise policies.
   registry->RegisterListPref(prefs::kRestrictAccountsToPatterns);

ios/chrome/browser/shared/model/prefs/pref_names.cc

@@ -250,6 +250,11 @@ const char kOverflowMenuDestinationUsageHistory[] =
 // carousel.
 const char kOverflowMenuNewDestinations[] = "overflow_menu.new_destinations";
 
+// List preference which tracks the current order of the overflow menu's
+// destinations.
+const char kOverflowMenuDestinationsOrder[] =
+    "overflow_menu.destinations_order";
+
 // Boolean that is true when Suggest support is enabled.
 const char kSearchSuggestEnabled[] = "search.suggest_enabled";
 

ios/chrome/browser/shared/model/prefs/pref_names.h

@@ -62,6 +62,7 @@ extern const char kNTPFollowingFeedSortType[];
 extern const char kDefaultFollowingFeedSortTypeChanged[];
 extern const char kOverflowMenuDestinationUsageHistory[];
 extern const char kOverflowMenuNewDestinations[];
+extern const char kOverflowMenuDestinationsOrder[];
 extern const char kPrintingEnabled[];
 extern const char kSearchSuggestEnabled[];
 extern const char kTrackPricesOnTabsEnabled[];

ios/chrome/browser/ui/popup_menu/overflow_menu/destination_usage_history/destination_usage_history.mm

@@ -486,9 +486,22 @@ - (void)flushToPrefs {
   ScopedDictPrefUpdate historyUpdate(
       _prefService, prefs::kOverflowMenuDestinationUsageHistory);
 
+  base::Value::List* possibleStoredRanking =
+      historyUpdate->FindList(kRankingKey);
+  bool hasStoredRanking = possibleStoredRanking != nullptr;
+  base::Value::List storedRanking;
+  if (hasStoredRanking) {
+    storedRanking = possibleStoredRanking->Clone();
+  }
+
   // Clear the existing usage history and ranking stored in Prefs.
   historyUpdate->clear();
 
+  // Keep stored ranking if it still exists.
+  if (hasStoredRanking) {
+    historyUpdate->Set(kRankingKey, storedRanking.Clone());
+  }
+
   // Flush the new usage history to Prefs.
   for (const auto& dayHistory : _usageHistory) {
     for (const auto& dayUsage : dayHistory.second) {

ios/chrome/browser/ui/popup_menu/overflow_menu/destination_usage_history/destination_usage_history_unittest.mm

@@ -491,8 +491,8 @@ ScopedDictPrefUpdate update(prefs_.get(),
   ScopedDictPrefUpdate update(prefs_.get(),
                               prefs::kOverflowMenuDestinationUsageHistory);
 
-  // Has one entry for today's seeded history.
-  EXPECT_EQ(update->size(), (size_t)1);
+  // Has one entry for ranking, and one entry for today's seeded history.
+  EXPECT_EQ(update->size(), (size_t)2);
   EXPECT_NE(update->Find(base::NumberToString(TodaysDay().InDays())), nullptr);
   EXPECT_EQ(update->Find(base::NumberToString(recently_expired_day.InDays())),
             nullptr);

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_mediator_unittest.mm

@@ -225,6 +225,8 @@ void CreateLocalStatePrefs() {
         prefs::kOverflowMenuNewDestinations, PrefRegistry::LOSSY_PREF);
     localStatePrefs_->registry()->RegisterDictionaryPref(
         prefs::kOverflowMenuDestinationUsageHistory, PrefRegistry::LOSSY_PREF);
+    localStatePrefs_->registry()->RegisterListPref(
+        prefs::kOverflowMenuDestinationsOrder);
   }
 
   void SetUpBookmarks() {

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_orderer.mm

@@ -76,13 +76,9 @@
 // converts each string to an overflow_menu::Destination, then appends each
 // destination to a vector (`to` vector). Skips over invalid or malformed list
 // items.
-void AppendDestinationsToVector(const base::Value::List* from,
+void AppendDestinationsToVector(const base::Value::List& from,
                                 std::vector<overflow_menu::Destination>& to) {
-  if (!from) {
-    return;
-  }
-  const base::Value::List& fromRef = *from;
-  for (const auto& value : fromRef) {
+  for (const auto& value : from) {
     if (!value.is_string()) {
       continue;
     }
@@ -161,28 +157,44 @@ - (void)recordClickForDestination:(overflow_menu::Destination)destination {
 #pragma mark - Private
 
 - (void)loadDataFromPrefs {
-  const base::Value::Dict& storedUsageHistory =
-      _localStatePrefs->GetDict(prefs::kOverflowMenuDestinationUsageHistory);
+  // First try to load new pref.
+  const base::Value::List& storedRanking =
+      _localStatePrefs->GetList(prefs::kOverflowMenuDestinationsOrder);
+  if (storedRanking.size() > 0) {
+    AppendDestinationsToVector(storedRanking, _ranking);
+    return;
+  }
 
-  AppendDestinationsToVector(storedUsageHistory.FindList(kRankingKey),
-                             _ranking);
+  // Fall back to old key.
+  ScopedDictPrefUpdate storedUsageHistoryUpdate(
+      _localStatePrefs, prefs::kOverflowMenuDestinationUsageHistory);
+  base::Value::List* oldRanking =
+      storedUsageHistoryUpdate->FindList(kRankingKey);
+  if (!oldRanking) {
+    return;
+  }
+  base::Value::List& oldRankingRef = *oldRanking;
+  _localStatePrefs->SetList(prefs::kOverflowMenuDestinationsOrder,
+                            oldRankingRef.Clone());
+
+  AppendDestinationsToVector(oldRankingRef, _ranking);
+  storedUsageHistoryUpdate->Remove(kRankingKey);
 }
 
 - (void)flushToPrefs {
   if (!_localStatePrefs) {
     return;
   }
 
-  ScopedDictPrefUpdate historyUpdate(
-      _localStatePrefs, prefs::kOverflowMenuDestinationUsageHistory);
   // Flush the new ranking to Prefs.
   base::Value::List ranking;
 
   for (overflow_menu::Destination destination : _ranking) {
     ranking.Append(overflow_menu::StringNameForDestination(destination));
   }
 
-  historyUpdate->Set(kRankingKey, std::move(ranking));
+  _localStatePrefs->SetList(prefs::kOverflowMenuDestinationsOrder,
+                            std::move(ranking));
 }
 
 - (NSArray<OverflowMenuDestination*>*)

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_orderer_unittest.mm

@@ -8,6 +8,7 @@
 #error "This file requires ARC support."
 #endif
 
+#import "base/strings/string_number_conversions.h"
 #import "base/values.h"
 #import "components/prefs/pref_registry_simple.h"
 #import "components/prefs/testing_pref_service.h"
@@ -27,6 +28,12 @@
 // based on device size.
 static constexpr int kVisibleDestinationsCount = 5;
 
+// A time delta from the Unix epoch to the beginning of the current day.
+base::TimeDelta TodaysDay() {
+  return base::Days(
+      (base::Time::Now() - base::Time::UnixEpoch()).InDaysFloored());
+}
+
 }  // namespace
 
 class OverflowMenuOrdererTest : public PlatformTest {
@@ -58,6 +65,7 @@ void CreatePrefs() {
         prefs::kOverflowMenuDestinationUsageHistory, PrefRegistry::LOSSY_PREF);
     prefs_->registry()->RegisterListPref(prefs::kOverflowMenuNewDestinations,
                                          PrefRegistry::LOSSY_PREF);
+    prefs_->registry()->RegisterListPref(prefs::kOverflowMenuDestinationsOrder);
   }
 
   OverflowMenuDestination* CreateOverflowMenuDestination(
@@ -122,12 +130,47 @@ void CreatePrefs() {
   [overflow_menu_orderer_
       sortedDestinationsFromCarouselDestinations:sample_destinations];
 
-  const base::Value::Dict& stored_usage_history =
+  const base::Value::List& stored_ranking =
+      prefs_->GetList(prefs::kOverflowMenuDestinationsOrder);
+
+  EXPECT_EQ(stored_ranking.size(), sample_destinations.count);
+}
+
+// Tests that the old pref format (kOverflowMenuDestinationUsageHistory as a
+// dict containing both usage history and ranking) is correctly migrated to the
+// new format (kOverflowMenuDestinationUsageHistory containing just usage
+// history and kOverflowMenuDestinationsOrder containing ranking).
+TEST_F(OverflowMenuOrdererTest, MigratesRanking) {
+  CreatePrefs();
+
+  base::Value::List old_ranking =
+      base::Value::List()
+          .Append(overflow_menu::StringNameForDestination(
+              overflow_menu::Destination::Bookmarks))
+          .Append(overflow_menu::StringNameForDestination(
+              overflow_menu::Destination::History));
+  base::Value::Dict old_usage_history =
+      base::Value::Dict()
+          .Set(base::NumberToString(TodaysDay().InDays()),
+               base::Value::Dict().Set(
+                   overflow_menu::StringNameForDestination(
+                       overflow_menu::Destination::Bookmarks),
+                   5))
+          .Set("ranking", old_ranking.Clone());
+
+  prefs_->SetDict(prefs::kOverflowMenuDestinationUsageHistory,
+                  std::move(old_usage_history));
+
+  overflow_menu_orderer_ = [[OverflowMenuOrderer alloc] initWithIsIncognito:NO];
+
+  // Set prefs here to force orderer to load and migrate.
+  overflow_menu_orderer_.localStatePrefs = prefs_.get();
+
+  const base::Value::List& new_ranking =
+      prefs_->GetList(prefs::kOverflowMenuDestinationsOrder);
+  const base::Value::Dict& new_usage_history =
       prefs_->GetDict(prefs::kOverflowMenuDestinationUsageHistory);
 
-  // Has two entries for the ranking and usage history.
-  EXPECT_EQ(stored_usage_history.size(), (size_t)2);
-  EXPECT_NE(stored_usage_history.FindList("ranking"), nullptr);
-  EXPECT_EQ(stored_usage_history.FindList("ranking")->size(),
-            sample_destinations.count);
+  EXPECT_EQ(new_ranking, old_ranking);
+  EXPECT_EQ(1ul, new_usage_history.size());
 }