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..b1f020188b7195e7a7ce9b461a6abb089e7096f8 100644
--- a/chrome/browser/image_decoder.cc
+++ b/chrome/browser/image_decoder.cc
@@ -13,36 +13,120 @@
 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) {
+namespace {

[Lei Zhang, 2015/03/25 21:44:46]

nit: add a blank line after this and before the end of the anonymous namespace.
There should be 2 spaces in the ending comment.

[Theresa, 2015/03/25 23:13:15]

Done.

+// static, Leaky to allow access from any thread.
+base::LazyInstance<ImageDecoder>::Leaky g_decoder_ = LAZY_INSTANCE_INITIALIZER;

[Lei Zhang, 2015/03/25 21:44:46]

nit: just |g_decoder| since it's not a member variable.

[Theresa, 2015/03/25 23:13:15]

Done.

+} // namespace
+
+ImageDecoder::ImageDecoder()
+    : image_request_id_counter_(0), last_request_(base::TimeTicks::Now()) {
+  // A single ImageDecoder instance should live for the life of the program.
+  // Explicitly add a reference so the object isn't deleted.
+  AddRef();
+}
+
+ImageDecoder::~ImageDecoder() {
 }
 
-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::ImageRequest::ImageRequest(
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner)
+    : task_runner_(task_runner) {
 }
 
-ImageDecoder::~ImageDecoder() {}
+ImageDecoder::ImageRequest::~ImageRequest() {
+  ImageDecoder::Cancel(this);
+}
 
-void ImageDecoder::Start(scoped_refptr<base::SequencedTaskRunner> task_runner) {
-  task_runner_ = task_runner;
+// static
+void ImageDecoder::Start(ImageRequest* image_request,
+                         const std::string& image_data,
+                         ImageCodec image_codec,
+                         bool shrink_to_fit) {
+  DCHECK(image_request);
+  DCHECK(image_request->task_runner());
   BrowserThread::PostTask(
-     BrowserThread::IO, FROM_HERE,
-     base::Bind(&ImageDecoder::DecodeImageInSandbox, this, image_data_));
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &ImageDecoder::DecodeImageInSandbox,
+          g_decoder_.Pointer(), image_request,
+          std::vector<unsigned char>(image_data.begin(), image_data.end()),
+          image_codec, shrink_to_fit));
+}
+
+// static
+void ImageDecoder::Cancel(ImageRequest* image_request) {
+  DCHECK(image_request);
+  g_decoder_.Pointer()->CancelImpl(image_request);
+}
+
+void ImageDecoder::DecodeImageInSandbox(
+    ImageRequest* image_request,
+    const std::vector<unsigned char>& image_data,
+    ImageCodec image_codec,
+    bool shrink_to_fit) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

[Lei Zhang, 2015/03/25 21:44:46]

DCHECK_CURRENTLY_ON()

[Theresa, 2015/03/25 23:13:15]

Done.

+  if (!utility_process_host_) {
+    StartBatchMode();
+  }
+
+  last_request_ = base::TimeTicks::Now();

[Lei Zhang, 2015/03/25 21:44:46]

If |batch_mode_timer_| is running, can't you use base::Timer::Reset() here,
instead of tracking a separate |last_request_| ?

[Theresa, 2015/03/25 23:13:15]

Yes, Reset() would effectively do the same thing. I chose not to use it, because
for some use cases (e.g. scrolling through bookmarks), hundreds of image
decoding requests may come in at once and calling Reset on the timer each time
would cause more churn than checking if batch mode should be stopped every 5
seconds.

The code for Timer::Reset() doesn't look like it has super high overhead if
there is an existing scheduled task that can be reused, though, so I'm not
opposed to changing this to a OneShotTimer w/ calls to Reset(). 

+  base::AutoLock lock(map_lock_);
+  image_request_id_map_.insert(
+      std::make_pair(image_request_id_counter_, image_request));
+
+  switch (image_codec) {
+    case ROBUST_JPEG_CODEC:
+      utility_process_host_->Send(new ChromeUtilityMsg_RobustJPEGDecodeImage(
+          image_data, image_request_id_counter_));
+      break;
+    case DEFAULT_CODEC:
+      utility_process_host_->Send(new ChromeUtilityMsg_DecodeImage(
+          image_data, shrink_to_fit, image_request_id_counter_));
+      break;
+  }
+
+  ++image_request_id_counter_;
+}
+
+void ImageDecoder::CancelImpl(ImageRequest* image_request) {
+  base::AutoLock lock(map_lock_);
+  for (auto it = image_request_id_map_.begin();
+       it != image_request_id_map_.end();) {
+    if (it->second == image_request) {
+      image_request_id_map_.erase(it++);
+    } else {
+      ++it;
+    }
+  }
+}
+
+void ImageDecoder::StartBatchMode() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  utility_process_host_ =
+      UtilityProcessHost::Create(this, base::MessageLoopProxy::current().get())
+          ->AsWeakPtr();
+  utility_process_host_->StartBatchMode();
+  batch_mode_timer_.Start(
+      FROM_HERE, base::TimeDelta::FromSeconds(kBatchModeTimeoutSeconds),
+      this, &ImageDecoder::StopBatchMode);
+}
+
+void ImageDecoder::StopBatchMode() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if ((base::TimeTicks::Now() - last_request_)
+      < base::TimeDelta::FromSeconds(kBatchModeTimeoutSeconds)) {
+    return;
+  }
+
+  if (utility_process_host_) {
+    utility_process_host_->EndBatchMode();
+    utility_process_host_.reset();
+  }
+  batch_mode_timer_.Stop();
 }
 
-bool ImageDecoder::OnMessageReceived(const IPC::Message& message) {
+bool ImageDecoder::OnMessageReceived(
+    const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ImageDecoder, message)
     IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_DecodeImage_Succeeded,
@@ -54,28 +138,32 @@ 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::OnDecodeImageSucceeded(
+    const SkBitmap& decoded_image,
+    int id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  base::AutoLock lock(map_lock_);
+  auto it = image_request_id_map_.find(id);
+  if (it != image_request_id_map_.end()) {
+    ImageRequest* image_request = it->second;
+    image_request->task_runner()->PostTask(
+        FROM_HERE, base::Bind(&ImageRequest::OnImageDecoded,
+                              base::Unretained(image_request), decoded_image));
 
-void ImageDecoder::OnDecodeImageFailed() {
-  DCHECK(task_runner_->RunsTasksOnCurrentThread());
-  if (delegate_)
-    delegate_->OnDecodeImageFailed(this);
+    image_request_id_map_.erase(it);
+  }
 }
 
-void ImageDecoder::DecodeImageInSandbox(
-    const std::vector<unsigned char>& image_data) {
+void ImageDecoder::OnDecodeImageFailed(int 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));
-  } else {
-    utility_process_host->Send(
-        new ChromeUtilityMsg_DecodeImage(image_data, shrink_to_fit_));
+  base::AutoLock lock(map_lock_);
+  auto it = image_request_id_map_.find(id);
+  if (it != image_request_id_map_.end()) {
+    ImageRequest* image_request = it->second;
+    image_request->task_runner()->PostTask(
+        FROM_HERE, base::Bind(&ImageRequest::OnDecodeImageFailed,
+                              base::Unretained(image_request)));
+
+    image_request_id_map_.erase(it);
   }
 }