Fetch images for ContentSuggestions

Fetch images for ContentSuggestions

BUG=694220
Review-Url: https://codereview.chromium.org/2701833003
Cr-Commit-Position: refs/heads/master@{#451829}
Committed: https://chromium.googlesource.com/chromium/src/+/9bd3a256d47b5d8f8418bdd89eb4a9f72686ceeb

[gambard, 2017-02-20 14:33:11]

Description was changed from

==========
Fetch images for ContentSuggestions

BUG=686728
==========

to

==========
Fetch images for ContentSuggestions

BUG=694220
==========

[gambard, 2017-02-20 14:49:03]

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

[gambard, 2017-02-20 14:49:04]

PTAL.

[gambard, 2017-02-20 15:17:23]

Patchset #5 (id:80001) has been deleted

[marq (ping after 24h), 2017-02-21 11:27:09]

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
(right):

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:89:
ntp_snippets::ContentSuggestion::ID SuggestionIDForSectionID(
Should this be a method on ContentSuggestionsCategoryWrapper?

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
(right):

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:56:
self.imageBeingFetched = YES;
I think it would be better to set this to YES in the delegate method for a
couple of reasons.

- It will be close to where it is set to NO, so it's easy to verify.

- If the delegate is nil, then it is never set to YES (because it isn't being
fetched).

That does mean that it's up to the delegate to maintain the state of this
variable; that should probably be documented in the header.

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_image_fetcher.h
(right):

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_image_fetcher.h:10:
#include <string>
Doesn't look like this is needed.

[gambard, 2017-02-21 15:07:53]

Thanks, PTAL.

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
(right):

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:89:
ntp_snippets::ContentSuggestion::ID SuggestionIDForSectionID(
On 2017/02/21 11:27:08, marq wrote:
> Should this be a method on ContentSuggestionsCategoryWrapper?

Maybe. As they are only used in this file, I am not sure at the moment, and this
is why there are implemented as C function. If I need to move them around it
would be very easy.

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
(right):

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:56:
self.imageBeingFetched = YES;
On 2017/02/21 11:27:08, marq wrote:
> I think it would be better to set this to YES in the delegate method for a
> couple of reasons.
> 
> - It will be close to where it is set to NO, so it's easy to verify.
> 
> - If the delegate is nil, then it is never set to YES (because it isn't being
> fetched).
> 
> That does mean that it's up to the delegate to maintain the state of this
> variable; that should probably be documented in the header.

Done.

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_image_fetcher.h
(right):

https://codereview.chromium.org/2701833003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_image_fetcher.h:10:
#include <string>
On 2017/02/21 11:27:08, marq wrote:
> Doesn't look like this is needed.

Done.

[marq (ping after 24h), 2017-02-21 15:54:31]

lgtm

[gambard, 2017-02-21 18:10:12]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-02-21 18:11:02]

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-21 20:15:34]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487700612422440,
"parent_rev": "ff8e6a1d5aef75e6c0a8b87fc16c11d5cb69edac", "commit_rev":
"9bd3a256d47b5d8f8418bdd89eb4a9f72686ceeb"}

[commit-bot: I haz the power, 2017-02-21 20:16:32]

Description was changed from

==========
Fetch images for ContentSuggestions

BUG=694220
==========

to

==========
Fetch images for ContentSuggestions

BUG=694220

Review-Url: https://codereview.chromium.org/2701833003
Cr-Commit-Position: refs/heads/master@{#451829}
Committed:
https://chromium.googlesource.com/chromium/src/+/9bd3a256d47b5d8f8418bdd89eb4...
==========

[commit-bot: I haz the power, 2017-02-21 20:16:33]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/9bd3a256d47b5d8f8418bdd89eb4...