Display offline availability on ContentSuggestions

Display offline availability on ContentSuggestions

This CL displays the offline availability of the ReadingList items in
ContentSuggestions.

BUG=706296, 702241
Review-Url: https://codereview.chromium.org/2796273002
Cr-Commit-Position: refs/heads/master@{#462416}
Committed: https://chromium.googlesource.com/chromium/src/+/892e302345218f7ed2ecfd9295a6011df806026e

[gambard, 2017-04-05 10:42:30]

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

[gambard, 2017-04-05 10:42:30]

PTAL.

[Olivier, 2017-04-05 11:58:11]

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

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion_extra.h:13:
@interface ContentSuggestionReadingListExtra : NSObject
the file name should probably include ReadingList, or the class name should not
contain it.

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion_extra.h:19:
@property(nonatomic, assign) ReadingListUIDistillationStatus status;
Why not ReadingListSuggestionExtra::ReadingListSuggestionDistilledState?

[gambard, 2017-04-05 12:24:43]

Thanks, PTAL.

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

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion_extra.h:13:
@interface ContentSuggestionReadingListExtra : NSObject
On 2017/04/05 11:58:11, Olivier Robin wrote:
> the file name should probably include ReadingList, or the class name should
not
> contain it.

The idea is to have a file defining all the extra information needed (for
Reading List, Bookmarks...). If you think we should have one extra per file I
can rename this one.

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion_extra.h:19:
@property(nonatomic, assign) ReadingListUIDistillationStatus status;
On 2017/04/05 11:58:11, Olivier Robin wrote:
> Why not ReadingListSuggestionExtra::ReadingListSuggestionDistilledState?

Because ReadingListSuggestionExtra::ReadingListSuggestionDistilledState is in
components and the components files cannot be include from the UI layer (i.e.
this file).

[Olivier, 2017-04-05 13:07:24]

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

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion_extra.h:13:
@interface ContentSuggestionReadingListExtra : NSObject
On 2017/04/05 12:24:43, gambard wrote:
> On 2017/04/05 11:58:11, Olivier Robin wrote:
> > the file name should probably include ReadingList, or the class name should
> not
> > contain it.
> 
> The idea is to have a file defining all the extra information needed (for
> Reading List, Bookmarks...). If you think we should have one extra per file I
> can rename this one.

So may be add _builders to the file name? or something similar?

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion_extra.h:19:
@property(nonatomic, assign) ReadingListUIDistillationStatus status;
On 2017/04/05 12:24:43, gambard wrote:
> On 2017/04/05 11:58:11, Olivier Robin wrote:
> > Why not ReadingListSuggestionExtra::ReadingListSuggestionDistilledState?
> 
> Because ReadingListSuggestionExtra::ReadingListSuggestionDistilledState is in
> components and the components files cannot be include from the UI layer (i.e.
> this file).

Why?
Is there a circular dependency?

[Olivier, 2017-04-05 13:45:22]

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

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:374:
UIStatusFromModelStatus(ReadingListStateFromSuggestionState(
IIUC, UIStatusFromModelStatus will be a really common name.
it should be renamed ReadingListUIStatusFromModelStatus or moved to a
reading_list namespace.

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

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion_extra.h:15: +
(ContentSuggestionReadingListExtra*)extraWithStatus:
reading list status is read/unread.
ExtraWithDistillationStatus?

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion_extra.h:19:
@property(nonatomic, assign) ReadingListUIDistillationStatus status;
On 2017/04/05 13:07:23, Olivier Robin wrote:
> On 2017/04/05 12:24:43, gambard wrote:
> > On 2017/04/05 11:58:11, Olivier Robin wrote:
> > > Why not ReadingListSuggestionExtra::ReadingListSuggestionDistilledState?
> > 
> > Because ReadingListSuggestionExtra::ReadingListSuggestionDistilledState is
in
> > components and the components files cannot be include from the UI layer
(i.e.
> > this file).
> 
> Why?
> Is there a circular dependency?

Ack, discussed offline.

[Olivier, 2017-04-05 14:03:44]

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

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:376:
->distilled_state));
I think the real problem (that confuses me most) is not the existence of
UIStatusFromModelStatus but the existence of ReadingListSuggestionDistilledState
which was discussed in the previous CL.
So this is out of the scope of this CL and this is fine here.

[gambard, 2017-04-06 09:28:50]

Thanks, PTAL.

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

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:374:
UIStatusFromModelStatus(ReadingListStateFromSuggestionState(
On 2017/04/05 13:45:22, Olivier Robin wrote:
> IIUC, UIStatusFromModelStatus will be a really common name.
> it should be renamed ReadingListUIStatusFromModelStatus or moved to a
> reading_list namespace.

Done.

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:376:
->distilled_state));
On 2017/04/05 14:03:44, Olivier Robin wrote:
> I think the real problem (that confuses me most) is not the existence of
> UIStatusFromModelStatus but the existence of
ReadingListSuggestionDistilledState
> which was discussed in the previous CL.
> So this is out of the scope of this CL and this is fine here.

Acknowledged.

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

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion_extra.h:13:
@interface ContentSuggestionReadingListExtra : NSObject
On 2017/04/05 13:07:23, Olivier Robin wrote:
> On 2017/04/05 12:24:43, gambard wrote:
> > On 2017/04/05 11:58:11, Olivier Robin wrote:
> > > the file name should probably include ReadingList, or the class name
should
> > not
> > > contain it.
> > 
> > The idea is to have a file defining all the extra information needed (for
> > Reading List, Bookmarks...). If you think we should have one extra per file
I
> > can rename this one.
> 
> So may be add _builders to the file name? or something similar?

Done.

https://codereview.chromium.org/2796273002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion_extra.h:15: +
(ContentSuggestionReadingListExtra*)extraWithStatus:
On 2017/04/05 13:45:22, Olivier Robin wrote:
> reading list status is read/unread.
> ExtraWithDistillationStatus?

Done.

[Olivier, 2017-04-06 09:35:14]

lgtm

[gambard, 2017-04-06 09:35:49]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-04-06 09:36:15]

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-04-06 10:39:12]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491471349823500,
"parent_rev": "15cf6391043873e64d75e9b40b549cbe5314aea5", "commit_rev":
"892e302345218f7ed2ecfd9295a6011df806026e"}

[commit-bot: I haz the power, 2017-04-06 10:40:01]

Description was changed from

==========
Display offline availability on ContentSuggestions

This CL displays the offline availability of the ReadingList items in
ContentSuggestions.

BUG=706296, 702241
==========

to

==========
Display offline availability on ContentSuggestions

This CL displays the offline availability of the ReadingList items in
ContentSuggestions.

BUG=706296, 702241

Review-Url: https://codereview.chromium.org/2796273002
Cr-Commit-Position: refs/heads/master@{#462416}
Committed:
https://chromium.googlesource.com/chromium/src/+/892e302345218f7ed2ecfd9295a6...
==========

[commit-bot: I haz the power, 2017-04-06 10:40:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/892e302345218f7ed2ecfd9295a6...