Make image_decoder a Leaky LazyInstance

chrome/browser/image_decoder.h

Index: chrome/browser/image_decoder.h
diff --git a/chrome/browser/image_decoder.h b/chrome/browser/image_decoder.h
index 2206dd9f454ebcbfc3a29674cf41e1d42eb594ee..36111a8534be71ecddf719dd56fd3042b7d61c6c 100644
--- a/chrome/browser/image_decoder.h
+++ b/chrome/browser/image_decoder.h
@@ -5,30 +5,32 @@
 #ifndef CHROME_BROWSER_IMAGE_DECODER_H_
 #define CHROME_BROWSER_IMAGE_DECODER_H_
 
+#include <map>
 #include <string>
 #include <vector>
 
 #include "base/compiler_specific.h"
+#include "base/lazy_instance.h"
 #include "base/memory/ref_counted.h"
 #include "base/threading/sequenced_worker_pool.h"
+#include "content/public/browser/utility_process_host.h"
 #include "content/public/browser/utility_process_host_client.h"
 
 class SkBitmap;
 
-// Decodes an image in a sandboxed process.
-class ImageDecoder : public content::UtilityProcessHostClient {
+// Singleton to decode images in a sandboxed process.
+class ImageDecoder {
  public:
   class Delegate {
    public:
     // Called when image is decoded.
     // |decoder| is used to identify the image in case of decoding several

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

This comment needs to be updated.

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

Done.

     // images simultaneously.
-    virtual void OnImageDecoded(const ImageDecoder* decoder,
-                                const SkBitmap& decoded_image) = 0;
+    virtual void OnImageDecoded(const SkBitmap& decoded_image) = 0;
 
     // Called when decoding image failed. Delegate can do some cleanup in
     // this handler.
-    virtual void OnDecodeImageFailed(const ImageDecoder* decoder) {}
+    virtual void OnDecodeImageFailed() {}
 
    protected:
     virtual ~Delegate() {}
@@ -39,44 +41,97 @@ class ImageDecoder : public content::UtilityProcessHostClient {
     ROBUST_JPEG_CODEC,  // Restrict decoding to robust jpeg codec.
   };
 
-  ImageDecoder(Delegate* delegate,
-               const std::string& image_data,
-               ImageCodec image_codec);
-
-  ImageDecoder(Delegate* delegate,
-               const std::vector<char>& image_data,
-               ImageCodec image_codec);
+  static ImageDecoder* GetInstance();
 
   // Starts asynchronous image decoding. Once finished, the callback will be
   // posted back to |task_runner|.
-  void Start(scoped_refptr<base::SequencedTaskRunner> task_runner);
-
-  const std::vector<unsigned char>& get_image_data() const {
-    return image_data_;
-  }
-
-  void set_delegate(Delegate* delegate) { delegate_ = delegate; }
-  void set_shrink_to_fit(bool shrink_to_fit) { shrink_to_fit_ = shrink_to_fit; }
+  void Start(Delegate* delegate,
+             const std::string& image_data,
+             ImageCodec image_codec,
+             scoped_refptr<base::SequencedTaskRunner> task_runner,

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

Just an aside, it's slightly better to pass a scoped_refptr<T> arg as const
scoped_refptr<T>&. That being said, the majority of Chrome code doesn't bother
(and we may or may not change guidance on the future, depending on whether or
not scoped_refptr becomes movable). This is more informational than anything, as
it's probably not worth changing given the uncertain state of affairs.

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

Acknowledged.

+             bool shrink_to_fit = false);

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

The Google C++ style guide forbids default args =(

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

Done.

 
  private:
-  // It's a reference counted object, so destructor is private.
-  ~ImageDecoder() override;
-
-  // Overidden from UtilityProcessHostClient:
-  bool OnMessageReceived(const IPC::Message& message) override;
-
-  // IPC message handlers.
-  void OnDecodeImageSucceeded(const SkBitmap& decoded_image);
-  void OnDecodeImageFailed();
-
-  // Launches sandboxed process that will decode the image.
-  void DecodeImageInSandbox(const std::vector<unsigned char>& image_data);
+  friend struct base::DefaultLazyInstanceTraits<ImageDecoder>;
+  ImageDecoder();
+  ~ImageDecoder();
+
+  class ImageDecoderImpl : public content::UtilityProcessHostClient {

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

I can't say I'm enthused about this delegate. I don't have any good alternatives
though. 

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

Acknowledged. I agree, but I can't think of any good ways around it.

+    public:
+      ImageDecoderImpl();
+      void Start(Delegate* delegate,
+                 const std::string& image_data,
+                 ImageCodec image_codec,
+                 scoped_refptr<base::SequencedTaskRunner> task_runner,
+                 bool shrink_to_fit = false);
+
+    private:
+      // It's a reference counted object, so destructor is private.
+      ~ImageDecoderImpl() override;
+
+      // Starts UtilityProcessHost in batch mode and calls
+      // PostDelayedStopBatchModeTask.
+      void StartBatchMode(Delegate* delegate,
+                          const std::string& image_data,
+                          ImageCodec image_codec,
+                          bool shrink_to_fit);
+
+      // Posts a delayed task to stop the UtilityProcessHost running in
+      // batch mode.
+      void PostDelayedStopBatchModeTask();
+
+      // Stops batch mode if no requests have come in since
+      // |batch_mode_timeout_seconds_|; else calls PostDelayedStopBatchModeTask.
+      void StopBatchMode();
+
+      // Overidden from UtilityProcessHostClient.
+      bool OnMessageReceived(const IPC::Message& message) override;
+
+      // IPC message handlers.
+      void OnDecodeImageSucceeded(const SkBitmap& decoded_image, int id);
+      void OnDecodeImageFailed(int id);
+
+      void NotifyDelegateDecodeSucceeded(const SkBitmap& decoded_image,
+                                         Delegate* delegate);
+      void NotifyDelegateDecodeFailed(Delegate* delegate);
+
+      // Sends a request to the sandboxed process to decode the image.
+      void DecodeImageInSandbox(Delegate* delegate,
+                                const std::vector<unsigned char>& image_data,
+                                ImageCodec image_codec,
+                                bool shrink_to_fit,
+                                int delegate_id);
+
+      // id to use for the next Start request that comes in.
+      int delegate_id_counter_ = 0;

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

Warning: personal preference. I prefer not to use in-class initialization if
there's only one constructor. But it doesn't seem like a big deal.

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

Done.

+
+      // Map of request id -> Delegate*
+      std::map<int, Delegate*> delegate_map_;
+
+      // Map of request id -> SequencedTaskRunner
+      std::map<int, scoped_refptr<base::SequencedTaskRunner>> task_runner_map_;
+
+      // The SequencedTaskRunner provided when Start is called before batch mode
+      // is started. base::Bind does not accept scoped_refptr objects, so the
+      // value must be saved instead.
+      scoped_refptr<base::SequencedTaskRunner> task_runner_at_start_;
+
+      // The UtilityProcessHost requests are sent to.
+      base::WeakPtr<content::UtilityProcessHost> utility_process_host_;
+
+      // The time Start was last called.
+      time_t last_request_ = 0;

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

This should probably be base::TimeTicks.

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

Done.

+
+      // How many seconds to wait after the last request has been received
+      // before ending batch mode.
+      const int batch_mode_timeout_seconds_ = 5;

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

kBatchModeTimeoutSeconds

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

Done.

+
+      // True iff utility_process_host_ has been created and started in
+      // batch bode.
+      bool batch_mode_started_ = false;
+  };
 
-  Delegate* delegate_;
-  std::vector<unsigned char> image_data_;
-  const ImageCodec image_codec_;
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
-  bool shrink_to_fit_; // if needed for IPC msg size limit
+  scoped_refptr<ImageDecoderImpl> image_decoder_impl_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageDecoder);
 };