[NTP::SectionOrder] Replace CategoryFactory with a category ranker.

[NTP::SectionOrder] Replace CategoryFactory with a category ranker.

This CL:
1) Adds CategoryRanker interface;
2) Adds a default constant category ranker implementation (based on
   CategoryFactory);
3) Replaces CategoryFactory with a new ranker;
4) Factory methods are moved to Category.

This is the initial step of moving to personalised section order.

BUG=673659
Committed: https://crrev.com/7456f5ae0859872c69398b583d6331ed47d70586
Cr-Commit-Position: refs/heads/master@{#439451}

[vitaliii, 2016-12-13 10:59:01]

Patchset #1 (id:1) has been deleted

[vitaliii, 2016-12-13 11:08:40]

Patchset #1 (id:20001) has been deleted

[vitaliii, 2016-12-13 11:09:04]

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

[commit-bot: I haz the power, 2016-12-13 11:09:26]

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

[vitaliii, 2016-12-13 11:10:02]

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

[vitaliii, 2016-12-13 11:10:02]

[Large CL, many files]

Hi Marc,

Could you have a look at my CL?

[commit-bot: I haz the power, 2016-12-13 11:14:49]

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

[commit-bot: I haz the power, 2016-12-13 11:14:50]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Marc Treib, 2016-12-13 12:22:42]

*Much* nicer overall!

https://codereview.chromium.org/2568033005/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:370:
content_suggestions_service_->section_ranker()->OnSuggestionOpened(
It'd be nice not to expose the section ranker. Similar for the user classifier
actually. Maybe there should just be an OnSuggestionOpened method on
ContentSuggestionsService that does all this?

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/category.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/category.cc:21: const int id =
static_cast<int>(known_category);
nit: just inline into the return

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

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/category.h:77: // Creates a category from an ID as
returned by |Category::id()|.
nit: "Category::" isn't required

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/category.h:79: static Category FromIDValue(int id);
nit: Move all the static "create" methods to the top?

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

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:323: // Probider order for
categories.
s/Probider/Provides/ ?

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:605:
section_ranker_->AppendCategoryIfNecessary(snippet_category);
Hm. This is one downside of the new design: Before, the Factory (which did the
sorting) always knew about all categories. Now, it's possible to create a
category but forget to register it. What would the ranker do in that case?

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:1322: // The
ranker may not persist the order, thus, it is stored by the provider.
Hm. Makes sense, but this is quite subtle. Do we have a test for this?

Or maybe, we could make persistence part of the ranker's contract. That'd make
it hard to clean up old categories though.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/section_rankers/default_constant_section_ranker.h
(right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/default_constant_section_ranker.h:16: //
TODO(vitaliii): Add unit tests.
Yes please. At the very least, port the CategoryFactoryTests over.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/default_constant_section_ranker.h:17:
class DefaultConstantSectionRanker : public SectionRanker {
Remove the "Default" - that's not part of the class definition, and it's bound
to change (hopefully!)

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/default_constant_section_ranker.h:22:
bool Compare(Category left, Category right) const override;
nitty nit: Add a "CategoryRanker implementation." comment, and remove the empty
lines between the methods?

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/section_rankers/section_ranker.h (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:13: // TODO(vitaliii):
Ensure that changes in the order are propagated to the UI.
This should have a bug assigned.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:16: // The order can be
changed.
What does this mean?

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:17: class SectionRanker
{
CategoryRanker.
"Section" is the UI term, which we're not using around here.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:19: // Compare two
categories. True means that |left| precedes |right|, i.e. it
"Compares two categories", similar below. The style is:
// Does the thing.
void DoTheThing();

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:25: // after this call
should not reflect the history in any way.
After this call, the category order may not depend on this history range
anymore.
?

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:29: // the score 0,
otherwise does nothing. Before the data for new categories
"score 0" is an implementation detail of some possible implementations of the
interface, so it shouldn't be here.
The same more or less applies to the rest of the comment.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:36: // Suggestion click
data.
This comment isn't useful.
"Called whenever a suggestion is opened by the user." ?

[vitaliii, 2016-12-13 14:33:11]

Description was changed from

==========
[NTP::SectionOrder] Replace CategoryFactory with a section ranker.

This CL:
1) Adds SectionRanker interface;
2) Adds a default constant section ranker implementation (based on
   CategoryFactory);
3) Replaces CategoryFactory with a new ranker;
4) Factory methods are moved to Category.

This is the initial step of moving to personalised section order.

BUG=673659
==========

to

==========
[NTP::SectionOrder] Replace CategoryFactory with a category ranker.

This CL:
1) Adds CategoryRanker interface;
2) Adds a default constant category ranker implementation (based on
   CategoryFactory);
3) Replaces CategoryFactory with a new ranker;
4) Factory methods are moved to Category.

This is the initial step of moving to personalised section order.

BUG=673659
==========

[vitaliii, 2016-12-14 08:24:21]

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

[commit-bot: I haz the power, 2016-12-14 08:24:39]

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, 2016-12-14 08:29:56]

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

[commit-bot: I haz the power, 2016-12-14 08:29:57]

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

[vitaliii, 2016-12-14 08:47:48]

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

[commit-bot: I haz the power, 2016-12-14 08:47:56]

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

[vitaliii, 2016-12-14 08:47:57]

Patchset #2 (id:60001) has been deleted

[vitaliii, 2016-12-14 08:59:38]

Hi Marc,

I addressed your comments.

PTAL.

