Splitting the concept of UMA consent, and should UMA report.

Splitting the concept of UMA consent, and should UMA report.

The motivation here is to support sampling clients. When a client isn't in the sample, reporting will be disabled, even if they've consented.

Knowing consent is still important to make sure the client_id is created, and the high entropy source is used for variations selection. Otherwise, clients would be shuffled into different variations whenever they come in or out of the sample.

BUG=609987
Committed: https://crrev.com/a5d188392c54189a2a044b8c436209995db1ed9d
Cr-Commit-Position: refs/heads/master@{#393333}

[jwd, 2016-05-09 21:00:15]

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

[jwd, 2016-05-11 15:15:33]

Ok, it's ready for a full look now.

[jwd, 2016-05-11 15:15:33]

Ok, it's ready for a full look now.

[Alexei Svitkine (slow), 2016-05-11 17:16:28]

https://codereview.chromium.org/1958003003/diff/320001/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/320001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:88:
std::unique_ptr<AwEnabledStateProvider> enabled_state_provider_;
Nit: How about just making AwMetricsServiceClient implement the interface? Then,
you don't need an extra member/class.

https://codereview.chromium.org/1958003003/diff/320001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right):

https://codereview.chromium.org/1958003003/diff/320001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_services_manager_client.h:30: class
ChromeEnabledStateProvider;
Nit: Suggest making this a class in the anon namespace rather than a member
class. Since you use the interface for the member, no need to declare it in the
header.

https://codereview.chromium.org/1958003003/diff/320001/chromecast/browser/met...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/320001/chromecast/browser/met...
chromecast/browser/metrics/cast_metrics_service_client.h:92: class
CastEnabledStateProvider;
Nit: Same comment as for Aw client - just make it implement this.

https://codereview.chromium.org/1958003003/diff/320001/components/metrics/ena...
File components/metrics/enabled_state_provider.h (right):

https://codereview.chromium.org/1958003003/diff/320001/components/metrics/ena...
components/metrics/enabled_state_provider.h:19: // being given.
Nit: Please expand comment to shed light of why this would be different. Suggest
just mentioning sampling as an example and explaining.

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

https://codereview.chromium.org/1958003003/diff/320001/components/metrics/met...
components/metrics/metrics_state_manager.cc:75: bool
MetricsStateManager::IsMetricsReportingEnabled() {
Can you expand the comment in the header about the intricacies.

https://codereview.chromium.org/1958003003/diff/320001/components/metrics/tes...
File components/metrics/test_enabled_state_provider.cc (right):

https://codereview.chromium.org/1958003003/diff/320001/components/metrics/tes...
components/metrics/test_enabled_state_provider.cc:11: }
Nit: Add an empty line.

[Alexei Svitkine (slow), 2016-05-11 17:20:14]

Also, be aware that a new MetricsServiceClient is being added for Blimp here
that will need these changes: https://codereview.chromium.org/1945393002/

And one other thing - could you expand the CL description to clearly mention the
motivation for this - i.e. allowing the ability to sample users from the client
side who have given consent.

[jwd, 2016-05-11 18:30:01]

Description was changed from

==========
Splitting the concept of UMA consent, and should UMA report.

BUG=609987
==========

to

==========
Splitting the concept of UMA consent, and should UMA report. The motivation here
is to support sampling clients. When a client isn't in the sample, reporting
will be disabled, even if they've consented. 

Knowing consent is still important to make sure the client_id is created, and
the high entropy source is used for variations selection. Otherwise, clients
would be shuffled into different variations whenever they come in or out of the
sample.

BUG=609987
==========

[jwd, 2016-05-11 18:31:17]

On 2016/05/11 17:20:14, Alexei Svitkine wrote:
> Also, be aware that a new MetricsServiceClient is being added for Blimp here
> that will need these changes: https://codereview.chromium.org/1945393002/
Ah, ok, I'll coordinate. I'll probably land this after the Blimp change.
> 
> And one other thing - could you expand the CL description to clearly mention
the
> motivation for this - i.e. allowing the ability to sample users from the
client
> side who have given consent.
Done.

[jwd, 2016-05-11 18:31:38]

https://codereview.chromium.org/1958003003/diff/320001/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/320001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:88:
std::unique_ptr<AwEnabledStateProvider> enabled_state_provider_;
On 2016/05/11 17:16:28, Alexei Svitkine wrote:
> Nit: How about just making AwMetricsServiceClient implement the interface?
Then,
> you don't need an extra member/class.

Done.

https://codereview.chromium.org/1958003003/diff/320001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right):

https://codereview.chromium.org/1958003003/diff/320001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_services_manager_client.h:30: class
ChromeEnabledStateProvider;
On 2016/05/11 17:16:28, Alexei Svitkine wrote:
> Nit: Suggest making this a class in the anon namespace rather than a member
> class. Since you use the interface for the member, no need to declare it in
the
> header.

I made it a member class to get access to
ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled() through
ChromeMetricsServicesManagerClient's friendship. This seemed nicer than exposing
a public method on ChromeMetricsServicesManagerClient, and give
ChromeEnabledStateProvider a pointer to the ChromeMetricsServicesManagerClient
instance.

https://codereview.chromium.org/1958003003/diff/320001/chromecast/browser/met...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/320001/chromecast/browser/met...
chromecast/browser/metrics/cast_metrics_service_client.h:92: class
CastEnabledStateProvider;
On 2016/05/11 17:16:28, Alexei Svitkine wrote:
> Nit: Same comment as for Aw client - just make it implement this.

Done.

https://codereview.chromium.org/1958003003/diff/320001/components/metrics/ena...
File components/metrics/enabled_state_provider.h (right):

https://codereview.chromium.org/1958003003/diff/320001/components/metrics/ena...
components/metrics/enabled_state_provider.h:19: // being given.
On 2016/05/11 17:16:28, Alexei Svitkine wrote:
> Nit: Please expand comment to shed light of why this would be different.
Suggest
> just mentioning sampling as an example and explaining.

Done.

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

https://codereview.chromium.org/1958003003/diff/320001/components/metrics/met...
components/metrics/metrics_state_manager.cc:75: bool
MetricsStateManager::IsMetricsReportingEnabled() {
On 2016/05/11 17:16:28, Alexei Svitkine wrote:
> Can you expand the comment in the header about the intricacies.

Done.

https://codereview.chromium.org/1958003003/diff/320001/components/metrics/tes...
File components/metrics/test_enabled_state_provider.cc (right):

https://codereview.chromium.org/1958003003/diff/320001/components/metrics/tes...
components/metrics/test_enabled_state_provider.cc:11: }
On 2016/05/11 17:16:28, Alexei Svitkine wrote:
> Nit: Add an empty line.

Done.

[Alexei Svitkine (slow), 2016-05-11 18:37:26]

lgtm % comments

In your CL description, please add a newline after the first sentence (so that
your first sentence corresponds to the CL subject on its own line).

https://codereview.chromium.org/1958003003/diff/340001/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/340001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:72: //
metrics::EnabledStateProvider implementation:
Nit: New suggested syntax for these comments is just:

// metrics::EnableStateProvider:

https://codereview.chromium.org/1958003003/diff/340001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right):

https://codereview.chromium.org/1958003003/diff/340001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_services_manager_client.h:30: class
ChromeEnabledStateProvider;
Nit: This can be made an anon namespace class in the .cc file only and not be
listed here.

https://codereview.chromium.org/1958003003/diff/340001/chromecast/browser/met...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/340001/chromecast/browser/met...
chromecast/browser/metrics/cast_metrics_service_client.h:87: //
::metrics::EnabledStateProvider implementation:
Nit: Ditto.

https://codereview.chromium.org/1958003003/diff/340001/ios/chrome/browser/met...
File ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.h
(right):

https://codereview.chromium.org/1958003003/diff/340001/ios/chrome/browser/met...
ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.h:30:
class IOSChromeEnabledStateProvider;
Nit: Move to be an anon namespace class and remove this.

[jwd, 2016-05-11 19:16:48]

Description was changed from

==========
Splitting the concept of UMA consent, and should UMA report. The motivation here
is to support sampling clients. When a client isn't in the sample, reporting
will be disabled, even if they've consented. 

Knowing consent is still important to make sure the client_id is created, and
the high entropy source is used for variations selection. Otherwise, clients
would be shuffled into different variations whenever they come in or out of the
sample.

BUG=609987
==========

to

==========
Splitting the concept of UMA consent, and should UMA report.

The motivation here is to support sampling clients. When a client isn't in the
sample, reporting will be disabled, even if they've consented. 

Knowing consent is still important to make sure the client_id is created, and
the high entropy source is used for variations selection. Otherwise, clients
would be shuffled into different variations whenever they come in or out of the
sample.

BUG=609987
==========

[jwd, 2016-05-11 19:17:04]

https://codereview.chromium.org/1958003003/diff/340001/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/340001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:72: //
metrics::EnabledStateProvider implementation:
On 2016/05/11 18:37:26, Alexei Svitkine wrote:
> Nit: New suggested syntax for these comments is just:
> 
> // metrics::EnableStateProvider:

Done.

https://codereview.chromium.org/1958003003/diff/340001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right):

https://codereview.chromium.org/1958003003/diff/340001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_services_manager_client.h:30: class
ChromeEnabledStateProvider;
On 2016/05/11 18:37:26, Alexei Svitkine wrote:
> Nit: This can be made an anon namespace class in the .cc file only and not be
> listed here.

Adding a comment.

https://codereview.chromium.org/1958003003/diff/340001/chromecast/browser/met...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/340001/chromecast/browser/met...
chromecast/browser/metrics/cast_metrics_service_client.h:87: //
::metrics::EnabledStateProvider implementation:
On 2016/05/11 18:37:26, Alexei Svitkine wrote:
> Nit: Ditto.

Done.

https://codereview.chromium.org/1958003003/diff/340001/ios/chrome/browser/met...
File ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.h
(right):

https://codereview.chromium.org/1958003003/diff/340001/ios/chrome/browser/met...
ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.h:30:
class IOSChromeEnabledStateProvider;
On 2016/05/11 18:37:26, Alexei Svitkine wrote:
> Nit: Move to be an anon namespace class and remove this.

Adding comment.

[jwd, 2016-05-11 20:01:14]

jwd@chromium.org changed reviewers:
+ lpromero@chromium.org, sgurun@chromium.org

[jwd, 2016-05-11 20:02:10]

+sgurun@ for webview owners
+lpromero@ for ios

[lpromero, 2016-05-11 20:24:42]

lgtm

[sgurun-gerrit only, 2016-05-11 20:44:39]

sgurun@chromium.org changed reviewers:
+ paulmiller@chromium.org

[sgurun-gerrit only, 2016-05-11 20:44:59]

On 2016/05/11 20:24:42, lpromero wrote:
> lgtm

sending to Paul for review. I will rubber stamp.

[paulmiller, 2016-05-11 21:31:34]

I named the WV functions assuming that "enabled" == "consented". Some of these
names become slightly confusing if that's no longer the case. But I can fix that
in a separate change.

https://codereview.chromium.org/1958003003/diff/230020/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/230020/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:73: bool IsConsentGiven()
override;
This looks like it can be called before AwMetricsServiceClient initialization,
so please group it with those similar functions at the top (where it says "These
may be called at any time.")

[jwd, 2016-05-11 22:00:49]

On 2016/05/11 21:31:34, paulmiller wrote:
> I named the WV functions assuming that "enabled" == "consented". Some of these
> names become slightly confusing if that's no longer the case. But I can fix
that
> in a separate change.
> 
Agreed it is confusing.
I don't currently have plans to implement any behaviour changes for WV. When I
do make changes that make IsConsentGiven != IsReportingEnabled, it will be in
the browser implementations of the provider, and not in the component code.

>
https://codereview.chromium.org/1958003003/diff/230020/android_webview/browse...
> File android_webview/browser/aw_metrics_service_client.h (right):
> 
>
https://codereview.chromium.org/1958003003/diff/230020/android_webview/browse...
> android_webview/browser/aw_metrics_service_client.h:73: bool IsConsentGiven()
> override;
> This looks like it can be called before AwMetricsServiceClient initialization,
> so please group it with those similar functions at the top (where it says
"These
> may be called at any time.")

[jwd, 2016-05-11 22:01:04]

https://codereview.chromium.org/1958003003/diff/230020/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/230020/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:73: bool IsConsentGiven()
override;
On 2016/05/11 21:31:35, paulmiller wrote:
> This looks like it can be called before AwMetricsServiceClient initialization,
> so please group it with those similar functions at the top (where it says
"These
> may be called at any time.")

Done.

[paulmiller, 2016-05-11 22:05:48]

android_webview/ lgtm

[sgurun-gerrit only, 2016-05-11 22:06:47]

On 2016/05/11 22:05:48, paulmiller wrote:
> android_webview/ lgtm

aw lgtm rubber stamp

[jwd, 2016-05-12 01:32:20]

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

[jwd, 2016-05-12 01:32:22]

+jessicag for blimp changes.

[Jess, 2016-05-12 16:56:19]

jessicag@chromium.org changed reviewers:
+ kmarshall@chromium.org
- jessicag@chromium.org

[Jess, 2016-05-12 16:57:36]

On 2016/05/12 16:56:19, Jess wrote:
> mailto:jessicag@chromium.org changed reviewers:
> + mailto:kmarshall@chromium.org
> - mailto:jessicag@chromium.org

Unfortunately I am not an owner on blimp.  Adding kmarshall@ who reviewed my
changes to the blimp files.

[Kevin M, 2016-05-12 17:58:46]

blimp/ lgtm % supernit

https://codereview.chromium.org/1958003003/diff/430001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/430001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.h:67: //
metrics::EnabledStateProvider:
nit: "metrics::EnabledStateProvider implementation. "

(for consistency with other comments like this in blimp/)

[jwd, 2016-05-12 18:09:45]

https://codereview.chromium.org/1958003003/diff/430001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client.h (right):

https://codereview.chromium.org/1958003003/diff/430001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.h:67: //
metrics::EnabledStateProvider:
On 2016/05/12 17:58:46, Kevin M wrote:
> nit: "metrics::EnabledStateProvider implementation. "
> 
> (for consistency with other comments like this in blimp/)

Done.

[jwd, 2016-05-12 18:37:52]

The CQ bit was checked by jwd@chromium.org

[jwd, 2016-05-12 18:37:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, lpromero@chromium.org, paulmiller@chromium.org,
sgurun@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1958003003/#ps450001
(title: " ")

[commit-bot: I haz the power, 2016-05-12 18:38:12]

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

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

Description was changed from

==========
Splitting the concept of UMA consent, and should UMA report.

The motivation here is to support sampling clients. When a client isn't in the
sample, reporting will be disabled, even if they've consented. 

Knowing consent is still important to make sure the client_id is created, and
the high entropy source is used for variations selection. Otherwise, clients
would be shuffled into different variations whenever they come in or out of the
sample.

BUG=609987
==========

to

==========
Splitting the concept of UMA consent, and should UMA report.

The motivation here is to support sampling clients. When a client isn't in the
sample, reporting will be disabled, even if they've consented. 

Knowing consent is still important to make sure the client_id is created, and
the high entropy source is used for variations selection. Otherwise, clients
would be shuffled into different variations whenever they come in or out of the
sample.

BUG=609987
==========

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

Committed patchset #24 (id:450001)

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

Description was changed from

==========
Splitting the concept of UMA consent, and should UMA report.

The motivation here is to support sampling clients. When a client isn't in the
sample, reporting will be disabled, even if they've consented. 

Knowing consent is still important to make sure the client_id is created, and
the high entropy source is used for variations selection. Otherwise, clients
would be shuffled into different variations whenever they come in or out of the
sample.

BUG=609987
==========

to

==========
Splitting the concept of UMA consent, and should UMA report.

The motivation here is to support sampling clients. When a client isn't in the
sample, reporting will be disabled, even if they've consented.

Knowing consent is still important to make sure the client_id is created, and
the high entropy source is used for variations selection. Otherwise, clients
would be shuffled into different variations whenever they come in or out of the
sample.

BUG=609987

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

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

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