[NTP::SectionOrder] Add a flag to choose category ranker.

[NTP::SectionOrder] Add a flag to choose category ranker.

This CL adds a flag to choose a category ranker.
The default is ConstantCategoryRanker.

I had to add FEATURE_WITH_VARIATIONS_VALUE_TYPE (but not
MULTI_VALUE_TYPE), because the latter presumably cannot be controled via
variations service.

BUG=675946
Committed: https://crrev.com/a64fb41f45ac025608887c192ecdddf1be477846
Cr-Commit-Position: refs/heads/master@{#441332}

[vitaliii, 2016-12-21 14:34:47]

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

[commit-bot: I haz the power, 2016-12-21 14:35:00]

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-21 14:36:56]

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

[commit-bot: I haz the power, 2016-12-21 14:36: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...)
  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-21 14:37:56]

Patchset #1 (id:1) has been deleted

[vitaliii, 2016-12-22 08:27:40]

Patchset #2 (id:40001) has been deleted

[vitaliii, 2016-12-22 08:27:53]

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

[commit-bot: I haz the power, 2016-12-22 08:28:06]

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-22 08:28:57]

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

[vitaliii, 2016-12-22 08:28:57]

Hi jkrcal@,

PTAL.

https://codereview.chromium.org/2595883002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/2595883002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13952: -      </message>
Trailing whitespaces were automatically removed here.

[commit-bot: I haz the power, 2016-12-22 10:00:22]

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

[commit-bot: I haz the power, 2016-12-22 10:00:23]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jkrcal, 2016-12-22 10:06:01]

Generally, I am not happy about having the param and value names as literals on
two places.

As I anyway ask you to do the same for the iOS factory, it would be on three
places. 

We have no solution for variation params so far. My suggestion: 
 - add to features.h
     - an enum {CONSTANT, CLICK_BASED}
     - a function that reads the param and returns the enum.
     - the constant name of the parameter and of the click based value
 - refer to these constants from about_flags
 - use the function and a switch in the factories

This way, whenever you add a new ranker, you need to change the enum in
features.h and the compiler reminds you to update both factories (or any other
places) The compiler will not remind you to update about_flags but this is not a
big deal (and maybe not always desired).

If we remove the parameter completely, the compiler will also remind us to
remove it from about_flags.

