Preset variation parameters for NTPSnippets feature.

Preset variation parameters for NTPSnippets feature.

The CL uses the new FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE type. It gives users more options for the NTPSnippets feature to choose from in chrome://flags.

BUG=606320
Committed: https://crrev.com/b050f1526d5016e5e70d72682f4bb64b5ec2d50f
Cr-Commit-Position: refs/heads/master@{#401020}

[jkrcal, 2016-06-17 12:10:05]

Description was changed from

==========
Preset variation parameters for NTPSnippets feature.

Uses the new FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE type.

BUG=606320
==========

to

==========
Preset variation parameters for NTPSnippets feature.

The CL uses the new FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE type. It gives
users more options for the NTPSnippets feature to choose from in chrome://flags.

BUG=606320
==========

[jkrcal, 2016-06-17 12:10:18]

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

[jkrcal, 2016-06-17 12:10:19]

PTAL

[Marc Treib, 2016-06-17 12:34:13]

https://codereview.chromium.org/2041783002/diff/80001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2041783002/diff/80001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:524: const FeatureEntry::FeatureParam
kNTPSnippetsFeatureVariationOnlyTopSites[] = {
I find "TopSites" misleading here - we don't even use TopSites so far. I'd just
call this somewith with "HostRestrict" probably.

https://codereview.chromium.org/2041783002/diff/80001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:530: {"fetching_personalization", "non_personal"},
This seems wrong; should probably be "personal"?

[jkrcal, 2016-06-17 13:18:52]

Thanks! PTAL, again.

https://codereview.chromium.org/2041783002/diff/80001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2041783002/diff/80001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:524: const FeatureEntry::FeatureParam
kNTPSnippetsFeatureVariationOnlyTopSites[] = {
On 2016/06/17 12:34:13, Marc Treib wrote:
> I find "TopSites" misleading here - we don't even use TopSites so far. I'd
just
> call this somewith with "HostRestrict" probably.

Done.

https://codereview.chromium.org/2041783002/diff/80001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:530: {"fetching_personalization", "non_personal"},
On 2016/06/17 12:34:13, Marc Treib wrote:
> This seems wrong; should probably be "personal"?

Done.
(Huh, I had yet another mistake there compared to the specification in
crbug.com/606320.)

[Marc Treib, 2016-06-17 13:43:54]

https://codereview.chromium.org/2041783002/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2041783002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags.cc:518: {"fetching_host_restrict", "off"},
Hm, the "Default" variant should probably not list any parameters? We already
have default values in the code for when the params are unset. Let's try to keep
the number of (potentially) different "defaults" to a minimum :)

https://codereview.chromium.org/2041783002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags.cc:536: {"", kNTPSnippetsFeatureVariationDefault, 2},
The "2"s here are for the number of parameters? Setting that explicitly seems
error-prone... but I don't see a good way of avoiding it.

[jkrcal, 2016-06-17 14:13:43]

PTAL, again.

https://codereview.chromium.org/2041783002/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2041783002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags.cc:518: {"fetching_host_restrict", "off"},
On 2016/06/17 13:43:54, Marc Treib wrote:
> Hm, the "Default" variant should probably not list any parameters? We already
> have default values in the code for when the params are unset. Let's try to
keep
> the number of (potentially) different "defaults" to a minimum :)

Good point, done.
(I had actually to change the defaults in the code to match what was here.)

https://codereview.chromium.org/2041783002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags.cc:536: {"", kNTPSnippetsFeatureVariationDefault, 2},
On 2016/06/17 13:43:54, Marc Treib wrote:
> The "2"s here are for the number of parameters? Setting that explicitly seems
> error-prone... but I don't see a good way of avoiding it.

I do not see it either as it is all statically defined.
(On the other hand, I am far from being a c++ guru, so feel free to prove me
wrong! :)

[Marc Treib, 2016-06-17 15:14:50]

LGTM, thanks!

https://codereview.chromium.org/2041783002/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2041783002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags.cc:536: {"", kNTPSnippetsFeatureVariationDefault, 2},
On 2016/06/17 14:13:43, jkrcal wrote:
> On 2016/06/17 13:43:54, Marc Treib wrote:
> > The "2"s here are for the number of parameters? Setting that explicitly
seems
> > error-prone... but I don't see a good way of avoiding it.
> 
> I do not see it either as it is all statically defined.
> (On the other hand, I am far from being a c++ guru, so feel free to prove me
> wrong! :)

Well, there's an arraysize() macro (in base/macros.h) which might be a bit nicer
than explicitly stating the number. It's not perfect because then you have to
spell the name twice.
Alternatively, it might be possible to do something with std::array, but a) I'm
not familiar enough with that to say for sure, and b) I'm not even sure it'd be
worth the effort.
So I'd probably just go with arraysize. Ultimately your call though; anyway it
should be pretty easy to diagnose if you specify a wrong number.

