Get rid of UserImageManagerImpl::Job::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"
@@ skipping 221 matching lines @@
   // image.
   void SetToPath(const base::FilePath& path,
                  int image_index,
                  const GURL& image_url,
                  bool resize);
 
  private:
   // Called back after an image has been loaded from disk.
   void OnLoadImageDone(bool save, const user_manager::UserImage& user_image);
 
-  // Updates the user object with |user_image_|.
-  void UpdateUser();
+  // Updates the user object with |user_image|.
+  void UpdateUser(const user_manager::UserImage& user_image);
 
   // Saves |user_image_| to disk in JPEG format. Local state will be updated
   // when a callback indicates that the image has been saved.
-  void SaveImageAndUpdateLocalState();
+  void SaveImageAndUpdateLocalState(const user_manager::UserImage& user_image);
 
-  // Called back after the |user_image_| has been saved to
+  // 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);
 
   // Updates the user image in local state, setting it to one of the
-  // default images or the saved |user_image_|, depending on
+  // default images or the saved user image, depending on
   // |image_index_|.
   void UpdateLocalState();
 
   // Notifies the |parent_| that the Job is done.
   void NotifyJobDone();
 
   const std::string& user_id() const { return parent_->user_id(); }
 
   UserImageManagerImpl* parent_;
 
   // Whether one of the Load*() or Set*() methods has been run already.
   bool run_;
 
   int image_index_;
   GURL image_url_;
   base::FilePath image_path_;
 
