Crop the user-specified profile image for WebUI

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"
@@ skipping 146 matching lines @@
       return default_user_image::kHistogramImageFromCamera;
     case user_manager::User::USER_IMAGE_PROFILE:
       return default_user_image::kHistogramImageFromProfile;
     default:
       return image_index;
   }
 }
 
 bool SaveImage(const user_manager::UserImage& user_image,
                const base::FilePath& image_path) {
-  user_manager::UserImage safe_image;
-  const user_manager::UserImage::Bytes* encoded_image = NULL;
+  // 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.
+  DCHECK(user_image.is_safe_format());

[hashimoto, 2016/03/02 07:33:58]

Instead of DCHECK, shouldn't we have LOG(ERROR) when this condition doesn't
hold?

[satorux1, 2016/03/02 08:23:51]

Done.

+  // However, check the value just in case because an unsafe image should
+  // never be saved.
   if (!user_image.is_safe_format()) {
-    safe_image = user_manager::UserImage::CreateAndEncode(user_image.image());
-    encoded_image = &safe_image.image_bytes();
-    UMA_HISTOGRAM_MEMORY_KB("UserImage.RecodedJpegSize", encoded_image->size());
-  } else if (user_image.has_image_bytes()) {
-    encoded_image = &user_image.image_bytes();
-  } else {
-    NOTREACHED() << "image data bytes missing.";
     return false;
   }
 
-  if (!encoded_image->size() ||
+  const user_manager::UserImage::Bytes& image_bytes = user_image.image_bytes();
+  if (!image_bytes.size() ||

[hashimoto, 2016/03/02 07:33:58]

nit: image_bytes.empty()?

[satorux1, 2016/03/02 08:23:51]

Done.

       base::WriteFile(image_path,
-                      reinterpret_cast<const char*>(&(*encoded_image)[0]),
-                      encoded_image->size()) == -1) {
+                      reinterpret_cast<const char*>(&image_bytes[0]),

[hashimoto, 2016/03/02 07:33:58]

&image_bytes[0] -> image_bytes.data()?

nit: I'm not sure why UserImage::Bytes' element type is unsigned char while most
code in base and net uses char as the byte type.
Can't we just change UserImage::Bytes' type to std::vector<char> and eliminate
this reinterpret_cast?

[satorux1, 2016/03/02 08:23:51]

Done
I also had the same question. My guess is that it's because JPEGCodec::Encode()
takes vector<unsigned char>* for the output parameter.

https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/codec/jpeg_...

+                      image_bytes.size()) == -1) {
     LOG(ERROR) << "Failed to save image to file.";
     return false;
   }
 
   return true;
 }
 
 }  // namespace
 
 // static
@@ skipping 831 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