SyncToSigninMigration: Plumb migration logic on all platforms
No behavioral changes as the feature is disabled by default. The rest of
the changes, including IdentityManager's pref registration, are pure
refactorings (at least outside tests).
The ios-specific logic in browser_prefs.mm, introduced in
https://crrev.com/c/4983933, is now ported to other platforms, which use
the non-ios counterpart browser_prefs.cc.
A browser test is introduced for Bookmarks, which diverge most from
iOS in terms of how the storage of account bookmarks is implemented.
These integration tests also prove that the iOS implementation can be
reused as is. Therefore, the ifdefs are removed from
GetSyncToSigninMigrationDecision(), the implementation unified and the
unit-tests enabled everywhere. Outside iOS, the plan is that a single
BookmarkModel exists, but it will still use two separate JSON files for
storage, and hence the migration logic can be shared across platforms.
Change-Id: I0e3816b1742ac77e396c525f9ea6e7af3afd33ed
Bug: 40282890, 41493391
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5307157
Commit-Queue: Mikel Astiz <[email protected]>
Reviewed-by: Victor Vianna <[email protected]>
Reviewed-by: Marc Treib <[email protected]>
Reviewed-by: David Roger <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1265229}
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index 06a311d3..b0901ea 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -104,6 +104,7 @@
#include "components/autofill/core/common/autofill_prefs.h"
#include "components/blocked_content/safe_browsing_triggered_popup_blocker.h"
#include "components/breadcrumbs/core/breadcrumbs_status.h"
+#include "components/browser_sync/sync_to_signin_migration.h"
#include "components/browsing_data/core/pref_names.h"
#include "components/certificate_transparency/pref_names.h"
#include "components/commerce/core/pref_names.h"
@@ -2706,6 +2707,10 @@
// Deprecated 02/2024
profile_prefs->ClearPref(kResetCheckDefaultBrowser);
+ // Added 02/2024, but DO NOT REMOVE after the usual year!
+ // TODO(crbug.com/40282890): Remove ~one year after full launch.
+ browser_sync::MaybeMigrateSyncingUserToSignedIn(profile_path, profile_prefs);
+
// Please don't delete the following line. It is used by PRESUBMIT.py.
// END_MIGRATE_OBSOLETE_PROFILE_PREFS
diff --git a/chrome/browser/prefs/browser_prefs_unittest.cc b/chrome/browser/prefs/browser_prefs_unittest.cc
index 1ad48da..d52d59f 100644
--- a/chrome/browser/prefs/browser_prefs_unittest.cc
+++ b/chrome/browser/prefs/browser_prefs_unittest.cc
@@ -8,9 +8,13 @@
#include "base/files/file_path.h"
#include "build/build_config.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.h"
+#include "chrome/browser/profiles/pref_service_builder_utils.h"
#include "components/performance_manager/public/user_tuning/prefs.h"
#include "components/sync/base/pref_names.h"
#include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "content/public/test/browser_task_environment.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace {
@@ -23,8 +27,20 @@
class BrowserPrefsTest : public testing::Test {
protected:
- BrowserPrefsTest() { RegisterUserProfilePrefs(prefs_.registry()); }
+ BrowserPrefsTest() {
+ // Invoking `RegisterUserProfilePrefs()` isn't sufficient as some migration
+ // code relies on prefs registered by the keyed service factories, via
+ // BrowserContextKeyedService::RegisterProfilePrefs().
+ ChromeBrowserMainExtraPartsProfiles::
+ EnsureBrowserContextKeyedServiceFactoriesBuilt();
+ RegisterProfilePrefs(/*is_signin_profile=*/false,
+ g_browser_process->GetApplicationLocale(),
+ prefs_.registry());
+ }
+ // The task environment is needed because some keyed services CHECK for things
+ // like content::BrowserThread::CurrentlyOn(content::BrowserThread::UI).
+ content::BrowserTaskEnvironment task_environment_;
sync_preferences::TestingPrefServiceSyncable prefs_;
};
diff --git a/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc b/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc
index da26245..5cc9008 100644
--- a/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc
@@ -32,6 +32,7 @@
#include "components/bookmarks/browser/bookmark_node.h"
#include "components/bookmarks/browser/url_and_title.h"
#include "components/bookmarks/common/bookmark_metrics.h"
+#include "components/browser_sync/browser_sync_switches.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"
#include "components/keyed_service/core/keyed_service.h"
#include "components/sync/base/client_tag_hash.h"
@@ -1702,7 +1703,8 @@
ExcludeDataTypesFromCheckForDataTypeFailures({syncer::BOOKMARKS});
}
-// Android doesn't currently support PRE_ tests, see crbug.com/1117345.
+// Android doesn't currently support PRE_ tests, see crbug.com/40200835 or
+// crbug.com/40145099.
#if !BUILDFLAG(IS_ANDROID)
IN_PROC_BROWSER_TEST_F(
SingleClientBookmarksSyncTestWithDisabledReuploadBookmarks,
@@ -2465,6 +2467,79 @@
}
#endif // !BUILDFLAG(IS_ANDROID)
+// Android doesn't currently support PRE_ tests, see crbug.com/40200835 or
+// crbug.com/40145099.
+#if !BUILDFLAG(IS_ANDROID)
+class SingleClientBookmarksSyncTestWithEnabledMigrateSyncingUserToSignedIn
+ : public SingleClientBookmarksWithAccountStorageSyncTest {
+ protected:
+ SingleClientBookmarksSyncTestWithEnabledMigrateSyncingUserToSignedIn() {
+ if (content::IsPreTest()) {
+ features_override_.InitAndDisableFeature(
+ switches::kMigrateSyncingUserToSignedIn);
+ } else {
+ features_override_.InitWithFeatures(
+ /*enabled_features=*/{syncer::kReplaceSyncPromosWithSignInPromos,
+ switches::kMigrateSyncingUserToSignedIn},
+ /*disabled_features=*/{});
+ }
+ }
+
+ const std::string kTestTitle = "Test Title";
+
+ private:
+ base::test::ScopedFeatureList features_override_;
+};
+
+IN_PROC_BROWSER_TEST_F(
+ SingleClientBookmarksSyncTestWithEnabledMigrateSyncingUserToSignedIn,
+ PRE_SyncToSigninMigration) {
+ ASSERT_TRUE(SetupClients());
+ AddFolder(kSingleProfileIndex, kTestTitle);
+
+ // Setup sync, wait for its completion, and make sure changes were synced.
+ ASSERT_TRUE(SetupSync());
+ ASSERT_TRUE(BookmarkModelMatchesFakeServerChecker(
+ kSingleProfileIndex, GetSyncService(kSingleProfileIndex),
+ GetFakeServer())
+ .Wait());
+
+ BookmarkModel* model = GetBookmarkModel(kSingleProfileIndex);
+
+ ASSERT_THAT(model->bookmark_bar_node()->children(),
+ ElementsAre(IsFolderWithTitle(kTestTitle)));
+
+ // Sync-the-feature is on, so account bookmarks should not exist.
+ ASSERT_THAT(model->account_bookmark_bar_node(), IsNull());
+}
+
+IN_PROC_BROWSER_TEST_F(
+ SingleClientBookmarksSyncTestWithEnabledMigrateSyncingUserToSignedIn,
+ SyncToSigninMigration) {
+ // Mimic the user being offline to verify that account bookmarks are loaded
+ // from disk instead of being redownloaded.
+ fake_server::FakeServerHttpPostProvider::DisableNetwork();
+
+ ASSERT_TRUE(SetupClients());
+ ASSERT_TRUE(GetClient(kSingleProfileIndex)->AwaitEngineInitialization());
+ ASSERT_FALSE(GetSyncService(kSingleProfileIndex)->IsSyncFeatureEnabled());
+ ASSERT_TRUE(GetSyncService(kSingleProfileIndex)
+ ->GetUserSettings()
+ ->GetSelectedTypes()
+ .Has(syncer::UserSelectableType::kBookmarks));
+
+ BookmarkModel* model = GetBookmarkModel(kSingleProfileIndex);
+
+ // Local bookmarks should be empty.
+ ASSERT_THAT(model->bookmark_bar_node()->children(), IsEmpty());
+
+ // Account bookmarks should continue existing even while offline.
+ ASSERT_THAT(model->account_bookmark_bar_node(), NotNull());
+ EXPECT_THAT(model->account_bookmark_bar_node()->children(),
+ ElementsAre(IsFolderWithTitle(kTestTitle)));
+}
+#endif // !BUILDFLAG(IS_ANDROID)
+
#endif // !BUILDFLAG(IS_CHROMEOS_ASH)
} // namespace
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index f47ed4b..a5ad9677 100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -1498,6 +1498,7 @@
"//chrome:chrome_android_core",
"//chrome/android:delegate_public_impl_java",
"//components/bookmarks/browser",
+ "//components/browser_sync:switches",
"//components/password_manager/core/browser/sharing",
"//components/reading_list/core:test_support",
"//components/reading_list/features:flags",
diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc
index 2967d98..eee6bf2 100644
--- a/chrome/test/base/testing_profile.cc
+++ b/chrome/test/base/testing_profile.cc
@@ -364,8 +364,6 @@
else
CreateTestingPrefService();
- MigrateObsoleteProfilePrefs(prefs_.get(), GetPath());
-
if (is_supervised_profile)
SetIsSupervisedProfile();
@@ -450,6 +448,7 @@
simple_dependency_manager_->RegisterProfilePrefsForServices(pref_registry);
browser_context_dependency_manager_->RegisterProfilePrefsForServices(
pref_registry);
+ MigrateObsoleteProfilePrefs(prefs_.get(), GetPath());
}
FullBrowserTransitionManager::Get()->OnProfileCreated(this);
diff --git a/components/browser_sync/sync_to_signin_migration.cc b/components/browser_sync/sync_to_signin_migration.cc
index b71f4ea..7741a73 100644
--- a/components/browser_sync/sync_to_signin_migration.cc
+++ b/components/browser_sync/sync_to_signin_migration.cc
@@ -88,7 +88,7 @@
// client), or has an unknown/invalid value (which should never happen).
return SyncToSigninMigrationDecision::kDontMigrateSyncStatusUndefined;
}
- // TODO(crbug.com/1486420): After some number of attempts, treat
+ // TODO(crbug.com/40282890): After some number of attempts, treat
// "initializing" or "undefined/unknown" as "Sync disabled" and go ahead with
// the migration?
@@ -156,17 +156,10 @@
// Selected-data-types prefs: No reverse migration - the user will just go
// back to their previous Sync settings.
-#if BUILDFLAG(IS_IOS)
// Bookmarks: The forward migration is an atomic file move. Either that
// happened, in which case the Sync machinery will clean up the account store
// and start over with the local-or-syncable store. Or the file move didn't
// happen for some reason. Either way, nothing to be done here.
-#else
- // TODO(crbug.com/1503647): On platforms other than iOS, the forward migration
- // for bookmarks isn't implemented yet, so the reverse migration can't be
- // implemented either.
- NOTIMPLEMENTED();
-#endif // BUILDFLAG(IS_IOS)
// Passwords: Same as bookmarks, this is an atomic file move. Nothing to be
// done here.
@@ -353,7 +346,6 @@
migration_successful && (error == base::File::Error::FILE_OK);
}
-#if BUILDFLAG(IS_IOS)
// Move bookmarks json file, if bookmark sync is enabled.
if (bookmarks_decision == SyncToSigninMigrationDataTypeDecision::kMigrate) {
base::FilePath from_path =
@@ -369,11 +361,6 @@
migration_successful =
migration_successful && (error == base::File::Error::FILE_OK);
}
-#else
- // TODO(crbug.com/1503647): On platforms other than iOS, the on-disk layout of
- // bookmarks may be different (no two separate JSON files).
- NOTIMPLEMENTED();
-#endif // BUILDFLAG(IS_IOS)
// Reading list: Set migration pref. The ModelTypeStoreServiceImpl will read
// it, and instruct the ModelTypeStoreBackend to actually migrate the data.
diff --git a/components/browser_sync/sync_to_signin_migration_unittest.cc b/components/browser_sync/sync_to_signin_migration_unittest.cc
index 3cf97ce3..75a3b64 100644
--- a/components/browser_sync/sync_to_signin_migration_unittest.cc
+++ b/components/browser_sync/sync_to_signin_migration_unittest.cc
@@ -704,8 +704,6 @@
}
};
-// The Bookmarks migration isn't implemented on platforms other than iOS yet.
-#if BUILDFLAG(IS_IOS)
TEST_F(SyncToSigninMigrationDataTypesTest, MoveBookmarks_BothExist) {
// Both bookmark stores exist on disk. The account store is empty, since it
// was unused pre-migration. This is the typical pre-migration state.
@@ -841,7 +839,6 @@
-base::File::FILE_ERROR_ACCESS_DENIED, 1);
}
#endif // BUILDFLAG(IS_POSIX)
-#endif // BUILDFLAG(IS_IOS)
TEST_F(SyncToSigninMigrationDataTypesTest, MovePasswords_BothExist) {
// Both password stores exist on disk. The account store is empty, since it
diff --git a/components/sync/service/sync_feature_status_for_migrations_recorder.cc b/components/sync/service/sync_feature_status_for_migrations_recorder.cc
index 18ffa45..946b0f7 100644
--- a/components/sync/service/sync_feature_status_for_migrations_recorder.cc
+++ b/components/sync/service/sync_feature_status_for_migrations_recorder.cc
@@ -54,7 +54,7 @@
// Start observing the SyncService, and query the initial state. This ensures
// that any pre-existing pref values get overridden, and can't carry over into
// this or future runs of Chrome.
- // TODO(crbug.com/1486420): Find a way to clear the prefs even if this class
+ // TODO(crbug.com/40282890): Find a way to clear the prefs even if this class
// doesn't even get instantiated, e.g. due to --disable-sync.
sync->AddObserver(this);
OnStateChanged(sync);
diff --git a/ios/chrome/browser/shared/model/prefs/browser_prefs.mm b/ios/chrome/browser/shared/model/prefs/browser_prefs.mm
index f3c9bade..d336998 100644
--- a/ios/chrome/browser/shared/model/prefs/browser_prefs.mm
+++ b/ios/chrome/browser/shared/model/prefs/browser_prefs.mm
@@ -948,7 +948,7 @@
kActivityBucketLastReportedDateArrayKey, prefs, defaults);
// Added 10/2023, but DO NOT REMOVE after the usual year!
- // TODO(crbug.com/1486420): Remove ~one year after full launch.
+ // TODO(crbug.com/40282890): Remove ~one year after full launch.
browser_sync::MaybeMigrateSyncingUserToSignedIn(state_path, prefs);
// Added 12/2023.
