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..bd7758c03338b18814c4060132e43b2b2566969c 100644
--- a/chrome/browser/image_decoder.cc
+++ b/chrome/browser/image_decoder.cc
@@ -13,38 +13,129 @@
 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();

[dcheng, 2015/02/27 16:47:17]

It might be better to put this in the initializer. Then you mark the member
const as well?

[dcheng, 2015/02/27 16:47:17]

It might be better to put this in the initializer. Then you can mark the member
const as well.

[Theresa, 2015/03/04 03:10:06]

Done.

[Theresa, 2015/03/04 03:10:06]

Done.

+}
+
+ImageDecoder::~ImageDecoder() {
+}
+
+void ImageDecoder::Start(Delegate* delegate,
+                         const std::string& image_data,
+                         ImageCodec image_codec,
+                         scoped_refptr<base::SequencedTaskRunner> task_runner,
+                         bool shrink_to_fit) {
+  image_decoder_impl_->Start(delegate, image_data, image_codec,
+                             task_runner, shrink_to_fit);
+}
+
+// ImageDecoder::ImageDecoderImpl Methods
+
+ImageDecoder::ImageDecoderImpl::ImageDecoderImpl(){

[dcheng, 2015/02/27 16:47:17]

clang-format should catch these random missing spaces, though it still mangles
IPC message map macros, so you'll need to be a bit careful.

[Theresa, 2015/03/04 03:10:06]

Done.

+}
+
+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 the task runner is null, we can't post tasks back to it when the
+  // image is decoded. Notify the delegate if possible and return.
+  if (!task_runner) {

[dcheng, 2015/02/27 16:47:17]

This seems like a programming error that we should be DCHECK()ing on, rather
than trying to handle it gracefully somehow.

[Theresa, 2015/03/04 03:10:06]

Done.

+    if (delegate) {

[dcheng, 2015/02/27 16:47:17]

IMO, |delegate| should probably be required too.

[Theresa, 2015/03/04 03:10:06]

Done.

+      delegate->OnDecodeImageFailed();
+    }
+    return;
+  }
+
+  if (!batch_mode_started_) {
+    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_ = time(0);
+  delegate_map_[delegate_id_counter_] = delegate;

[dcheng, 2015/02/27 16:47:17]

I'd recommend using IDMap here. Also, it's probably better to just maintain one
map than two parallel maps.

[Theresa, 2015/03/04 03:10:06]

I changed it to one map instead of two parallel maps. IDMap can't be accessed
from multiple threads (it can be created on one thread and accessed from a
different, consistent thread post-construction, but not multiple threads
post-construction), so the DCHECK's in that class for CalledOnValidThread()
fail.

+  task_runner_map_[delegate_id_counter_] = task_runner;
+
   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_;
 }
 
-bool ImageDecoder::OnMessageReceived(const IPC::Message& message) {
+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);
+}
+
+void ImageDecoder::ImageDecoderImpl::PostDelayedStopBatchModeTask() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  BrowserThread::PostDelayedTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&ImageDecoder::ImageDecoderImpl::StopBatchMode,
+                 this),
+      base::TimeDelta::FromSeconds(batch_mode_timeout_seconds_));
+}
+
+void ImageDecoder::ImageDecoderImpl::StopBatchMode() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if ((time(0) - last_request_) < batch_mode_timeout_seconds_) {

[dcheng, 2015/02/27 16:47:17]

I think we can use base::DelayTimer here to simplify the code, instead of having
a lot of our own tracking logic.

[Theresa, 2015/03/04 03:10:06]

The timer api's aren't thread safe, so they won't work here.

[Theresa, 2015/03/05 16:53:05]

I rethought this and spent some time playing around with it last night. I can
work around the non-thread-safe nature by putting the DelayTimer creation and
Reset calls in StartBatchMode and DecodeImageInSandbox. I have a patchset
stashed that does this, but there are some crashes I need to debug. One thing I
don't like about using the DelayTimer is in the case I'm testing, the bookmarks
grid view, if you scroll past hundreds of bookmarks, hundreds of requests are
made to decode images and Reset is called on the timer hundreds of time causing
a bunch of churn. In the current approach, a delayed task is posted at most once
every 5 seconds (rather than reset hundreds of times), so I'd like to stick with
the current approach. If that's not okay, I can debug the crashes and change it
to a delay timer.

For the IDMap (other comments above), there is in theory a similar workaround
where I could insert and remove items solely from the methods that run on the IO
thread, but I'd have to work around the issue of scoped ptrs not being Bindable
because the task_runners would have to be passed to DecodeImageInSandbox.

+    PostDelayedStopBatchModeTask();
+    return;
+  }
+
+  if (utility_process_host_.get()) {
+    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 +145,63 @@ 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 (delegate_map_.find(id) != delegate_map_.end()
+      && task_runner_map_.find(id) != task_runner_map_.end()) {
+    task_runner_map_[id]->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &ImageDecoder::ImageDecoderImpl::NotifyDelegateDecodeSucceeded,
+            this,
+            decoded_image,
+            delegate_map_[id]));
+
+    delegate_map_.erase(id);
+    task_runner_map_.erase(id);
+  }
+}
+
+void ImageDecoder::ImageDecoderImpl::OnDecodeImageFailed(int id) {
+  if (delegate_map_.find(id) != delegate_map_.end()
+      && task_runner_map_.find(id) != task_runner_map_.end()) {
+    task_runner_map_[id]->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &ImageDecoder::ImageDecoderImpl::NotifyDelegateDecodeFailed,
+            this,
+            delegate_map_[id]));
+
+    delegate_map_.erase(id);
+    task_runner_map_.erase(id);
+  }
+}
+
+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));
   }
 }