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..24d864352ba9a59c73955c5dfd4e948710644e66 100644
--- a/chrome/browser/image_decoder.h
+++ b/chrome/browser/image_decoder.h
@@ -5,33 +5,47 @@
 #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/synchronization/lock.h"
 #include "base/threading/sequenced_worker_pool.h"
+#include "base/timer/timer.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 {
+// This is a helper class for decoding images safely in a utility process. To
+// use this, call ImageDecoder::Start(...) on any thread.
+//
+// Internally, all of the work happens on the IO thread, and then
+// the result (ImageRequest::OnImageDecoded or
+// ImageRequest::OnDecodeImageFailed) is posted back to the task runner
+// specified when Start(...) was called.
+class ImageDecoder {
  public:
-  class Delegate {
+  class ImageRequest {
    public:
     // Called when image is decoded.
-    // |decoder| is used to identify the image in case of decoding several
-    // 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
+    // Called when decoding image failed. ImageRequest can do some cleanup in
     // this handler.
-    virtual void OnDecodeImageFailed(const ImageDecoder* decoder) {}
+    virtual void OnDecodeImageFailed() {}
+
+    base::SequencedTaskRunner* task_runner() const { return t.get(); }
 
    protected:
-    virtual ~Delegate() {}
+    explicit ImageRequest(const scoped_refptr<base::SequencedTaskRunner>& t);
+    virtual ~ImageRequest();
+
+   private:
+    const scoped_refptr<base::SequencedTaskRunner> t;

[dcheng, 2015/03/23 11:52:09]

Members should definitely have a descriptive name (task_runner_). Sorry that my
example snippet implied otherwise. =(

[Theresa, 2015/03/23 17:33:10]

Done.

   };
 
   enum ImageCodec {
@@ -39,44 +53,83 @@ 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);
-
   // 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_;
-  }
+  static void Start(ImageRequest* image_request,
+                    const std::string& image_data,
+                    ImageCodec image_codec,
+                    bool shrink_to_fit);
 
-  void set_delegate(Delegate* delegate) { delegate_ = delegate; }
-  void set_shrink_to_fit(bool shrink_to_fit) { shrink_to_fit_ = shrink_to_fit; }
+  // Removes all instances of image_request from the IDMap, ensuring callbacks

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

Note for self: update this comment to no longer refer to IDMap

[Theresa, 2015/03/23 17:33:10]

Done.

+  // are not made to the image_request after it is destroyed.
+  static void Cancel(ImageRequest* image_request);
 
  private:
-  // It's a reference counted object, so destructor is private.
-  ~ImageDecoder() override;
+  friend struct base::DefaultLazyInstanceTraits<ImageDecoder>;
+  static ImageDecoder* GetInstance();

[dcheng, 2015/03/23 11:52:09]

This should be unnecessary to expose in the header now.

[Theresa, 2015/03/23 17:33:10]

I think it still needs to be declared here before it's defined in the c++ file.

+  ImageDecoder();
+  ~ImageDecoder();
+
+  class ImageDecoderImpl : public content::UtilityProcessHostClient {
+   public:
+    ImageDecoderImpl();
+
+    // Sends a request to the sandboxed process to decode the image. Starts
+    // batch mode if necessary.
+    void DecodeImageInSandbox(ImageRequest* image_request,
+                              const std::vector<unsigned char>& image_data,
+                              ImageCodec image_codec,
+                              bool shrink_to_fit);
+
+    void Cancel(ImageRequest* image_request);
+
+   private:
+    using RequestMap = std::map<int, ImageRequest*>;
+
+    // It's a reference counted object, so destructor is private.
+    ~ImageDecoderImpl() override;
 
-  // Overidden from UtilityProcessHostClient:
-  bool OnMessageReceived(const IPC::Message& message) override;
+    // Starts UtilityProcessHost in batch mode and starts batch_mode_timer_.
+    void StartBatchMode();
 
-  // IPC message handlers.
-  void OnDecodeImageSucceeded(const SkBitmap& decoded_image);
-  void OnDecodeImageFailed();
+    // Stops batch mode if no requests have come in since kBatchModeTimeout.
+    void StopBatchMode();
 
-  // Launches sandboxed process that will decode the image.
-  void DecodeImageInSandbox(const std::vector<unsigned char>& image_data);
+    // 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);
+
+    // id to use for the next Start request that comes in.
+    int image_request_id_counter_;
+
+    // Map of request id's to ImageRequests.
+    RequestMap image_request_id_map_;
+
+    // Protects image_request_id_map_;
+    base::Lock map_lock_;

[dcheng, 2015/03/23 11:52:09]

This needs to protect the counter as well, since Start() is callable on any
thread (this appears to be the case already, but let's explicitly mention that
here)

[Theresa, 2015/03/23 17:33:10]

Done.

+
+    // The UtilityProcessHost requests are sent to.
+    base::WeakPtr<content::UtilityProcessHost> utility_process_host_;
+
+    // Calls StopBatchMode after kBatchModeTimeout has elapsed.
+    base::RepeatingTimer<ImageDecoderImpl> batch_mode_timer_;
+
+    // The time Start was last called.
+    base::TimeTicks last_request_;
+
+    // How long to wait after the last request has been received before ending
+    // batch mode.
+    const base::TimeDelta kBatchModeTimeout = base::TimeDelta::FromSeconds(5);

[dcheng, 2015/03/23 11:52:09]

Just move this constant into the .cc, since it's an implementation detail.

Also, doesn't this introduce a new static initializer? I'm surprised the bots
are OK with this.

[Theresa, 2015/03/23 17:33:10]

Moved. I don't think it introduced a static initializer (nothing in the
ImageDecoderImpl is static), but I'm honestly not sure.

[dcheng, 2015/03/24 05:18:18]

I've confirmed locally that this does introduce a static initializer.

In general, any global variable of non-POD type will introduce a static
initializer (LazyInstance and Singleton are very carefully written to avoid
generating static initializers).

The usual idiom is:
const int kBatchModeTimeoutInSeconds = 5;
if ((x - y) > base::TimeDelta::FromSeconds(kBatchModeTimeoutInSeconds)) {
  // foo
}

[Theresa, 2015/03/24 23:51:42]

If it's in image_decoder.cc, do the static intializers still get created? I
could also change the const back to an int and use it to create the TimeDelta in
 image_decoder.cc.

[dcheng, 2015/03/25 14:10:09]

Yeah, it will still create a static initializer =/

[Theresa, 2015/03/25 17:13:45]

Done. Changed back to a const int.

+
+    // True iff utility_process_host_ has been created and started in
+    // batch mode.
+    bool batch_mode_started_;
+  };
 
-  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
+  const scoped_refptr<ImageDecoderImpl> image_decoder_impl_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageDecoder);
 };