Skip to content

Commit

Permalink
Cleanup image_fetcher::ImageFetcher
Browse files Browse the repository at this point in the history 
Follow-up CL to address comments on https://codereview.chromium.org/787903003
after the CL landed:
- cleanup #includes in image_fetcher.{h,mm}
- use const ref to scoped_refptr to avoid inc/dec ref churn
- prefer base::ThreadTaskRunnerHandle::Get() to MessageLoopProxy::current()
- remove std::string concatenation

BUG=None

Review URL: https://codereview.chromium.org/805713004

Cr-Commit-Position: refs/heads/master@{#308347}
  • Loading branch information
@sdefresne
sdefresne authored and Commit bot committed Dec 15, 2014
1 parent f86c0ad commit 83fcfd3
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 63 deletions.
37 changes: 22 additions & 15 deletions ios/chrome/browser/net/image_fetcher.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,35 +7,42 @@

#include <map>

#include "base/compiler_specific.h"
#include "base/mac/bind_objc_block.h"
#include "base/mac/scoped_nsobject.h"
#include "base/mac/scoped_block.h"
#include "base/memory/ref_counted.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 GURL;
@class NSData;

namespace base {
class SequencedWorkerPool;
}

namespace net {
class URLRequestContextGetter;
}

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);
using ImageFetchedCallback =
void (^)(const GURL& url, int http_response_code, NSData* data);

// 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
// processing, with the exception of WebP encoded images. Those are decoded and
// 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);
const scoped_refptr<base::SequencedWorkerPool>& decoding_pool);
~ImageFetcher() override;

