Skip to content

Commit

Permalink
Remove migration code for old user images
Browse files Browse the repository at this point in the history 
The switch from PNG to JPEG format was done more than three
years ago in M24. The plan was to wait for a couple of milestones
before removing the migration code.

The worst case scenario of the removal is that users who
upgrade from M23 or older to M51 (very unlikely) will lose
their custom user images (on the other hand, google profile
images will be downloaded and saved once they log in).

BUG=154370
TEST=login screen and user image settings work as before

Review URL: https://codereview.chromium.org/1752303004

Cr-Commit-Position: refs/heads/master@{#380075}
  • Loading branch information
satorux authored and Commit bot committed Mar 9, 2016
1 parent efaa412 commit 8f00bd2
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 295 deletions.
235 changes: 55 additions & 180 deletions chrome/browser/chromeos/login/users/avatar/user_image_manager_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,34 +150,24 @@ class UserImageManagerTest : public LoginManagerTest,
account_id, account_id.GetUserEmail(), false);
}

// Stores old (pre-migration) user image info.
void SetOldUserImageInfo(const AccountId& account_id,
int image_index,
const base::FilePath& image_path) {
RegisterUser(account_id.GetUserEmail());
DictionaryPrefUpdate images_pref(local_state_, "UserImages");
base::DictionaryValue* image_properties = new base::DictionaryValue();
image_properties->Set("index", new base::FundamentalValue(image_index));
image_properties->Set(
"path" , new base::StringValue(image_path.value()));
images_pref->SetWithoutPathExpansion(account_id.GetUserEmail(),
image_properties);
}

// Verifies user image info in |images_pref| dictionary.
void ExpectUserImageInfo(const base::DictionaryValue* images_pref,
const AccountId& account_id,
// Verifies user image info.
void ExpectUserImageInfo(const AccountId& account_id,
int image_index,
const base::FilePath& image_path) {
const base::DictionaryValue* images_pref =
local_state_->GetDictionary(UserImageManagerImpl::kUserImageProperties);
ASSERT_TRUE(images_pref);
const base::DictionaryValue* image_properties = NULL;
images_pref->GetDictionaryWithoutPathExpansion(account_id.GetUserEmail(),
&image_properties);
ASSERT_TRUE(image_properties);
int actual_image_index;
std::string actual_image_path;
ASSERT_TRUE(image_properties->GetInteger("index", &actual_image_index) &&
image_properties->GetString("path", &actual_image_path));
ASSERT_TRUE(
image_properties->GetInteger(UserImageManagerImpl::kImageIndexNodeName,
&actual_image_index) &&
image_properties->GetString(UserImageManagerImpl::kImagePathNodeName,
&actual_image_path));
EXPECT_EQ(image_index, actual_image_index);
EXPECT_EQ(image_path.value(), actual_image_path);
}
Expand All @@ -193,43 +183,6 @@ class UserImageManagerTest : public LoginManagerTest,
ASSERT_FALSE(image_properties);
}

// Verifies that old user image info matches |image_index| and |image_path|
// and that new user image info does not exist.
void ExpectOldUserImageInfo(const AccountId& account_id,
int image_index,
const base::FilePath& image_path) {
ExpectUserImageInfo(local_state_->GetDictionary("UserImages"), account_id,
image_index, image_path);
ExpectNoUserImageInfo(local_state_->GetDictionary("user_image_info"),
account_id);
}

// Verifies that new user image info matches |image_index| and |image_path|
// and that old user image info does not exist.
void ExpectNewUserImageInfo(const AccountId& account_id,
int image_index,
const base::FilePath& image_path) {
ExpectUserImageInfo(local_state_->GetDictionary("user_image_info"),
account_id, image_index, image_path);
ExpectNoUserImageInfo(local_state_->GetDictionary("UserImages"),
account_id);
}

// Sets bitmap |resource_id| as image for |account_id| and saves it to disk.
void SaveUserImagePNG(const AccountId& account_id, int resource_id) {
base::FilePath image_path = GetUserImagePath(account_id, "png");
scoped_refptr<base::RefCountedStaticMemory> image_data(
ResourceBundle::GetSharedInstance().LoadDataResourceBytesForScale(
resource_id, ui::SCALE_FACTOR_100P));
int written = base::WriteFile(
image_path,
reinterpret_cast<const char*>(image_data->front()),
image_data->size());
EXPECT_EQ(static_cast<int>(image_data->size()), written);
SetOldUserImageInfo(account_id, user_manager::User::USER_IMAGE_EXTERNAL,
image_path);
}

