Add support for transparent/translucent pixels in the user image

chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.h"
 
 #include <stddef.h>
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/rand_util.h"
 #include "base/sequenced_task_runner.h"
+#include "base/strings/string_util.h"
 #include "base/task_runner_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/login/helper.h"
 #include "chrome/browser/chromeos/login/users/avatar/user_image_loader.h"
@@ skipping 21 matching lines @@
 
 // Delay betweeen user login and attempt to update user's profile data.
 const int kProfileDataDownloadDelaySec = 10;
 
 // Interval betweeen retries to update user's profile data.
 const int kProfileDataDownloadRetryIntervalSec = 300;
 
 // Delay betweeen subsequent profile refresh attempts (24 hrs).
 const int kProfileRefreshIntervalSec = 24 * 3600;
 
-const char kSafeImagePathExtension[] = ".jpg";
-
 // Enum for reporting histograms about profile picture download.
 enum ProfileDownloadResult {
   kDownloadSuccessChanged,
   kDownloadSuccess,
   kDownloadFailure,
   kDownloadDefault,
   kDownloadCached,
 
   // Must be the last, convenient count.
   kDownloadResultsCount
@@ skipping 81 matching lines @@
       base::WriteFile(image_path,
                       reinterpret_cast<const char*>(image_bytes->front()),
                       image_bytes->size()) == -1) {
     LOG(ERROR) << "Failed to save image to file.";
     return false;
   }
 
   return true;
 }
 
+// Returns the robust codec enum for the given image path's extension.
+// The user image is always stored in either JPEG or PNG.
+ImageDecoder::ImageCodec ChooseRobustCodecFromPath(
+    const base::FilePath& image_path) {
+  const std::string utf8_path = image_path.AsUTF8Unsafe();
+  if (base::EndsWith(utf8_path, ".jpg", base::CompareCase::INSENSITIVE_ASCII))

[xiyuan, 2016/11/30 00:04:22]

nit: image_path.Extension() == FILE_PATH_LITERAL(".jpg")

[satorux1, 2016/12/01 07:43:36]

Good idea. Wanted to make it case-insensitive but it's not necessary as the
image is always saved with a lower case extension by the code here.

+    return ImageDecoder::ROBUST_JPEG_CODEC;
+  if (base::EndsWith(utf8_path, ".png", base::CompareCase::INSENSITIVE_ASCII))
+    return ImageDecoder::ROBUST_PNG_CODEC;
+
+  NOTREACHED() << "Invalid path: " << image_path.AsUTF8Unsafe();
+  return ImageDecoder::ROBUST_JPEG_CODEC;
+}
+
+// Returns the suffix for the given image format, that should be JPEG or PNG.
+const char* ChooseExtensionFromImageFormat(
+    user_manager::UserImage::ImageFormat image_format) {
+  switch (image_format) {
+    case user_manager::UserImage::FORMAT_JPEG:
+      return ".jpg";
+    case user_manager::UserImage::FORMAT_PNG:
+      return ".png";
+    default:
+      NOTREACHED() << "Invalid format: " << image_format;
+      return ".jpg";
+  }
+}
+
 }  // namespace
 
 const char UserImageManagerImpl::kUserImageProperties[] = "user_image_info";
 const char UserImageManagerImpl::kImagePathNodeName[] = "path";
 const char UserImageManagerImpl::kImageIndexNodeName[] = "index";
 const char UserImageManagerImpl::kImageURLNodeName[] = "url";
 
 // static
 void UserImageManager::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(UserImageManagerImpl::kUserImageProperties);
@@ skipping 51 matching lines @@
                        std::unique_ptr<user_manager::UserImage> user_image);
 
   // Updates the user object with |user_image|.
   void UpdateUser(std::unique_ptr<user_manager::UserImage> user_image);
 
   // Updates the user object with |user_image|, and saves the image
   // bytes. Local state will be updated as needed.
   void UpdateUserAndSaveImage(
       std::unique_ptr<user_manager::UserImage> user_image);
 
-  // Saves |image_bytes| to disk in JPEG format if
+  // Saves |image_bytes| to disk in |image_format| if
   // |image_is_safe_format|. Local state will be updated as needed.
   void SaveImageAndUpdateLocalState(
       bool image_is_safe_format,
-      scoped_refptr<base::RefCountedBytes> image_bytes);
+      scoped_refptr<base::RefCountedBytes> image_bytes,
+      user_manager::UserImage::ImageFormat image_format);
 
   // Called back after the user image has been saved to
   // disk. Updates the user image information in local state. The
   // information is only updated if |success| is true (indicating that
   // the image was saved successfully) or the user image is the
   // profile image (indicating that even if the image could not be
   // saved because it is not available right now, it will be
   // downloaded eventually).
   void OnSaveImageDone(bool success);
 
