Cleanup image_fetcher::ImageFetcher

Cleanup image_fetcher::ImageFetcher

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

Committed: https://crrev.com/83fcfd3fc6b4fc6fe83e3478e272c3d6daeb13fe
Cr-Commit-Position: refs/heads/master@{#308347}

[sdefresne, 2014-12-15 13:12:26]

sdefresne@chromium.org changed reviewers:
+ droger@chromium.org, rsleevi@chromium.org

[sdefresne, 2014-12-15 13:12:26]

droger: please take a look
rsleevi: FYI

[droger, 2014-12-15 13:23:25]

lgtm

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
File ios/chrome/browser/net/image_fetcher.h (left):

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
ios/chrome/browser/net/image_fetcher.h:13: #include "base/memory/weak_ptr.h"
weak_ptr is probably still needed for WeakPtrFactory.

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
File ios/chrome/browser/net/image_fetcher.mm (left):

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
ios/chrome/browser/net/image_fetcher.mm:9: #include "base/bind.h"
Looks like bind.h is still needed.

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
File ios/chrome/browser/net/image_fetcher.mm (right):

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
ios/chrome/browser/net/image_fetcher.mm:56: static const char kWEBPMimeType[] =
"image/webp";
The constant should be above the functions.

[sdefresne, 2014-12-15 13:43:26]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2014-12-15 13:43:32]

Thank you for the review.

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
File ios/chrome/browser/net/image_fetcher.h (left):

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
ios/chrome/browser/net/image_fetcher.h:13: #include "base/memory/weak_ptr.h"
On 2014/12/15 at 13:23:25, droger wrote:
> weak_ptr is probably still needed for WeakPtrFactory.

Done.

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
File ios/chrome/browser/net/image_fetcher.mm (left):

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
ios/chrome/browser/net/image_fetcher.mm:9: #include "base/bind.h"
On 2014/12/15 at 13:23:25, droger wrote:
> Looks like bind.h is still needed.

Done.

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
File ios/chrome/browser/net/image_fetcher.mm (right):

https://codereview.chromium.org/805713004/diff/1/ios/chrome/browser/net/image...
ios/chrome/browser/net/image_fetcher.mm:56: static const char kWEBPMimeType[] =
"image/webp";
On 2014/12/15 at 13:23:25, droger wrote:
> The constant should be above the functions.

Done.

[commit-bot: I haz the power, 2014-12-15 13:43:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/805713004/20001

[commit-bot: I haz the power, 2014-12-15 14:31:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-15 14:32:18]

Patchset 2 (id:??) landed as
https://crrev.com/83fcfd3fc6b4fc6fe83e3478e272c3d6daeb13fe
Cr-Commit-Position: refs/heads/master@{#308347}

[justincohen, 2014-12-17 13:52:38]

justincohen@chromium.org changed reviewers:
+ justincohen@chromium.org

[justincohen, 2014-12-17 13:52:39]

https://codereview.chromium.org/805713004/diff/20001/ios/chrome/browser/net/i...
File ios/chrome/browser/net/image_fetcher.h (left):

https://codereview.chromium.org/805713004/diff/20001/ios/chrome/browser/net/i...
ios/chrome/browser/net/image_fetcher.h:16: #include
"net/url_request/url_request_context_getter.h"
I see lots of these with this removal:

In file included from ../../ios/chrome/browser/net/image_fetcher.h:11:
../../base/memory/ref_counted.h:378:6: error: member access into incomplete type
'net::URLRequestContextGetter'
  ptr->AddRef();

[sdefresne, 2014-12-17 14:06:43]

On 2014/12/17 at 13:52:39, justincohen wrote:
>
https://codereview.chromium.org/805713004/diff/20001/ios/chrome/browser/net/i...
> File ios/chrome/browser/net/image_fetcher.h (left):
> 
>
https://codereview.chromium.org/805713004/diff/20001/ios/chrome/browser/net/i...
> ios/chrome/browser/net/image_fetcher.h:16: #include
"net/url_request/url_request_context_getter.h"
> I see lots of these with this removal:
> 
> In file included from ../../ios/chrome/browser/net/image_fetcher.h:11:
> ../../base/memory/ref_counted.h:378:6: error: member access into incomplete
type 'net::URLRequestContextGetter'
>   ptr->AddRef();

I think the include of #include "net/url_request/url_request_context_getter.h"
is not required in image_fetcher.h since the constructor and destructor are not
inlined (and thus the scoped_refptr inc/dec-ref will only happen in the
compilation unit, image_fetcher.cc, that do provide an implementation of them
and this file do include the required header).

It is possible however that client code assumed that image_fetcher.h included
net/url_request/url_request_context_getter.h and forgot to include it (which is
a client code error). If the client code do use
scoped_refptr<net::URLRequestContextGetter> which I think it highly likely, then
it will have to include the corresponding header.

[justincohen, 2014-12-17 14:10:41]

sgtm.  There were 5 cases where it did not already have that include.