Extend VariationParamsManager to support feature associations.

Extend VariationParamsManager to support feature associations.

Previously, feature associations for variation parameters were not easy
to register in unit-tests. This CL extends VariationParamsManager to do
it automatically.

BUG=645447
Committed: https://crrev.com/ce21e97f5e7a87e3cef9459dd578279e24f553d8
Cr-Commit-Position: refs/heads/master@{#436433}

[jkrcal, 2016-09-09 13:03:52]

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

[jkrcal, 2016-09-09 13:03:52]

Alexei, PTAL at variations_associated_data.*

Marc, PTAL at the ntp_snippets stuff. This is just a preparation CL to be able
to read variation params in  ntp_snippets_fetcher by feature name and to be able
to change them in the unit-test.

[Alexei Svitkine (slow), 2016-09-09 18:33:02]

https://codereview.chromium.org/2321273003/diff/1/components/variations/varia...
File components/variations/variations_associated_data.cc (right):

https://codereview.chromium.org/2321273003/diff/1/components/variations/varia...
components/variations/variations_associated_data.cc:301:
base::FeatureList::ClearInstanceForTesting();
Clearing the instance is a surprising effect, ideally it should restore it to
what it was before.

This is already done by ScopedFeatureList here, for example:

https://cs.chromium.org/chromium/src/base/test/scoped_feature_list.h

Maybe this class should have that as a member?

Or perhaps that class should be extended with this functionality?

[Marc Treib, 2016-09-12 09:27:57]

ntp_snippets lgtm

[jkrcal, 2016-09-12 15:39:18]

Alexei, thanks for the comment. PTAL, again!

https://codereview.chromium.org/2321273003/diff/1/components/variations/varia...
File components/variations/variations_associated_data.cc (right):

https://codereview.chromium.org/2321273003/diff/1/components/variations/varia...
components/variations/variations_associated_data.cc:301:
base::FeatureList::ClearInstanceForTesting();
On 2016/09/09 18:33:02, Alexei Svitkine (very slow) wrote:
> Clearing the instance is a surprising effect, ideally it should restore it to
> what it was before.
> 
> This is already done by ScopedFeatureList here, for example:
> 
> https://cs.chromium.org/chromium/src/base/test/scoped_feature_list.h
> 
> Maybe this class should have that as a member?
> 
> Or perhaps that class should be extended with this functionality?

Makes sense, thanks! I applied the ScopedFeatureList, which is in a test-only
build target. So, I've split the VariationParamManager into a different
test-only file.

[Alexei Svitkine (slow), 2016-09-12 16:54:14]

lgtm

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
File components/variations/variations_params_manager.cc (right):

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
components/variations/variations_params_manager.cc:15: 
Nit: Remove empty line

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
components/variations/variations_params_manager.cc:72: 
Nit: Remove empty line

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
File components/variations/variations_params_manager.h (right):

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
components/variations/variations_params_manager.h:25: 
Nit: Remove empty line

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
components/variations/variations_params_manager.h:73: 
Nit: Remove empty line

[jkrcal, 2016-09-23 13:39:26]

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

[commit-bot: I haz the power, 2016-09-23 13:39:39]

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

[jkrcal, 2016-09-23 13:40:10]

Thanks, Alexei!

Sorry for the delay, I had more pressing stuff to deal with.

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
File components/variations/variations_params_manager.cc (right):

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
components/variations/variations_params_manager.cc:15: 
On 2016/09/12 16:54:13, Alexei Svitkine (very slow) wrote:
> Nit: Remove empty line

Done.

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
components/variations/variations_params_manager.cc:72: 
On 2016/09/12 16:54:13, Alexei Svitkine (very slow) wrote:
> Nit: Remove empty line

Done.

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
File components/variations/variations_params_manager.h (right):

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
components/variations/variations_params_manager.h:25: 
On 2016/09/12 16:54:13, Alexei Svitkine (very slow) wrote:
> Nit: Remove empty line

Done.

https://codereview.chromium.org/2321273003/diff/20001/components/variations/v...
components/variations/variations_params_manager.h:73: 
On 2016/09/12 16:54:13, Alexei Svitkine (very slow) wrote:
> Nit: Remove empty line

Done.

[commit-bot: I haz the power, 2016-09-23 13:42:54]

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

[commit-bot: I haz the power, 2016-09-23 13:42:55]

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

[jkrcal, 2016-11-22 15:25:20]

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

[commit-bot: I haz the power, 2016-11-22 15:25:37]

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-11-22 15:50:27]

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

[commit-bot: I haz the power, 2016-11-22 15:50:28]

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

[jkrcal, 2016-11-23 07:05:42]

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

[commit-bot: I haz the power, 2016-11-23 07:05:47]

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-11-23 07:41:32]

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

[commit-bot: I haz the power, 2016-11-23 07:41:33]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jkrcal, 2016-11-23 17:38:28]

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