WDYT?

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:5: #include
"chrome/browser/ntp_snippets/content_suggestions_service_factory.h"
Can you the same changes for the iOS factory please?

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:208:
std::string category_ranker_parameter = variations::GetVariationParamValue(
Can you use variations::GetVariationParamValueByFeatureName?

We try to move reading all variation params to this way as it allows us to split
the variation configs into multiple files (multiple studies).

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:215: if
(category_ranker_parameter == "constant") {
Do we need this option? I would have just 
 - Default    = constant, 
 - Enabled    = constant, 
 - Enabled (click based), 
 - Disabled    = constant, bit unfortunately

[tschumann, 2016-12-22 10:31:25]

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

[tschumann, 2016-12-22 10:31:26]

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:215: if
(category_ranker_parameter == "constant") {
On 2016/12/22 10:06:01, jkrcal wrote:
> Do we need this option? I would have just 
>  - Default    = constant, 
>  - Enabled    = constant, 
>  - Enabled (click based), 
>  - Disabled    = constant, bit unfortunately

i like the idea of using the strings in less places and an enum instead. in that
case, the default semantics would also live in a single place (where the strings
get turned into the enum) and here, we could have a simple switch over the value
and no need to interpret defaults etc.

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

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

[commit-bot: I haz the power, 2016-12-22 13:02:13]

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-22 13:11:37]

I addressed your comments, 

please have a look.

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:5: #include
"chrome/browser/ntp_snippets/content_suggestions_service_factory.h"
On 2016/12/22 10:06:01, jkrcal wrote:
> Can you the same changes for the iOS factory please?

Done.

However, I cannot prevent duplication of the function from enum to the ranker
itself.

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:208:
std::string category_ranker_parameter = variations::GetVariationParamValue(
On 2016/12/22 10:06:01, jkrcal wrote:
> Can you use variations::GetVariationParamValueByFeatureName?
> 
> We try to move reading all variation params to this way as it allows us to
split
> the variation configs into multiple files (multiple studies).

Done.

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:215: if
(category_ranker_parameter == "constant") {
On 2016/12/22 10:06:01, jkrcal wrote:
> Do we need this option? I would have just 
>  - Default    = constant, 
>  - Enabled    = constant, 
>  - Enabled (click based), 
>  - Disabled    = constant, bit unfortunately

I think - yes.

1) I treat all of "Default", "Enabled" and "Disabled" as default ranker, so it
is nice to have an option to choose constant ranker once default is changed to
something else.
2) If one is not sure which ranker is considered default, one can explicitly
force constant ranker if needed.

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:215: if
(category_ranker_parameter == "constant") {
On 2016/12/22 10:31:26, tschumann wrote:
> On 2016/12/22 10:06:01, jkrcal wrote:
> > Do we need this option? I would have just 
> >  - Default    = constant, 
> >  - Enabled    = constant, 
> >  - Enabled (click based), 
> >  - Disabled    = constant, bit unfortunately
> 
> i like the idea of using the strings in less places and an enum instead. in
that
> case, the default semantics would also live in a single place (where the
strings
> get turned into the enum) and here, we could have a simple switch over the
value
> and no need to interpret defaults etc.

Done.

[vitaliii, 2016-12-22 13:11:37]

I addressed your comments, 

please have a look.

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:5: #include
"chrome/browser/ntp_snippets/content_suggestions_service_factory.h"
On 2016/12/22 10:06:01, jkrcal wrote:
> Can you the same changes for the iOS factory please?

Done.

However, I cannot prevent duplication of the function from enum to the ranker
itself.

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:208:
std::string category_ranker_parameter = variations::GetVariationParamValue(
On 2016/12/22 10:06:01, jkrcal wrote:
> Can you use variations::GetVariationParamValueByFeatureName?
> 
> We try to move reading all variation params to this way as it allows us to
split
> the variation configs into multiple files (multiple studies).

Done.

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:215: if
(category_ranker_parameter == "constant") {
On 2016/12/22 10:06:01, jkrcal wrote:
> Do we need this option? I would have just 
>  - Default    = constant, 
>  - Enabled    = constant, 
>  - Enabled (click based), 
>  - Disabled    = constant, bit unfortunately

I think - yes.

1) I treat all of "Default", "Enabled" and "Disabled" as default ranker, so it
is nice to have an option to choose constant ranker once default is changed to
something else.
2) If one is not sure which ranker is considered default, one can explicitly
force constant ranker if needed.

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:215: if
(category_ranker_parameter == "constant") {
On 2016/12/22 10:31:26, tschumann wrote:
> On 2016/12/22 10:06:01, jkrcal wrote:
> > Do we need this option? I would have just 
> >  - Default    = constant, 
> >  - Enabled    = constant, 
> >  - Enabled (click based), 
> >  - Disabled    = constant, bit unfortunately
> 
> i like the idea of using the strings in less places and an enum instead. in
that
> case, the default semantics would also live in a single place (where the
strings
> get turned into the enum) and here, we could have a simple switch over the
value
> and no need to interpret defaults etc.

Done.

[jkrcal, 2016-12-22 13:39:16]

lgtm with a nit

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:5: #include
"chrome/browser/ntp_snippets/content_suggestions_service_factory.h"
On 2016/12/22 13:11:37, vitaliii wrote:
> On 2016/12/22 10:06:01, jkrcal wrote:
> > Can you the same changes for the iOS factory please?
> 
> Done.
> 
> However, I cannot prevent duplication of the function from enum to the ranker
> itself.

That's okay, thanks!

https://codereview.chromium.org/2595883002/diff/60001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:215: if
(category_ranker_parameter == "constant") {
On 2016/12/22 13:11:37, vitaliii wrote:
> On 2016/12/22 10:31:26, tschumann wrote:
> > On 2016/12/22 10:06:01, jkrcal wrote:
> > > Do we need this option? I would have just 
> > >  - Default    = constant, 
> > >  - Enabled    = constant, 
> > >  - Enabled (click based), 
> > >  - Disabled    = constant, bit unfortunately
> > 
> > i like the idea of using the strings in less places and an enum instead. in
> that
> > case, the default semantics would also live in a single place (where the
> strings
> > get turned into the enum) and here, we could have a simple switch over the
> value
> > and no need to interpret defaults etc.
> 
> Done.

Makes sense. Ack.

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

https://codereview.chromium.org/2595883002/diff/80001/components/ntp_snippets...
components/ntp_snippets/features.h:47: const char kCategoryRankerParameter[] =
"category_ranker";
can you make these extern as well and define them in the .cc file?

[vitaliii, 2016-12-22 13:47:11]

I addressed your nit, no need to look.

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

https://codereview.chromium.org/2595883002/diff/80001/components/ntp_snippets...
components/ntp_snippets/features.h:47: const char kCategoryRankerParameter[] =
"category_ranker";
On 2016/12/22 13:39:15, jkrcal wrote:
> can you make these extern as well and define them in the .cc file?

Done.

[vitaliii, 2016-12-22 13:47:17]

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

[commit-bot: I haz the power, 2016-12-22 13:47:28]

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-22 13:49:10]

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

[vitaliii, 2016-12-22 13:49:11]

Hi noyau@,

Could you have a look at my changes of
ios_chrome_content_suggestions_service_factory.cc?

[vitaliii, 2016-12-22 13:53:36]

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

[vitaliii, 2016-12-22 13:53:36]

Hi jwd@,

could you have a look at my histograms.xml change?
I added a new about_flags flag based on FEATURE_WITH_VARIATIONS_VALUE_TYPE, so I
had to 3 new values to the histograms.xml.

[noyau (Ping after 24h), 2016-12-22 14:06:52]

The flags should be declared on ios as well, to be possibly changed via finch on
this platform. 

See ios/chrome/browser/about_flags.mm

https://codereview.chromium.org/2595883002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2595883002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15138: +        NTP category ranker.
This flag controls the category ranker for the categories of the suggestion
service. I would avoid the use of NTP here, as NTP is only one of the surface
this data may be published onto.

https://codereview.chromium.org/2595883002/diff/100001/ios/chrome/browser/ntp...
File
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/2595883002/diff/100001/ios/chrome/browser/ntp...
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:80:
PrefService* pref_service) {
Instead of duplicating this code in chrome/ and ios/chrome can you move it in
components instead?

[noyau (Ping after 24h), 2016-12-22 14:09:51]

On 2016/12/22 14:06:52, noyau wrote:
> [...] 
>
https://codereview.chromium.org/2595883002/diff/100001/ios/chrome/browser/ntp...
> File
>
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2595883002/diff/100001/ios/chrome/browser/ntp...
>
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:80:
> PrefService* pref_service) {
> Instead of duplicating this code in chrome/ and ios/chrome can you move it in
> components instead?
>
I see this was requested to be that way. I'm not sure I understand why?

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

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

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

Dry run: This issue passed the CQ dry run.

[vitaliii, 2016-12-23 11:00:32]

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

[commit-bot: I haz the power, 2016-12-23 11:00: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-23 11:06:45]

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

[commit-bot: I haz the power, 2016-12-23 11:06:49]

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...)

[vitaliii, 2016-12-23 12:03:12]

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

[commit-bot: I haz the power, 2016-12-23 12:03:20]

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

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

[commit-bot: I haz the power, 2016-12-23 12:11:29]

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

[vitaliii, 2016-12-23 12:17:48]

Patchset #6 (id:140001) has been deleted

[vitaliii, 2016-12-23 12:17:55]

Patchset #5 (id:120001) has been deleted

[vitaliii, 2016-12-23 12:48:42]

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

[commit-bot: I haz the power, 2016-12-23 12:48: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-23 13:12:51]

Hi noyau@,

I addressed your comments
(except iOS flag, which is going to be done after M57 FF, the bug is
crbug.com/676806).

https://codereview.chromium.org/2595883002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2595883002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15138: +        NTP category ranker.
On 2016/12/22 14:06:52, noyau wrote:
> This flag controls the category ranker for the categories of the suggestion
> service. I would avoid the use of NTP here, as NTP is only one of the surface
> this data may be published onto.

Done.

I changed public facing bit, but left NTP prefix internally (e.g. in message
names).

https://codereview.chromium.org/2595883002/diff/100001/ios/chrome/browser/ntp...
File
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/2595883002/diff/100001/ios/chrome/browser/ntp...
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:80:
PrefService* pref_service) {
On 2016/12/22 14:06:52, noyau wrote:
> Instead of duplicating this code in chrome/ and ios/chrome can you move it in
> components instead?
> 

Done.

[noyau (Ping after 24h), 2016-12-23 13:26:17]

https://codereview.chromium.org/2595883002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2595883002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15138: +        NTP category ranker.
On 2016/12/23 13:12:51, vitaliii wrote:
> On 2016/12/22 14:06:52, noyau wrote:
> > This flag controls the category ranker for the categories of the suggestion
> > service. I would avoid the use of NTP here, as NTP is only one of the
surface
> > this data may be published onto.
> 
> Done.
> 
> I changed public facing bit, but left NTP prefix internally (e.g. in message
> names).

I'm of the opinion that this code is not related to the NTP. It controls the
client back end, and this data may be presented on other places unrelated to the
NTP in the future. As such NTP is the wrong term to use here, and this prefix
should be removed.

IDS_FLAGS_SNIPPETS_CATEGORY_... is a better name.

[tschumann, 2016-12-23 13:27:02]

On 2016/12/23 13:12:51, vitaliii wrote:
> Hi noyau@,
> 
> I addressed your comments
> (except iOS flag, which is going to be done after M57 FF, the bug is
> crbug.com/676806).
> 
>
https://codereview.chromium.org/2595883002/diff/100001/chrome/app/generated_r...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/2595883002/diff/100001/chrome/app/generated_r...
> chrome/app/generated_resources.grd:15138: +        NTP category ranker.
> On 2016/12/22 14:06:52, noyau wrote:
> > This flag controls the category ranker for the categories of the suggestion
> > service. I would avoid the use of NTP here, as NTP is only one of the
surface
> > this data may be published onto.
> 
> Done.
> 
> I changed public facing bit, but left NTP prefix internally (e.g. in message
> names).
> 
>
https://codereview.chromium.org/2595883002/diff/100001/ios/chrome/browser/ntp...
> File
>
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2595883002/diff/100001/ios/chrome/browser/ntp...
>
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:80:
> PrefService* pref_service) {
> On 2016/12/22 14:06:52, noyau wrote:
> > Instead of duplicating this code in chrome/ and ios/chrome can you move it
in
> > components instead?
> > 
> 
> Done.

+1 for keeping the iOS related changes for another CL. Given that no set-up is
running that code, it's rather speculative right now. I'd rather do that wiring
when we actively work on the launch and properly test the integration. Right now
M57 on Android is the highest prio.

[tschumann, 2016-12-23 13:46:20]

https://codereview.chromium.org/2595883002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2595883002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15138: +        NTP category ranker.
On 2016/12/23 13:26:17, noyau wrote:
> On 2016/12/23 13:12:51, vitaliii wrote:
> > On 2016/12/22 14:06:52, noyau wrote:
> > > This flag controls the category ranker for the categories of the
suggestion
> > > service. I would avoid the use of NTP here, as NTP is only one of the
> surface
> > > this data may be published onto.
> > 
> > Done.
> > 
> > I changed public facing bit, but left NTP prefix internally (e.g. in message
> > names).
> 
> I'm of the opinion that this code is not related to the NTP. It controls the
> client back end, and this data may be presented on other places unrelated to
the
> NTP in the future. As such NTP is the wrong term to use here, and this prefix
> should be removed.
> 
> IDS_FLAGS_SNIPPETS_CATEGORY_... is a better name.

I agree with Eric (thanks for pointing it out! we tend to get blind to
NTP-related names).
Let's not use 'NTP' anymore for things which are not strictly tied to the NTP.

We deprecated the term 'SNIPPETS' though, as it's confusing with Google's idea
of a snippet. 
So IDS_FLAGS_SUGGESTIONS_CATEGORY_RANKER would be fine with me. (IMO, there's no
need for the 'name' suffix).

We should pay more attention on such names going forward.

[commit-bot: I haz the power, 2016-12-23 14:38:40]

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

[commit-bot: I haz the power, 2016-12-23 14:38:40]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-01-02 08:08:49]

Hi jkrcal@,

what do you think about "IDS_FLAGS_SUGGESTIONS_CATEGORY_RANKER"?

[jkrcal, 2017-01-02 09:01:37]

On 2017/01/02 08:08:49, vitaliii wrote:
> Hi jkrcal@,
> 
> what do you think about "IDS_FLAGS_SUGGESTIONS_CATEGORY_RANKER"?

To me, IDS_FLAGS_SUGGESTIONS_CATEGORY_RANKER is better than SNIPPETS because the
latter is a deprecated notion.
Does length matter here? What about
IDS_FLAGS_CONTENT_SUGGESTIONS_CATEGORY_RANKER to be very consistent with the
naming in the c++ code?

[vitaliii, 2017-01-02 12:44:04]

Hello noyau@,

I renamed the flag with "ContentSuggestions" prefix.
Could you have a look?

[noyau (Ping after 24h), 2017-01-02 12:57:05]

lgtm

[vitaliii, 2017-01-02 13:40:41]

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

[commit-bot: I haz the power, 2017-01-02 13:40:50]

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

[vitaliii, 2017-01-02 13:41:06]

Hi jwd@,

please have a look at my histograms.xml change.

[commit-bot: I haz the power, 2017-01-02 14:47:22]

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

[commit-bot: I haz the power, 2017-01-02 14:47:24]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-01-03 07:28:52]

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

[vitaliii, 2017-01-03 07:28:53]

Hi rkaplow@,

Unfortunately, I cannot reach jwd@,
could you please take a look at my histograms.xml change?
I just added a new flag to about_flags.cc and, therefore, corresponding values
to histograms.

[jwd, 2017-01-03 20:59:33]

histograms LGTM

[vitaliii, 2017-01-04 06:38:31]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-01-04 06:38:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org,
noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/2595883002/#ps220001
(title: "histograms.xml.")

[vitaliii, 2017-01-04 06:38:42]

The CQ bit was unchecked by vitaliii@chromium.org

[commit-bot: I haz the power, 2017-01-04 06:38:48]

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

[vitaliii, 2017-01-04 06:42:30]

The CQ bit was checked by vitaliii@chromium.org

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

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

[commit-bot: I haz the power, 2017-01-04 07:40:33]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1483512150860110,
"parent_rev": "3ba33c87bbd328fe1f2414ac8f6600348fb8efbb", "commit_rev":
"6a94fba04ddf0c954c4080c63436f76123ec3986"}

