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..e97caa76fa0b469fa0dcca61942a23a6de014e0c 100644
--- a/chrome/browser/image_decoder.h
+++ b/chrome/browser/image_decoder.h
@@ -5,33 +5,50 @@
 #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.
+// 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

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

From just looking at the header file, it's not obvious why we need |map_lock_|
when everything should be single-threaded internally.

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

Updated the comment. Cancel() doesn't run on the IO thread. Cancel() is
automatically called when classes extending ImageRequest are destroyed.
Previously Cancel() did run on the IO thread, but there was concern that the
Cancel() request would be tacked to the back of the IO thread's processing
queue, behind callbacks to the object that was trying to delete itself.

+// the result (ImageRequest::OnImageDecoded or

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

This part is a bit confusing - OnImageDecoded is not the result.

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

I changed "result" to "callback"

+// ImageRequest::OnDecodeImageFailed) is posted back to the task runner
+// specified when Start(...) was called.
 class ImageDecoder : public content::UtilityProcessHostClient {
  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 {

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

Does this need to be public?

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

ImageDecoder needs to be able to call it, and it's used in user_image_loader.cc
for a couple of DCHECKs.

+      return task_runner_.get();
+    }
 
    protected:
-    virtual ~Delegate() {}
+    explicit ImageRequest(
+        const scoped_refptr<base::SequencedTaskRunner>& task_runner);
+    virtual ~ImageRequest();
+
+   private:
+    const scoped_refptr<base::SequencedTaskRunner> task_runner_;

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

Document this, mention it's the thread to post back to.

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

Done.

   };
 
   enum ImageCodec {
@@ -39,44 +56,68 @@ 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);
+  // posted back to image_request's task_runner_.

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

nit: |task_runner|, ditto below.

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

Is the nit to rename task_runner_ to task_runner (without the trailing '_') or
to encase references to member variables in |'s?

[Lei Zhang, 2015/03/25 23:28:38]

Refer to variables as |variable|.

[Theresa, 2015/03/25 23:54:08]

Done.

+  static void Start(ImageRequest* image_request,

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

Since (ImageDecoder::DEFAULT_CODEC, false) are the most common arguments to
(image_codec, shrink_to_fit), you can rename this to StartWithOptions() and have
Start() omit those two parameters.

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

Done.

+                    const std::string& image_data,
+                    ImageCodec image_codec,
+                    bool shrink_to_fit);
 
-  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; }
+  // Removes all instances of image_request from image_request_id_map_,
+  // ensuring callbacks are not made to the image_request after it is destroyed.
+  static void Cancel(ImageRequest* image_request);
 
  private:
+  friend struct base::DefaultLazyInstanceTraits<ImageDecoder>;

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

nit: blank line after this

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

Done.

+  ImageDecoder();
   // It's a reference counted object, so destructor is private.
   ~ImageDecoder() override;
 
-  // Overidden from UtilityProcessHostClient:
+  // 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 CancelImpl(ImageRequest* image_request);
+
+  using RequestMap = std::map<int, ImageRequest*>;

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

Is this the new preferred method vs. using a typedef?

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

Yes, unless the header needs to be compatible with C
(https://chromium-cpp.appspot.com/)

+
+  // Starts UtilityProcessHost in batch mode and starts batch_mode_timer_.
+  void StartBatchMode();
+
+  // Stops batch mode if no requests have come in since kBatchModeTimeout.
+  void StopBatchMode();
+
+  // Overidden from UtilityProcessHostClient.
   bool OnMessageReceived(const IPC::Message& message) override;
 
   // IPC message handlers.
-  void OnDecodeImageSucceeded(const SkBitmap& decoded_image);
-  void OnDecodeImageFailed();
+  void OnDecodeImageSucceeded(const SkBitmap& decoded_image, int id);

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

nit: id -> request_id?

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

Done.

+  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_ and image_request_id_counter_.
+  base::Lock map_lock_;
+
+  // The UtilityProcessHost requests are sent to.
+  base::WeakPtr<content::UtilityProcessHost> utility_process_host_;
+
+  // Calls StopBatchMode after kBatchModeTimeout has elapsed.
+  base::RepeatingTimer<ImageDecoder> batch_mode_timer_;
 
-  // Launches sandboxed process that will decode the image.
-  void DecodeImageInSandbox(const std::vector<unsigned char>& image_data);
+  // The time Start was last called.
+  base::TimeTicks last_request_;
 
-  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
+  // How long to wait after the last request has been received before ending
+  // batch mode.
+  const int kBatchModeTimeoutSeconds = 5;

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

Can this just go into the .cc file?

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

Done.

 
   DISALLOW_COPY_AND_ASSIGN(ImageDecoder);
 };