// Start downloading the image at the given |url|. The |callback| will be
Expand All @@ -45,25 +52,25 @@ class ImageFetcher : public net::URLFetcherDelegate {
// This method assumes the request context getter has been set.
// (virtual for testing)
virtual void StartDownload(const GURL& url,
Callback callback,
ImageFetchedCallback 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);
virtual void StartDownload(const GURL& url, ImageFetchedCallback callback);

// A valid request context getter is required before starting the download.
// (virtual for testing)
virtual void SetRequestContextGetter(
net::URLRequestContextGetter* request_context_getter);
virtual void SetRequestContextGetter(const scoped_refptr<
net::URLRequestContextGetter>& request_context_getter);

// content::URLFetcherDelegate methods.
// net::URLFetcherDelegate:
void OnURLFetchComplete(const net::URLFetcher* source) override;

private:
// Runs the callback with the given arguments.
void RunCallback(const base::mac::ScopedBlock<Callback>& callback,
void RunCallback(const base::mac::ScopedBlock<ImageFetchedCallback>& callback,
const GURL& url,
const int http_response_code,
NSData* data);
Expand All @@ -72,7 +79,7 @@ class ImageFetcher : public net::URLFetcherDelegate {
// 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_;
std::map<const net::URLFetcher*, ImageFetchedCallback> downloads_in_progress_;
scoped_refptr<net::URLRequestContextGetter> request_context_getter_;

// The WeakPtrFactory is used to cancel callbacks if ImageFetcher is destroyed
Expand Down
56 changes: 29 additions & 27 deletions ios/chrome/browser/net/image_fetcher.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,15 @@
#import <Foundation/Foundation.h>

#include "base/bind.h"
#include "base/compiler_specific.h"
#include "base/location.h"
#include "base/logging.h"
#include "base/mac/scoped_block.h"
#include "base/memory/scoped_ptr.h"
#include "base/task_runner_util.h"
#include "base/mac/scoped_nsobject.h"
#include "base/threading/sequenced_worker_pool.h"
#include "ios/web/public/web_thread.h"
#include "ios/web/public/webp_decoder.h"
#include "net/base/load_flags.h"
#include "net/http/http_response_headers.h"
#include "net/url_request/url_fetcher.h"
#include "url/gurl.h"
#include "net/url_request/url_request_context_getter.h"

namespace {

Expand All @@ -45,6 +42,9 @@ void OnDataDecoded(NSData* data) override {
base::scoped_nsobject<NSMutableData> decoded_image_;
};

// Content-type header for WebP images.
static const char kWEBPMimeType[] = "image/webp";

// Returns a NSData object containing the decoded image.
// Returns nil in case of failure.
base::scoped_nsobject<NSData> DecodeWebpImage(
Expand All @@ -61,7 +61,7 @@ void OnDataDecoded(NSData* data) override {
namespace image_fetcher {

ImageFetcher::ImageFetcher(
const scoped_refptr<base::SequencedWorkerPool> decoding_pool)
const scoped_refptr<base::SequencedWorkerPool>& decoding_pool)
: request_context_getter_(nullptr),
weak_factory_(this),
decoding_pool_(decoding_pool) {
Expand All @@ -71,17 +71,15 @@ void OnDataDecoded(NSData* data) override {
ImageFetcher::~ImageFetcher() {
// Delete all the entries in the |downloads_in_progress_| map. This will in
// turn cancel all of the requests.
for (std::map<const net::URLFetcher*, Callback>::iterator it =
downloads_in_progress_.begin();
it != downloads_in_progress_.end(); ++it) {
[it->second release];
delete it->first;
for (const auto& pair : downloads_in_progress_) {
[pair.second release];
delete pair.first;
}
}

void ImageFetcher::StartDownload(
const GURL& url,
Callback callback,
ImageFetchedCallback callback,
const std::string& referrer,
net::URLRequest::ReferrerPolicy referrer_policy) {
DCHECK(request_context_getter_.get());
Expand All @@ -90,16 +88,18 @@ void OnDataDecoded(NSData* data) override {
this);
downloads_in_progress_[fetcher] = [callback copy];
fetcher->SetLoadFlags(
net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES);
net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES |
net::LOAD_DO_NOT_SEND_AUTH_DATA | net::LOAD_DO_NOT_PROMPT_FOR_LOGIN);
fetcher->SetRequestContext(request_context_getter_.get());
fetcher->SetReferrer(referrer);
fetcher->SetReferrerPolicy(referrer_policy);
fetcher->Start();
}

void ImageFetcher::StartDownload(const GURL& url, Callback callback) {
void ImageFetcher::StartDownload(
const GURL& url, ImageFetchedCallback callback) {
ImageFetcher::StartDownload(
url, callback, "", net::URLRequest::NEVER_CLEAR_REFERRER);
url, callback, std::string(), net::URLRequest::NEVER_CLEAR_REFERRER);
}

// Delegate callback that is called when URLFetcher completes. If the image
Expand All @@ -111,19 +111,20 @@ void OnDataDecoded(NSData* data) override {
return;
}

// Ensures that |fetcher| will be deleted even if we return early.
// Ensures that |fetcher| will be deleted in the event of early return.
scoped_ptr<const net::URLFetcher> fetcher_deleter(fetcher);

// Retrieves the callback and ensures that it will be deleted even if we
// return early.
base::mac::ScopedBlock<Callback> callback(downloads_in_progress_[fetcher]);
// Retrieves the callback and ensures that it will be deleted in the event
// of early return.
base::mac::ScopedBlock<ImageFetchedCallback> callback(
downloads_in_progress_[fetcher]);

// Remove |fetcher| from the map.
downloads_in_progress_.erase(fetcher);

// Make sure the request was successful. For "data" requests, the response
// code has no meaning, because there is no actual server (data is encoded
// directly in the URL). In that case, we set the response code to 200.
// directly in the URL). In that case, set the response code to 200 (OK).
const GURL& original_url = fetcher->GetOriginalURL();
const int http_response_code = original_url.SchemeIs("data") ?
200 : fetcher->GetResponseCode();
Expand All @@ -146,7 +147,7 @@ void OnDataDecoded(NSData* data) override {
if (fetcher->GetResponseHeaders()) {
std::string mime_type;
fetcher->GetResponseHeaders()->GetMimeType(&mime_type);
if (mime_type == "image/webp") {
if (mime_type == kWEBPMimeType) {
base::PostTaskAndReplyWithResult(decoding_pool_.get(),
FROM_HERE,
base::Bind(&DecodeWebpImage, data),
Expand All @@ -161,15 +162,16 @@ void OnDataDecoded(NSData* data) override {
(callback.get())(original_url, http_response_code, data);
}

void ImageFetcher::RunCallback(const base::mac::ScopedBlock<Callback>& callback,
const GURL& url,
int http_response_code,
NSData* data) {
void ImageFetcher::RunCallback(
const base::mac::ScopedBlock<ImageFetchedCallback>& callback,
const GURL& url,
int http_response_code,
NSData* data) {
(callback.get())(url, http_response_code, data);
}

void ImageFetcher::SetRequestContextGetter(
net::URLRequestContextGetter* request_context_getter) {
const scoped_refptr<net::URLRequestContextGetter>& request_context_getter) {
request_context_getter_ = request_context_getter;
}

Expand Down
35 changes: 15 additions & 20 deletions ios/chrome/browser/net/image_fetcher_unittest.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,12 @@

#import <UIKit/UIKit.h>

#include "base/mac/bind_objc_block.h"
#include "base/memory/scoped_ptr.h"
#include "base/message_loop/message_loop_proxy.h"
#include "base/mac/scoped_block.h"
#include "base/memory/ref_counted.h"
#include "base/run_loop.h"
#include "base/threading/sequenced_worker_pool.h"
#include "base/thread_task_runner_handle.h"
#include "net/http/http_response_headers.h"
#include "net/url_request/test_url_fetcher_factory.h"
#include "net/url_request/url_fetcher_delegate.h"
#include "net/url_request/url_request_test_util.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "testing/platform_test.h"
Expand Down Expand Up @@ -59,10 +57,16 @@
48,1,0,157,1,42,1,0,1,0,3,0,52,37,164,0,3,112,0,254,251,253,80,0
};

static char kTestUrl[] = "http://www.img.com";
static const char kTestUrl[] = "http://www.img.com";

static const char kWEBPHeaderResponse[] =
"HTTP/1.1 200 OK\0Content-type: image/webp\0\0";

} // namespace

// TODO(ios): remove the static_cast<UIImage*>(nil) once all the bots have
// Xcode 6.0 or later installed, http://crbug.com/440857

class ImageFetcherTest : public PlatformTest {
protected:
ImageFetcherTest()
Expand All @@ -77,7 +81,7 @@
} copy]);
image_fetcher_->SetRequestContextGetter(
new net::TestURLRequestContextGetter(
base::MessageLoopProxy::current()));
base::ThreadTaskRunnerHandle::Get()));
}

~ImageFetcherTest() override { pool_->Shutdown(); }
Expand All @@ -93,7 +97,7 @@
}

base::MessageLoop loop_;
base::mac::ScopedBlock<image_fetcher::Callback> callback_;
base::mac::ScopedBlock<image_fetcher::ImageFetchedCallback> callback_;
net::TestURLFetcherFactory factory_;
scoped_refptr<base::SequencedWorkerPool> pool_;
scoped_ptr<image_fetcher::ImageFetcher> image_fetcher_;
Expand Down Expand Up @@ -132,11 +136,8 @@
fetcher->set_response_code(200);
fetcher->SetResponseString(
std::string((char*)kWEBPImage, sizeof(kWEBPImage)));
std::string kZero = std::string("\0", 1);
std::string header_string = std::string("HTTP/1.1 200 OK") + kZero +
"Content-type: image/webp" + kZero + kZero;
scoped_refptr<net::HttpResponseHeaders> headers(new net::HttpResponseHeaders(
header_string));
std::string(kWEBPHeaderResponse, arraysize(kWEBPHeaderResponse))));
fetcher->set_response_headers(headers);
fetcher->delegate()->OnURLFetchComplete(fetcher);
pool_->FlushForTesting();
Expand All @@ -149,11 +150,8 @@
net::TestURLFetcher* fetcher = SetupFetcher();
fetcher->set_response_code(200);
fetcher->SetResponseString("This is not a valid WebP image");
std::string kZero = std::string("\0", 1);
std::string header_string = std::string("HTTP/1.1 200 OK") + kZero +
"Content-type: image/webp" + kZero + kZero;
scoped_refptr<net::HttpResponseHeaders> headers(new net::HttpResponseHeaders(
header_string));
std::string(kWEBPHeaderResponse, arraysize(kWEBPHeaderResponse))));
fetcher->set_response_headers(headers);
fetcher->delegate()->OnURLFetchComplete(fetcher);
pool_->FlushForTesting();
Expand All @@ -167,11 +165,8 @@
fetcher->set_response_code(200);
fetcher->SetResponseString(
std::string((char*)kWEBPImage, sizeof(kWEBPImage)));
std::string kZero = std::string("\0", 1);
std::string header_string = std::string("HTTP/1.1 200 OK") + kZero +
"Content-type: image/webp" + kZero + kZero;
scoped_refptr<net::HttpResponseHeaders> headers(new net::HttpResponseHeaders(
header_string));
std::string(kWEBPHeaderResponse, arraysize(kWEBPHeaderResponse))));
fetcher->set_response_headers(headers);
fetcher->delegate()->OnURLFetchComplete(fetcher);
// Delete the image fetcher, and check that the callback is not called.
Expand Down
3 changes: 2 additions & 1 deletion ios/chrome/browser/suggestions/image_fetcher_impl.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "base/threading/sequenced_worker_pool.h"
#include "ios/chrome/browser/net/image_fetcher.h"
#include "net/url_request/url_request_context_getter.h"
#include "skia/ext/skia_utils_ios.h"

namespace suggestions {
Expand Down Expand Up @@ -40,7 +41,7 @@
}
// Copy url reference so it's retained.
const GURL page_url(url);
image_fetcher::Callback fetcher_callback =
image_fetcher::ImageFetchedCallback fetcher_callback =
^(const GURL& original_url, int response_code, NSData* data) {
if (data) {
// Most likely always returns 1x images.
Expand Down

0 comments on commit 83fcfd3

Please sign in to comment.