Pass the ImageDecoder to the ImageFetcher at creation time.

Pass the ImageDecoder to the ImageFetcher at creation time and move image_fetcher_impl.* to components/image_fetcher

BUG=609127
Committed: https://crrev.com/532bb2389045e77e3a5cd5c08ab55e2e70408637
Cr-Commit-Position: refs/heads/master@{#402829}

[markusheintz_, 2016-06-29 13:01:30]

rm ios image decoder from CL

[markusheintz_, 2016-06-29 13:38:13]

Move image_fetcher_impl.* to components/image_fetcher

[markusheintz_, 2016-06-29 13:41:21]

Description was changed from

==========
Pass the ImageDecoder to the ImageFetcher at creation time.

BUG=TBD
==========

to

==========
Pass the ImageDecoder to the ImageFetcher at creation time and move
image_fetcher_impl.* to components/image_fetcher

BUG=TBD
==========

[markusheintz_, 2016-06-29 13:42:28]

Description was changed from

==========
Pass the ImageDecoder to the ImageFetcher at creation time and move
image_fetcher_impl.* to components/image_fetcher

BUG=TBD
==========

to

==========
Pass the ImageDecoder to the ImageFetcher at creation time and move
image_fetcher_impl.* to components/image_fetcher

BUG=609127
==========

[markusheintz_, 2016-06-29 13:45:43]

markusheintz@chromium.org changed reviewers:
+ bauerb@google.com, treib@chromium.org

[markusheintz_, 2016-06-29 13:46:33]

Bernhard and Marc would you mind taking a look at this CL.

I have not move the browsertest to components/image_fetcher yet. But I'm looking
at it.

[markusheintz_, 2016-06-29 13:46:46]

markusheintz@chromium.org changed reviewers:
+ bauerb@chromium.org
- bauerb@google.com

[markusheintz_, 2016-06-29 13:47:18]

Adding the bauerb@chromium.org

PTAL

thanks

[Bernhard Bauer, 2016-06-29 13:55:58]

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
File components/image_fetcher/image_fetcher_impl.h (right):

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.h:37: image_fetcher::ImageDecoder*
image_decoder,
Pass |image_decoder| as a unique_ptr? For ease of construction you can use
base::MakeUnique<ImageDecoderImpl>() at call sites.

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.h:42:
image_fetcher::ImageFetcherDelegate* delegate) override;
Could you add a comment that the |delegate| has to be alive during the lifetime
of this object and that it's the caller's responsibility to ensure that?

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.h:84: net::URLRequestContextGetter*
url_request_context_;
Should this keep a scoped_refptr?

[Marc Treib, 2016-06-29 13:57:33]

Mostly looks good, just some small comments

https://codereview.chromium.org/2074153002/diff/80001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right):

https://codereview.chromium.org/2074153002/diff/80001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:121:
base::MakeUnique<ImageFetcherImpl>(new suggestions::ImageDecoderImpl(),
"suggestions::" isn't required, there's a "using" above

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
File components/image_fetcher/image_fetcher_impl.h (right):

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.h:33: // image_fetcher::ImageFetcher
implementation.
nit: This comment is a bit redundant now :)

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.h:37: image_fetcher::ImageDecoder*
image_decoder,
This should be a unique_ptr, since the ImageFetcherImpl takes ownership

[markusheintz_, 2016-06-29 14:36:27]

Addressed comments treib@, bauerb@

[markusheintz_, 2016-06-29 14:36:46]

https://codereview.chromium.org/2074153002/diff/80001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right):

https://codereview.chromium.org/2074153002/diff/80001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:121:
base::MakeUnique<ImageFetcherImpl>(new suggestions::ImageDecoderImpl(),
On 2016/06/29 13:57:33, Marc Treib wrote:
> "suggestions::" isn't required, there's a "using" above

Done.

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
File components/image_fetcher/image_fetcher_impl.h (right):

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.h:33: // image_fetcher::ImageFetcher
implementation.
On 2016/06/29 13:57:33, Marc Treib wrote:
> nit: This comment is a bit redundant now :)

Comment removed

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.h:37: image_fetcher::ImageDecoder*
image_decoder,
On 2016/06/29 13:57:33, Marc Treib wrote:
> This should be a unique_ptr, since the ImageFetcherImpl takes ownership

Done.

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.h:37: image_fetcher::ImageDecoder*
image_decoder,
On 2016/06/29 13:55:58, Bernhard Bauer wrote:
> Pass |image_decoder| as a unique_ptr? For ease of construction you can use
> base::MakeUnique<ImageDecoderImpl>() at call sites.

Done.

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.h:42:
image_fetcher::ImageFetcherDelegate* delegate) override;
On 2016/06/29 13:55:58, Bernhard Bauer wrote:
> Could you add a comment that the |delegate| has to be alive during the
lifetime
> of this object and that it's the caller's responsibility to ensure that?

Done.

https://codereview.chromium.org/2074153002/diff/80001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.h:84: net::URLRequestContextGetter*
url_request_context_;
On 2016/06/29 13:55:58, Bernhard Bauer wrote:
> Should this keep a scoped_refptr? 

Done.

[Marc Treib, 2016-06-29 14:41:33]

lgtm

https://codereview.chromium.org/2074153002/diff/100001/components/image_fetch...
File components/image_fetcher/image_fetcher_impl.cc (right):