[jkrcal, 2016-06-17 23:18:43]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-06-17 23:18:44]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2041783002/#ps140001
(title: "After code review #3")

[commit-bot: I haz the power, 2016-06-17 23:19:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041783002/140001

[commit-bot: I haz the power, 2016-06-17 23:21:06]

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

[commit-bot: I haz the power, 2016-06-17 23:21:07]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jkrcal, 2016-06-18 07:09:07]

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

[commit-bot: I haz the power, 2016-06-18 07:09:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041783002/180001

[commit-bot: I haz the power, 2016-06-18 07:21:04]

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

[commit-bot: I haz the power, 2016-06-18 07:21:05]

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

[jkrcal, 2016-06-20 14:42:05]

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

[commit-bot: I haz the power, 2016-06-20 14:42:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041783002/200001

[commit-bot: I haz the power, 2016-06-20 15:01:56]

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

[commit-bot: I haz the power, 2016-06-20 15:01:58]

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

[jkrcal, 2016-06-21 15:56:37]

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

[commit-bot: I haz the power, 2016-06-21 15:56:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041783002/220001

[commit-bot: I haz the power, 2016-06-21 16:37:00]

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

[commit-bot: I haz the power, 2016-06-21 16:37:01]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-06-21 17:07:29]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-06-21 17:07:30]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2041783002/#ps220001
(title: "Removing unittests that test non-default behaviour")

[commit-bot: I haz the power, 2016-06-21 17:07:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041783002/220001

[commit-bot: I haz the power, 2016-06-21 17:12:29]

Description was changed from

==========
Preset variation parameters for NTPSnippets feature.

The CL uses the new FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE type. It gives
users more options for the NTPSnippets feature to choose from in chrome://flags.

BUG=606320
==========

to

==========
Preset variation parameters for NTPSnippets feature.

The CL uses the new FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE type. It gives
users more options for the NTPSnippets feature to choose from in chrome://flags.

BUG=606320
==========

[commit-bot: I haz the power, 2016-06-21 17:12:30]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-06-21 17:15:14]

Description was changed from

==========
Preset variation parameters for NTPSnippets feature.

The CL uses the new FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE type. It gives
users more options for the NTPSnippets feature to choose from in chrome://flags.

BUG=606320
==========

to

==========
Preset variation parameters for NTPSnippets feature.

The CL uses the new FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE type. It gives
users more options for the NTPSnippets feature to choose from in chrome://flags.

BUG=606320

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

[commit-bot: I haz the power, 2016-06-21 17:15:15]

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

[jkrcal, 2016-06-21 17:25:56]

Marc, FYI I changed it to the arraysize macro.
I also removed two unittests for the fetcher that relied on host_restriction
being "on". I will put the unittests back once we have code to set the
parameters values in unittests.

[Marc Treib, 2016-06-21 18:59:35]

On 2016/06/21 17:25:56, jkrcal wrote:
> Marc, FYI I changed it to the arraysize macro.
> I also removed two unittests for the fetcher that relied on host_restriction
> being "on". I will put the unittests back once we have code to set the
> parameters values in unittests.

From Chris' reply on the mail thread, it seems there is already some way to
force variation params in tests?

[jkrcal, 2016-06-23 08:23:17]

On 2016/06/21 18:59:35, Marc Treib wrote:
> On 2016/06/21 17:25:56, jkrcal wrote:
> > Marc, FYI I changed it to the arraysize macro.
> > I also removed two unittests for the fetcher that relied on host_restriction
> > being "on". I will put the unittests back once we have code to set the
> > parameters values in unittests.
> 
> From Chris' reply on the mail thread, it seems there is already some way to
> force variation params in tests?

I wanted to wait until the CL of Chris lands.
I'll put the unittests back early next week.