[commit-bot: I haz the power, 2017-01-04 07:41:05]

Description was changed from

==========
[NTP::SectionOrder] Add a flag to choose category ranker.

This CL adds a flag to choose a category ranker.
The default is ConstantCategoryRanker.

I had to add FEATURE_WITH_VARIATIONS_VALUE_TYPE (but not
MULTI_VALUE_TYPE), because the latter presumably cannot be controled via
variations service.

BUG=675946
==========

to

==========
[NTP::SectionOrder] Add a flag to choose category ranker.

This CL adds a flag to choose a category ranker.
The default is ConstantCategoryRanker.

I had to add FEATURE_WITH_VARIATIONS_VALUE_TYPE (but not
MULTI_VALUE_TYPE), because the latter presumably cannot be controled via
variations service.

BUG=675946

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

[commit-bot: I haz the power, 2017-01-04 07:41:07]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2017-01-04 07:45:16]

Description was changed from

==========
[NTP::SectionOrder] Add a flag to choose category ranker.

This CL adds a flag to choose a category ranker.
The default is ConstantCategoryRanker.

I had to add FEATURE_WITH_VARIATIONS_VALUE_TYPE (but not
MULTI_VALUE_TYPE), because the latter presumably cannot be controled via
variations service.

BUG=675946

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

to

==========
[NTP::SectionOrder] Add a flag to choose category ranker.

This CL adds a flag to choose a category ranker.
The default is ConstantCategoryRanker.

I had to add FEATURE_WITH_VARIATIONS_VALUE_TYPE (but not
MULTI_VALUE_TYPE), because the latter presumably cannot be controled via
variations service.

BUG=675946

Committed: https://crrev.com/a64fb41f45ac025608887c192ecdddf1be477846
Cr-Commit-Position: refs/heads/master@{#441332}
==========

[commit-bot: I haz the power, 2017-01-04 07:45:17]

Patchset 8 (id:??) landed as
https://crrev.com/a64fb41f45ac025608887c192ecdddf1be477846
Cr-Commit-Position: refs/heads/master@{#441332}