[M104] Do not run move migration if copy migration is completed.
(cherry picked from commit 08613ce4b1965038eef2466fc2ab9792087ff0ac)
Bug: 1340438
Test: unit_tests, tast run $DUT lacros.Migrate.*
Change-Id: I3ab6bb52b48b765c3daa8331189cbc2f189a5a50
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3733870
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Yuta Hijikata <ythjkt@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1020225}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740224
Cr-Commit-Position: refs/branch-heads/5112@{#639}
Cr-Branched-From: b13d3fe7b3c47a56354ef54b221008afa754412e-refs/heads/main@{#1012729}
diff --git a/chrome/browser/ash/crosapi/browser_data_migrator.cc b/chrome/browser/ash/crosapi/browser_data_migrator.cc
index da323bf..d2360f53 100644
--- a/chrome/browser/ash/crosapi/browser_data_migrator.cc
+++ b/chrome/browser/ash/crosapi/browser_data_migrator.cc
@@ -306,27 +306,19 @@
return true;
}
- crosapi::browser_util::MigrationMode mode =
- crosapi::browser_util::GetMigrationMode(user, policy_init_state);
-
- if (crosapi::browser_util::IsProfileMigrationCompletedForUser(
- local_state, user_id_hash, mode)) {
+ if (crosapi::browser_util::IsCopyOrMoveProfileMigrationCompletedForUser(
+ local_state, user_id_hash)) {
// TODO(crbug.com/1277848): Once `BrowserDataMigrator` stabilises,
// remove this log message.
+ if (crosapi::browser_util::IsProfileMigrationCompletedForUser(
+ local_state, user_id_hash,
+ crosapi::browser_util::MigrationMode::kMove)) {
+ LOG(WARNING) << "Profile move migration has been completed already.";
+ }
LOG(WARNING) << "Profile migration has been completed already.";
return false;
}
- // TODO(crbug.com/1277848): Once `BrowserDataMigrator` stabilises,
- // remove this log message.
- if (mode == crosapi::browser_util::MigrationMode::kMove &&
- crosapi::browser_util::IsProfileMigrationCompletedForUser(
- local_state, user_id_hash,
- crosapi::browser_util::MigrationMode::kCopy)) {
- LOG(WARNING) << "Only copy migration is marked as completed. Running "
- "profile move migraiton.";
- }
-
return true;
}
diff --git a/chrome/browser/ash/crosapi/browser_data_migrator_unittest.cc b/chrome/browser/ash/crosapi/browser_data_migrator_unittest.cc
index 5b41227..4719b57 100644
--- a/chrome/browser/ash/crosapi/browser_data_migrator_unittest.cc
+++ b/chrome/browser/ash/crosapi/browser_data_migrator_unittest.cc
@@ -494,8 +494,11 @@
}
{
- // If copy migration is marked as completed but not move then migration
- // should run if move migration is enabled.
+ // If copy migration is marked as completed then migration should not run
+ // even if move migration is enabled.
+ // TODO(crbug.com/1340438): This previously checked the opposite where the
+ // expectation was to run move migration even if copy migration is
+ // completed. Revisit this part if we decide to restore that behaviour.
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeatures(
{ash::features::kLacrosSupport, ash::features::kLacrosPrimary,
@@ -505,7 +508,7 @@
EXPECT_EQ(crosapi::browser_util::GetMigrationMode(
user, crosapi::browser_util::PolicyInitState::kAfterInit),
crosapi::browser_util::MigrationMode::kMove);
- EXPECT_TRUE(BrowserDataMigratorImpl::MaybeRestartToMigrateInternal(
+ EXPECT_FALSE(BrowserDataMigratorImpl::MaybeRestartToMigrateInternal(
user->GetAccountId(), user->username_hash(),
crosapi::browser_util::PolicyInitState::kAfterInit));
}
diff --git a/chrome/browser/ash/crosapi/browser_util.cc b/chrome/browser/ash/crosapi/browser_util.cc
index 356817d4..8553ae5 100644
--- a/chrome/browser/ash/crosapi/browser_util.cc
+++ b/chrome/browser/ash/crosapi/browser_util.cc
@@ -367,12 +367,10 @@
user_manager::UserManager::Get()->GetPrimaryUser()->GetAccountId())) {
PrefService* local_state = g_browser_process->local_state();
// Note that local_state can be nullptr in tests.
- if (local_state &&
- !IsProfileMigrationCompletedForUser(
- local_state,
- user_manager::UserManager::Get()->GetPrimaryUser()->username_hash(),
- GetMigrationMode(user_manager::UserManager::Get()->GetPrimaryUser(),
- PolicyInitState::kAfterInit))) {
+ if (local_state && !IsCopyOrMoveProfileMigrationCompletedForUser(
+ local_state, user_manager::UserManager::Get()
+ ->GetPrimaryUser()
+ ->username_hash())) {
// If migration has not been completed, do not enable lacros.
return false;
}
@@ -457,9 +455,8 @@
return false;
// If migration is already completed, it is not necessary to run again.
- if (IsProfileMigrationCompletedForUser(
- user_manager->GetLocalState(), user->username_hash(),
- GetMigrationMode(user, PolicyInitState::kAfterInit))) {
+ if (IsCopyOrMoveProfileMigrationCompletedForUser(
+ user_manager->GetLocalState(), user->username_hash())) {
return false;
}
@@ -910,6 +907,15 @@
return MigrationMode::kCopy;
}
+bool IsCopyOrMoveProfileMigrationCompletedForUser(
+ PrefService* local_state,
+ const std::string& user_id_hash) {
+ // Completion of profile move migration sets copy migration as completed so
+ // only checking completion of copy migration is sufficient.
+ return IsProfileMigrationCompletedForUser(local_state, user_id_hash,
+ MigrationMode::kCopy);
+}
+
bool IsProfileMigrationCompletedForUser(PrefService* local_state,
const std::string& user_id_hash,
MigrationMode mode) {
diff --git a/chrome/browser/ash/crosapi/browser_util.h b/chrome/browser/ash/crosapi/browser_util.h
index e225697..d8edb49 100644
--- a/chrome/browser/ash/crosapi/browser_util.h
+++ b/chrome/browser/ash/crosapi/browser_util.h
@@ -321,6 +321,14 @@
MigrationMode GetMigrationMode(const user_manager::User* user,
PolicyInitState policy_init_state);
+// Returns true if either copy or move migration is completed. Used as a wrapper
+// over `IsProfileMigrationCompletedForUser()`.
+// TODO(crbug.com/1340438): This function is introduced to prevent running
+// profile move migration for users who have already completed copy migration.
+bool IsCopyOrMoveProfileMigrationCompletedForUser(
+ PrefService* local_state,
+ const std::string& user_id_hash);
+
// Checks if profile migration has been completed. This is reset if profile
// migration is initiated for example due to lacros data directory being wiped.
bool IsProfileMigrationCompletedForUser(PrefService* local_state,
diff --git a/chrome/browser/ash/crosapi/browser_util_unittest.cc b/chrome/browser/ash/crosapi/browser_util_unittest.cc
index 64fa19b..bb6d9a6 100644
--- a/chrome/browser/ash/crosapi/browser_util_unittest.cc
+++ b/chrome/browser/ash/crosapi/browser_util_unittest.cc
@@ -784,6 +784,31 @@
&pref_service_, user_id_hash, browser_util::MigrationMode::kMove));
}
+TEST_F(BrowserUtilTest, IsCopyOrMoveProfileMigrationCompletedForUser) {
+ const std::string user_id_hash = "abcd";
+ // `IsCopyOrMoveProfileMigrationCompletedForUser()` should return
+ // false by default.
+ EXPECT_FALSE(browser_util::IsCopyOrMoveProfileMigrationCompletedForUser(
+ &pref_service_, user_id_hash));
+
+ // Setting copy migration as completed makes
+ // `IsCopyOrMoveProfileMigrationCompletedForUser()` return true.
+ browser_util::SetProfileMigrationCompletedForUser(
+ &pref_service_, user_id_hash, browser_util::MigrationMode::kCopy);
+ EXPECT_TRUE(browser_util::IsCopyOrMoveProfileMigrationCompletedForUser(
+ &pref_service_, user_id_hash));
+
+ browser_util::ClearProfileMigrationCompletedForUser(&pref_service_,
+ user_id_hash);
+
+ // Setting move migration as completed makes
+ // `IsCopyOrMoveProfileMigrationCompletedForUser()` return true.
+ browser_util::SetProfileMigrationCompletedForUser(
+ &pref_service_, user_id_hash, browser_util::MigrationMode::kMove);
+ EXPECT_TRUE(browser_util::IsCopyOrMoveProfileMigrationCompletedForUser(
+ &pref_service_, user_id_hash));
+}
+
TEST_F(BrowserUtilTest, IsAshBrowserSyncEnabled) {
{
browser_util::SetLacrosEnabledForTest(false);