[commit-bot: I haz the power, 2016-11-23 17:39:10]

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

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

Alexei, I am sorry for the delay. I've been sick for very very long. Now I
cannot land the CL as it is so I had to change it further. Could you PTAL,
again?

Explanations of notable differences since patch set 7:
 1) gn did not like the file being placed in unit_tests so I created
test_support lib (which seems standard for this kind of code)
 2) I changed the new param to the set of feature names, it makes much better
sense given what happens under the hood (and is simpler to use if you have many
params that you want to associate to one feature).
 3) I added another overloading of the constructor so that users are not forced
to specify the features.
 4) I address two more uses of VariationParamsManager (one of them is mimicking
exactly the new feature I am adding, so I could simplify the code).

[commit-bot: I haz the power, 2016-11-23 18:19:18]

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

[commit-bot: I haz the power, 2016-11-23 18:19:22]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jkrcal, 2016-11-24 09:51:38]

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

[commit-bot: I haz the power, 2016-11-24 09:52:05]

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-11-24 11:53:16]

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

[commit-bot: I haz the power, 2016-11-24 11:53:17]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[Alexei Svitkine (slow), 2016-11-24 19:29:41]

lgtm

https://codereview.chromium.org/2321273003/diff/250001/chrome/browser/compone...
File chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc
(right):

https://codereview.chromium.org/2321273003/diff/250001/chrome/browser/compone...
chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:140:
constexpr char kFeatureName[] = "ExperimentalSwReporterEngine";
Nit: Rename to kFeatureAndTrialName

https://codereview.chromium.org/2321273003/diff/250001/components/variations/...
File components/variations/variations_params_manager.cc (right):

https://codereview.chromium.org/2321273003/diff/250001/components/variations/...
components/variations/variations_params_manager.cc:19: const char
kGroupTesting[] = "Testing";
Nit: Add a comment about how this is used.

https://codereview.chromium.org/2321273003/diff/250001/components/variations/...
components/variations/variations_params_manager.cc:24:
variations::AssociateVariationParams(trial_name, kGroupTesting, param_values);
Nit: No need for variations:: here

https://codereview.chromium.org/2321273003/diff/250001/components/variations/...
components/variations/variations_params_manager.cc:80:
variations::testing::ClearAllVariationIDs();
Nit: Remove namespaces

https://codereview.chromium.org/2321273003/diff/250001/components/variations/...
components/variations/variations_params_manager.cc:84:
variations::testing::ClearAllVariationParams();
Nit: Ditto.

[jkrcal, 2016-11-25 13:23:21]

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

[commit-bot: I haz the power, 2016-11-25 13:23:31]

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

[jkrcal, 2016-11-25 13:23:37]

Thanks, Alexei!

https://codereview.chromium.org/2321273003/diff/250001/chrome/browser/compone...
File chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc
(right):

https://codereview.chromium.org/2321273003/diff/250001/chrome/browser/compone...
chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:140:
constexpr char kFeatureName[] = "ExperimentalSwReporterEngine";
On 2016/11/24 19:29:40, Alexei Svitkine (slow) wrote:
> Nit: Rename to kFeatureAndTrialName

Done.

https://codereview.chromium.org/2321273003/diff/250001/components/variations/...
File components/variations/variations_params_manager.cc (right):

https://codereview.chromium.org/2321273003/diff/250001/components/variations/...
components/variations/variations_params_manager.cc:19: const char
kGroupTesting[] = "Testing";
On 2016/11/24 19:29:40, Alexei Svitkine (slow) wrote:
> Nit: Add a comment about how this is used.

Done.

https://codereview.chromium.org/2321273003/diff/250001/components/variations/...
components/variations/variations_params_manager.cc:24:
variations::AssociateVariationParams(trial_name, kGroupTesting, param_values);
On 2016/11/24 19:29:40, Alexei Svitkine (slow) wrote:
> Nit: No need for variations:: here

Done.

https://codereview.chromium.org/2321273003/diff/250001/components/variations/...
components/variations/variations_params_manager.cc:80:
variations::testing::ClearAllVariationIDs();
On 2016/11/24 19:29:40, Alexei Svitkine (slow) wrote:
> Nit: Remove namespaces

I guess I cannot remove it here and below as it would resolve to the function in
this class (cycling into a stack overflow).

https://codereview.chromium.org/2321273003/diff/250001/components/variations/...
components/variations/variations_params_manager.cc:84:
variations::testing::ClearAllVariationParams();
On 2016/11/24 19:29:40, Alexei Svitkine (slow) wrote:
> Nit: Ditto.

Ditto.

[commit-bot: I haz the power, 2016-11-25 13:26:03]

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

[commit-bot: I haz the power, 2016-11-25 13:26:04]

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

[jkrcal, 2016-11-25 13:37:11]

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

[commit-bot: I haz the power, 2016-11-25 13:37:27]

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-11-25 14:38:31]

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

