Add support for transparent/translucent pixels in the user image

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

Index: chrome/browser/chromeos/login/users/avatar/user_image_loader.cc
diff --git a/chrome/browser/chromeos/login/users/avatar/user_image_loader.cc b/chrome/browser/chromeos/login/users/avatar/user_image_loader.cc
index 6ec5da88b56fedd562451a5acac3ea336cd59abb..2489790dc48cc5d220b71ebde55e4a7855c46471 100644
--- a/chrome/browser/chromeos/login/users/avatar/user_image_loader.cc
+++ b/chrome/browser/chromeos/login/users/avatar/user_image_loader.cc
@@ -47,11 +47,15 @@ struct ImageInfo {
 
 // Crops |image| to the square format and downsizes the image to
 // |target_size| in pixels. On success, returns the bytes representation and
-// stores the cropped image in |bitmap|. On failure, returns nullptr.
-scoped_refptr<base::RefCountedBytes> CropImage(const SkBitmap& image,
-                                               int target_size,
-                                               SkBitmap* bitmap) {
+// stores the cropped image in |bitmap|, and the format of the bytes
+// representation in |image_format|. On failure, returns nullptr.
+scoped_refptr<base::RefCountedBytes> CropImage(
+    const SkBitmap& image,
+    int target_size,
+    SkBitmap* bitmap,
+    user_manager::UserImage::ImageFormat* image_format) {
   DCHECK_GT(target_size, 0);
+  DCHECK(image_format);
 
   SkBitmap final_image;
   // Auto crop the image, taking the largest square in the center.
@@ -70,13 +74,32 @@ scoped_refptr<base::RefCountedBytes> CropImage(const SkBitmap& image,
   }
 
   // Encode the cropped image to web-compatible bytes representation
+  *image_format = user_manager::UserImage::ChooseImageFormat(final_image);
   scoped_refptr<base::RefCountedBytes> encoded =
-      user_manager::UserImage::Encode(final_image);
+      user_manager::UserImage::Encode(final_image, *image_format);
   if (encoded)

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

Does it make sense to reset image_format to FORMAT_UNKNOWN if the encoding
failed?

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

No, I think. The function returns nullptr on error, and I think we don't need to
guarantee anything about |bitmap| and |image_format| on error. Updated the
function comment.

     bitmap->swap(final_image);
   return encoded;
 }
 
+// Returns the image format for the bytes representation of the user image
+// from the image codec used for loading the image.
+user_manager::UserImage::ImageFormat ChooseImageFormatFromCodec(
+    ImageDecoder::ImageCodec image_codec) {
+  switch (image_codec) {
+    case ImageDecoder::ROBUST_JPEG_CODEC:
+      return user_manager::UserImage::FORMAT_JPEG;
+    case ImageDecoder::ROBUST_PNG_CODEC:
+      return user_manager::UserImage::FORMAT_PNG;
+    case ImageDecoder::DEFAULT_CODEC:
+      // The default codec can accept many kinds of image formats, hence the
+      // image format of the bytes representation is unknown.
+      return user_manager::UserImage::FORMAT_UNKNOWN;
+  }
+  NOTREACHED();
+  return user_manager::UserImage::FORMAT_UNKNOWN;
+}
+
 // Handles the decoded image returned from ImageDecoder through the
 // ImageRequest interface.
 class UserImageRequest : public ImageDecoder::ImageRequest {
@@ -100,11 +123,13 @@ class UserImageRequest : public ImageDecoder::ImageRequest {
 
   // Called after the image is cropped (and downsized) as needed.
   void OnImageCropped(SkBitmap* bitmap,
+                      user_manager::UserImage::ImageFormat* image_format,
                       scoped_refptr<base::RefCountedBytes> bytes);
 
   // Called after the image is finalized. |image_bytes_regenerated| is true
   // if |image_bytes| is regenerated from the cropped image.
   void OnImageFinalized(const SkBitmap& image,
+                        user_manager::UserImage::ImageFormat image_format,
                         scoped_refptr<base::RefCountedBytes> image_bytes,
                         bool image_bytes_regenerated);
 
@@ -123,19 +148,27 @@ void UserImageRequest::OnImageDecoded(const SkBitmap& decoded_image) {
     // Cropping an image could be expensive, hence posting to the background
     // thread.
     SkBitmap* bitmap = new SkBitmap;
+    user_manager::UserImage::ImageFormat* image_format =

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

auto* is appropriate here

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

Done.

+        new user_manager::UserImage::ImageFormat(
+            user_manager::UserImage::FORMAT_UNKNOWN);
     base::PostTaskAndReplyWithResult(
         background_task_runner_.get(), FROM_HERE,
-        base::Bind(&CropImage, decoded_image, target_size, bitmap),
+        base::Bind(&CropImage, decoded_image, target_size, bitmap,
+                   image_format),
         base::Bind(&UserImageRequest::OnImageCropped,
-                   weak_ptr_factory_.GetWeakPtr(), base::Owned(bitmap)));
+                   weak_ptr_factory_.GetWeakPtr(), base::Owned(bitmap),
+                   base::Owned(image_format)));
   } else {
-    OnImageFinalized(decoded_image, image_data_,
+    const user_manager::UserImage::ImageFormat image_format =
+        ChooseImageFormatFromCodec(image_info_.image_codec);
+    OnImageFinalized(decoded_image, image_format, image_data_,
                      false /* image_bytes_regenerated */);
   }
 }
 
 void UserImageRequest::OnImageCropped(
     SkBitmap* bitmap,
+    user_manager::UserImage::ImageFormat* image_format,
     scoped_refptr<base::RefCountedBytes> bytes) {
   DCHECK_GT(image_info_.pixels_per_side, 0);
 
@@ -143,11 +176,13 @@ void UserImageRequest::OnImageCropped(
     OnDecodeImageFailed();
     return;
   }
-  OnImageFinalized(*bitmap, bytes, true /* image_bytes_regenerated */);
+  OnImageFinalized(*bitmap, *image_format, bytes,
+                   true /* image_bytes_regenerated */);
 }
 
 void UserImageRequest::OnImageFinalized(
     const SkBitmap& image,
+    user_manager::UserImage::ImageFormat image_format,
     scoped_refptr<base::RefCountedBytes> image_bytes,
     bool image_bytes_regenerated) {
   SkBitmap final_image = image;
@@ -158,9 +193,10 @@ void UserImageRequest::OnImageFinalized(
       gfx::ImageSkia::CreateFrom1xBitmap(final_image);
   final_image_skia.MakeThreadSafe();
   std::unique_ptr<user_manager::UserImage> user_image(
-      new user_manager::UserImage(final_image_skia, image_bytes));
+      new user_manager::UserImage(final_image_skia, image_bytes, image_format));
   user_image->set_file_path(image_info_.file_path);
   if (image_info_.image_codec == ImageDecoder::ROBUST_JPEG_CODEC ||
+      image_info_.image_codec == ImageDecoder::ROBUST_PNG_CODEC ||

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

This is a change in behavior right?

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

Yes. Added some comment to clarify the intention here.

       image_bytes_regenerated)
     user_image->MarkAsSafe();
   image_info_.loaded_cb.Run(std::move(user_image));