// Returns the image path for user |account_id| with specified |extension|.
base::FilePath GetUserImagePath(const AccountId& account_id,
const std::string& extension) {
Expand Down Expand Up @@ -334,99 +287,22 @@ class UserImageManagerTest : public LoginManagerTest,
DISALLOW_COPY_AND_ASSIGN(UserImageManagerTest);
};

IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_DefaultUserImagePreserved) {
// Setup an old default (stock) user image.
ScopedUserManagerEnabler(new MockUserManager);
SetOldUserImageInfo(test_account_id1_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
}

IN_PROC_BROWSER_TEST_F(UserImageManagerTest, DefaultUserImagePreserved) {
user_manager::UserManager::Get()->GetUsers(); // Load users.
// Old info preserved.
ExpectOldUserImageInfo(test_account_id1_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
LogIn(test_account_id1_);
// Image info is migrated now.
ExpectNewUserImageInfo(test_account_id1_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
}

IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_OtherUsersUnaffected) {
// Setup two users with stock images.
ScopedUserManagerEnabler(new MockUserManager);
SetOldUserImageInfo(test_account_id1_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
SetOldUserImageInfo(test_account_id2_,
default_user_image::kFirstDefaultImageIndex + 1,
base::FilePath());
}

IN_PROC_BROWSER_TEST_F(UserImageManagerTest, OtherUsersUnaffected) {
user_manager::UserManager::Get()->GetUsers(); // Load users.
// Old info preserved.
ExpectOldUserImageInfo(test_account_id1_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
ExpectOldUserImageInfo(test_account_id2_,
default_user_image::kFirstDefaultImageIndex + 1,
base::FilePath());
LogIn(test_account_id1_);
// Image info is migrated for the first user and unaffected for the rest.
ExpectNewUserImageInfo(test_account_id1_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
ExpectOldUserImageInfo(test_account_id2_,
default_user_image::kFirstDefaultImageIndex + 1,
base::FilePath());
}

IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_PRE_NonJPEGImageFromFile) {
// Setup a user with non-JPEG image.
ScopedUserManagerEnabler(new MockUserManager);
SaveUserImagePNG(test_account_id1_,
default_user_image::kDefaultImageResourceIDs
[default_user_image::kFirstDefaultImageIndex]);
}
IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_SaveAndLoadUserImage) {
RegisterUser(test_account_id1_.GetUserEmail());

IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_NonJPEGImageFromFile) {
user_manager::UserManager::Get()->GetUsers(); // Load users.
// Old info preserved.
ExpectOldUserImageInfo(test_account_id1_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(test_account_id1_, "png"));
const user_manager::User* user =
user_manager::UserManager::Get()->FindUser(test_account_id1_);
EXPECT_TRUE(user->image_is_stub());

base::RunLoop run_loop;
PrefChangeRegistrar pref_change_registrar_;
pref_change_registrar_.Init(local_state_);
pref_change_registrar_.Add("UserImages", run_loop.QuitClosure());
LogIn(test_account_id1_);

// Wait for migration.
run_loop.Run();

// Image info is migrated and the image is converted to JPG.
ExpectNewUserImageInfo(test_account_id1_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(test_account_id1_, "jpg"));
user = user_manager::UserManager::Get()->GetLoggedInUser();
ASSERT_TRUE(user);
EXPECT_FALSE(user->image_is_safe_format());
// Check image dimensions.
const gfx::ImageSkia& saved_image = default_user_image::GetDefaultImage(
// Setup a user with JPEG image.
run_loop_.reset(new base::RunLoop);
const gfx::ImageSkia& image = default_user_image::GetDefaultImage(
default_user_image::kFirstDefaultImageIndex);
EXPECT_EQ(saved_image.width(), user->GetImage().width());
EXPECT_EQ(saved_image.height(), user->GetImage().height());
UserImageManager* user_image_manager =
ChromeUserManager::Get()->GetUserImageManager(test_account_id1_);
user_image_manager->SaveUserImage(
user_manager::UserImage::CreateAndEncode(image));
run_loop_->Run();
}

IN_PROC_BROWSER_TEST_F(UserImageManagerTest, NonJPEGImageFromFile) {
// Ensures that the user image in JPEG format is loaded correctly.
IN_PROC_BROWSER_TEST_F(UserImageManagerTest, SaveAndLoadUserImage) {
user_manager::UserManager::Get()->GetUsers(); // Load users.
const user_manager::User* user =
user_manager::UserManager::Get()->FindUser(test_account_id1_);
Expand All @@ -437,7 +313,7 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, NonJPEGImageFromFile) {
chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED,
content::NotificationService::AllSources()).Wait();
}
// Now the migrated image is used.
// The image should be in the safe format.
EXPECT_TRUE(user->image_is_safe_format());
// Check image dimensions. Images can't be compared since JPEG is lossy.
const gfx::ImageSkia& saved_image = default_user_image::GetDefaultImage(
Expand Down Expand Up @@ -468,9 +344,9 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, SaveUserDefaultImageIndex) {
EXPECT_TRUE(user->HasDefaultImage());
EXPECT_EQ(default_user_image::kFirstDefaultImageIndex, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(default_image, user->GetImage()));
ExpectNewUserImageInfo(test_account_id1_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
ExpectUserImageInfo(test_account_id1_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
}

IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_SaveUserImage) {
Expand Down Expand Up @@ -501,9 +377,9 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, SaveUserImage) {
EXPECT_FALSE(user->HasDefaultImage());
EXPECT_EQ(user_manager::User::USER_IMAGE_EXTERNAL, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(custom_image, user->GetImage()));
ExpectNewUserImageInfo(test_account_id1_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(test_account_id1_, "jpg"));
ExpectUserImageInfo(test_account_id1_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(test_account_id1_, "jpg"));

const scoped_ptr<gfx::ImageSkia> saved_image =
test::ImageLoader(GetUserImagePath(test_account_id1_, "jpg")).Load();
Expand Down Expand Up @@ -540,9 +416,9 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, SaveUserImageFromFile) {
EXPECT_FALSE(user->HasDefaultImage());
EXPECT_EQ(user_manager::User::USER_IMAGE_EXTERNAL, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(*custom_image, user->GetImage()));
ExpectNewUserImageInfo(test_account_id1_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(test_account_id1_, "jpg"));
ExpectUserImageInfo(test_account_id1_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(test_account_id1_, "jpg"));

const scoped_ptr<gfx::ImageSkia> saved_image =
test::ImageLoader(GetUserImagePath(test_account_id1_, "jpg")).Load();
Expand Down Expand Up @@ -587,9 +463,8 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, SaveUserImageFromProfileImage) {
EXPECT_FALSE(user->HasDefaultImage());
EXPECT_EQ(user_manager::User::USER_IMAGE_PROFILE, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(profile_image, user->GetImage()));
ExpectNewUserImageInfo(test_account_id1_,
user_manager::User::USER_IMAGE_PROFILE,
GetUserImagePath(test_account_id1_, "jpg"));
ExpectUserImageInfo(test_account_id1_, user_manager::User::USER_IMAGE_PROFILE,
GetUserImagePath(test_account_id1_, "jpg"));

const scoped_ptr<gfx::ImageSkia> saved_image =
test::ImageLoader(GetUserImagePath(test_account_id1_, "jpg")).Load();
Expand Down Expand Up @@ -641,9 +516,9 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest,
EXPECT_TRUE(user->HasDefaultImage());
EXPECT_EQ(default_user_image::kFirstDefaultImageIndex, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(default_image, user->GetImage()));
ExpectNewUserImageInfo(test_account_id1_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
ExpectUserImageInfo(test_account_id1_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
}

class UserImageManagerPolicyTest : public UserImageManagerTest,
Expand Down Expand Up @@ -757,9 +632,9 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerPolicyTest, DISABLED_SetAndClear) {
EXPECT_FALSE(user->HasDefaultImage());
EXPECT_EQ(user_manager::User::USER_IMAGE_EXTERNAL, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(*policy_image_, user->GetImage()));
ExpectNewUserImageInfo(enterprise_account_id_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(enterprise_account_id_, "jpg"));
ExpectUserImageInfo(enterprise_account_id_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(enterprise_account_id_, "jpg"));

scoped_ptr<gfx::ImageSkia> saved_image =
test::ImageLoader(GetUserImagePath(enterprise_account_id_, "jpg")).Load();
Expand Down Expand Up @@ -791,8 +666,8 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerPolicyTest, DISABLED_SetAndClear) {
const gfx::ImageSkia& default_image =
default_user_image::GetDefaultImage(default_image_index);
EXPECT_TRUE(test::AreImagesEqual(default_image, user->GetImage()));
ExpectNewUserImageInfo(enterprise_account_id_, default_image_index,
base::FilePath());
ExpectUserImageInfo(enterprise_account_id_, default_image_index,
base::FilePath());

// Choose a different user image. Verify that the chosen user image is set and
// persisted.
Expand All @@ -810,8 +685,8 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerPolicyTest, DISABLED_SetAndClear) {
EXPECT_TRUE(user->HasDefaultImage());
EXPECT_EQ(user_image_index, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(user_image, user->GetImage()));
ExpectNewUserImageInfo(enterprise_account_id_, user_image_index,
base::FilePath());
ExpectUserImageInfo(enterprise_account_id_, user_image_index,
base::FilePath());
}

IN_PROC_BROWSER_TEST_F(UserImageManagerPolicyTest, PRE_PolicyOverridesUser) {
Expand Down Expand Up @@ -846,9 +721,9 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerPolicyTest, PolicyOverridesUser) {
EXPECT_TRUE(user->HasDefaultImage());
EXPECT_EQ(default_user_image::kFirstDefaultImageIndex, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(default_image, user->GetImage()));
ExpectNewUserImageInfo(enterprise_account_id_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());
ExpectUserImageInfo(enterprise_account_id_,
default_user_image::kFirstDefaultImageIndex,
base::FilePath());

// Set policy. Verify that the policy-provided user image is downloaded, set
// and persisted, overriding the previously set image.
Expand All @@ -864,9 +739,9 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerPolicyTest, PolicyOverridesUser) {
EXPECT_FALSE(user->HasDefaultImage());
EXPECT_EQ(user_manager::User::USER_IMAGE_EXTERNAL, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(*policy_image_, user->GetImage()));
ExpectNewUserImageInfo(enterprise_account_id_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(enterprise_account_id_, "jpg"));
ExpectUserImageInfo(enterprise_account_id_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(enterprise_account_id_, "jpg"));

scoped_ptr<gfx::ImageSkia> saved_image =
test::ImageLoader(GetUserImagePath(enterprise_account_id_, "jpg")).Load();
Expand Down Expand Up @@ -911,9 +786,9 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerPolicyTest, UserDoesNotOverridePolicy) {
EXPECT_FALSE(user->HasDefaultImage());
EXPECT_EQ(user_manager::User::USER_IMAGE_EXTERNAL, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(*policy_image_, user->GetImage()));
ExpectNewUserImageInfo(enterprise_account_id_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(enterprise_account_id_, "jpg"));
ExpectUserImageInfo(enterprise_account_id_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(enterprise_account_id_, "jpg"));

scoped_ptr<gfx::ImageSkia> saved_image =
test::ImageLoader(GetUserImagePath(enterprise_account_id_, "jpg")).Load();
Expand All @@ -933,9 +808,9 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerPolicyTest, UserDoesNotOverridePolicy) {
EXPECT_FALSE(user->HasDefaultImage());
EXPECT_EQ(user_manager::User::USER_IMAGE_EXTERNAL, user->image_index());
EXPECT_TRUE(test::AreImagesEqual(*policy_image_, user->GetImage()));
ExpectNewUserImageInfo(enterprise_account_id_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(enterprise_account_id_, "jpg"));
ExpectUserImageInfo(enterprise_account_id_,
user_manager::User::USER_IMAGE_EXTERNAL,
GetUserImagePath(enterprise_account_id_, "jpg"));

saved_image =
test::ImageLoader(GetUserImagePath(enterprise_account_id_, "jpg")).Load();
Expand Down
Loading

0 comments on commit 8f00bd2

Please sign in to comment.