Make image_decoder a Leaky LazyInstance

chrome/browser/image_decoder.cc

Index: chrome/browser/image_decoder.cc
diff --git a/chrome/browser/image_decoder.cc b/chrome/browser/image_decoder.cc
index 2f9e83b7ab8953f41eb2d5a4bfc091e8f7e81ce7..ab467d92531949d5e30094c2aa4b8b43a0fc3cb4 100644
--- a/chrome/browser/image_decoder.cc
+++ b/chrome/browser/image_decoder.cc
@@ -13,38 +13,125 @@
 using content::BrowserThread;
 using content::UtilityProcessHost;
 
-ImageDecoder::ImageDecoder(Delegate* delegate,
-                           const std::string& image_data,
-                           ImageCodec image_codec)
-    : delegate_(delegate),
-      image_data_(image_data.begin(), image_data.end()),
-      image_codec_(image_codec),
-      task_runner_(NULL),
-      shrink_to_fit_(false) {
-}
-
-ImageDecoder::ImageDecoder(Delegate* delegate,
-                           const std::vector<char>& image_data,
-                           ImageCodec image_codec)
-    : delegate_(delegate),
-      image_data_(image_data.begin(), image_data.end()),
-      image_codec_(image_codec),
-      task_runner_(NULL),
-      shrink_to_fit_(false) {
-}
-
-ImageDecoder::~ImageDecoder() {}
-
-void ImageDecoder::Start(scoped_refptr<base::SequencedTaskRunner> task_runner) {
-  task_runner_ = task_runner;
+// static, Leaky to allow access from any thread.
+base::LazyInstance<ImageDecoder>::Leaky image_decoder_instance_ =
+    LAZY_INSTANCE_INITIALIZER;
+
+// static
+ImageDecoder* ImageDecoder::GetInstance() {
+  return image_decoder_instance_.Pointer();
+}
+
+ImageDecoder::ImageDecoder() : image_decoder_impl_(new ImageDecoderImpl()) {
+}
+
+ImageDecoder::~ImageDecoder() {
+}
+
+ImageDecoder::ImageDecoderImpl::RequestInfo::RequestInfo(
+    Delegate* delegate,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : delegate(delegate), task_runner(task_runner) {
+}
+
+ImageDecoder::ImageDecoderImpl::RequestInfo::~RequestInfo() {
+}
+
+void ImageDecoder::Start(Delegate* delegate,
+                         const std::string& image_data,
+                         ImageCodec image_codec,
+                         scoped_refptr<base::SequencedTaskRunner> task_runner,
+                         bool shrink_to_fit) {
+  DCHECK(task_runner);
+  DCHECK(delegate);
+  image_decoder_impl_->Start(delegate, image_data, image_codec, task_runner,
+                             shrink_to_fit);
+}
+
+// ImageDecoder::ImageDecoderImpl Methods
+
+ImageDecoder::ImageDecoderImpl::ImageDecoderImpl()
+    : delegate_id_counter_(0),
+      last_request_(base::TimeTicks::Now()),
+      batch_mode_started_(false) {
+}
+
+ImageDecoder::ImageDecoderImpl::~ImageDecoderImpl() {
+}
+
+void ImageDecoder::ImageDecoderImpl::Start(
+    Delegate* delegate,
+    const std::string& image_data,
+    ImageCodec image_codec,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    bool shrink_to_fit) {
+  if (!batch_mode_started_) {

[dcheng, 2015/03/10 15:49:12]

Would it make the code simpler if we did something like this:
if (!batch_mode_started_) {
  /* start task */
}

/* unconditionally post the task to DecodeImageInSandbox here */


That way, I think you can move the ID allocation and map mutation entirely to
the IO thread (in DecodeImageInSandbox), so we can just use IDMap. This means we
post two tasks on the initial startup, but I think that's OK?

[Theresa, 2015/03/10 23:50:54]

Yes, that would be simpler. I shuffled things around so that StartBatchMode is
called from DecodeImageInSandbox; all this method does now is post a task to
DecodeImageInSandbox. I think the new approach is much cleaner and easier to
follow. The latest patchset also moves all of the work (IDMap manipulation,
checks for !batch_mode_started_, etc) to the IO thread so that we don't have to
worry about objects being accessed from multiple threads.

+    task_runner_at_start_ = task_runner;
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ImageDecoder::ImageDecoderImpl::StartBatchMode, this,
+                   delegate, image_data, image_codec, shrink_to_fit));
+    return;
+  }
+
+  last_request_ = base::TimeTicks::Now();
+  RequestInfo* request_info = new RequestInfo(delegate, task_runner);
+  request_info_id_map_.insert(
+      std::pair<int, RequestInfo*>(delegate_id_counter_, request_info));
+
   BrowserThread::PostTask(
-     BrowserThread::IO, FROM_HERE,
-     base::Bind(&ImageDecoder::DecodeImageInSandbox, this, image_data_));
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &ImageDecoder::ImageDecoderImpl::DecodeImageInSandbox, this, delegate,
+          std::vector<unsigned char>(image_data.begin(), image_data.end()),
+          image_codec, shrink_to_fit, delegate_id_counter_));
+
+  ++delegate_id_counter_;
+}
+
+void ImageDecoder::ImageDecoderImpl::StartBatchMode(
+    Delegate* delegate,
+    const std::string& image_data,
+    ImageCodec image_codec,
+    bool shrink_to_fit) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  utility_process_host_ =
+      UtilityProcessHost::Create(this, base::MessageLoopProxy::current().get())
+          ->AsWeakPtr();
+  utility_process_host_->StartBatchMode();
+  batch_mode_started_ = true;
+  PostDelayedStopBatchModeTask();
+  Start(delegate, image_data, image_codec, task_runner_at_start_,
+        shrink_to_fit);
 }
 
