Use the same design for all suggestions

Use the same design for all suggestions

All the suggestions (Reading List and Suggested Articles) should have the same
design. This CL removes the use of the Reading List Cells in ContentSuggestions.

BUG=706296
Review-Url: https://codereview.chromium.org/2865183003
Cr-Commit-Position: refs/heads/master@{#470935}
Committed: https://chromium.googlesource.com/chromium/src/+/057d7cae6af79ac7fb8c43080d0499a2673e2952

[gambard, 2017-05-09 15:29:23]

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

[gambard, 2017-05-09 15:29:23]

PTAL.

[jif, 2017-05-10 09:06:19]

looks good

https://codereview.chromium.org/2865183003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc
(right):

https://codereview.chromium.org/2865183003/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:222:
entry->DistillationTime() / base::Time::kMicrosecondsPerSecond;
Can you change the description of DistillationTime() to:

  // Returns the time distillation was done. The value is in microseconds since
Jan 1st
  // 1970. Returns 0 if the entry was not distilled.

?

It can be done in the same CL.

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/BUILD.gn (right):

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/BUILD.gn:57:
"resources/content_suggestions_offline@2x.png",
the asset is not really @2x.

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item.h
(right):

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item.h:52:
@property(nonatomic, assign) BOOL hasImage;
mayHaveImage?
mightHaveImage?
canHaveImage?

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item.mm
(right):

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item.mm:273:
self.imageSize.constant = 0;
initialize the variables in the same order please :-)

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item_unittest.mm
(right):

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item_unittest.mm:19:
// Tests that configureCell: set all the fields of the cell except the image and
sets

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:293:
if (!weakItem || !weakSelf)
weakItem could be released after line 293; it should be a strongItem.

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:509:
// Returns an suggestion item built with the |suggestion|.
Returns a suggestion

[gambard, 2017-05-10 09:23:26]

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

[gambard, 2017-05-10 09:23:28]

Thanks, PTAL.
treib@: PTAL at components/ntp_snippets/
sdefresnes@: PTAL at components/reading_list/

https://codereview.chromium.org/2865183003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc
(right):

https://codereview.chromium.org/2865183003/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:222:
entry->DistillationTime() / base::Time::kMicrosecondsPerSecond;
On 2017/05/10 09:06:19, jif wrote:
> Can you change the description of DistillationTime() to:
> 
>   // Returns the time distillation was done. The value is in microseconds
since
> Jan 1st
>   // 1970. Returns 0 if the entry was not distilled.
> 
> ?
> 
> It can be done in the same CL.

Done.

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/BUILD.gn (right):

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/BUILD.gn:57:
"resources/content_suggestions_offline@2x.png",
On 2017/05/10 09:06:19, jif wrote:
> the asset is not really @2x.

Haha! I knew it looked bad!
Done.

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item.h
(right):

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item.h:52:
@property(nonatomic, assign) BOOL hasImage;
On 2017/05/10 09:06:19, jif wrote:
> mayHaveImage?
> mightHaveImage?
> canHaveImage?

I have updated the comment.

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item.mm
(right):

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item.mm:273:
self.imageSize.constant = 0;
On 2017/05/10 09:06:19, jif wrote:
> initialize the variables in the same order please :-)

Done.

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item_unittest.mm
(right):

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_item_unittest.mm:19:
// Tests that configureCell: set all the fields of the cell except the image and
On 2017/05/10 09:06:19, jif wrote:
> sets

Done.

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:293:
if (!weakItem || !weakSelf)
On 2017/05/10 09:06:19, jif wrote:
> weakItem could be released after line 293; it should be a strongItem.

Done.

https://codereview.chromium.org/2865183003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:509:
// Returns an suggestion item built with the |suggestion|.
On 2017/05/10 09:06:19, jif wrote:
> Returns a suggestion

Done.

[jif, 2017-05-10 09:36:11]

lgtm

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