-  user_manager::UserImage user_image_;
-
   base::WeakPtrFactory<Job> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
 UserImageManagerImpl::Job::Job(UserImageManagerImpl* parent)
     : parent_(parent),
       run_(false),
       weak_factory_(this) {
 }
 
 UserImageManagerImpl::Job::~Job() {
 }
 
 void UserImageManagerImpl::Job::LoadImage(base::FilePath image_path,
                                           const int image_index,
                                           const GURL& image_url) {
   DCHECK(!run_);
   run_ = true;
 
   image_index_ = image_index;
   image_url_ = image_url;
   image_path_ = image_path;
 
   if (image_index_ >= 0 &&
       image_index_ < default_user_image::kDefaultImagesCount) {
     // Load one of the default images. This happens synchronously.
-    user_image_ = user_manager::UserImage(
+    const user_manager::UserImage user_image(
         default_user_image::GetDefaultImage(image_index_));
-    UpdateUser();
+    UpdateUser(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.
     DCHECK(!image_path_.empty());
     user_image_loader::StartWithFilePath(
         parent_->background_task_runner_, image_path_,
         ImageDecoder::ROBUST_JPEG_CODEC,
         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_);
   run_ = true;
 
   DCHECK_LE(0, default_image_index);
   DCHECK_GT(default_user_image::kDefaultImagesCount, default_image_index);
 
   image_index_ = default_image_index;
-  user_image_ = user_manager::UserImage(
+  const user_manager::UserImage user_image(
       default_user_image::GetDefaultImage(image_index_));
 
-  UpdateUser();
+  UpdateUser(user_image);
   UpdateLocalState();
   NotifyJobDone();
 }
 
 void UserImageManagerImpl::Job::SetToImage(
     int image_index,
     const user_manager::UserImage& user_image) {
   DCHECK(!run_);
   run_ = true;
 
   DCHECK(image_index == user_manager::User::USER_IMAGE_EXTERNAL ||
          image_index == user_manager::User::USER_IMAGE_PROFILE);
 
   image_index_ = image_index;
-  user_image_ = user_image;
 
-  UpdateUser();
-  SaveImageAndUpdateLocalState();
+  UpdateUser(user_image);
+  SaveImageAndUpdateLocalState(user_image);
 }
 
 void UserImageManagerImpl::Job::SetToImageData(scoped_ptr<std::string> data) {
   DCHECK(!run_);
   run_ = true;
 
   image_index_ = user_manager::User::USER_IMAGE_EXTERNAL;
 
   // This method uses ROBUST_JPEG_CODEC, not DEFAULT_CODEC:
   // * This is necessary because the method is used to update the user image
@@ skipping 24 matching lines @@
   DCHECK(!path.empty());
   user_image_loader::StartWithFilePath(
       parent_->background_task_runner_, path, ImageDecoder::DEFAULT_CODEC,
       resize ? login::kMaxUserImageSize : 0,
       base::Bind(&Job::OnLoadImageDone, weak_factory_.GetWeakPtr(), true));
 }
 
 void UserImageManagerImpl::Job::OnLoadImageDone(
     bool save,
     const user_manager::UserImage& user_image) {
-  user_image_ = user_image;
-  UpdateUser();
+  UpdateUser(user_image);
   if (save)
-    SaveImageAndUpdateLocalState();
+    SaveImageAndUpdateLocalState(user_image);
   else
     NotifyJobDone();
 }
 
-void UserImageManagerImpl::Job::UpdateUser() {
+void UserImageManagerImpl::Job::UpdateUser(
+    const user_manager::UserImage& user_image) {
   user_manager::User* user = parent_->GetUserAndModify();
   if (!user)
     return;
 
-  if (!user_image_.image().isNull()) {
-    user->SetImage(user_image_, image_index_);
+  if (!user_image.image().isNull()) {
+    user->SetImage(user_image, image_index_);
   } else {
     user->SetStubImage(
         user_manager::UserImage(
             *ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
                 IDR_PROFILE_PICTURE_LOADING)),
         image_index_,
         false);
   }
   user->SetImageURL(image_url_);
 
   parent_->OnJobChangedUserImage();
 }
 
-void UserImageManagerImpl::Job::SaveImageAndUpdateLocalState() {
+void UserImageManagerImpl::Job::SaveImageAndUpdateLocalState(
+    const user_manager::UserImage& user_image) {
   base::FilePath user_data_dir;
   PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
   image_path_ = user_data_dir.Append(user_id() + kSafeImagePathExtension);
 
   base::PostTaskAndReplyWithResult(
-      parent_->background_task_runner_.get(),
-      FROM_HERE,
-      base::Bind(&SaveImage, user_image_, image_path_),
+      parent_->background_task_runner_.get(), FROM_HERE,
+      base::Bind(&SaveImage, user_image, 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 89 matching lines @@
                      image_index,
                      true);
   DCHECK(!image_path.empty() ||
          image_index == user_manager::User::USER_IMAGE_PROFILE);
   if (image_path.empty()) {
     // Return if the profile image is to be used but has not been downloaded
     // yet. The profile image will be downloaded after login.
     return;
   }
 
   job_.reset(new Job(this));

[satorux1, 2016/03/14 03:54:40]

BTW, the Job class looks rather awkward to me. I think we can get rid of the
class as follows:

1) replace the member variables image_(image|url|path)_ in the class to local
variables and pass them to other functions as needed
2) make some of the functions in the class free functions in anonymous namespace
3) make the rest of the functions in the class part of UserImageManagerImpl
class

If you think it's a good, I'll give it a try.

[hashimoto, 2016/03/14 05:59:16]

I agree the design of the class is not pretty, but you have to figure out how to
implement the same functionality served by WeakPtrFactory which allows you to
ignore the result of the previous job, by calling job_.reset().
If you think this can be done easily, then I think it might be something OK to
do.

[satorux1, 2016/03/14 06:10:26]

That's a good point. I'll keep the code as is for now

   job_->LoadImage(base::FilePath(image_path), image_index, image_url);
 }
 
 void UserImageManagerImpl::UserLoggedIn(bool user_is_new,
                                         bool user_is_local) {
   const user_manager::User* user = GetUser();
   if (user_is_new) {
     if (!user_is_local)
       SetInitialUserImage();
   } else {
@@ skipping 374 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