Add extra information for ReadingList ContentSuggestion

Add extra information for ReadingList ContentSuggestion

The Reading List suggestions need more information about the state of the
distillation of the Reading List entry.

BUG=702241
Review-Url: https://codereview.chromium.org/2780793002
Cr-Commit-Position: refs/heads/master@{#460719}
Committed: https://chromium.googlesource.com/chromium/src/+/38f2a30295a7b589b5746c3c7d1a6a5ef6cc3438

[gambard, 2017-03-28 13:09:10]

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

[gambard, 2017-03-28 13:09:11]

PTAL.

[Marc Treib, 2017-03-28 13:38:20]

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.cc (right):

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.cc:57: void
ContentSuggestion::set_reading_list_suggestion_extra(
empty line before

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:20: // ReadingListSuggestionExtra
contains addition data which is only available for
nit: s/addition/additional/

nit: Keep the order consistent with the setters, i.e. move this below
RecentTabSuggestionExtra

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:33: GURL favicon_page_url;
Can't this be derived from the URL?
Generally, +jkrcal for favicons, since various things are in progress there.

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

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:160:
std::vector<ContentSuggestion>* suggestions) {
I don't like out params, and IMO converting has little to do with adding to a
list. Could we just return the created ContentSuggestion, and emplace it at the
call site?

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:172:
if (entry->DistilledState() == ReadingListEntry::PROCESSED) {
switch?
Wrap this in a global helper method?

[Marc Treib, 2017-03-28 13:39:23]

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

[Marc Treib, 2017-03-28 13:39:23]

...and actually +jkrcal. Please see the question about favicons below.

[Olivier, 2017-03-28 13:43:46]

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:23: // State of the distillation a
suggestion.
Whi not use ReadingListEntry::DistillationState ?
At least, add a comment saying enums should stay synced.

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

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:172:
if (entry->DistilledState() == ReadingListEntry::PROCESSED) {
switch?

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

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.h:59: //
Converts |entry| to ContentSuggestion and add it to |suggestions|.
nit: The main goal of the method is to add the entry. Conversion is a tool.
So either rename it AddEntryToSuggestions, or make is return a
ContentSuggestion.

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.h:60:
void ConvertEntry(const ReadingListEntry* entry,
const & ?

[gambard, 2017-03-29 06:51:24]

Thanks, PTAL.

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.cc (right):

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.cc:57: void
ContentSuggestion::set_reading_list_suggestion_extra(
On 2017/03/28 13:38:20, Marc Treib wrote:
> empty line before

Done.

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:20: // ReadingListSuggestionExtra
contains addition data which is only available for
On 2017/03/28 13:38:20, Marc Treib wrote:
> nit: s/addition/additional/
> 
> nit: Keep the order consistent with the setters, i.e. move this below
> RecentTabSuggestionExtra

Done.

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:23: // State of the distillation a
suggestion.
On 2017/03/28 13:43:45, Olivier Robin wrote:
> Whi not use ReadingListEntry::DistillationState ?
> At least, add a comment saying enums should stay synced.

I am not using ReadingListEntry::DistillationState directly because we want to
remove the dependence of components/ntp_snippets on components/reading_list

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:33: GURL favicon_page_url;
On 2017/03/28 13:38:20, Marc Treib wrote:
> Can't this be derived from the URL?
> Generally, +jkrcal for favicons, since various things are in progress there.

No, this is specific to ReadingList: you can have an entry with the url
chromium.org but there is a redirection to foo.bar, the distillation (= download
for offline availability) ends up at foo.bar. The favicon_page_url will be
foo.bar to display the favicon relative to the final page.
The favicon fetching is done through the standard favicon fetcher component.

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

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:160:
std::vector<ContentSuggestion>* suggestions) {
On 2017/03/28 13:38:20, Marc Treib wrote:
> I don't like out params, and IMO converting has little to do with adding to a
> list. Could we just return the created ContentSuggestion, and emplace it at
the
> call site?

That was my first idea, but I followed the pattern used in BookmarksProvider :)
Done.

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:172:
if (entry->DistilledState() == ReadingListEntry::PROCESSED) {
On 2017/03/28 13:43:45, Olivier Robin wrote:
> switch?

Done.

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:172:
if (entry->DistilledState() == ReadingListEntry::PROCESSED) {
On 2017/03/28 13:38:20, Marc Treib wrote:
> switch?
> Wrap this in a global helper method?

Done.

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

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.h:59: //
Converts |entry| to ContentSuggestion and add it to |suggestions|.
On 2017/03/28 13:43:46, Olivier Robin wrote:
> nit: The main goal of the method is to add the entry. Conversion is a tool.
> So either rename it AddEntryToSuggestions, or make is return a
> ContentSuggestion.

Done.

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.h:60:
void ConvertEntry(const ReadingListEntry* entry,
On 2017/03/28 13:43:46, Olivier Robin wrote:
> const & ?

I have a pointer (returned by the Reading List model).

[Marc Treib, 2017-03-29 08:24:58]

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:33: GURL favicon_page_url;
On 2017/03/29 06:51:24, gambard wrote:
> On 2017/03/28 13:38:20, Marc Treib wrote:
> > Can't this be derived from the URL?
> > Generally, +jkrcal for favicons, since various things are in progress there.
> 
> No, this is specific to ReadingList: you can have an entry with the url
> http://chromium.org but there is a redirection to foo.bar, the distillation (=
download
> for offline availability) ends up at foo.bar. The favicon_page_url will be
> foo.bar to display the favicon relative to the final page.
> The favicon fetching is done through the standard favicon fetcher component.

Redirects are officially The Worst :(
Alright then, though I'd still like to get a sanity check from jkrcal, since I'm
not familiar with the latest state on favicons.

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

https://codereview.chromium.org/2780793002/diff/20001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.h:60:
void ConvertEntry(const ReadingListEntry* entry,
On 2017/03/29 06:51:24, gambard wrote:
> On 2017/03/28 13:43:46, Olivier Robin wrote:
> > const & ?
> 
> I have a pointer (returned by the Reading List model).

But you can still pass a reference here, to make clear that it can't be null.
(And that the pointer won't be kept around, though that's maybe less relevant
here.)

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:62:
ReadingListSuggestionDistilledState distilled_state;
nit: Give this a default value? (Can be inline " = whatever;")
Just because I don't like uninitialized values.

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
File
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc
(right):

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc:16:
break;
No "break" required after "return"

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc:26:
}
I think some compilers (Windows?) will force you to add a dummy return here. In
any case, add NOTREACHED()?

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc:46:
}
Same here: No "break" after "return", and NOTREACHED()

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

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:167:
suggestion.set_snippet_text(
As per the comment on the other CL, I think this should be set_publisher_name

[gambard, 2017-03-29 09:54:07]

Thanks, PTAL.

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:62:
ReadingListSuggestionDistilledState distilled_state;
On 2017/03/29 08:24:58, Marc Treib wrote:
> nit: Give this a default value? (Can be inline " = whatever;")
> Just because I don't like uninitialized values.

Done.

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
File
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc
(right):

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc:16:
break;
On 2017/03/29 08:24:58, Marc Treib wrote:
> No "break" required after "return"

Done.

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc:26:
}
On 2017/03/29 08:24:58, Marc Treib wrote:
> I think some compilers (Windows?) will force you to add a dummy return here.
In
> any case, add NOTREACHED()?

Done.

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc:46:
}
On 2017/03/29 08:24:58, Marc Treib wrote:
> Same here: No "break" after "return", and NOTREACHED()

Done.

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

https://codereview.chromium.org/2780793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:167:
suggestion.set_snippet_text(
On 2017/03/29 08:24:58, Marc Treib wrote:
> As per the comment on the other CL, I think this should be set_publisher_name

Wasn't rebased yet.
Done.

[Marc Treib, 2017-03-29 09:59:43]

lgtm

Thanks!

[Olivier, 2017-03-30 08:51:11]

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:54: // duplicated here to prevent a
dependence on ReadingList.
nit s/prevent/avoid/ ?

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:56: PROCESSING,
The enums are not synced at the moment. can you comment and/or add missing
states?

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
File
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc
(right):

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc:17:
return ReadingListEntry::PROCESSING;
WAITING seems to be a moew default state than PROCESSING.

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc:37:
PROCESSING;
nit: PROCESSING does not really match WILL_RETRY || PROCESSING || WAITING.

Rename it to NOT_PROCESSED or PENDING ?

[gambard, 2017-03-30 08:59:09]

Thanks, PTAL.

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:54: // duplicated here to prevent a
dependence on ReadingList.
On 2017/03/30 08:51:11, Olivier Robin wrote:
> nit s/prevent/avoid/ ?

Done.

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:56: PROCESSING,
On 2017/03/30 08:51:11, Olivier Robin wrote:
> The enums are not synced at the moment. can you comment and/or add missing
> states?

Done.

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
File
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc
(right):

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc:17:
return ReadingListEntry::PROCESSING;
On 2017/03/30 08:51:11, Olivier Robin wrote:
> WAITING seems to be a moew default state than PROCESSING.

Done.

https://codereview.chromium.org/2780793002/diff/60001/components/ntp_snippets...
components/ntp_snippets/reading_list/reading_list_distillation_state_util.cc:37:
PROCESSING;
On 2017/03/30 08:51:11, Olivier Robin wrote:
> nit: PROCESSING does not really match WILL_RETRY || PROCESSING || WAITING.
> 
> Rename it to NOT_PROCESSED or PENDING ?

Done.

[Olivier, 2017-03-30 09:01:29]

lgtm

[gambard, 2017-03-30 09:01:48]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-03-30 09:01:49]

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

[commit-bot: I haz the power, 2017-03-30 09:02:17]

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-30 10:36:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490864508956420,
"parent_rev": "d6744241b196c16f0946a00301aebeb7672d4166", "commit_rev":
"38f2a30295a7b589b5746c3c7d1a6a5ef6cc3438"}

[commit-bot: I haz the power, 2017-03-30 10:37:18]

Description was changed from

==========
Add extra information for ReadingList ContentSuggestion

The Reading List suggestions need more information about the state of the
distillation of the Reading List entry.

BUG=702241
==========

to

==========
Add extra information for ReadingList ContentSuggestion

The Reading List suggestions need more information about the state of the
distillation of the Reading List entry.

BUG=702241

Review-Url: https://codereview.chromium.org/2780793002
Cr-Commit-Position: refs/heads/master@{#460719}
Committed:
https://chromium.googlesource.com/chromium/src/+/38f2a30295a7b589b5746c3c7d1a...
==========

[commit-bot: I haz the power, 2017-03-30 10:37:19]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/38f2a30295a7b589b5746c3c7d1a...