Adds a Fetch() method to the ContentSuggestionService which asks any provider to provide more conte…

Adds a Fetch() method to the ContentSuggestionService which asks any provider to provide more content for the chosen category. There is an implementation for the NtpSnippetsService.

BUG=634892
Committed: https://crrev.com/83578aa7e1848253943204912238c0c74d0b9e84
Cr-Commit-Position: refs/heads/master@{#429554}

[tschumann, 2016-11-03 11:06:15]

tschumann@chromium.org changed reviewers:
+ dgn@chromium.org, markusheintz@chromium.org

[tschumann, 2016-11-03 11:06:16]

here it is, the one CL to rule em all.

This is a patch of the latest snapshot from
https://codereview.chromium.org/2421463002 plus the follow-ups on my own
comments (sent this morning).

dgn@ this should pretty much unblock you. We only need to expose the new Fetch()
method to the Java interface. If you don't have this already I can do that in a
follow-up.

[tschumann, 2016-11-03 11:06:26]

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

[commit-bot: I haz the power, 2016-11-03 11:06:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[markusheintz_, 2016-11-03 11:42:42]

On 2016/11/03 11:06:16, tschumann wrote:
> here it is, the one CL to rule em all.
> 
> This is a patch of the latest snapshot from
> https://codereview.chromium.org/2421463002 plus the follow-ups on my own
> comments (sent this morning).
> 
> dgn@ this should pretty much unblock you. We only need to expose the new
Fetch()
> method to the Java interface. If you don't have this already I can do that in
a
> follow-up.

LGTM with two nits

[markusheintz_, 2016-11-03 11:43:54]

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:229: void
FilterCategories(FetchedCategoriesVector* categories);
A comment would be nice that describes the filtering.

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_service.h (right):

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service.h:217:
base::Optional<FetchingCallback> fetching_callback,
Given that this is optional now a comment describing when it is passed in would
be nice.

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:512:
service->Fetch(articles_category(), known_ids, callback);
nit: It would be nice to pass in the articles_category as an param. Otherwise
it's not obvious that this utility method depends on this method.

[tschumann, 2016-11-03 11:55:47]

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:229: void
FilterCategories(FetchedCategoriesVector* categories);
On 2016/11/03 11:43:54, markusheintz_ wrote:
> A comment would be nice that describes the filtering.

Done.

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_service.h (right):

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service.h:217:
base::Optional<FetchingCallback> fetching_callback,
On 2016/11/03 11:43:54, markusheintz_ wrote:
> Given that this is optional now a comment describing when it is passed in
would
> be nice.

good point! done. In fact, we should change NotifyMoreSuggestions() to take a
FetchingCallback parameter. Added TODO.

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2473483006/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:512:
service->Fetch(articles_category(), known_ids, callback);
On 2016/11/03 11:43:54, markusheintz_ wrote:
> nit: It would be nice to pass in the articles_category as an param. Otherwise
> it's not obvious that this utility method depends on this method.

very valid point. done.

[tschumann, 2016-11-03 11:56:18]

The CQ bit was checked by tschumann@chromium.org

[tschumann, 2016-11-03 11:56:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
markusheintz@chromium.org
Link to the patchset: https://codereview.chromium.org/2473483006/#ps20001
(title: "comments by Markus.")

[commit-bot: I haz the power, 2016-11-03 11:56:35]

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, 2016-11-03 13:19:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-03 13:22:23]

Description was changed from

==========
Adds a Fetch() method to the ContentSuggestionService which asks any provider to
provide more content for the chosen category. There is an implementation for the
NtpSnippetsService.

BUG=634892
==========

to

==========
Adds a Fetch() method to the ContentSuggestionService which asks any provider to
provide more content for the chosen category. There is an implementation for the
NtpSnippetsService.

BUG=634892

Committed: https://crrev.com/83578aa7e1848253943204912238c0c74d0b9e84
Cr-Commit-Position: refs/heads/master@{#429554}
==========

[commit-bot: I haz the power, 2016-11-03 13:22:24]

Patchset 2 (id:??) landed as
https://crrev.com/83578aa7e1848253943204912238c0c74d0b9e84
Cr-Commit-Position: refs/heads/master@{#429554}

[Marc Treib, 2016-11-07 14:28:22]

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

[Marc Treib, 2016-11-07 14:28:24]

Bunch of post-commit comments - mostly nits, but one which I think is an actual
bug.

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

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:112: FetchingCallback
callback) = 0;
This should be a const&

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

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:37: class
NTPSnippetsFetcher;
Not needed

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.cc:755:
categories->erase(category_it + 1, categories->end());
After looking at this for the 10th time, I'm now convinced that it's not only
ugly, it also doesn't work :D
The first erase() call will invalidate category_it (or at least "reinterpret"
it), so the second erase() call will do either nothing or maybe
unexpected/undefined things.

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.cc:767: // and filter here.
I find this comment super confusing. In my version of the CL, I had changed it
to:
// Filter out unwanted categories if necessary.
// TODO(fhorschig): As soon as the server supports filtering by
// that instead of over-fetching and filtering here.

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_service.cc:316:
/*fetch_more=*/false, std::set<std::string>(),
/*known_suggestion_ids=*/

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_service.cc:646: // fetching_callback
?

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_service.h (right):

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_service.h:366: CategoryContentMap
categories_;
Why this change? CategoryContentMap isn't used anywhere else, so I'd prefer to
leave out the typedef.

[tschumann, 2016-11-08 16:57:35]

follow-ups addressed in https://codereview.chromium.org/2485933003

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

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:112: FetchingCallback
callback) = 0;
On 2016/11/07 14:28:23, Marc Treib wrote:
> This should be a const&

Done.

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

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:37: class
NTPSnippetsFetcher;
On 2016/11/07 14:28:23, Marc Treib wrote:
> Not needed

Done.

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.cc:755:
categories->erase(category_it + 1, categories->end());
On 2016/11/07 14:28:23, Marc Treib wrote:
> After looking at this for the 10th time, I'm now convinced that it's not only
> ugly, it also doesn't work :D
> The first erase() call will invalidate category_it (or at least "reinterpret"
> it), so the second erase() call will do either nothing or maybe
> unexpected/undefined things.

Good catch! I'll fix it.

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.cc:767: // and filter here.
On 2016/11/07 14:28:23, Marc Treib wrote:
> I find this comment super confusing. In my version of the CL, I had changed it
> to:
> // Filter out unwanted categories if necessary.
> // TODO(fhorschig): As soon as the server supports filtering by
> // that instead of over-fetching and filtering here.

Done.

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_service.cc:316:
/*fetch_more=*/false, std::set<std::string>(),
On 2016/11/07 14:28:23, Marc Treib wrote:
> /*known_suggestion_ids=*/

changed in the meantime.

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_service.cc:646: // fetching_callback
On 2016/11/07 14:28:23, Marc Treib wrote:
> ?
 went away in the meantime.

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_service.h (right):

https://codereview.chromium.org/2473483006/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_service.h:366: CategoryContentMap
categories_;
On 2016/11/07 14:28:23, Marc Treib wrote:
> Why this change? CategoryContentMap isn't used anywhere else, so I'd prefer to
> leave out the typedef.

Done.