Propogate RAPPOR permission changes

Propogate RAPPOR permission changes

BUG=

Committed: https://crrev.com/873fb7c9c7f799399e9b45277f5420c637b28680
Cr-Commit-Position: refs/heads/master@{#317172}

[Steven Holte, 2015-02-19 00:40:49]

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

[Steven Holte, 2015-02-19 00:40:49]

Still not sure what the best way of doing this is.  It's a bit ugly to have to
locally store may_upload/may_record, but there isn't really a reliable way
inspect this state.

[Alexei Svitkine (slow), 2015-02-19 15:04:59]

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_services_manager.cc:66: base::Unretained(this)));
Don't use Unretained. Instead, use a weak pointer factory.

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_services_manager.cc:157:
GetRapporService()->Update(GetRapporRecordingLevel(may_record_), may_upload_);
I don't understand this.

Now that RAPPOR has its own pref, why is UMA's may_record_ and may_upload_
relevant anymore? Shouldn't it be checked the rappor pref instead? What am I
missing?

[Alexei Svitkine (slow), 2015-02-19 15:13:45]

Storing the values seems OK to me % comments.

Please associate this with a BUG=

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_services_manager.cc:157:
GetRapporService()->Update(GetRapporRecordingLevel(may_record_), may_upload_);
On 2015/02/19 15:04:58, Alexei Svitkine wrote:
> I don't understand this.
> 
> Now that RAPPOR has its own pref, why is UMA's may_record_ and may_upload_
> relevant anymore? Shouldn't it be checked the rappor pref instead? What am I
> missing?

Ah, just read your other CL and it makes sense now - since without the trial on,
we want to keep using UMA state. Can you add a comment to that extent?

[Steven Holte, 2015-02-19 20:25:12]

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_services_manager.cc:66: base::Unretained(this)));
On 2015/02/19 15:04:58, Alexei Svitkine wrote:
> Don't use Unretained. Instead, use a weak pointer factory.

I'm not sure I understand the logic here.  Why here and not in
GetMetricsStateManager?  Since pref_change_registrar_ is member of "this", there
is enough information here to know that "this" will outlive the callback.

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_services_manager.cc:157:
GetRapporService()->Update(GetRapporRecordingLevel(may_record_), may_upload_);
On 2015/02/19 15:13:45, Alexei Svitkine wrote:
> On 2015/02/19 15:04:58, Alexei Svitkine wrote:
> > I don't understand this.
> > 
> > Now that RAPPOR has its own pref, why is UMA's may_record_ and may_upload_
> > relevant anymore? Shouldn't it be checked the rappor pref instead? What am I
> > missing?
> 
> Ah, just read your other CL and it makes sense now - since without the trial
on,
> we want to keep using UMA state. Can you add a comment to that extent?

That's not the only reason.  Even with the new rappor pref, the UMA state
determines whether we send FINE metrics or only COARSE metrics.

[Alexei Svitkine (slow), 2015-02-19 20:32:01]

lgtm

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_services_manager.cc:66: base::Unretained(this)));
On 2015/02/19 20:25:12, Steven Holte wrote:
> On 2015/02/19 15:04:58, Alexei Svitkine wrote:
> > Don't use Unretained. Instead, use a weak pointer factory.
> 
> I'm not sure I understand the logic here.  Why here and not in
> GetMetricsStateManager?  Since pref_change_registrar_ is member of "this",
there
> is enough information here to know that "this" will outlive the callback.

You're right.

I was thinking of the case where the callback would be posted as a task. In that
case, in the meanwhile, this object, the registrar, and everything can go away
in the meanwhile without invalidating the posted task.

But since all the pref stuff is done on UI thread and there's no posting of
tasks, this is safe. 

Technically, it could break if impl. of PrefChangeRegistrar changes to post its
callback. But you're right that other places use this same pattern, so they
would all need to be updated anyway in such a case.

https://codereview.chromium.org/937013002/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_services_manager.cc:157:
GetRapporService()->Update(GetRapporRecordingLevel(may_record_), may_upload_);
On 2015/02/19 20:25:12, Steven Holte wrote:
> On 2015/02/19 15:13:45, Alexei Svitkine wrote:
> > On 2015/02/19 15:04:58, Alexei Svitkine wrote:
> > > I don't understand this.
> > > 
> > > Now that RAPPOR has its own pref, why is UMA's may_record_ and may_upload_
> > > relevant anymore? Shouldn't it be checked the rappor pref instead? What am
I
> > > missing?
> > 
> > Ah, just read your other CL and it makes sense now - since without the trial
> on,
> > we want to keep using UMA state. Can you add a comment to that extent?
> 
> That's not the only reason.  Even with the new rappor pref, the UMA state
> determines whether we send FINE metrics or only COARSE metrics.

Acknowledged.

[Steven Holte, 2015-02-19 22:42:35]

New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org

[Steven Holte, 2015-02-19 22:43:05]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2015-02-19 22:43:51]

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

[commit-bot: I haz the power, 2015-02-19 23:41:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-19 23:42:58]

Patchset 2 (id:??) landed as
https://crrev.com/873fb7c9c7f799399e9b45277f5420c637b28680
Cr-Commit-Position: refs/heads/master@{#317172}