Upstream image_fetcher::ImageFetcher

ios/chrome/browser/net/image_fetcher.h

+// Copyright 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_CHROME_BROWSER_NET_IMAGE_FETCHER_H_
+#define IOS_CHROME_BROWSER_NET_IMAGE_FETCHER_H_
+
+#include <map>
+
+#include "base/compiler_specific.h"
+#include "base/mac/bind_objc_block.h"
+#include "base/mac/scoped_nsobject.h"
+#include "base/memory/weak_ptr.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+@class NSData;
+
+namespace image_fetcher {
+
+// Callback that informs of the download of an image encoded in |data|,
+// downloaded from |url|, and with the http status |http_response_code|. If the
+// url is a data URL, |http_response_code| is always 200.
+typedef void (^Callback)(const GURL& url, int http_response_code, NSData* data);

[Ryan Sleevi, 2014/12/12 23:32:00]

nit: Naming - while it's totally ok to call this callback because it's
namespaced in image_fetcher, this sort of shadowing may be overloading the
ubiquitous base::Callback when people read the code. I'd nominally nit to
suggest a different name, but this is legal syntax, just... a tad confusing.

[sdefresne, 2014/12/15 13:11:57]

Done.

+
+// Utility class that will retrieve an image from an URL. The image is returned
+// as NSData which can be used with +[UIImage imageWithData:]. This class
+// usually returns the raw bytes retrieved from the network without any
+// processing with the exception of WebP encoded images. Those are decoded and

[Ryan Sleevi, 2014/12/12 23:32:00]

s/processing with/processing, with/

[sdefresne, 2014/12/15 13:11:58]

Done.

+// then reencoded in a format suitable for UIImage.
+// An instance of this class can download a number of images at the same time.
+class ImageFetcher : public net::URLFetcherDelegate {
+ public:
+  // The WorkerPool is used to eventually decode the image.
+  explicit ImageFetcher(
+      const scoped_refptr<base::SequencedWorkerPool> decoding_pool);

[Ryan Sleevi, 2014/12/12 23:32:00]

Pass as const-ref, not const. Avoid refcount churn.

[Ryan Sleevi, 2014/12/12 23:32:00]

DESIGN: Prefer passing in a SequencedTaskRunner, rather than a worker pool. Only
use pools if you're creating them yourself.

You can get a SequencedTaskRunner from a given pool by creating an STR for a
token you supply.

This allows callers to avoid instantiating a pool if they would rather use a
dedicated/common thread, and makes it easier for tests (which can then just test
on the main thread).

[sdefresne, 2014/12/15 13:11:58]

Done.
We want a pool since some of our clients uses a single ImageFetcher to fetch
multiple images and we'd like the decoding to be done in parallel if possible
(i.e. if the pool has multiple thread). We cannot use
web::WebThread::GetBlockingPool() since it is not initialized during unit tests
(need to create web::TestWebThread and web::TestWebThreadBundle for that).
Created http://crbug.com/442292 to track this.

[Ryan Sleevi, 2014/12/15 20:40:39]

If you want requests to happen serially, but on any thread, you can use the
SequencedTaskRunner from the SWP with a given token.
If you want requests to happen in parallel, then you can just pass in a
TaskRunner (which a SequencedWorkerPool is). Tasks may be started at any time
(e.g. while other tasks are running), which matches your stated preference.

Both of these interfaces avoid needing to directly depend on a
SequencedWorkerPool.

That is, you should consider SequencedWorkerPool to be an implementation detail,
and instead code against the interfaces that express the API contract you want.
SequencedWorkerPool may be one way to fill that, but it doesn't have to be the
only way.

+  ~ImageFetcher() override;
+
+  // Start downloading the image at the given |url|. The |callback| will be
+  // called with the downloaded image, or nil if any error happened. The
+  // |referrer| and |referrer_policy| will be passed on to the underlying
+  // URLFetcher.
+  // This method assumes the request context getter has been set.
+  // (virtual for testing)

[Ryan Sleevi, 2014/12/12 23:32:00]

None of your tests appear to override these methods. Unnecessary, it would seem?

[sdefresne, 2014/12/15 13:11:57]

The methods are overridden in downstream tests that want to mock ImageFetcher.

+  virtual void StartDownload(const GURL& url,
+                             Callback callback,
+                             const std::string& referrer,
+                             net::URLRequest::ReferrerPolicy referrer_policy);
+
+  // Helper method to call StartDownload without a referrer.
+  // (virtual for testing)
+  virtual void StartDownload(const GURL& url, Callback callback);
+
+  // A valid request context getter is required before starting the download.
+  // (virtual for testing)
+  virtual void SetRequestContextGetter(
+      net::URLRequestContextGetter* request_context_getter);

[Ryan Sleevi, 2014/12/12 23:32:00]

Since you're going to take a reference to the URCG, you should pass the URCG as
a "const scoped_refptr<>&" to better express the ownership transfer-share
happening here.

[sdefresne, 2014/12/15 13:11:57]

Done.

+
+  // content::URLFetcherDelegate methods.
+  void OnURLFetchComplete(const net::URLFetcher* source) override;
+
+ private:
+  // Runs the callback with the given arguments.
+  void RunCallback(const base::mac::ScopedBlock<Callback>& callback,
+                   const GURL& url,
+                   const int http_response_code,
+                   NSData* data);
+
+  // Tracks open download requests.  The key is the URLFetcher object doing the
+  // fetch; the value is the callback to use when the download request
+  // completes. When a download request completes, the URLFetcher must be
+  // deleted and the callback called and released.
+  std::map<const net::URLFetcher*, Callback> downloads_in_progress_;
+  scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
+
+  // The WeakPtrFactory is used to cancel callbacks if ImageFetcher is destroyed
+  // during WebP decoding.
+  base::WeakPtrFactory<ImageFetcher> weak_factory_;
+
+  // The pool used to decode images if necessary.
+  const scoped_refptr<base::SequencedWorkerPool> decoding_pool_;
+};
+
+}  // namespace image_fetcher
+
+#endif  // IOS_CHROME_BROWSER_NET_IMAGE_FETCHER_H_