https://codereview.chromium.org/2865183003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:293:
strongSelf = weakSelf;
strongSelf and strongItem should be the first thing done in the block.

The test would become:
if (!strongItem || !strongSelf)

[gambard, 2017-05-10 12:17:13]

Thanks!

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

https://codereview.chromium.org/2865183003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:293:
strongSelf = weakSelf;
On 2017/05/10 09:36:11, jif wrote:
> strongSelf and strongItem should be the first thing done in the block.
> 
> The test would become:
> if (!strongItem || !strongSelf)

Done.

[gambard, 2017-05-10 12:17:30]

The CQ bit was checked by gambard@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 12:18:14]

Dry run: 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-05-10 13:39:04]

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

[commit-bot: I haz the power, 2017-05-10 13:39:05]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-05-10 16:09:14]

components/reading_list/core/reading_list_entry.h lgtm

[Marc Treib, 2017-05-11 09:05:05]

components/ntp_snippets LGTM with optional comments

https://codereview.chromium.org/2865183003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc
(right):

https://codereview.chromium.org/2865183003/diff/80001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:222:
entry->DistillationTime() / base::Time::kMicrosecondsPerSecond;
Any reason DistillationTime is an int64 rather than a base::Time?

Generally, I'd recommend using the Time types as much as possible, and in
particular not doing any arithmetic on ints-that-represent-time, since that's
hard to read (IMO) and error prone.
Here you could do something like
Time::UnixEpoch() + TimeDelta::FromMicroseconds(DistillationTime())

https://codereview.chromium.org/2865183003/diff/80001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:223:
if (entry_time == 0) {
Check DistillationTime() == 0 directly, before doing any arithmetic on it?

[gambard, 2017-05-11 11:13:37]

Thanks!

https://codereview.chromium.org/2865183003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc
(right):

https://codereview.chromium.org/2865183003/diff/80001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:222:
entry->DistillationTime() / base::Time::kMicrosecondsPerSecond;
On 2017/05/11 09:05:05, Marc Treib wrote:
> Any reason DistillationTime is an int64 rather than a base::Time?
> 
> Generally, I'd recommend using the Time types as much as possible, and in
> particular not doing any arithmetic on ints-that-represent-time, since that's
> hard to read (IMO) and error prone.
> Here you could do something like
> Time::UnixEpoch() + TimeDelta::FromMicroseconds(DistillationTime())

I am not sure. The comment says "to avoid conversion". I have created a bug to
look more closely into it.

https://codereview.chromium.org/2865183003/diff/80001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:223:
if (entry_time == 0) {
On 2017/05/11 09:05:05, Marc Treib wrote:
> Check DistillationTime() == 0 directly, before doing any arithmetic on it?

Done.

[gambard, 2017-05-11 11:13:42]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-05-11 11:13:42]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
treib@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2865183003/#ps100001
(title: "Address comments")

[commit-bot: I haz the power, 2017-05-11 11:14:20]

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-05-11 13:05:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494501222041500,
"parent_rev": "52bf555607be9fcfa8ad89601b0554db3c9af96b", "commit_rev":
"057d7cae6af79ac7fb8c43080d0499a2673e2952"}

[commit-bot: I haz the power, 2017-05-11 13:05:25]

Description was changed from

==========
Use the same design for all suggestions

All the suggestions (Reading List and Suggested Articles) should have the same
design. This CL removes the use of the Reading List Cells in ContentSuggestions.

BUG=706296
==========

to

==========
Use the same design for all suggestions

All the suggestions (Reading List and Suggested Articles) should have the same
design. This CL removes the use of the Reading List Cells in ContentSuggestions.

BUG=706296

Review-Url: https://codereview.chromium.org/2865183003
Cr-Commit-Position: refs/heads/master@{#470935}
Committed:
https://chromium.googlesource.com/chromium/src/+/057d7cae6af79ac7fb8c43080d04...
==========

[commit-bot: I haz the power, 2017-05-11 13:05:26]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/057d7cae6af79ac7fb8c43080d04...