-bool ImageDecoder::OnMessageReceived(const IPC::Message& message) {
+void ImageDecoder::ImageDecoderImpl::PostDelayedStopBatchModeTask() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  BrowserThread::PostDelayedTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&ImageDecoder::ImageDecoderImpl::StopBatchMode, this),
+      base::TimeDelta::FromSeconds(kBatchModeTimeoutSeconds));
+}
+
+void ImageDecoder::ImageDecoderImpl::StopBatchMode() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if ((base::TimeTicks::Now() - last_request_) <
+      base::TimeDelta::FromSeconds(kBatchModeTimeoutSeconds)) {
+    PostDelayedStopBatchModeTask();
+    return;
+  }
+
+  if (utility_process_host_.get()) {

[dcheng, 2015/03/10 15:49:12]

Nit: .get() should be unnecessary to check the null-ity of a smart pointer.

[Theresa, 2015/03/10 23:50:54]

Done.

+    utility_process_host_->EndBatchMode();
+    utility_process_host_.reset();
+  }
+  batch_mode_started_ = false;
+}
+
+bool ImageDecoder::ImageDecoderImpl::OnMessageReceived(
+    const IPC::Message& message) {
   bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(ImageDecoder, message)
+  IPC_BEGIN_MESSAGE_MAP(ImageDecoder::ImageDecoderImpl, message)
     IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_DecodeImage_Succeeded,
                         OnDecodeImageSucceeded)
     IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_DecodeImage_Failed,
