Get Suggestions image only if it is not empty

Get Suggestions image only if it is not empty

This CL makes sure that the ContentSuggestions update the
ContentSuggestionsArticles only if an image has been fetched.

BUG=697361
Review-Url: https://codereview.chromium.org/2720423003
Cr-Commit-Position: refs/heads/master@{#453926}
Committed: https://chromium.googlesource.com/chromium/src/+/a260bac5596e34409d6e7842b98e8024772c2e59

[gambard, 2017-03-01 08:22:21]

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

[gambard, 2017-03-01 08:22:22]

PTAL

[Olivier, 2017-03-01 08:33:56]

https://codereview.chromium.org/2720423003/diff/1/ios/chrome/browser/ui/conte...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
(right):

https://codereview.chromium.org/2720423003/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:64:
[self.delegate loadImageForArticleItem:self];
This looks async. Is it OK to use _image just after this call?

[gambard, 2017-03-01 08:37:13]

https://codereview.chromium.org/2720423003/diff/1/ios/chrome/browser/ui/conte...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
(right):

https://codereview.chromium.org/2720423003/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:64:
[self.delegate loadImageForArticleItem:self];
On 2017/03/01 08:33:56, Olivier Robin wrote:
> This looks async. Is it OK to use _image just after this call?

It is indeed async. This is only called if the image is nil and has never been
called before.
Using image after is necessary: if the item reuse a cell containing an image it
needs to remove it.

[Olivier, 2017-03-01 12:42:08]

LGTM, nits

https://codereview.chromium.org/2720423003/diff/1/ios/chrome/browser/ui/conte...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
(right):

https://codereview.chromium.org/2720423003/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:64:
[self.delegate loadImageForArticleItem:self];
On 2017/03/01 08:37:12, gambard wrote:
> On 2017/03/01 08:33:56, Olivier Robin wrote:
> > This looks async. Is it OK to use _image just after this call?
> 
> It is indeed async. This is only called if the image is nil and has never been
> called before.
> Using image after is necessary: if the item reuse a cell containing an image
it
> needs to remove it.

I think it would be worth add a comment for this before  setting the image.

https://codereview.chromium.org/2720423003/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:68:
cell.imageView.image = _image;
nit: use accessor here to be consistent with above.

[gambard, 2017-03-01 12:55:49]

Thanks!

https://codereview.chromium.org/2720423003/diff/1/ios/chrome/browser/ui/conte...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
(right):

https://codereview.chromium.org/2720423003/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:64:
[self.delegate loadImageForArticleItem:self];
On 2017/03/01 12:42:08, Olivier Robin wrote:
> On 2017/03/01 08:37:12, gambard wrote:
> > On 2017/03/01 08:33:56, Olivier Robin wrote:
> > > This looks async. Is it OK to use _image just after this call?
> > 
> > It is indeed async. This is only called if the image is nil and has never
been
> > called before.
> > Using image after is necessary: if the item reuse a cell containing an image
> it
> > needs to remove it.
> 
> I think it would be worth add a comment for this before  setting the image.

Done.

https://codereview.chromium.org/2720423003/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:68:
cell.imageView.image = _image;
On 2017/03/01 12:42:08, Olivier Robin wrote:
> nit: use accessor here to be consistent with above.

It should be updated in another CL.

[gambard, 2017-03-01 12:55:52]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-03-01 12:55:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2720423003/#ps20001
(title: "Add comment")

[commit-bot: I haz the power, 2017-03-01 12:56:10]

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-03-01 13:04:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 13:04:01]

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gambard, 2017-03-01 13:35:34]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-03-01 13:35:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2720423003/#ps30001
(title: "Fix tests")

[commit-bot: I haz the power, 2017-03-01 13:35:49]

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-03-01 13:47:44]

CQ is committing da patch.

Bot data: {"patchset_id": 30001, "attempt_start_ts": 1488375334759850,
"parent_rev": "7b2a21a7ff27c79e88cc5a64cece2890e8aacb1f", "commit_rev":
"a260bac5596e34409d6e7842b98e8024772c2e59"}

[commit-bot: I haz the power, 2017-03-01 13:48:16]

Description was changed from

==========
Get Suggestions image only if it is not empty

This CL makes sure that the ContentSuggestions update the
ContentSuggestionsArticles only if an image has been fetched.

BUG=697361
==========

to

==========
Get Suggestions image only if it is not empty

This CL makes sure that the ContentSuggestions update the
ContentSuggestionsArticles only if an image has been fetched.

BUG=697361

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

[commit-bot: I haz the power, 2017-03-01 13:48:18]

Committed patchset #3 (id:30001) as
https://chromium.googlesource.com/chromium/src/+/a260bac5596e34409d6e7842b98e...