[commit-bot: I haz the power, 2016-11-25 14:38:32]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-11-25 14:43:39]

jkrcal@chromium.org changed reviewers:
+ joenotcharles@chromium.org, malaykeshav@chromium.org, sorin@chromium.org,
xiyuan@chromium.org

[jkrcal, 2016-11-25 14:43:40]

I've touched more components:

xiyuan, could you PTAL at hats_finch_helper_unittest.cc?
 (also adding malaykeshav who is the original author of this file)

sorin, could you PTAL at sw_reporter_installer_win_unittest.cc?
 (also adding joenotcharles who is the original author of this file)

[Joe Mason, 2016-11-25 16:17:54]

On 2016/11/25 14:43:40, jkrcal wrote:
> sorin, could you PTAL at sw_reporter_installer_win_unittest.cc?
>  (also adding joenotcharles who is the original author of this file)

sw_reporter_installer_win_unittest.cc changes lgtm

[xiyuan, 2016-11-28 23:29:31]

hats_finch_helper_unittest.cc lgtm

[jkrcal, 2016-12-05 14:40:49]

On 2016/11/25 16:17:54, Joe Mason wrote:
> On 2016/11/25 14:43:40, jkrcal wrote:
> > sorin, could you PTAL at sw_reporter_installer_win_unittest.cc?
> >  (also adding joenotcharles who is the original author of this file)
> 
> sw_reporter_installer_win_unittest.cc changes lgtm

sorin, I still need your owner's lgtm.

[Sorin Jianu, 2016-12-05 17:39:34]

lgtm

Thank you! Component updater lgtm

https://codereview.chromium.org/2321273003/diff/310001/chrome/browser/compone...
File chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc
(right):

https://codereview.chromium.org/2321273003/diff/310001/chrome/browser/compone...
chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:150:
kFeatureAndTrialName /*trial_name*/, params_with_group,
Maybe use // style comments instead of /**/ for consistency with the code in
this compilation unit.

[jkrcal, 2016-12-05 19:52:37]

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

[commit-bot: I haz the power, 2016-12-05 19:53:12]

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-05 21:09:05]

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

[commit-bot: I haz the power, 2016-12-05 21:09:06]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-12-05 21:46:54]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-12-05 21:46:55]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
asvitkine@chromium.org, xiyuan@chromium.org, joenotcharles@chromium.org,
sorin@chromium.org
Link to the patchset: https://codereview.chromium.org/2321273003/#ps330001
(title: "Last comments")

[jkrcal, 2016-12-05 21:47:15]

Thanks, everybody!

https://codereview.chromium.org/2321273003/diff/310001/chrome/browser/compone...
File chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc
(right):

https://codereview.chromium.org/2321273003/diff/310001/chrome/browser/compone...
chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:150:
kFeatureAndTrialName /*trial_name*/, params_with_group,
On 2016/12/05 17:39:34, Sorin Jianu wrote:
> Maybe use // style comments instead of /**/ for consistency with the code in
> this compilation unit. 

Done.

(I hope I've understood you correctly)

[commit-bot: I haz the power, 2016-12-05 21:47:51]

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

[Sorin Jianu, 2016-12-05 21:55:54]

On 2016/12/05 21:47:15, jkrcal wrote:
> Thanks, everybody!
> 
>
https://codereview.chromium.org/2321273003/diff/310001/chrome/browser/compone...
> File chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2321273003/diff/310001/chrome/browser/compone...
> chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc:150:
> kFeatureAndTrialName /*trial_name*/, params_with_group,
> On 2016/12/05 17:39:34, Sorin Jianu wrote:
> > Maybe use // style comments instead of /**/ for consistency with the code in
> > this compilation unit. 
> 
> Done.
> 
> (I hope I've understood you correctly)
Indeed, thank so much!

[commit-bot: I haz the power, 2016-12-05 22:36:50]

CQ is committing da patch.

Bot data: {"patchset_id": 330001, "attempt_start_ts": 1480974414644440,
"parent_rev": "0c4550b44d407746f4c514e3a3e2d80a918e69a6", "commit_rev":
"c5d274e160c37d9dadc4fc3c32994b8787fd55a7"}

[commit-bot: I haz the power, 2016-12-05 22:37:23]

Committed patchset #18 (id:330001)

[commit-bot: I haz the power, 2016-12-05 22:40:35]

Description was changed from

==========
Extend VariationParamsManager to support feature associations.

Previously, feature associations for variation parameters were not easy
to register in unit-tests. This CL extends VariationParamsManager to do
it automatically.


BUG=645447
==========

to

==========
Extend VariationParamsManager to support feature associations.

Previously, feature associations for variation parameters were not easy
to register in unit-tests. This CL extends VariationParamsManager to do
it automatically.

BUG=645447

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

[commit-bot: I haz the power, 2016-12-05 22:40:36]

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