@@ skipping 44 matching lines @@
       image_index_ < default_user_image::kDefaultImagesCount) {
     // Load one of the default images. This happens synchronously.
     std::unique_ptr<user_manager::UserImage> user_image(
         new user_manager::UserImage(
             default_user_image::GetDefaultImage(image_index_)));
     UpdateUser(std::move(user_image));
     NotifyJobDone();
   } else if (image_index_ == user_manager::User::USER_IMAGE_EXTERNAL ||
              image_index_ == user_manager::User::USER_IMAGE_PROFILE) {
     // Load the user image from a file referenced by |image_path|. This happens
-    // asynchronously. ROBUST_JPEG_CODEC can be used here because
-    // LoadImage() is called only for users whose user image has previously
-    // been set by one of the Set*() methods, which transcode to JPEG format.
+    // asynchronously. ROBUST_JPEG_CODEC or ROBUST_PNG_CODEC can be used here
+    // because LoadImage() is called only for users whose user image has
+    // previously been set by one of the Set*() methods, which transcode to
+    // JPEG or PNG format.
     DCHECK(!image_path_.empty());
     user_image_loader::StartWithFilePath(
         parent_->background_task_runner_, image_path_,
-        ImageDecoder::ROBUST_JPEG_CODEC,
+        ChooseRobustCodecFromPath(image_path_),
         0,  // Do not crop.
         base::Bind(&Job::OnLoadImageDone, weak_factory_.GetWeakPtr(), false));
   } else {
     NOTREACHED();
     NotifyJobDone();
   }
 }
 
 void UserImageManagerImpl::Job::SetToDefaultImage(int default_image_index) {
   DCHECK(!run_);
@@ skipping 97 matching lines @@
   parent_->OnJobChangedUserImage();
 }
 
 void UserImageManagerImpl::Job::UpdateUserAndSaveImage(
     std::unique_ptr<user_manager::UserImage> user_image) {
   const bool image_is_safe_format = user_image->is_safe_format();
   // Create a reference before user_image is passed.
   scoped_refptr<base::RefCountedBytes> image_bytes;
   if (image_is_safe_format)
     image_bytes = user_image->image_bytes();
+  const user_manager::UserImage::ImageFormat image_format =
+      user_image->image_format();
 
   UpdateUser(std::move(user_image));
 
-  SaveImageAndUpdateLocalState(image_is_safe_format, image_bytes);
+  SaveImageAndUpdateLocalState(image_is_safe_format, image_bytes, image_format);
 }
 
 void UserImageManagerImpl::Job::SaveImageAndUpdateLocalState(
     bool image_is_safe_format,
-    scoped_refptr<base::RefCountedBytes> image_bytes) {
+    scoped_refptr<base::RefCountedBytes> image_bytes,
+    user_manager::UserImage::ImageFormat image_format) {
   base::FilePath user_data_dir;
   PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-  image_path_ = user_data_dir.Append(user_id() + kSafeImagePathExtension);
+  image_path_ = user_data_dir.AppendASCII(
+      user_id() + ChooseExtensionFromImageFormat(image_format));
 
   // This should always be true, because of the following reasons:
   //
   // 1) Profile image from Google account -> UserImage is created with
   //    CreateAndEncode() that generates safe bytes representation.
   // 2) Profile image from user-specified image -> The bytes representation
   //    is regenerated after the original image is decoded and cropped.
   // 3) Profile image from policy (via OnExternalDataFetched()) -> JPEG is
   //    only allowed and ROBUST_JPEG_CODEC is used.
   //
   // However, check the value just in case because an unsafe image should
   // never be saved.
   if (!image_is_safe_format) {
     LOG(ERROR) << "User image is not in safe format";
     OnSaveImageDone(false);
     return;
   }
 
   base::PostTaskAndReplyWithResult(
       parent_->background_task_runner_.get(), FROM_HERE,
       base::Bind(&SaveImage, image_bytes, image_path_),
       base::Bind(&Job::OnSaveImageDone, weak_factory_.GetWeakPtr()));
 }
 
 void UserImageManagerImpl::Job::OnSaveImageDone(bool success) {
-  if (success || image_index_ == user_manager::User::USER_IMAGE_PROFILE)
+  if (success || image_index_ == user_manager::User::USER_IMAGE_PROFILE) {
+    // Remove the old image file if the path is different. This can happen if

[achuithb, 2016/11/30 00:26:48]

Should this code be browser tested?

[satorux1, 2016/12/01 07:43:36]

Thank you for pointing this out! I've just added a test. It was my bad that the
original patch didn't have it.

Also revised the logic here to save and delete files in one go.

+    // the user image format is changed from JPEG to PNG or vice versa.
+    PrefService* local_state = g_browser_process->local_state();
+    // Because the user ID (i.e. email address) contains '.', the code here
+    // cannot use the dots notation (path expantion) hence is verbose.
+    const base::DictionaryValue* prefs_images =
+        local_state->GetDictionary(kUserImageProperties);
+    if (prefs_images) {
+      const base::DictionaryValue* image_properties = nullptr;
+      prefs_images->GetDictionaryWithoutPathExpansion(user_id(),
+                                                      &image_properties);
+      if (image_properties) {
+        std::string value;
+        image_properties->GetString(kImagePathNodeName, &value);
+        const base::FilePath original_path =
+            base::FilePath::FromUTF8Unsafe(value);
+        if (!original_path.empty() && original_path != image_path_) {
+          parent_->background_task_runner_->PostTask(
+              FROM_HERE, base::Bind(base::IgnoreResult(&base::DeleteFile),
+                                    base::FilePath(original_path), false));
+        }
+      }
+    }
+
     UpdateLocalState();
+  }
   NotifyJobDone();
 }
 
 void UserImageManagerImpl::Job::UpdateLocalState() {
   // Ignore if data stored or cached outside the user's cryptohome is to be
   // treated as ephemeral.
   if (parent_->user_manager_->IsUserNonCryptohomeDataEphemeral(
           AccountId::FromUserEmail(user_id())))
     return;
 
@@ skipping 33 matching lines @@
 UserImageManagerImpl::~UserImageManagerImpl() {}
 
 void UserImageManagerImpl::LoadUserImage() {
   PrefService* local_state = g_browser_process->local_state();
   const base::DictionaryValue* prefs_images =
       local_state->GetDictionary(kUserImageProperties);
   if (!prefs_images)
     return;
   user_manager::User* user = GetUserAndModify();
 
-  const base::DictionaryValue* image_properties = NULL;
+  const base::DictionaryValue* image_properties = nullptr;
   prefs_images->GetDictionaryWithoutPathExpansion(user_id(), &image_properties);
 
   // If the user image for |user_id| is managed by policy and the policy-set
   // image is being loaded and persisted right now, let that job continue. It
   // will update the user image when done.
   if (IsUserImageManaged() && job_.get())
     return;
 
   if (!image_properties) {
     SetInitialUserImage();
@@ skipping 108 matching lines @@
     return;
   job_.reset(new Job(this));
   job_->SetToPath(path, user_manager::User::USER_IMAGE_EXTERNAL, GURL(), true);
 }
 
 void UserImageManagerImpl::SaveUserImageFromProfileImage() {
   if (IsUserImageManaged())
     return;
   // Use the profile image if it has been downloaded already. Otherwise, use a
   // stub image (gray avatar).
+  std::unique_ptr<user_manager::UserImage> user_image;
+  if (downloaded_profile_image_.isNull()) {
+    user_image = base::WrapUnique(new user_manager::UserImage);
+  } else {
+    user_image = user_manager::UserImage::CreateAndEncode(
+        downloaded_profile_image_, user_manager::UserImage::ChooseImageFormat(
+                                       *downloaded_profile_image_.bitmap()));
+  }
   job_.reset(new Job(this));
   job_->SetToImage(user_manager::User::USER_IMAGE_PROFILE,
-                   downloaded_profile_image_.isNull()
-                       ? base::WrapUnique(new user_manager::UserImage)
-                       : user_manager::UserImage::CreateAndEncode(
-                             downloaded_profile_image_));
+                   std::move(user_image));
   // If no profile image has been downloaded yet, ensure that a download is
   // started.
   if (downloaded_profile_image_.isNull())
     DownloadProfileData(kProfileDownloadReasonProfileImageChosen);
 }
 
 void UserImageManagerImpl::DeleteUserImage() {
   job_.reset();
   DeleteUserImageAndLocalStateEntry(kUserImageProperties);
 }
@@ skipping 244 matching lines @@
 
   std::string image_path;
   image_properties->GetString(kImagePathNodeName, &image_path);
   if (!image_path.empty()) {
     background_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(base::IgnoreResult(&base::DeleteFile),
                    base::FilePath(image_path),
                    false));
   }
-  update->RemoveWithoutPathExpansion(user_id(), NULL);
+  update->RemoveWithoutPathExpansion(user_id(), nullptr);
 }
 
 void UserImageManagerImpl::OnJobChangedUserImage() {
   if (GetUser()->is_logged_in())
     TryToInitDownloadedProfileImage();
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED,
       content::Source<UserImageManagerImpl>(this),
       content::Details<const user_manager::User>(GetUser()));
@@ skipping 26 matching lines @@
 }
 
 bool UserImageManagerImpl::IsUserLoggedInAndHasGaiaAccount() const {
   const user_manager::User* user = GetUser();
   if (!user)
     return false;
   return user->is_logged_in() && user->HasGaiaAccount();
 }
 
 }  // namespace chromeos