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 68 matching lines @@
     case user_manager::User::USER_IMAGE_EXTERNAL:
       // TODO(ivankr): Distinguish this from selected from file.
       return default_user_image::kHistogramImageFromCamera;
     case user_manager::User::USER_IMAGE_PROFILE:
       return default_user_image::kHistogramImageFromProfile;
     default:
       return image_index;
   }
 }
 
-bool SaveImage(scoped_refptr<base::RefCountedBytes> image_bytes,
-               const base::FilePath& image_path) {
+// Saves |image_bytes| at |image_path|, and delete the old file at
+// |old_image_path| if needed.
+bool SaveAndDeleteImage(scoped_refptr<base::RefCountedBytes> image_bytes,
+                        const base::FilePath& image_path,
+                        const base::FilePath& old_image_path) {
   if (image_bytes->size() == 0 ||
       base::WriteFile(image_path,
                       reinterpret_cast<const char*>(image_bytes->front()),
                       image_bytes->size()) == -1) {
-    LOG(ERROR) << "Failed to save image to file.";
+    LOG(ERROR) << "Failed to save image to file: " << image_path.AsUTF8Unsafe();
     return false;
   }
+  if (!old_image_path.empty() && old_image_path != image_path) {
+    if (!base::DeleteFile(old_image_path, false /* recursive */)) {
+      LOG(ERROR) << "Failed to delete old image: "
+                 << old_image_path.AsUTF8Unsafe();
+      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) {
+  if (image_path.Extension() == FILE_PATH_LITERAL(".jpg"))
+    return ImageDecoder::ROBUST_JPEG_CODEC;
+  if (image_path.Extension() == FILE_PATH_LITERAL(".png"))
+    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) {
-  base::FilePath user_data_dir;
-  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-  image_path_ = user_data_dir.Append(user_id() + kSafeImagePathExtension);
+    scoped_refptr<base::RefCountedBytes> image_bytes,
+    user_manager::UserImage::ImageFormat image_format) {
+  // This can happen if a stub profile image is chosen (i.e. the profile
+  // image hasn't been downloaded yet)

[achuithb, 2016/12/02 08:28:18]

nit: period

[satorux1, 2016/12/05 01:57:40]

Done.

+  if (!image_bytes) {
+    OnSaveImageDone(false);
+    return;

[satorux1, 2016/12/02 06:42:58]

Added this to fix the following NOTREACHED() failure in
ChooseExtensionFromImageFormat():

[17019:17019:1201/005029.191350:FATAL:user_image_manager_impl.cc(198)] Check
failed: false. Invalid format: 2

This happened because the image format is unknown if a stub profile image is
used. This code path is exercised in the following two tests:

UserImageManagerTest.ProfileImageDownloadDoesNotClobber
UserImageManagerTest.SaveUserImageFromProfileImage

[achuithb, 2016/12/02 08:28:18]

Acknowledged.

+  }
 
   // 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::FilePath user_data_dir;
+  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
+  // TODO(crbug.com/670557): Use GetAccountIdKey() instead of user_id().
+  image_path_ = user_data_dir.AppendASCII(
+      user_id() + ChooseExtensionFromImageFormat(image_format));

[satorux1, 2016/12/02 06:42:58]

Moved this code here, as there is no need to generate the image path earlier.

[achuithb, 2016/12/02 08:28:18]

Acknowledged.

+
+  // The old image file should be removed if the path is different. This
+  // can happen if the user image format is changed from JPEG to PNG or
+  // vice versa.
+  base::FilePath old_image_path;
+  // Because the user ID (i.e. email address) contains '.', the code here
+  // cannot use the dots notation (path expantion) hence is verbose.
+  PrefService* local_state = g_browser_process->local_state();
+  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);
+      old_image_path = base::FilePath::FromUTF8Unsafe(value);
+    }
+  }
+
   base::PostTaskAndReplyWithResult(
       parent_->background_task_runner_.get(), FROM_HERE,
-      base::Bind(&SaveImage, image_bytes, image_path_),
+      base::Bind(&SaveAndDeleteImage, image_bytes, image_path_, old_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)
     UpdateLocalState();
   NotifyJobDone();
 }
 
 void UserImageManagerImpl::Job::UpdateLocalState() {
@@ skipping 39 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