Use component ImageFetcher instead of iOS one.

Use component ImageFetcher instead of iOS one.

The ImageFetcher used in iOS is equivalent to the ImageFetcher used in
components with the iOS ImageDecoder.

BUG=689020
Review-Url: https://codereview.chromium.org/2697043005
Cr-Commit-Position: refs/heads/master@{#451610}
Committed: https://chromium.googlesource.com/chromium/src/+/e91c63172c67544b11170de90e82dcd71a0f5799

[gambard, 2017-02-17 08:16:08]

gambard@chromium.org changed reviewers:
+ justincohen@chromium.org, noyau@chromium.org

[gambard, 2017-02-17 08:16:09]

PTAL.

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/ntp_snip...
File
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/ntp_snip...
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:169:
CreateIOSImageDecoder(web::WebThread::GetBlockingPool()),
I have used the same pool as the one used by the the ImageFetcher I am replacing
but I don't understand why we are not using "task_runner" here, as we do for the
ImageDecoder at line 171.

[Marc Treib, 2017-02-17 09:40:03]

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

[Marc Treib, 2017-02-17 09:40:04]

Nice! Drive-by non-owners LGTM :)

[noyau (Ping after 24h), 2017-02-17 10:04:18]

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/ntp_snip...
File
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/ntp_snip...
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:169:
CreateIOSImageDecoder(web::WebThread::GetBlockingPool()),
On 2017/02/17 08:16:09, gambard wrote:
> I have used the same pool as the one used by the the ImageFetcher I am
replacing
> but I don't understand why we are not using "task_runner" here, as we do for
the
> ImageDecoder at line 171.

Probably historical. The taskrunner is createed on line 152 with the same
blocking pool anyway.

But I'm confused why there is a need for two image decoders? Is it a transient
state while you remove old code?

[Marc Treib, 2017-02-17 10:10:31]

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/ntp_snip...
File
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/ntp_snip...
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:169:
CreateIOSImageDecoder(web::WebThread::GetBlockingPool()),
On 2017/02/17 10:04:18, noyau wrote:
> On 2017/02/17 08:16:09, gambard wrote:
> > I have used the same pool as the one used by the the ImageFetcher I am
> replacing
> > but I don't understand why we are not using "task_runner" here, as we do for
> the
> > ImageDecoder at line 171.
> 
> Probably historical. The taskrunner is createed on line 152 with the same
> blocking pool anyway.
> 
> But I'm confused why there is a need for two image decoders? Is it a transient
> state while you remove old code?

Yup, transient - in fact, we have the same problem (both Fetcher and Decoder) in
other places. Removing the special iOS implementation of ImageFetcherImpl will
allow us to make this all better :)

[gambard, 2017-02-17 10:11:16]

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/ntp_snip...
File
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/ntp_snip...
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:169:
CreateIOSImageDecoder(web::WebThread::GetBlockingPool()),
On 2017/02/17 10:04:18, noyau wrote:
> On 2017/02/17 08:16:09, gambard wrote:
> > I have used the same pool as the one used by the the ImageFetcher I am
> replacing
> > but I don't understand why we are not using "task_runner" here, as we do for
> the
> > ImageDecoder at line 171.
> 
> Probably historical. The taskrunner is createed on line 152 with the same
> blocking pool anyway.
> 
> But I'm confused why there is a need for two image decoders? Is it a transient
> state while you remove old code?

No, the RemoteSuggestionsProviderImpl (living in components/ntp_snippets) takes
an ImageFetcher and an ImageDecoder.
This is because the images fetched are cached in a database, not decoded
(probably to save space). When retrieving them, it needs to decode it.
It can be removed once we remove the iOS implementation.

[Marc Treib, 2017-02-17 10:13:19]

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/ntp_snip...
File
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/ntp_snip...
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:169:
CreateIOSImageDecoder(web::WebThread::GetBlockingPool()),
On 2017/02/17 10:11:16, gambard wrote:
> On 2017/02/17 10:04:18, noyau wrote:
> > On 2017/02/17 08:16:09, gambard wrote:
> > > I have used the same pool as the one used by the the ImageFetcher I am
> > replacing
> > > but I don't understand why we are not using "task_runner" here, as we do
for
> > the
> > > ImageDecoder at line 171.
> > 
> > Probably historical. The taskrunner is createed on line 152 with the same
> > blocking pool anyway.
> > 
> > But I'm confused why there is a need for two image decoders? Is it a
transient
> > state while you remove old code?
> 
> No, the RemoteSuggestionsProviderImpl (living in components/ntp_snippets)
takes
> an ImageFetcher and an ImageDecoder.
> This is because the images fetched are cached in a database, not decoded
> (probably to save space). When retrieving them, it needs to decode it.
> It can be removed once we remove the iOS implementation.

Concurrent comments ftw :D

Some more detail: Once the same ImageFetcherImpl is used everywhere, my plan is
to add a "GetImageDecoder()" on it (since the fetcher contains a decoder
anyway). Then we won't have to pass the decoder in separately.

[justincohen, 2017-02-17 14:24:31]

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/suggesti...
File ios/chrome/browser/suggestions/suggestions_service_factory.mm (right):

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/suggesti...
ios/chrome/browser/suggestions/suggestions_service_factory.mm:102: 
nit remove new spaces

[gambard, 2017-02-17 14:26:01]

Thanks, PTAL.

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/suggesti...
File ios/chrome/browser/suggestions/suggestions_service_factory.mm (right):

https://codereview.chromium.org/2697043005/diff/1/ios/chrome/browser/suggesti...
ios/chrome/browser/suggestions/suggestions_service_factory.mm:102: 
On 2017/02/17 14:24:31, justincohen wrote:
> nit remove new spaces

I find it way more readable with the new line (added on purpose).
I can remove it if you want.

[gambard, 2017-02-20 10:32:11]

gambard@chromium.org changed reviewers:
+ sdefresne@chromium.org

[gambard, 2017-02-20 10:32:12]

+sdefresne, PTAL.

[sdefresne, 2017-02-20 10:35:16]

lgtm

[gambard, 2017-02-20 10:35:38]

Thanks!

[gambard, 2017-02-20 10:35:42]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-02-20 10:35:54]

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, 2017-02-20 13:01:39]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487586942689420, "parent_rev":
"fc155c2704ad95817d98efb4c3dbec239c4c17cb", "commit_rev":
"e91c63172c67544b11170de90e82dcd71a0f5799"}

[commit-bot: I haz the power, 2017-02-20 13:02:39]

Description was changed from

==========
Use component ImageFetcher instead of iOS one.

The ImageFetcher used in iOS is equivalent to the ImageFetcher used in
components with the iOS ImageDecoder.

BUG=689020
==========

to

==========
Use component ImageFetcher instead of iOS one.

The ImageFetcher used in iOS is equivalent to the ImageFetcher used in
components with the iOS ImageDecoder.

BUG=689020

Review-Url: https://codereview.chromium.org/2697043005
Cr-Commit-Position: refs/heads/master@{#451610}
Committed:
https://chromium.googlesource.com/chromium/src/+/e91c63172c67544b11170de90e82...
==========

[commit-bot: I haz the power, 2017-02-20 13:02:41]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/e91c63172c67544b11170de90e82...