https://codereview.chromium.org/2568033005/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:370:
content_suggestions_service_->section_ranker()->OnSuggestionOpened(
On 2016/12/13 12:22:42, Marc Treib wrote:
> It'd be nice not to expose the section ranker. Similar for the user classifier
> actually. Maybe there should just be an OnSuggestionOpened method on
> ContentSuggestionsService that does all this?

user_classifier is used in many places (e.g.
snippets_internals_message_handler.cc and RemoteSuggestionsProvider), so it is
not clear how not to expose it. Thus, for sure, the change should not be in this
CL.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/category.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/category.cc:21: const int id =
static_cast<int>(known_category);
On 2016/12/13 12:22:42, Marc Treib wrote:
> nit: just inline into the return

Done.

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

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/category.h:77: // Creates a category from an ID as
returned by |Category::id()|.
On 2016/12/13 12:22:42, Marc Treib wrote:
> nit: "Category::" isn't required

Done.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/category.h:79: static Category FromIDValue(int id);
On 2016/12/13 12:22:42, Marc Treib wrote:
> nit: Move all the static "create" methods to the top?

Done.

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

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:323: // Probider order for
categories.
On 2016/12/13 12:22:42, Marc Treib wrote:
> s/Probider/Provides/ ?

Done.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:605:
section_ranker_->AppendCategoryIfNecessary(snippet_category);
On 2016/12/13 12:22:42, Marc Treib wrote:
> Hm. This is one downside of the new design: Before, the Factory (which did the
> sorting) always knew about all categories. Now, it's possible to create a
> category but forget to register it. What would the ranker do in that case?

Currently all non-registered categories are in the end.
Among themselves they are sorted by increasing the category id.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:1322: // The
ranker may not persist the order, thus, it is stored by the provider.
On 2016/12/13 12:22:42, Marc Treib wrote:
> Hm. Makes sense, but this is quite subtle. Do we have a test for this?
> 
> Or maybe, we could make persistence part of the ranker's contract. That'd make
> it hard to clean up old categories though.

Done.

We did not have a test for this (actually, I have got a feeling that previously
it did not work). I added a test.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/section_rankers/default_constant_section_ranker.h
(right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/default_constant_section_ranker.h:16: //
TODO(vitaliii): Add unit tests.
On 2016/12/13 12:22:42, Marc Treib wrote:
> Yes please. At the very least, port the CategoryFactoryTests over.

But not in this CL.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/default_constant_section_ranker.h:17:
class DefaultConstantSectionRanker : public SectionRanker {
On 2016/12/13 12:22:42, Marc Treib wrote:
> Remove the "Default" - that's not part of the class definition, and it's bound
> to change (hopefully!)

Done.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/default_constant_section_ranker.h:22:
bool Compare(Category left, Category right) const override;
On 2016/12/13 12:22:42, Marc Treib wrote:
> nitty nit: Add a "CategoryRanker implementation." comment, and remove the
empty
> lines between the methods?

Done.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/section_rankers/section_ranker.h (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:13: // TODO(vitaliii):
Ensure that changes in the order are propagated to the UI.
On 2016/12/13 12:22:42, Marc Treib wrote:
> This should have a bug assigned.

Done.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:16: // The order can be
changed.
On 2016/12/13 12:22:42, Marc Treib wrote:
> What does this mean?

Done.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:17: class SectionRanker
{
On 2016/12/13 12:22:42, Marc Treib wrote:
> CategoryRanker.
> "Section" is the UI term, which we're not using around here.

Done.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:19: // Compare two
categories. True means that |left| precedes |right|, i.e. it
On 2016/12/13 12:22:42, Marc Treib wrote:
> "Compares two categories", similar below. The style is:
> // Does the thing.
> void DoTheThing();

Done.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:25: // after this call
should not reflect the history in any way.
On 2016/12/13 12:22:42, Marc Treib wrote:
> After this call, the category order may not depend on this history range
> anymore.
> ?

Done.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:29: // the score 0,
otherwise does nothing. Before the data for new categories
On 2016/12/13 12:22:42, Marc Treib wrote:
> "score 0" is an implementation detail of some possible implementations of the
> interface, so it shouldn't be here.
> The same more or less applies to the rest of the comment.

Done.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/section_ranker.h:36: // Suggestion click
data.
On 2016/12/13 12:22:42, Marc Treib wrote:
> This comment isn't useful.
> "Called whenever a suggestion is opened by the user." ?

Done.

[commit-bot: I haz the power, 2016-12-14 09:06:29]

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

[commit-bot: I haz the power, 2016-12-14 09:06:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vitaliii, 2016-12-14 10:12:30]

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

[commit-bot: I haz the power, 2016-12-14 10:12:43]

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

[Marc Treib, 2016-12-14 10:24:31]

https://codereview.chromium.org/2568033005/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:370:
content_suggestions_service_->section_ranker()->OnSuggestionOpened(
On 2016/12/14 08:59:38, vitaliii wrote:
> On 2016/12/13 12:22:42, Marc Treib wrote:
> > It'd be nice not to expose the section ranker. Similar for the user
classifier
> > actually. Maybe there should just be an OnSuggestionOpened method on
> > ContentSuggestionsService that does all this?
> 
> user_classifier is used in many places (e.g.
> snippets_internals_message_handler.cc and RemoteSuggestionsProvider), so it is
> not clear how not to expose it. Thus, for sure, the change should not be in
this
> CL.

Agreed, I wasn't suggesting to do that in this CL.
What we could consider is making a ContentSuggestionsService::OnSuggestionOpened
method, so that we don't have to call two separate things here (neither of which
the bridge should have to know about).
(The metrics:: call should probably stay separate for now though - if we want to
pipe that through the service, then we should do the same for all the metrics
calls, and that's not for this CL.)

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:605:
section_ranker_->AppendCategoryIfNecessary(snippet_category);
On 2016/12/14 08:59:38, vitaliii wrote:
> On 2016/12/13 12:22:42, Marc Treib wrote:
> > Hm. This is one downside of the new design: Before, the Factory (which did
the
> > sorting) always knew about all categories. Now, it's possible to create a
> > category but forget to register it. What would the ranker do in that case?
> 
> Currently all non-registered categories are in the end.
> Among themselves they are sorted by increasing the category id.

Should it be some kind of error if someone tries to compare two categories that
haven't been registered?
I'm a bit worried that now it's super easy to forget to register a category,
which could lead to all sorts of subtle bugs with the ordering...

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:1322: // The
ranker may not persist the order, thus, it is stored by the provider.
On 2016/12/14 08:59:38, vitaliii wrote:
> On 2016/12/13 12:22:42, Marc Treib wrote:
> > Hm. Makes sense, but this is quite subtle. Do we have a test for this?
> > 
> > Or maybe, we could make persistence part of the ranker's contract. That'd
make
> > it hard to clean up old categories though.
> 
> Done.
> 
> We did not have a test for this (actually, I have got a feeling that
previously
> it did not work). I added a test.

What exactly do you think didn't work before?

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/section_rankers/default_constant_section_ranker.h
(right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/default_constant_section_ranker.h:16: //
TODO(vitaliii): Add unit tests.
On 2016/12/14 08:59:38, vitaliii wrote:
> On 2016/12/13 12:22:42, Marc Treib wrote:
> > Yes please. At the very least, port the CategoryFactoryTests over.
> 
> But not in this CL.

No need to write additional tests in this CL, but you removed a number of
CategoryFactoryTests. Those should be re-added as CategoryRankerTests in this
CL.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/category.h (right):

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/category.h:71: struct CompareByID;
nitty nit: This should probably stay on top (types before methods). Sorry for
not being clear previously.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/category_ranker.h (right):

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/category_rankers/category_ranker.h:16: // Orders
sections (represented as categories).
nitty nit: don't talk about sections (they don't exist here)

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/constant_category_ranker.cc
(right):

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:38: //
Ignored.
...because this implementation doesn't store any history-related data.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/constant_category_ranker.h
(right):

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/category_rankers/constant_category_ranker.h:17: class
ConstantCategoryRanker : public CategoryRanker {
Add a class-level comment? Something like "Simple implementation of a
CategoryRanker that ..."

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:134:
class MutliCategoryJsonBuilder {
s/Mutli/Multi/g

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:503:
category_ranker_ = base::MakeUnique<ConstantCategoryRanker>();
If this needs to be reset here, then it should also be created along with the
provider in MakeSnippetsService...
(but maybe it doesn't actually need to be reset?)

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:508:
std::vector<Category> SortCategories(std::vector<Category> categories) {
Either pass by const ref, or pass in a pointer and sort in-place.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:908:
EXPECT_EQ(ordered_categories, SortCategories(unordered_categories));
Hm, I find this hard to follow - it's not easy to see IMO how the SortCategories
helper depends on the provider.
This also kinda tests the ranker more than the provider. I think what we should
really test is that the AppendCategory methods on the ranker are called in the
correct order. Since the ranker is already injectable, that should be fairly
straightforward: Instead of an actual ranker, pass in a Mock.
WDYT?

[Marc Treib, 2016-12-14 10:26:54]

Ah, one comment about the tool: Please try to upload rebases as a separate patch
set; that makes the diffs much easier to read. (Not a huge deal, I also tend to
forget...)

[commit-bot: I haz the power, 2016-12-14 12:27:18]

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

[commit-bot: I haz the power, 2016-12-14 12:27:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vitaliii, 2016-12-14 13:34:28]

vitaliii@chromium.org changed reviewers:
+ tschumann@chromium.org

[tschumann, 2016-12-15 11:50:26]

lgtm

really just nits and I don't feel strongly about them

https://codereview.chromium.org/2568033005/diff/100001/chrome/browser/ntp_sni...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2568033005/diff/100001/chrome/browser/ntp_sni...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:165:
service->category_ranker(), service->user_classifier(), scheduler,
that's a bit of an anti-pattern but okay for now (as we do the same with the
user_classifier).
As we create those instances within this factory, it would be cleaner to pass
them intothe function instead of abusing the service to carry them through (the
service shouldn't have to expose those members).

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/category_ranker.h (right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/category_ranker.h:27: virtual void
ClearHistory(base::Time begin, base::Time end) = 0;
is this actually a necessary requirement? Would be good to double-check  so that
we don't make our lives unnecessarily hard.
Given you already did the work, we can move ahead as is but getting clarity
would be nice (maybe you already have ;-)).

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.cc
(right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:34: return
left.id() < right.id();
that's a bit confusing. What are the cases where left.id() < right.id() would
return something different than the loop above?

Would be great to document the fallback.

Ideally, the ranker would know them all... That was actually a nice property
with the factory as that would see every instantiated category object and could
have a more complete view. But I also see that this is speculation for now.
Let's see how things go -- worst case we have to redo this exercise :-)

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:51:
////////////////////////////////////////////////////////////////////////////////
i'd drop this section comment. Ideally we don't get so large files that we need
visual separators.
Marc is this something common we should keep doing?

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.h
(right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.h:16: //
TODO(vitaliii): Add unit tests.
i treat this as a DO_NOT_SUBMIT ;-) Not sure if this works in chromium though...

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider.cc:606:
category_ranker_->AppendCategoryIfNecessary(snippet_category);
in google3, a common idiom is to call such methods MaybeAppendCategory(...)

i'm not too happy with the 'ifNecessary' part, maybe 'ifMissing' or 'ifNew'?

Then again, I think this logic needs more work anyways and probably will involve
further renamings.

[Marc Treib, 2016-12-15 12:22:57]

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.cc
(right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:51:
////////////////////////////////////////////////////////////////////////////////
On 2016/12/15 11:50:26, tschumann wrote:
> i'd drop this section comment. Ideally we don't get so large files that we
need
> visual separators.
> Marc is this something common we should keep doing?

No, it's not particularly common. I don't write them, but so far I haven't
complained when others did.
I guess I'd slightly prefer to leave them out (as you say, if they're needed,
then maybe the file is too big), but I don't feel strong enough to really argue.

[tschumann, 2016-12-15 13:05:20]

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:605:
section_ranker_->AppendCategoryIfNecessary(snippet_category);
On 2016/12/14 10:24:30, Marc Treib wrote:
> On 2016/12/14 08:59:38, vitaliii wrote:
> > On 2016/12/13 12:22:42, Marc Treib wrote:
> > > Hm. This is one downside of the new design: Before, the Factory (which did
> the
> > > sorting) always knew about all categories. Now, it's possible to create a
> > > category but forget to register it. What would the ranker do in that case?
> > 
> > Currently all non-registered categories are in the end.
> > Among themselves they are sorted by increasing the category id.
> 
> Should it be some kind of error if someone tries to compare two categories
that
> haven't been registered?
> I'm a bit worried that now it's super easy to forget to register a category,
> which could lead to all sorts of subtle bugs with the ordering...

We need to clearly handle the cases where a category can enter the system: there
are 3 places I know of: reading from the database, getting requests from the
remote server, and reading categories from prefs.
We need to take care of those. IMO, consolidating this in one class might make
sense (categoryregistry?)  which knows about all categories and keeps the state
persisted.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.cc
(right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:51:
////////////////////////////////////////////////////////////////////////////////
On 2016/12/15 12:22:56, Marc Treib wrote:
> On 2016/12/15 11:50:26, tschumann wrote:
> > i'd drop this section comment. Ideally we don't get so large files that we
> need
> > visual separators.
> > Marc is this something common we should keep doing?
> 
> No, it's not particularly common. I don't write them, but so far I haven't
> complained when others did.
> I guess I'd slightly prefer to leave them out (as you say, if they're needed,
> then maybe the file is too big), but I don't feel strong enough to really
argue.

Then let's leave them out.

[vitaliii, 2016-12-15 15:12:46]

Patchset #4 (id:120001) has been deleted

[vitaliii, 2016-12-15 15:25:31]

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

[commit-bot: I haz the power, 2016-12-15 15:25:41]

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, 2016-12-15 15:27:43]

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

[commit-bot: I haz the power, 2016-12-15 15:27:44]

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

[vitaliii, 2016-12-15 15:29:56]

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

[commit-bot: I haz the power, 2016-12-15 15:30:09]

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

[vitaliii, 2016-12-15 15:30:13]

I addressed your comments, PTAL.

https://codereview.chromium.org/2568033005/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:370:
content_suggestions_service_->section_ranker()->OnSuggestionOpened(
On 2016/12/14 10:24:30, Marc Treib wrote:
> On 2016/12/14 08:59:38, vitaliii wrote:
> > On 2016/12/13 12:22:42, Marc Treib wrote:
> > > It'd be nice not to expose the section ranker. Similar for the user
> classifier
> > > actually. Maybe there should just be an OnSuggestionOpened method on
> > > ContentSuggestionsService that does all this?
> > 
> > user_classifier is used in many places (e.g.
> > snippets_internals_message_handler.cc and RemoteSuggestionsProvider), so it
is
> > not clear how not to expose it. Thus, for sure, the change should not be in
> this
> > CL.
> 
> Agreed, I wasn't suggesting to do that in this CL.
> What we could consider is making a
ContentSuggestionsService::OnSuggestionOpened
> method, so that we don't have to call two separate things here (neither of
which
> the bridge should have to know about).
> (The metrics:: call should probably stay separate for now though - if we want
to
> pipe that through the service, then we should do the same for all the metrics
> calls, and that's not for this CL.)

I added a TODO.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:605:
section_ranker_->AppendCategoryIfNecessary(snippet_category);
On 2016/12/14 10:24:30, Marc Treib wrote:
> On 2016/12/14 08:59:38, vitaliii wrote:
> > On 2016/12/13 12:22:42, Marc Treib wrote:
> > > Hm. This is one downside of the new design: Before, the Factory (which did
> the
> > > sorting) always knew about all categories. Now, it's possible to create a
> > > category but forget to register it. What would the ranker do in that case?
> > 
> > Currently all non-registered categories are in the end.
> > Among themselves they are sorted by increasing the category id.
> 
> Should it be some kind of error if someone tries to compare two categories
that
> haven't been registered?
> I'm a bit worried that now it's super easy to forget to register a category,
> which could lead to all sorts of subtle bugs with the ordering...

Done.

I do not understand why you are concerned about 2 unregistered categories, but
not 1.
I added DCHECKs to |Compare| that both categories have been registered.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:605:
section_ranker_->AppendCategoryIfNecessary(snippet_category);
On 2016/12/15 13:05:20, tschumann wrote:
> On 2016/12/14 10:24:30, Marc Treib wrote:
> > On 2016/12/14 08:59:38, vitaliii wrote:
> > > On 2016/12/13 12:22:42, Marc Treib wrote:
> > > > Hm. This is one downside of the new design: Before, the Factory (which
did
> > the
> > > > sorting) always knew about all categories. Now, it's possible to create
a
> > > > category but forget to register it. What would the ranker do in that
case?
> > > 
> > > Currently all non-registered categories are in the end.
> > > Among themselves they are sorted by increasing the category id.
> > 
> > Should it be some kind of error if someone tries to compare two categories
> that
> > haven't been registered?
> > I'm a bit worried that now it's super easy to forget to register a category,
> > which could lead to all sorts of subtle bugs with the ordering...
> 
> We need to clearly handle the cases where a category can enter the system:
there
> are 3 places I know of: reading from the database, getting requests from the
> remote server, and reading categories from prefs.
> We need to take care of those. IMO, consolidating this in one class might make
> sense (categoryregistry?)  which knows about all categories and keeps the
state
> persisted.

There are 2 places actually (the database does not enforce the order).
Currently in both cases the order is added to CategoryRanker. Before storing the
order is obtained from CategoryRanker. Note that dynamic rankers will have to
store the order themselves.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:1322: // The
ranker may not persist the order, thus, it is stored by the provider.
On 2016/12/14 10:24:30, Marc Treib wrote:
> On 2016/12/14 08:59:38, vitaliii wrote:
> > On 2016/12/13 12:22:42, Marc Treib wrote:
> > > Hm. Makes sense, but this is quite subtle. Do we have a test for this?
> > > 
> > > Or maybe, we could make persistence part of the ranker's contract. That'd
> make
> > > it hard to clean up old categories though.
> > 
> > Done.
> > 
> > We did not have a test for this (actually, I have got a feeling that
> previously
> > it did not work). I added a test.
> 
> What exactly do you think didn't work before?

I thought that remote categories were not registered previously.
However, I understood my mistake. Previously The registration was done in
NTPSnippetsFetcher, which is very non obvious place and I missed it.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/section_rankers/default_constant_section_ranker.h
(right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/section_rankers/default_constant_section_ranker.h:16: //
TODO(vitaliii): Add unit tests.
On 2016/12/14 10:24:31, Marc Treib wrote:
> On 2016/12/14 08:59:38, vitaliii wrote:
> > On 2016/12/13 12:22:42, Marc Treib wrote:
> > > Yes please. At the very least, port the CategoryFactoryTests over.
> > 
> > But not in this CL.
> 
> No need to write additional tests in this CL, but you removed a number of
> CategoryFactoryTests. Those should be re-added as CategoryRankerTests in this
> CL.

Done.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/category.h (right):

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/category.h:71: struct CompareByID;
On 2016/12/14 10:24:31, Marc Treib wrote:
> nitty nit: This should probably stay on top (types before methods). Sorry for
> not being clear previously.

Done.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/category_ranker.h (right):

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/category_rankers/category_ranker.h:16: // Orders
sections (represented as categories).
On 2016/12/14 10:24:31, Marc Treib wrote:
> nitty nit: don't talk about sections (they don't exist here)

Done.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/constant_category_ranker.cc
(right):

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:38: //
Ignored.
On 2016/12/14 10:24:31, Marc Treib wrote:
> ...because this implementation doesn't store any history-related data.

Done.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/constant_category_ranker.h
(right):

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/category_rankers/constant_category_ranker.h:17: class
ConstantCategoryRanker : public CategoryRanker {
On 2016/12/14 10:24:31, Marc Treib wrote:
> Add a class-level comment? Something like "Simple implementation of a
> CategoryRanker that ..."

Done.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:134:
class MutliCategoryJsonBuilder {
On 2016/12/14 10:24:31, Marc Treib wrote:
> s/Mutli/Multi/g

Done.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:503:
category_ranker_ = base::MakeUnique<ConstantCategoryRanker>();
On 2016/12/14 10:24:31, Marc Treib wrote:
> If this needs to be reset here, then it should also be created along with the
> provider in MakeSnippetsService...
> (but maybe it doesn't actually need to be reset?)

Done. 

It does, ResetSnippetsService must simulate Chrome restart.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:508:
std::vector<Category> SortCategories(std::vector<Category> categories) {
On 2016/12/14 10:24:31, Marc Treib wrote:
> Either pass by const ref, or pass in a pointer and sort in-place.

Done.

The function was removed completely.

https://codereview.chromium.org/2568033005/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:908:
EXPECT_EQ(ordered_categories, SortCategories(unordered_categories));
On 2016/12/14 10:24:31, Marc Treib wrote:
> Hm, I find this hard to follow - it's not easy to see IMO how the
SortCategories
> helper depends on the provider.
> This also kinda tests the ranker more than the provider. I think what we
should
> really test is that the AppendCategory methods on the ranker are called in the
> correct order. Since the ranker is already injectable, that should be fairly
> straightforward: Instead of an actual ranker, pass in a Mock.
> WDYT?

Done.

Rewrote with the mock, but I am not sure whether this became any better (there
are a lot of "surprises" inside RemoteSuggestionsProvider).

https://codereview.chromium.org/2568033005/diff/100001/chrome/browser/ntp_sni...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2568033005/diff/100001/chrome/browser/ntp_sni...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:165:
service->category_ranker(), service->user_classifier(), scheduler,
On 2016/12/15 11:50:26, tschumann wrote:
> that's a bit of an anti-pattern but okay for now (as we do the same with the
> user_classifier).
> As we create those instances within this factory, it would be cleaner to pass
> them intothe function instead of abusing the service to carry them through
(the
> service shouldn't have to expose those members).

That's true, but currently user_classifier is used in the bridge and
snippets_internals, so this is out of scope for this CL. Marc has already
pointed this out and I added a TODO (ntp_snippets_bridge.cc:370).

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/category_ranker.h (right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/category_ranker.h:27: virtual void
ClearHistory(base::Time begin, base::Time end) = 0;
On 2016/12/15 11:50:26, tschumann wrote:
> is this actually a necessary requirement? Would be good to double-check  so
that
> we don't make our lives unnecessarily hard.
> Given you already did the work, we can move ahead as is but getting clarity
> would be nice (maybe you already have ;-)).

My reasoning for this was that we had to provide such method for suggestion
providers.
Overall, this is an internal interface with at most 3 implementations in the
nearest future, so it should be ok to change it accordingly if anything changes.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.cc
(right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:34: return
left.id() < right.id();
On 2016/12/15 11:50:26, tschumann wrote:
> that's a bit confusing. What are the cases where left.id() < right.id() would
> return something different than the loop above?

When remote categories are sent in an order not sorted by id.

> Would be great to document the fallback.

Marc has noted this, we agreed to require registering all categories explicitly
(I have added LOG(DFATAL)), so this fallback is rather to satisfy sort contract
(I added a comment explaining this).
 
> Ideally, the ranker would know them all... That was actually a nice property
> with the factory as that would see every instantiated category object and
could
> have a more complete view. But I also see that this is speculation for now.
> Let's see how things go -- worst case we have to redo this exercise :-) 

While it was a nice property, the registration was implicit (so one could end up
with unpredictable order) and remote categories were registered in Fetcher,
which is not nice. Overall, currently we have only one provider who registers
remote categories, so this does not seem to be a problem. I also like the new
explicit order approach, because now if one cares about the order, they can just
grab the ranker and define the order, without worrying that it is handled
differently e.g. in Fetcher.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:51:
////////////////////////////////////////////////////////////////////////////////
On 2016/12/15 13:05:20, tschumann wrote:
> On 2016/12/15 12:22:56, Marc Treib wrote:
> > On 2016/12/15 11:50:26, tschumann wrote:
> > > i'd drop this section comment. Ideally we don't get so large files that we
> > need
> > > visual separators.
> > > Marc is this something common we should keep doing?
> > 
> > No, it's not particularly common. I don't write them, but so far I haven't
> > complained when others did.
> > I guess I'd slightly prefer to leave them out (as you say, if they're
needed,
> > then maybe the file is too big), but I don't feel strong enough to really
> argue.
> 
> Then let's leave them out.

Done.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.h
(right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.h:16: //
TODO(vitaliii): Add unit tests.
On 2016/12/15 11:50:26, tschumann wrote:
> i treat this as a DO_NOT_SUBMIT ;-) Not sure if this works in chromium
though...

The idea was to submit this large CL and then immediately proceed with tests.
However, Marc noticed this and in the end I rewrote former CategoryFactory tests
and added new ones.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider.cc:606:
category_ranker_->AppendCategoryIfNecessary(snippet_category);
On 2016/12/15 11:50:26, tschumann wrote:
> in google3, a common idiom is to call such methods MaybeAppendCategory(...)
> 
> i'm not too happy with the 'ifNecessary' part, maybe 'ifMissing' or 'ifNew'?
> 
> Then again, I think this logic needs more work anyways and probably will
involve
> further renamings.

I do not like "Maybe", because then it is not clear when the category is
actually added.

As for other suggestions, here are occurrence counts in Chrome:
- ifNecessary -> 695;
- ifMissing -> 49;
- ifNew -> 17;

So I would prefer to leave it as it is now.

[Marc Treib, 2016-12-15 16:01:19]

Thanks! LGTM with some last nits.

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/category_ranker.cc (right):

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
components/ntp_snippets/category_rankers/category_ranker.cc:9:
CategoryRanker::~CategoryRanker() = default;
I think this could go in the header? (But doesn't really matter I guess)

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.cc
(right):

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:25:
AppendKnownCategory(KnownCategories::ARTICLES);
Hm, does anything rely on ARTICLES getting registered here? Would be nice to get
rid of this, so that the server could decide to put some other category first.
(It's okay to keep this as is for now, but please add a TODO(treib) to do
something about it!)

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:416:
return base::MakeUnique<ConstantCategoryRanker>();
No need for a helper function for this; just call MakeUnique directly where it's
needed.

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:844:
SetCategoryRanker(std::unique_ptr<CategoryRanker>(mock_ranker));
This is a bit yucky, ownership-wise. How about:
auto mock_ranker = base::MakeUnique<MockCategoryRanker>();
MockCategoryRanker* raw_mock_ranker = mock_ranker.get();
SetCategoryRanker(std::move(mock_ranker));

(and then EXPECT on the raw_mock_ranker). That way, there's no owning raw
pointer around.

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:924: auto
mock_ranker = new MockCategoryRanker();
Same here.

[vitaliii, 2016-12-15 16:51:28]

Patchset #6 (id:180001) has been deleted

[vitaliii, 2016-12-15 16:51:54]

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

[commit-bot: I haz the power, 2016-12-15 16:52:26]

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

[vitaliii, 2016-12-15 16:53:50]

Hi treib@,

I addressed your nits, no need to look.

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/category_ranker.cc (right):

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
components/ntp_snippets/category_rankers/category_ranker.cc:9:
CategoryRanker::~CategoryRanker() = default;
On 2016/12/15 16:01:18, Marc Treib wrote:
> I think this could go in the header? (But doesn't really matter I guess)

Done.

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.cc
(right):

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:25:
AppendKnownCategory(KnownCategories::ARTICLES);
On 2016/12/15 16:01:18, Marc Treib wrote:
> Hm, does anything rely on ARTICLES getting registered here? Would be nice to
get
> rid of this, so that the server could decide to put some other category first.
> (It's okay to keep this as is for now, but please add a TODO(treib) to do
> something about it!)

TODO - done.

Yes, at least ContentSuggestionsService has 4 tests relying on this.
Namely, there are mock providers and they do not register categories.
I believe it should be trivial to change category there to some other known
category.
However, this may become completely irrelevant once we have a dynamic ranker,
so I would not do this now.

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:416:
return base::MakeUnique<ConstantCategoryRanker>();
On 2016/12/15 16:01:18, Marc Treib wrote:
> No need for a helper function for this; just call MakeUnique directly where
it's
> needed.

Done.

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:844:
SetCategoryRanker(std::unique_ptr<CategoryRanker>(mock_ranker));
On 2016/12/15 16:01:18, Marc Treib wrote:
> This is a bit yucky, ownership-wise. How about:
> auto mock_ranker = base::MakeUnique<MockCategoryRanker>();
> MockCategoryRanker* raw_mock_ranker = mock_ranker.get();
> SetCategoryRanker(std::move(mock_ranker));
> 
> (and then EXPECT on the raw_mock_ranker). That way, there's no owning raw
> pointer around.

Done.

https://codereview.chromium.org/2568033005/diff/140001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:924: auto
mock_ranker = new MockCategoryRanker();
On 2016/12/15 16:01:18, Marc Treib wrote:
> Same here.

Done.

[vitaliii, 2016-12-15 16:54:20]

vitaliii@chromium.org changed reviewers:
+ noyau@chromium.org

[vitaliii, 2016-12-15 16:54:20]

Hi noyau@,

could you please have a look at my change of ios service factory?

[vitaliii, 2016-12-15 16:57:15]

Hi bauerb@,

could you please have a look at my change of
snippets_internals_message_handler.cc?

BTW, shouldn't we add a Munich-based per file owner for
snippets_internals_message_handler.cc?

[vitaliii, 2016-12-15 16:58:13]

vitaliii@chromium.org changed reviewers:
+ bauerb@chromium.org

[vitaliii, 2016-12-15 16:58:14]

Hi bauerb@,

could you please have a look at my change of
snippets_internals_message_handler.cc?

BTW, shouldn't we add a Munich-based per file owner for
snippets_internals_message_handler.cc?

[commit-bot: I haz the power, 2016-12-15 17:48:49]

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

[commit-bot: I haz the power, 2016-12-15 17:48:51]

Dry run: This issue passed the CQ dry run.

[tschumann, 2016-12-15 18:23:13]

lgtm

overall design looks good. some final nits, mostly following up on discussions
about the future direction.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:605:
section_ranker_->AppendCategoryIfNecessary(snippet_category);
On 2016/12/15 15:30:12, vitaliii wrote:
> On 2016/12/15 13:05:20, tschumann wrote:
> > On 2016/12/14 10:24:30, Marc Treib wrote:
> > > On 2016/12/14 08:59:38, vitaliii wrote:
> > > > On 2016/12/13 12:22:42, Marc Treib wrote:
> > > > > Hm. This is one downside of the new design: Before, the Factory (which
> did
> > > the
> > > > > sorting) always knew about all categories. Now, it's possible to
create
> a
> > > > > category but forget to register it. What would the ranker do in that
> case?
> > > > 
> > > > Currently all non-registered categories are in the end.
> > > > Among themselves they are sorted by increasing the category id.
> > > 
> > > Should it be some kind of error if someone tries to compare two categories
> > that
> > > haven't been registered?
> > > I'm a bit worried that now it's super easy to forget to register a
category,
> > > which could lead to all sorts of subtle bugs with the ordering...
> > 
> > We need to clearly handle the cases where a category can enter the system:
> there
> > are 3 places I know of: reading from the database, getting requests from the
> > remote server, and reading categories from prefs.
> > We need to take care of those. IMO, consolidating this in one class might
make
> > sense (categoryregistry?)  which knows about all categories and keeps the
> state
> > persisted.
> 
> There are 2 places actually (the database does not enforce the order).
> Currently in both cases the order is added to CategoryRanker. Before storing
the
> order is obtained from CategoryRanker. Note that dynamic rankers will have to
> store the order themselves.

Well, but when we start-up and load the categories from prefs, we need to tell
the ranker about them, no?
And if we read suggestions from the db, that's also a place where we can
potentially see a new category showing up (it shouldn't happen as we store all
known categories in prefs, but that's another place we need to check).
Maybe we should discuss this in person tomorrow -- that  said, it'a tangent to
this CL.

I might not (yet) convinced that the ranker should be involved in persistence.
Of course, it needs to make it's state available for persistence. But it
certainly should not be responsible for storing categories themselves. And
separating category persistence from persisting ranking data of the categories
seems sub-optimal.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:1322: // The
ranker may not persist the order, thus, it is stored by the provider.
On 2016/12/15 15:30:12, vitaliii wrote:
> On 2016/12/14 10:24:30, Marc Treib wrote:
> > On 2016/12/14 08:59:38, vitaliii wrote:
> > > On 2016/12/13 12:22:42, Marc Treib wrote:
> > > > Hm. Makes sense, but this is quite subtle. Do we have a test for this?
> > > > 
> > > > Or maybe, we could make persistence part of the ranker's contract.
That'd
> > make
> > > > it hard to clean up old categories though.
> > > 
> > > Done.
> > > 
> > > We did not have a test for this (actually, I have got a feeling that
> > previously
> > > it did not work). I added a test.
> > 
> > What exactly do you think didn't work before?
> 
> I thought that remote categories were not registered previously.
> However, I understood my mistake. Previously The registration was done in
> NTPSnippetsFetcher, which is very non obvious place and I missed it.

another reason to consolidate that logic in a more central place.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/category_ranker.h (right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/category_ranker.h:27: virtual void
ClearHistory(base::Time begin, base::Time end) = 0;
On 2016/12/15 15:30:13, vitaliii wrote:
> On 2016/12/15 11:50:26, tschumann wrote:
> > is this actually a necessary requirement? Would be good to double-check  so
> that
> > we don't make our lives unnecessarily hard.
> > Given you already did the work, we can move ahead as is but getting clarity
> > would be nice (maybe you already have ;-)).
> 
> My reasoning for this was that we had to provide such method for suggestion
> providers.
> Overall, this is an internal interface with at most 3 implementations in the
> nearest future, so it should be ok to change it accordingly if anything
changes.

What I actually mean, is are we required to delete the category ordering when
the user clears their history.
If not , then we should even bother about this.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.cc
(right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:34: return
left.id() < right.id();
On 2016/12/15 15:30:13, vitaliii wrote:
> On 2016/12/15 11:50:26, tschumann wrote:
> > that's a bit confusing. What are the cases where left.id() < right.id()
would
> > return something different than the loop above?
> 
> When remote categories are sent in an order not sorted by id.
> 
> > Would be great to document the fallback.
> 
> Marc has noted this, we agreed to require registering all categories
explicitly
> (I have added LOG(DFATAL)), so this fallback is rather to satisfy sort
contract
> (I added a comment explaining this).
>  
> > Ideally, the ranker would know them all... That was actually a nice property
> > with the factory as that would see every instantiated category object and
> could
> > have a more complete view. But I also see that this is speculation for now.
> > Let's see how things go -- worst case we have to redo this exercise :-) 
> 
> While it was a nice property, the registration was implicit (so one could end
up
> with unpredictable order) and remote categories were registered in Fetcher,
> which is not nice. Overall, currently we have only one provider who registers
> remote categories, so this does not seem to be a problem. I also like the new
> explicit order approach, because now if one cares about the order, they can
just
> grab the ranker and define the order, without worrying that it is handled
> differently e.g. in Fetcher.

I totally agree that it's good to get this consolidated.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.h
(right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.h:16: //
TODO(vitaliii): Add unit tests.
On 2016/12/15 15:30:13, vitaliii wrote:
> On 2016/12/15 11:50:26, tschumann wrote:
> > i treat this as a DO_NOT_SUBMIT ;-) Not sure if this works in chromium
> though...
> 
> The idea was to submit this large CL and then immediately proceed with tests.
> However, Marc noticed this and in the end I rewrote former CategoryFactory
tests
> and added new ones.

:-)  I totally got that idea and disagreed ;-). If the CL would have been too
big, then we'd simply split out a CL (say adding the ConstantCategoryRanker +
tests) submit that first and then submit the CL to integrate it.
Never submit new classes without tests -- even with the best intentions, people
rarely get back at "TODO() add tests". That aside, we don't want untested code
to end up in canary and create crashes and bug report noise.

Thanks for adding them !! :-D

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider.cc:606:
category_ranker_->AppendCategoryIfNecessary(snippet_category);
On 2016/12/15 15:30:13, vitaliii wrote:
> On 2016/12/15 11:50:26, tschumann wrote:
> > in google3, a common idiom is to call such methods MaybeAppendCategory(...)
> > 
> > i'm not too happy with the 'ifNecessary' part, maybe 'ifMissing' or 'ifNew'?
> > 
> > Then again, I think this logic needs more work anyways and probably will
> involve
> > further renamings.
> 
> I do not like "Maybe", because then it is not clear when the category is
> actually added.
> 
> As for other suggestions, here are occurrence counts in Chrome:
> - ifNecessary -> 695;
> - ifMissing -> 49;
> - ifNew -> 17;
> 
> So I would prefer to leave it as it is now.

For now it's fine. I'm not sure I agree with the "count in Chrome" approach in
this case. Method names highly depend on context.
"if necessary" conveys as much semantical info as "maybe" because they don't say
anything about the condition under which it is added. To be honest, I don't even
know if it matters.

I'm a bit afraid that we're exposing too many implementation details here. For
example what does "Append" mean here? But let's refactor this once we have the
new ranker and a better understanding of the functionality and contracts.

https://codereview.chromium.org/2568033005/diff/200001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2568033005/diff/200001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:170:
std::string GetTestJson(const std::vector<std::string>& snippets,
that's nice! can you add a TODO() to get rid of these convenience functions as
they don't provide a lot of value any more at the expense of additional
redirections?

https://codereview.chromium.org/2568033005/diff/200001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:854:
InSequence s;
please add a comment explaining what you test. ie. the order in which the
categories get added to the ranker matters as it determines the ordering of
categories.

https://codereview.chromium.org/2568033005/diff/200001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:932:
AppendCategoryIfNecessary(Category::FromRemoteCategory(1)));
do we have a constant we can use for the article category?

[vitaliii, 2016-12-16 08:14:55]

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

[commit-bot: I haz the power, 2016-12-16 08:15:12]

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

[vitaliii, 2016-12-16 08:15:43]

tschumann@, 

I addressed your comments, no need to look.

bauerb@ and noyau@ please have a look.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:605:
section_ranker_->AppendCategoryIfNecessary(snippet_category);
On 2016/12/15 18:23:13, tschumann wrote:
> On 2016/12/15 15:30:12, vitaliii wrote:
> > On 2016/12/15 13:05:20, tschumann wrote:
> > > On 2016/12/14 10:24:30, Marc Treib wrote:
> > > > On 2016/12/14 08:59:38, vitaliii wrote:
> > > > > On 2016/12/13 12:22:42, Marc Treib wrote:
> > > > > > Hm. This is one downside of the new design: Before, the Factory
(which
> > did
> > > > the
> > > > > > sorting) always knew about all categories. Now, it's possible to
> create
> > a
> > > > > > category but forget to register it. What would the ranker do in that
> > case?
> > > > > 
> > > > > Currently all non-registered categories are in the end.
> > > > > Among themselves they are sorted by increasing the category id.
> > > > 
> > > > Should it be some kind of error if someone tries to compare two
categories
> > > that
> > > > haven't been registered?
> > > > I'm a bit worried that now it's super easy to forget to register a
> category,
> > > > which could lead to all sorts of subtle bugs with the ordering...
> > > 
> > > We need to clearly handle the cases where a category can enter the system:
> > there
> > > are 3 places I know of: reading from the database, getting requests from
the
> > > remote server, and reading categories from prefs.
> > > We need to take care of those. IMO, consolidating this in one class might
> make
> > > sense (categoryregistry?)  which knows about all categories and keeps the
> > state
> > > persisted.
> > 
> > There are 2 places actually (the database does not enforce the order).
> > Currently in both cases the order is added to CategoryRanker. Before storing
> the
> > order is obtained from CategoryRanker. Note that dynamic rankers will have
to
> > store the order themselves.
> 
> Well, but when we start-up and load the categories from prefs, we need to tell
> the ranker about them, no?

Of course. We do this already in
RemoteSuggestionsProvider::RestoreCategoriesFromPrefs.

> And if we read suggestions from the db, that's also a place where we can
> potentially see a new category showing up (it shouldn't happen as we store all
> known categories in prefs, but that's another place we need to check).
> Maybe we should discuss this in person tomorrow -- that  said, it'a tangent to
> this CL.

I agree not to consider this question in this CL.
In my view, StoreCategoriesToPrefs() uses |category_contents_| and if something
is missing there, but present in the database, I would consider this a bug.

> I might not (yet) convinced that the ranker should be involved in persistence.

For this constant ranker, this is not clear. However, for the new dynamic
ranker, it must store the order by itself.

> Of course, it needs to make it's state available for persistence. But it
> certainly should not be responsible for storing categories themselves. And
> separating category persistence from persisting ranking data of the categories
> seems sub-optimal.

I am not sure what you mean by storing categories. The ranker cannot return a
category not present in sort request, thus, the choice of categories is up to
clients.

https://codereview.chromium.org/2568033005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:1322: // The
ranker may not persist the order, thus, it is stored by the provider.
On 2016/12/15 18:23:13, tschumann wrote:
> On 2016/12/15 15:30:12, vitaliii wrote:
> > On 2016/12/14 10:24:30, Marc Treib wrote:
> > > On 2016/12/14 08:59:38, vitaliii wrote:
> > > > On 2016/12/13 12:22:42, Marc Treib wrote:
> > > > > Hm. Makes sense, but this is quite subtle. Do we have a test for this?
> > > > > 
> > > > > Or maybe, we could make persistence part of the ranker's contract.
> That'd
> > > make
> > > > > it hard to clean up old categories though.
> > > > 
> > > > Done.
> > > > 
> > > > We did not have a test for this (actually, I have got a feeling that
> > > previously
> > > > it did not work). I added a test.
> > > 
> > > What exactly do you think didn't work before?
> > 
> > I thought that remote categories were not registered previously.
> > However, I understood my mistake. Previously The registration was done in
> > NTPSnippetsFetcher, which is very non obvious place and I missed it.
> 
> another reason to consolidate that logic in a more central place.

Acknowledged.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/category_ranker.h (right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/category_ranker.h:27: virtual void
ClearHistory(base::Time begin, base::Time end) = 0;
On 2016/12/15 18:23:13, tschumann wrote:
> On 2016/12/15 15:30:13, vitaliii wrote:
> > On 2016/12/15 11:50:26, tschumann wrote:
> > > is this actually a necessary requirement? Would be good to double-check 
so
> > that
> > > we don't make our lives unnecessarily hard.
> > > Given you already did the work, we can move ahead as is but getting
clarity
> > > would be nice (maybe you already have ;-)).
> > 
> > My reasoning for this was that we had to provide such method for suggestion
> > providers.
> > Overall, this is an internal interface with at most 3 implementations in the
> > nearest future, so it should be ok to change it accordingly if anything
> changes.
> 
> What I actually mean, is are we required to delete the category ordering when
> the user clears their history.
> If not , then we should even bother about this.

We are not required to do this (yet), however, I feel like we will and,
moreover, I was planning to add this function to snippets_internals. 
I am fine to remove it later if it happens to be not used.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.cc
(right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.cc:34: return
left.id() < right.id();
On 2016/12/15 18:23:13, tschumann wrote:
> On 2016/12/15 15:30:13, vitaliii wrote:
> > On 2016/12/15 11:50:26, tschumann wrote:
> > > that's a bit confusing. What are the cases where left.id() < right.id()
> would
> > > return something different than the loop above?
> > 
> > When remote categories are sent in an order not sorted by id.
> > 
> > > Would be great to document the fallback.
> > 
> > Marc has noted this, we agreed to require registering all categories
> explicitly
> > (I have added LOG(DFATAL)), so this fallback is rather to satisfy sort
> contract
> > (I added a comment explaining this).
> >  
> > > Ideally, the ranker would know them all... That was actually a nice
property
> > > with the factory as that would see every instantiated category object and
> > could
> > > have a more complete view. But I also see that this is speculation for
now.
> > > Let's see how things go -- worst case we have to redo this exercise :-) 
> > 
> > While it was a nice property, the registration was implicit (so one could
end
> up
> > with unpredictable order) and remote categories were registered in Fetcher,
> > which is not nice. Overall, currently we have only one provider who
registers
> > remote categories, so this does not seem to be a problem. I also like the
new
> > explicit order approach, because now if one cares about the order, they can
> just
> > grab the ranker and define the order, without worrying that it is handled
> > differently e.g. in Fetcher.
> 
> I totally agree that it's good to get this consolidated. 

Acknowledged.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/category_rankers/constant_category_ranker.h
(right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/category_rankers/constant_category_ranker.h:16: //
TODO(vitaliii): Add unit tests.
On 2016/12/15 18:23:13, tschumann wrote:
> On 2016/12/15 15:30:13, vitaliii wrote:
> > On 2016/12/15 11:50:26, tschumann wrote:
> > > i treat this as a DO_NOT_SUBMIT ;-) Not sure if this works in chromium
> > though...
> > 
> > The idea was to submit this large CL and then immediately proceed with
tests.
> > However, Marc noticed this and in the end I rewrote former CategoryFactory
> tests
> > and added new ones.
> 
> :-)  I totally got that idea and disagreed ;-). If the CL would have been too
> big, then we'd simply split out a CL (say adding the ConstantCategoryRanker +
> tests) submit that first and then submit the CL to integrate it.
> Never submit new classes without tests -- even with the best intentions,
people
> rarely get back at "TODO() add tests". That aside, we don't want untested code
> to end up in canary and create crashes and bug report noise.
> 
> Thanks for adding them !! :-D

Ack.

Now I see, fair enough.

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):

https://codereview.chromium.org/2568033005/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider.cc:606:
category_ranker_->AppendCategoryIfNecessary(snippet_category);
On 2016/12/15 18:23:13, tschumann wrote:
> On 2016/12/15 15:30:13, vitaliii wrote:
> > On 2016/12/15 11:50:26, tschumann wrote:
> > > in google3, a common idiom is to call such methods
MaybeAppendCategory(...)
> > > 
> > > i'm not too happy with the 'ifNecessary' part, maybe 'ifMissing' or
'ifNew'?
> > > 
> > > Then again, I think this logic needs more work anyways and probably will
> > involve
> > > further renamings.
> > 
> > I do not like "Maybe", because then it is not clear when the category is
> > actually added.
> > 
> > As for other suggestions, here are occurrence counts in Chrome:
> > - ifNecessary -> 695;
> > - ifMissing -> 49;
> > - ifNew -> 17;
> > 
> > So I would prefer to leave it as it is now.
> 
> For now it's fine. I'm not sure I agree with the "count in Chrome" approach in
> this case. Method names highly depend on context.
> "if necessary" conveys as much semantical info as "maybe" because they don't
say
> anything about the condition under which it is added. To be honest, I don't
even
> know if it matters.

Ack.

> I'm a bit afraid that we're exposing too many implementation details here. For
> example what does "Append" mean here? But let's refactor this once we have the
> new ranker and a better understanding of the functionality and contracts.

"Append" means that the category is added to the end. Both "Append" and
appending to the end were intended. I am happy to refactor this once we are more
certain regarding what we need.

https://codereview.chromium.org/2568033005/diff/200001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2568033005/diff/200001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:170:
std::string GetTestJson(const std::vector<std::string>& snippets,
On 2016/12/15 18:23:13, tschumann wrote:
> that's nice! can you add a TODO() to get rid of these convenience functions as
> they don't provide a lot of value any more at the expense of additional
> redirections?

Done.

https://codereview.chromium.org/2568033005/diff/200001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:854:
InSequence s;
On 2016/12/15 18:23:13, tschumann wrote:
> please add a comment explaining what you test. ie. the order in which the
> categories get added to the ranker matters as it determines the ordering of
> categories.

Done.

https://codereview.chromium.org/2568033005/diff/200001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:932:
AppendCategoryIfNecessary(Category::FromRemoteCategory(1)));
On 2016/12/15 18:23:13, tschumann wrote:
> do we have a constant we can use for the article category?

Done.

[commit-bot: I haz the power, 2016-12-16 09:16:49]

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

[commit-bot: I haz the power, 2016-12-16 09:16:50]

Dry run: This issue passed the CQ dry run.

[noyau (Ping after 24h), 2016-12-16 12:17:46]

ios lgtm

[vitaliii, 2016-12-16 15:45:17]

Patchset #9 (id:260001) has been deleted

[vitaliii, 2016-12-19 08:06:28]

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

[commit-bot: I haz the power, 2016-12-19 08:06:42]

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

[Bernhard Bauer, 2016-12-19 10:12:19]

WebUI LGTM.

If you want to add OWNERS for the snippets_internals UI, feel free to (I would
simultaneously do chrome/browser/resources/snippets_internals.* and
snippets_internals_message_handler.cc though).

[vitaliii, 2016-12-19 10:13:46]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2016-12-19 10:13:47]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
tschumann@chromium.org, noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/2568033005/#ps280001
(title: "rebase.")

[commit-bot: I haz the power, 2016-12-19 10:14:02]

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-12-19 11:13:28]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1482142426535230,
"parent_rev": "1ad256e12bd6a14efb2b7dc9335078e22ddca0e6", "commit_rev":
"600adeb559cfb62c20a8027410dbe27d460dba57"}

[commit-bot: I haz the power, 2016-12-19 11:13:56]

Description was changed from

==========
[NTP::SectionOrder] Replace CategoryFactory with a category ranker.

This CL:
1) Adds CategoryRanker interface;
2) Adds a default constant category ranker implementation (based on
   CategoryFactory);
3) Replaces CategoryFactory with a new ranker;
4) Factory methods are moved to Category.

This is the initial step of moving to personalised section order.

BUG=673659
==========

to

==========
[NTP::SectionOrder] Replace CategoryFactory with a category ranker.

This CL:
1) Adds CategoryRanker interface;
2) Adds a default constant category ranker implementation (based on
   CategoryFactory);
3) Replaces CategoryFactory with a new ranker;
4) Factory methods are moved to Category.

This is the initial step of moving to personalised section order.

BUG=673659

Review-Url: https://codereview.chromium.org/2568033005
==========

[commit-bot: I haz the power, 2016-12-19 11:13:57]

Committed patchset #9 (id:280001)

[commit-bot: I haz the power, 2016-12-19 11:15:40]

Description was changed from

==========
[NTP::SectionOrder] Replace CategoryFactory with a category ranker.

This CL:
1) Adds CategoryRanker interface;
2) Adds a default constant category ranker implementation (based on
   CategoryFactory);
3) Replaces CategoryFactory with a new ranker;
4) Factory methods are moved to Category.

This is the initial step of moving to personalised section order.

BUG=673659

Review-Url: https://codereview.chromium.org/2568033005
==========

to

==========
[NTP::SectionOrder] Replace CategoryFactory with a category ranker.

This CL:
1) Adds CategoryRanker interface;
2) Adds a default constant category ranker implementation (based on
   CategoryFactory);
3) Replaces CategoryFactory with a new ranker;
4) Factory methods are moved to Category.

This is the initial step of moving to personalised section order.

BUG=673659

Committed: https://crrev.com/7456f5ae0859872c69398b583d6331ed47d70586
Cr-Commit-Position: refs/heads/master@{#439451}
==========

[commit-bot: I haz the power, 2016-12-19 11:15:41]

Patchset 9 (id:??) landed as
https://crrev.com/7456f5ae0859872c69398b583d6331ed47d70586
Cr-Commit-Position: refs/heads/master@{#439451}