Add variations params api to get params from features.

Add variations params api to get params from features.

BUG=609184
Committed: https://crrev.com/07b903843153028c654105e6138d3ed891219b5f
Cr-Commit-Position: refs/heads/master@{#392142}

[jwd, 2016-05-02 22:42:51]

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

[jwd, 2016-05-02 22:42:51]

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

[jwd, 2016-05-03 15:27:58]

On 2016/05/02 22:42:52, Jesse Doherty wrote:

It's not clear to me if the params should be unavailable when the feature is
disabled. In a way, it feels right since the feature is disabled, it shouldn't
need params. But the field trial is still active, and the params are still
there.

[Alexei Svitkine (slow), 2016-05-03 17:04:26]

Looks great! I think current semantics (re: disabled features) is fine to start
off.

https://codereview.chromium.org/1941963002/diff/20001/base/feature_list.h
File base/feature_list.h (right):

https://codereview.chromium.org/1941963002/diff/20001/base/feature_list.h#new...
base/feature_list.h:146: static FieldTrial* GetFieldTrial(const Feature&
feature);
Can you also add a unit test for this API?

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

https://codereview.chromium.org/1941963002/diff/20001/components/variations/v...
components/variations/variations_associated_data.h:130: // Retvieves the set of
key-value |params| for the variation associated with the
Nit: Retrieves

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

https://codereview.chromium.org/1941963002/diff/20001/components/variations/v...
components/variations/variations_associated_data_unittest.cc:39:
scoped_refptr<base::FieldTrial> trial) {
Nit: Just pass by FieldTrial*.

https://codereview.chromium.org/1941963002/diff/20001/components/variations/v...
components/variations/variations_associated_data_unittest.cc:427: const
base::Feature kFeature{"TestFeature",
Nit: Maybe move this to the top of the function below kTrialName so that the
constants are together.

[jwd, 2016-05-03 20:12:27]

https://codereview.chromium.org/1941963002/diff/20001/base/feature_list.h
File base/feature_list.h (right):

https://codereview.chromium.org/1941963002/diff/20001/base/feature_list.h#new...
base/feature_list.h:146: static FieldTrial* GetFieldTrial(const Feature&
feature);
On 2016/05/03 17:04:25, Alexei Svitkine wrote:
> Can you also add a unit test for this API?

Done.

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

https://codereview.chromium.org/1941963002/diff/20001/components/variations/v...
components/variations/variations_associated_data.h:130: // Retvieves the set of
key-value |params| for the variation associated with the
On 2016/05/03 17:04:25, Alexei Svitkine wrote:
> Nit: Retrieves

Done.

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

https://codereview.chromium.org/1941963002/diff/20001/components/variations/v...
components/variations/variations_associated_data_unittest.cc:39:
scoped_refptr<base::FieldTrial> trial) {
On 2016/05/03 17:04:25, Alexei Svitkine wrote:
> Nit: Just pass by FieldTrial*.

Done.

https://codereview.chromium.org/1941963002/diff/20001/components/variations/v...
components/variations/variations_associated_data_unittest.cc:427: const
base::Feature kFeature{"TestFeature",
On 2016/05/03 17:04:25, Alexei Svitkine wrote:
> Nit: Maybe move this to the top of the function below kTrialName so that the
> constants are together.

Done.

[Alexei Svitkine (slow), 2016-05-03 20:24:05]

LGTM

Please create a crbug for this CL

[jwd, 2016-05-04 17:51:04]

Description was changed from

==========
Add variations params api to get params from features.

BUG=
==========

to

==========
Add variations params api to get params from features.

BUG=609184
==========

[jwd, 2016-05-04 17:51:20]

The CQ bit was checked by jwd@chromium.org

[commit-bot: I haz the power, 2016-05-04 17:52:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1941963002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1941963002/40001

[commit-bot: I haz the power, 2016-05-04 18:04:10]

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

[commit-bot: I haz the power, 2016-05-04 18:04:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jwd, 2016-05-06 18:58:31]

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

[jwd, 2016-05-06 18:58:32]

+danakj for base/BUILD.gn change. Just adding feature_list_unittest.cc, which
never made the move from gyp.

[danakj, 2016-05-06 18:59:44]

LGTM

[jwd, 2016-05-06 19:30:01]

The CQ bit was checked by jwd@chromium.org

[commit-bot: I haz the power, 2016-05-06 19:30:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1941963002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1941963002/40001

[commit-bot: I haz the power, 2016-05-06 20:39:53]

Description was changed from

==========
Add variations params api to get params from features.

BUG=609184
==========

to

==========
Add variations params api to get params from features.

BUG=609184
==========

[commit-bot: I haz the power, 2016-05-06 20:39:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-06 20:41:13]

Description was changed from

==========
Add variations params api to get params from features.

BUG=609184
==========

to

==========
Add variations params api to get params from features.

BUG=609184

Committed: https://crrev.com/07b903843153028c654105e6138d3ed891219b5f
Cr-Commit-Position: refs/heads/master@{#392142}
==========

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

Patchset 3 (id:??) landed as
https://crrev.com/07b903843153028c654105e6138d3ed891219b5f
Cr-Commit-Position: refs/heads/master@{#392142}