Use low entropy for studies that send experiment IDs to Google properties.

Use low entropy for studies that send experiment IDs to Google properties.

This applies to all clients, even those that have metrics reporting enabled. This will cause a reshuffle of any study that is sending experiment IDs.

BUG=612666
Committed: https://crrev.com/67c08f756c5907a7ff87fc59d2c6af947987881d
Cr-Commit-Position: refs/heads/master@{#394544}

[jwd, 2016-05-17 16:22:07]

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

[jwd, 2016-05-17 16:22:08]

For preliminary review. Still missing seed simulator changes.

[Alexei Svitkine (slow), 2016-05-17 16:31:35]

Looks great.

Can you add a unit test to test the new behavior change?

https://codereview.chromium.org/1984003002/diff/40001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/1984003002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.h:399: // |override_entropy_provider| is not NULL, then
it will be used for
Nit: not NULL -> not null

https://codereview.chromium.org/1984003002/diff/40001/components/variations/s...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1984003002/diff/40001/components/variations/s...
components/variations/service/variations_service.cc:602:
VariationsService::GetLowEntropyProvider() {
Nit: CreateLowEntropyProvider.

Although, maybe no need to have this wrapper at all?

Or is it here just to make the tests happy? If so, mention it in the declaration
in the header.

https://codereview.chromium.org/1984003002/diff/40001/components/variations/v...
File components/variations/variations_seed_processor.h (right):

https://codereview.chromium.org/1984003002/diff/40001/components/variations/v...
components/variations/variations_seed_processor.h:82: // Disables the trial if
|processed_study.is_expired| is true.
Nit: Document low_entropy_provider param

https://codereview.chromium.org/1984003002/diff/40001/components/variations/v...
components/variations/variations_seed_processor.h:86: base::FeatureList*
feature_list,
Nit: non-const params should be last.

[jwd, 2016-05-18 01:56:58]

Description was changed from

==========
Using low entropy for some studies.

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

to

==========
Use low entropy for studies that send experiment IDs to Google properties.

This applies to all clients, even those that have metrics reporting enabled.
This will cause a reshuffle of any study that is sending experiment IDs.

BUG=612666
==========

[jwd, 2016-05-18 13:24:18]

Tests added.

Ready for full review.

https://codereview.chromium.org/1984003002/diff/40001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/1984003002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.h:399: // |override_entropy_provider| is not NULL, then
it will be used for
On 2016/05/17 16:31:35, Alexei Svitkine wrote:
> Nit: not NULL -> not null

Done.

https://codereview.chromium.org/1984003002/diff/40001/components/variations/s...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1984003002/diff/40001/components/variations/s...
components/variations/service/variations_service.cc:602:
VariationsService::GetLowEntropyProvider() {
On 2016/05/17 16:31:35, Alexei Svitkine wrote:
> Nit: CreateLowEntropyProvider.
> 
> Although, maybe no need to have this wrapper at all?
> 
> Or is it here just to make the tests happy? If so, mention it in the
declaration
> in the header.

Done.

Yeah, it's for the tests.

https://codereview.chromium.org/1984003002/diff/40001/components/variations/v...
File components/variations/variations_seed_processor.h (right):

https://codereview.chromium.org/1984003002/diff/40001/components/variations/v...
components/variations/variations_seed_processor.h:82: // Disables the trial if
|processed_study.is_expired| is true.
On 2016/05/17 16:31:35, Alexei Svitkine wrote:
> Nit: Document low_entropy_provider param

Done.

https://codereview.chromium.org/1984003002/diff/40001/components/variations/v...
components/variations/variations_seed_processor.h:86: base::FeatureList*
feature_list,
On 2016/05/17 16:31:35, Alexei Svitkine wrote:
> Nit: non-const params should be last.

Done.

[Alexei Svitkine (slow), 2016-05-18 15:03:32]

LGTM % comments!

https://codereview.chromium.org/1984003002/diff/120001/base/test/mock_entropy...
File base/test/mock_entropy_provider.h (right):

https://codereview.chromium.org/1984003002/diff/120001/base/test/mock_entropy...
base/test/mock_entropy_provider.h:23: void set_entropy_value(double
entropy_value) {
Is there any reason to reset it after constructing it? If not, I'd make it a
ctor param.

https://codereview.chromium.org/1984003002/diff/120001/base/test/mock_entropy...
base/test/mock_entropy_provider.h:29: };
Nit: DISALLOW_COPY_AND_ASSIGN();

https://codereview.chromium.org/1984003002/diff/120001/components/metrics/met...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/1984003002/diff/120001/components/metrics/met...
components/metrics/metrics_state_manager.cc:165: const int
low_entropy_source_value = GetLowEntropySource();
Nit: Move this inside the if, so that it's not done redundantly if the if is
false.

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
File components/variations/service/variations_service.h (right):

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
components/variations/service/variations_service.h:183: // that send experiment
IDs to Google web properties. Exposed for testing.
Nit: "Exposed for testing." -> "Virtual for testing."

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
File components/variations/variations_seed_processor.h (right):

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
components/variations/variations_seed_processor.h:54: base::FeatureList*
feature_list,
Nit: This non-const param should be last.

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
components/variations/variations_seed_processor.h:57: static bool
ShouldStudyUseLowEntropy(const Study& study);
Nit: Add a comment.

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
components/variations/variations_seed_processor.h:86: // study.
Nit: Add the same comment to CreateTrialsFromSeed().

[jwd, 2016-05-18 18:31:40]

jwd@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[jwd, 2016-05-18 18:31:41]

+phajdan.jr@ for base/test change.

https://codereview.chromium.org/1984003002/diff/120001/base/test/mock_entropy...
File base/test/mock_entropy_provider.h (right):

https://codereview.chromium.org/1984003002/diff/120001/base/test/mock_entropy...
base/test/mock_entropy_provider.h:23: void set_entropy_value(double
entropy_value) {
On 2016/05/18 15:03:29, Alexei Svitkine wrote:
> Is there any reason to reset it after constructing it? If not, I'd make it a
> ctor param.

Done.

https://codereview.chromium.org/1984003002/diff/120001/base/test/mock_entropy...
base/test/mock_entropy_provider.h:29: };
On 2016/05/18 15:03:29, Alexei Svitkine wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN();

Done.

https://codereview.chromium.org/1984003002/diff/120001/components/metrics/met...
File components/metrics/metrics_state_manager.cc (right):

https://codereview.chromium.org/1984003002/diff/120001/components/metrics/met...
components/metrics/metrics_state_manager.cc:165: const int
low_entropy_source_value = GetLowEntropySource();
On 2016/05/18 15:03:30, Alexei Svitkine wrote:
> Nit: Move this inside the if, so that it's not done redundantly if the if is
> false.

Done.

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
File components/variations/service/variations_service.h (right):

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
components/variations/service/variations_service.h:183: // that send experiment
IDs to Google web properties. Exposed for testing.
On 2016/05/18 15:03:30, Alexei Svitkine wrote:
> Nit: "Exposed for testing." -> "Virtual for testing."

Done.

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
File components/variations/variations_seed_processor.h (right):

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
components/variations/variations_seed_processor.h:54: base::FeatureList*
feature_list,
On 2016/05/18 15:03:30, Alexei Svitkine wrote:
> Nit: This non-const param should be last.

Done.

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
components/variations/variations_seed_processor.h:57: static bool
ShouldStudyUseLowEntropy(const Study& study);
On 2016/05/18 15:03:31, Alexei Svitkine wrote:
> Nit: Add a comment.

Done.

https://codereview.chromium.org/1984003002/diff/120001/components/variations/...
components/variations/variations_seed_processor.h:86: // study.
On 2016/05/18 15:03:31, Alexei Svitkine wrote:
> Nit: Add the same comment to CreateTrialsFromSeed().

Done.

[Alexei Svitkine (slow), 2016-05-18 18:33:29]

still lgtm

https://codereview.chromium.org/1984003002/diff/140001/base/test/mock_entropy...
File base/test/mock_entropy_provider.h (right):

https://codereview.chromium.org/1984003002/diff/140001/base/test/mock_entropy...
base/test/mock_entropy_provider.h:17: MockEntropyProvider(double entropy_value);
Nit: explicit

[Paweł Hajdan Jr., 2016-05-18 18:46:49]

base/test LGTM

[jwd, 2016-05-18 19:21:47]

The CQ bit was checked by jwd@chromium.org

[jwd, 2016-05-18 19:21:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1984003002/#ps160001
(title: " ")

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

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

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

Description was changed from

==========
Use low entropy for studies that send experiment IDs to Google properties.

This applies to all clients, even those that have metrics reporting enabled.
This will cause a reshuffle of any study that is sending experiment IDs.

BUG=612666
==========

to

==========
Use low entropy for studies that send experiment IDs to Google properties.

This applies to all clients, even those that have metrics reporting enabled.
This will cause a reshuffle of any study that is sending experiment IDs.

BUG=612666
==========

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

Committed patchset #9 (id:160001)

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

Description was changed from

==========
Use low entropy for studies that send experiment IDs to Google properties.

This applies to all clients, even those that have metrics reporting enabled.
This will cause a reshuffle of any study that is sending experiment IDs.

BUG=612666
==========

to

==========
Use low entropy for studies that send experiment IDs to Google properties.

This applies to all clients, even those that have metrics reporting enabled.
This will cause a reshuffle of any study that is sending experiment IDs.

BUG=612666

Committed: https://crrev.com/67c08f756c5907a7ff87fc59d2c6af947987881d
Cr-Commit-Position: refs/heads/master@{#394544}
==========

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

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