https://codereview.chromium.org/2074153002/diff/100001/components/image_fetch...
components/image_fetcher/image_fetcher_impl.cc:21: new
image_fetcher::ImageDataFetcher(url_request_context_.get())) {
nit: "image_fetcher::" isn't required anymore :)

[markusheintz_, 2016-06-29 14:44:30]

Addressed comments treib@, bauerb@ & fix browsertest

[markusheintz_, 2016-06-29 14:55:02]

Addressed comments round2 treib@ & added missing imports *sigh*

[markusheintz_, 2016-06-29 14:56:08]

markusheintz@chromium.org changed reviewers:
+ blundell@chromium.org

[markusheintz_, 2016-06-29 14:56:24]

https://codereview.chromium.org/2074153002/diff/100001/components/image_fetch...
File components/image_fetcher/image_fetcher_impl.cc (right):

https://codereview.chromium.org/2074153002/diff/100001/components/image_fetch...
components/image_fetcher/image_fetcher_impl.cc:21: new
image_fetcher::ImageDataFetcher(url_request_context_.get())) {
On 2016/06/29 14:41:33, Marc Treib wrote:
> nit: "image_fetcher::" isn't required anymore :)

Done. Updated here and and in the rest of the file as well as in the header.

[markusheintz_, 2016-06-29 14:57:02]

Added blundell@ as reviewer.

Would you mind reviewing: components/image_fetcher.gypi ?

Thanks a lot

[blundell, 2016-06-29 15:08:24]

lgtm

You can add per-file OWNERS of the gypi that point to the OWNERS of the
component.

Out of curiosity (and not blocking this CL), remind me again why there's an
interface/impl separation for ImageFetcher given that they're living next to
each other?

[markusheintz_, 2016-06-29 15:15:58]

On 2016/06/29 15:08:24, blundell wrote:
> lgtm
> 
> You can add per-file OWNERS of the gypi that point to the OWNERS of the
> component.
> 

Will do.

> Out of curiosity (and not blocking this CL), remind me again why there's an
> interface/impl separation for ImageFetcher given that they're living next to
> each other?

Chicken and egg problem. There still exists a separate implementation for iOS.
But in order to remove this I need two things:
1) The Current impl in a common place
2) An ImageDecode impl for iOS

Essentially I'm slicing a big CL into multiple small. 

Sorry Should have made this clear in the description.
FWIW: I added a comment to the ImageFetcherImpl class

[blundell, 2016-06-29 15:16:50]

On 2016/06/29 15:15:58, markusheintz_ wrote:
> On 2016/06/29 15:08:24, blundell wrote:
> > lgtm
> > 
> > You can add per-file OWNERS of the gypi that point to the OWNERS of the
> > component.
> > 
> 
> Will do.
> 
> > Out of curiosity (and not blocking this CL), remind me again why there's an
> > interface/impl separation for ImageFetcher given that they're living next to
> > each other?
> 
> Chicken and egg problem. There still exists a separate implementation for iOS.
> But in order to remove this I need two things:
> 1) The Current impl in a common place
> 2) An ImageDecode impl for iOS
> 
> Essentially I'm slicing a big CL into multiple small. 
> 
> Sorry Should have made this clear in the description.
> FWIW: I added a comment to the ImageFetcherImpl class

SGTM, thanks!

[markusheintz_, 2016-06-29 15:23:01]

markusheintz@chromium.org changed reviewers:
+ fserb@chromium.org

[markusheintz_, 2016-06-29 15:23:32]

markusheintz@chromium.org changed reviewers:
- fserb@chromium.org

[markusheintz_, 2016-06-29 15:32:27]

The CQ bit was checked by markusheintz@chromium.org

[markusheintz_, 2016-06-29 15:32:28]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2074153002/#ps140001
(title: "Addressed comments round2 treib@ & added missing imports *sigh*")

[markusheintz_, 2016-06-29 15:32:46]

The CQ bit was unchecked by markusheintz@chromium.org

[Bernhard Bauer, 2016-06-29 15:46:29]

lgtm

[markusheintz_, 2016-06-29 15:47:55]

The CQ bit was checked by markusheintz@chromium.org

[commit-bot: I haz the power, 2016-06-29 15:48:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 16:10:44]

Description was changed from

==========
Pass the ImageDecoder to the ImageFetcher at creation time and move
image_fetcher_impl.* to components/image_fetcher

BUG=609127
==========

to

==========
Pass the ImageDecoder to the ImageFetcher at creation time and move
image_fetcher_impl.* to components/image_fetcher

BUG=609127
==========

[commit-bot: I haz the power, 2016-06-29 16:10:45]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-29 16:10:58]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 16:14:19]

Description was changed from

==========
Pass the ImageDecoder to the ImageFetcher at creation time and move
image_fetcher_impl.* to components/image_fetcher

BUG=609127
==========

to

==========
Pass the ImageDecoder to the ImageFetcher at creation time and move
image_fetcher_impl.* to components/image_fetcher

BUG=609127

Committed: https://crrev.com/532bb2389045e77e3a5cd5c08ab55e2e70408637
Cr-Commit-Position: refs/heads/master@{#402829}
==========

[commit-bot: I haz the power, 2016-06-29 16:14:21]

Patchset 8 (id:??) landed as
https://crrev.com/532bb2389045e77e3a5cd5c08ab55e2e70408637
Cr-Commit-Position: refs/heads/master@{#402829}