From 306034876eeb1f94705e5d0f852abae3abb2e12e Mon Sep 17 00:00:00 2001 From: Robbie Gibson Date: Thu, 8 Jun 2023 23:03:33 +0000 Subject: [PATCH] [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 cc42a3dd6c25ea42062b860a38720c77befd29c5) Bug: 1448271 Change-Id: Ib811a3c96771e6d0a1d39c4b7699458d6fe1fc05 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4557210 Reviewed-by: Benjamin Williams Reviewed-by: Rohit Rao Commit-Queue: Robbie Gibson Cr-Original-Commit-Position: refs/heads/main@{#1151333} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4594118 Bot-Commit: Rubber Stamper Cr-Commit-Position: refs/branch-heads/5790@{#508} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../shared/model/prefs/browser_prefs.mm | 1 + .../browser/shared/model/prefs/pref_names.cc | 5 ++ .../browser/shared/model/prefs/pref_names.h | 1 + .../destination_usage_history.mm | 13 +++++ .../destination_usage_history_unittest.mm | 4 +- .../overflow_menu_mediator_unittest.mm | 2 + .../overflow_menu/overflow_menu_orderer.mm | 38 ++++++++----- .../overflow_menu_orderer_unittest.mm | 55 +++++++++++++++++-- 8 files changed, 98 insertions(+), 21 deletions(-) diff --git a/ios/chrome/browser/shared/model/prefs/browser_prefs.mm b/ios/chrome/browser/shared/model/prefs/browser_prefs.mm index be46b2f07913f..e2a65da1c645f 100644 --- a/ios/chrome/browser/shared/model/prefs/browser_prefs.mm +++ b/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); diff --git a/ios/chrome/browser/shared/model/prefs/pref_names.cc b/ios/chrome/browser/shared/model/prefs/pref_names.cc index 4c35b2a0aac88..f078e9b8c4d91 100644 --- a/ios/chrome/browser/shared/model/prefs/pref_names.cc +++ b/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"; diff --git a/ios/chrome/browser/shared/model/prefs/pref_names.h b/ios/chrome/browser/shared/model/prefs/pref_names.h index d1b354ab4ad4c..5b4119e528f26 100644 --- a/ios/chrome/browser/shared/model/prefs/pref_names.h +++ b/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[]; diff --git a/ios/chrome/browser/ui/popup_menu/overflow_menu/destination_usage_history/destination_usage_history.mm b/ios/chrome/browser/ui/popup_menu/overflow_menu/destination_usage_history/destination_usage_history.mm index 77a79b1f9db19..0a638a45bb5ce 100644 --- a/ios/chrome/browser/ui/popup_menu/overflow_menu/destination_usage_history/destination_usage_history.mm +++ b/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) { diff --git a/ios/chrome/browser/ui/popup_menu/overflow_menu/destination_usage_history/destination_usage_history_unittest.mm b/ios/chrome/browser/ui/popup_menu/overflow_menu/destination_usage_history/destination_usage_history_unittest.mm index cb4f265a229ef..1b469b3a0d0f3 100644 --- a/ios/chrome/browser/ui/popup_menu/overflow_menu/destination_usage_history/destination_usage_history_unittest.mm +++ b/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); diff --git a/ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_mediator_unittest.mm b/ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_mediator_unittest.mm index c111bdf3aca5d..ac91d6b54d908 100644 --- a/ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_mediator_unittest.mm +++ b/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() { diff --git a/ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_orderer.mm b/ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_orderer.mm index ea322d07d2022..ca787c2c9402d 100644 --- a/ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_orderer.mm +++ b/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& 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,11 +157,28 @@ - (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 { @@ -173,8 +186,6 @@ - (void)flushToPrefs { return; } - ScopedDictPrefUpdate historyUpdate( - _localStatePrefs, prefs::kOverflowMenuDestinationUsageHistory); // Flush the new ranking to Prefs. base::Value::List ranking; @@ -182,7 +193,8 @@ ScopedDictPrefUpdate historyUpdate( ranking.Append(overflow_menu::StringNameForDestination(destination)); } - historyUpdate->Set(kRankingKey, std::move(ranking)); + _localStatePrefs->SetList(prefs::kOverflowMenuDestinationsOrder, + std::move(ranking)); } - (NSArray*) diff --git a/ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_orderer_unittest.mm b/ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_orderer_unittest.mm index 75875da9646ca..941086d00bca1 100644 --- a/ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_orderer_unittest.mm +++ b/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()); }