Make image_decoder a Leaky LazyInstance

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 17088c37b8f696be62bb2570811cb8cbb64f1c58..56bef09feb984115b398a10755dc6df35d56d4f3 100644
--- a/chrome/browser/chromeos/login/users/avatar/user_image_loader.cc
+++ b/chrome/browser/chromeos/login/users/avatar/user_image_loader.cc
@@ -31,6 +31,14 @@ UserImageLoader::ImageInfo::ImageInfo(const std::string& file_path,
 UserImageLoader::ImageInfo::~ImageInfo() {
 }
 
+UserImageLoader::ImageData::ImageData(const ImageInfo& image_info,
+                                      const std::string& data)
+    : image_info(image_info), data(data.begin(), data.end()) {
+}
+
+UserImageLoader::ImageData::~ImageData() {
+}
+
 UserImageLoader::UserImageLoader(
     ImageDecoder::ImageCodec image_codec,
     scoped_refptr<base::SequencedTaskRunner> background_task_runner)
@@ -79,25 +87,22 @@ void UserImageLoader::DecodeImage(const scoped_ptr<std::string> data,
                                   const ImageInfo& image_info) {
   DCHECK(background_task_runner_->RunsTasksOnCurrentThread());
 
-  scoped_refptr<ImageDecoder> image_decoder =
-      new ImageDecoder(this, *data, image_codec_);
-  image_info_map_.insert(std::make_pair(image_decoder.get(), image_info));
-  image_decoder->Start(background_task_runner_);
+  ImageData image_data(image_info, *data);
+  image_data_queue_.push(image_data);
+  ImageDecoder::GetInstance()->Start(this, *data, image_codec_,
+                                     background_task_runner_, false);
 }
 
-void UserImageLoader::OnImageDecoded(const ImageDecoder* decoder,
-                                     const SkBitmap& decoded_image) {
+void UserImageLoader::OnImageDecoded(const SkBitmap& decoded_image) {
   DCHECK(background_task_runner_->RunsTasksOnCurrentThread());
 
-  ImageInfoMap::iterator it = image_info_map_.find(decoder);
-  if (it == image_info_map_.end()) {
-    NOTREACHED();
-    return;
-  }
-  const std::string file_path = it->second.file_path;
-  const int target_size = it->second.pixels_per_side;
-  const LoadedCallback loaded_cb = it->second.loaded_cb;
-  image_info_map_.erase(it);
+  const ImageData& image_data = image_data_queue_.front();
+  const ImageInfo& image_info = image_data.image_info;
+  const std::string file_path = image_info.file_path;
+  const int target_size = image_info.pixels_per_side;
+  const LoadedCallback loaded_cb = image_info.loaded_cb;
+  std::vector<unsigned char> original_data = image_data.data;
+  image_data_queue_.pop();

[dcheng, 2015/03/19 14:25:46]

After thinking about this, I feel uncomfortable with depending on a FIFO
guarantee by Start() to guarantee correctness of the multiplexed requests.

I think it's better to either:
1) Use base::Bind() and bundle all the required state with the bound callback.
2) Disallow the use of a single ImageDispatcher for multiple requests.

Of these options, I would prefer #2.

#1 requires each call to Start() to bind two callbacks, and instantiates a bunch
of templates that probably won't be folded together, resulting in bloat.

#2 is similar to the existing model, except that places like this probably
wouldn't subclass ImageDelegate directly and would have a helper class subclass
it instead. We could also rename ImageDelegate to ImageRequest too (which is
probably a more appropriate name, since delegate typically implies that there's
only one of them). So we'd end up with something like:

class ImageRequest {
 public:
  virtual ~ImageRequest();
  virtual void OnImageDecoded(...) = 0;
  virtual void OnDecodeImageFailed(...) = 0;
  TaskRunner* task_runner() const { return t.get(); }

 protected:
  explicit ImageRequest(const scoped_refptr<TaskRunner>& t);

 private:
  const scoped_refptr<T> t;
};

and then starting/stopping it would look like:

ImageDecoder::Start(request);

and we could make it automatically cancel on destruction too:
ImageRequest::~ImageRequest() {
  ImageDecoder::Cancel(this);
}

(though of course this comes with its own bag of worms WRT thread-safety issues,
see below)

[Theresa, 2015/03/19 21:54:56]

I created a UserImageLoader::ImageRequest subclass that extends
ImageDecoder::Delegate; it keeps track of all the info needed in OnImageDecoded
and OnDecodeImageFailed. The queue is gone :)

I also changed the ImageDecoder::Delegate class to keep track of the task
runner; I really like this, because it's one less arg that has to get passed
around.

I haven't renamed Delegate -> ImageRequest yet. For cases like this, where a new
ImageRequest object will get created for each image to decode, "request" makes
sense to me. For most other cases, however, the classes using ImageDecoder
directly inherit from ImageDecoder::Delegate; they don't have or create multiple
delegates, they just are the "delegate". I don't particularly care about the
name, though, so if ImageRequest makes more sense with how we generally use the
terms in Chromium, I'll change it.

[dcheng, 2015/03/20 00:14:45]

I think it's OK for those single-shot instances to also subclass from
ImageRequest. Since they are single-shot requests, it makes sense that the class
itself subclasses ImageRequest directly (LogoDecoderDelegate is an instance of
this).

[Theresa, 2015/03/20 19:09:01]

Done.

 
   SkBitmap final_image = decoded_image;
 
@@ -126,8 +131,7 @@ void UserImageLoader::OnImageDecoded(const ImageDecoder* decoder,
   gfx::ImageSkia final_image_skia =
       gfx::ImageSkia::CreateFrom1xBitmap(final_image);
   final_image_skia.MakeThreadSafe();
-  user_manager::UserImage user_image(final_image_skia,
-                                     decoder->get_image_data());
+  user_manager::UserImage user_image(final_image_skia, original_data);
   user_image.set_file_path(file_path);
   if (image_codec_ == ImageDecoder::ROBUST_JPEG_CODEC)
     user_image.MarkAsSafe();
@@ -135,16 +139,12 @@ void UserImageLoader::OnImageDecoded(const ImageDecoder* decoder,
                                     base::Bind(loaded_cb, user_image));
 }
 
-void UserImageLoader::OnDecodeImageFailed(const ImageDecoder* decoder) {
+void UserImageLoader::OnDecodeImageFailed() {
   DCHECK(background_task_runner_->RunsTasksOnCurrentThread());
 
-  ImageInfoMap::iterator it = image_info_map_.find(decoder);
-  if (it == image_info_map_.end()) {
-    NOTREACHED();
-    return;
-  }
-  const LoadedCallback loaded_cb = it->second.loaded_cb;
-  image_info_map_.erase(it);
+  ImageInfo image_info = image_data_queue_.front().image_info;
+  const LoadedCallback loaded_cb = image_info.loaded_cb;
+  image_data_queue_.pop();
 
   foreground_task_runner_->PostTask(
       FROM_HERE, base::Bind(loaded_cb, user_manager::UserImage()));