@@ -54,28 +141,60 @@ bool ImageDecoder::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
-void ImageDecoder::OnDecodeImageSucceeded(const SkBitmap& decoded_image) {
-  DCHECK(task_runner_->RunsTasksOnCurrentThread());
-  if (delegate_)
-    delegate_->OnImageDecoded(this, decoded_image);
+void ImageDecoder::ImageDecoderImpl::OnDecodeImageSucceeded(
+    const SkBitmap& decoded_image,
+    int id) {
+  if (request_info_id_map_.find(id) != request_info_id_map_.end()) {
+    RequestInfo* request_info = request_info_id_map_.find(id)->second;

[dcheng, 2015/03/10 15:49:12]

Maybe:
auto& entry = request_info_id_map_.find(id);
if (entry != request_info_map_.end()) {
  RequestInfo* request_info = entry->second; 
  /* stuff */
  request_info_id_map_.erase(entry);
}

[Theresa, 2015/03/10 23:50:54]

Acknowledged.

+    request_info->task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &ImageDecoder::ImageDecoderImpl::NotifyDelegateDecodeSucceeded,
+            this, decoded_image, request_info->delegate));

[dcheng, 2015/03/10 15:49:12]

You can eliminate an indirection here, I think. Just do:

base::Bind(&Delegate::OnImageDecoded, request_info->delegate).

This brings to mind another question though:
How do we guarantee delegate is valid by the time we call it? The old
ImageDecoder implementation allowed delegates to "cancel" by letting them set
the delegate to null. This might be covered elsewhere and I'm just not seeing it
since I'm skimming through the patch atm.

[Theresa, 2015/03/10 23:50:54]

wrt to the indirection, ref_counted.h complains about ImageDecoder::Delegate not
having AddRef()/Release(). 

This is a snippet of the code I was trying to compile:
    request_info->task_runner->PostTask(
        FROM_HERE,
        base::Bind(&Delegate::OnImageDecoded,
                   request_info->delegate,
                   decoded_image));

Eliminated the indirection.

With the current patchset, there is no "cancel" mechanism built in to
image_decoder. The one class I saw explicitly ignoring results was
change_picture_options_handler.cc. The way I read the code in that class,
ignoring the results is for convenience/to avoid unnecessary work, but not
setting the delegate to NULL (and therefore not ignoring the result) shouldn't
break things.

If that's incorrect (or cancel is desired for other reasons), I can look at ways
to add back a cancel mechanism.

[Theresa, 2015/03/12 00:36:32]

The first part of this comment, above "Eliminated the indirection" should have
been deleted. I was just missing base::Unretained() around
request_info->delegate.

+
+    request_info_id_map_.erase(id);
+    delete request_info;
+    request_info = NULL;

[dcheng, 2015/03/10 15:49:12]

Note: we prefer nullptr to NULL in new code.

[Theresa, 2015/03/10 23:50:54]

Done.

+  }
+}
+
+void ImageDecoder::ImageDecoderImpl::OnDecodeImageFailed(int id) {
+  if (request_info_id_map_.find(id) != request_info_id_map_.end()) {
+    RequestInfo* request_info = request_info_id_map_.find(id)->second;

[dcheng, 2015/03/10 15:49:12]

I think it's OK to just store this directly in the std::map rather than using an
indirection. The struct is pretty small, and strictly speaking, we would leak
any RequestInfo entries if ImageDecoderImpl is destroyed.

(This comment is obsolete if we transition to IDMap)

[Theresa, 2015/03/10 23:50:54]

Acknowledged.

+    request_info->task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(&ImageDecoder::ImageDecoderImpl::NotifyDelegateDecodeFailed,
+                   this, request_info->delegate));
+
+    request_info_id_map_.erase(id);
+    delete request_info;
+    request_info = NULL;
+  }
+}
+
+void ImageDecoder::ImageDecoderImpl::NotifyDelegateDecodeSucceeded(
+    const SkBitmap& decoded_image,
+    Delegate* delegate) {
+  delegate->OnImageDecoded(decoded_image);
 }
 
-void ImageDecoder::OnDecodeImageFailed() {
-  DCHECK(task_runner_->RunsTasksOnCurrentThread());
-  if (delegate_)
-    delegate_->OnDecodeImageFailed(this);
+void ImageDecoder::ImageDecoderImpl::NotifyDelegateDecodeFailed(
+    Delegate* delegate) {
+  delegate->OnDecodeImageFailed();
 }
 
-void ImageDecoder::DecodeImageInSandbox(
-    const std::vector<unsigned char>& image_data) {
+void ImageDecoder::ImageDecoderImpl::DecodeImageInSandbox(
+    Delegate* delegate,
+    const std::vector<unsigned char>& image_data,
+    ImageCodec image_codec,
+    bool shrink_to_fit,
+    int delegate_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  UtilityProcessHost* utility_process_host;
-  utility_process_host = UtilityProcessHost::Create(this, task_runner_.get());
-  if (image_codec_ == ROBUST_JPEG_CODEC) {
-    utility_process_host->Send(
-        new ChromeUtilityMsg_RobustJPEGDecodeImage(image_data));
+  if (image_codec == ROBUST_JPEG_CODEC) {
+    utility_process_host_->Send(
+        new ChromeUtilityMsg_RobustJPEGDecodeImage(image_data, delegate_id));
   } else {
-    utility_process_host->Send(
-        new ChromeUtilityMsg_DecodeImage(image_data, shrink_to_fit_));
+    utility_process_host_->Send(new ChromeUtilityMsg_DecodeImage(
+        image_data, shrink_to_fit, delegate_id));
   }
 }