Record stats about kExtensionKnownDisabled with the PrefMetricsService.

Record stats about kExtensionKnownDisabled with the PrefMetricsService.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228934

[gab, 2013-10-15 18:41:29]

Pam/Bill, PTAL.

Thanks!
Gab

[bbudge, 2013-10-15 18:55:59]

LGTM with a few suggestions.

https://codereview.chromium.org/27273004/diff/1/chrome/browser/prefs/pref_met...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/27273004/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:20: #include
"chrome/browser/extensions/extension_prefs.h"
I don't think we need this any more.

https://codereview.chromium.org/27273004/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:46: const char
kUnregisteredPreference = "unregistered.preference";
I wonder if an empty string might be a better value. Or you could replace the
'.' by '_' or just 'unregistered'.

[gab, 2013-10-15 19:09:35]

Done, PTAL.

Thanks!

https://codereview.chromium.org/27273004/diff/1/chrome/browser/prefs/pref_met...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/27273004/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:20: #include
"chrome/browser/extensions/extension_prefs.h"
On 2013/10/15 18:56:00, bbudge1 wrote:
> I don't think we need this any more.

Done.

https://codereview.chromium.org/27273004/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:46: const char
kUnregisteredPreference = "unregistered.preference";
On 2013/10/15 18:56:00, bbudge1 wrote:
> I wonder if an empty string might be a better value. Or you could replace the
> '.' by '_' or just 'unregistered'.

Used "_" -- it's cuter ;)... but more seriously I think that's never likely to
be a real preference name.

[Alexei Svitkine (slow), 2013-10-15 19:11:16]

lgtm

[gab, 2013-10-15 21:32:09]

+battre for OWNERS, feel free to CQ

Thanks!
Gab

[gab, 2013-10-16 12:34:27]

Bernard PTAL, feel free to CQ.

Thanks!
Gab

[Bernhard Bauer, 2013-10-16 12:45:38]

LGTM with a somewhat unrelated opportunity for yak shaving. I'm gonna CQ as
requested, but it would be nice to fix in a followup commit.

https://codereview.chromium.org/27273004/diff/7001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/27273004/diff/7001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_metrics_service.cc:72: }  // namespace
Nit: Everything in here is const, which automatically gets internal linkage, so
the anonymous namespace isn't really necessary (and neither is the `static` two
lines above).

[commit-bot: I haz the power, 2013-10-16 12:45:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/27273004/7001

[gab, 2013-10-16 13:22:17]

Thanks, see reply below.

https://codereview.chromium.org/27273004/diff/7001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/27273004/diff/7001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_metrics_service.cc:72: }  // namespace
On 2013/10/16 12:45:38, Bernhard Bauer wrote:
> Nit: Everything in here is const, which automatically gets internal linkage,
so
> the anonymous namespace isn't really necessary (and neither is the `static`
two
> lines above).

I'm happy to clean this up in a follow-up CL, I agree that we should remove the
static two lines above as its redundant with the unnamed namespace, but I like
keeping the unnamed namespace itself to be explicit about the internal scope of
those variables (such is the style in most files in Chromium AFAIK).

WDYT?

[Bernhard Bauer, 2013-10-16 13:40:35]

On 2013/10/16 13:22:17, gab wrote:
> Thanks, see reply below.
> 
>
https://codereview.chromium.org/27273004/diff/7001/chrome/browser/prefs/pref_...
> File chrome/browser/prefs/pref_metrics_service.cc (right):
> 
>
https://codereview.chromium.org/27273004/diff/7001/chrome/browser/prefs/pref_...
> chrome/browser/prefs/pref_metrics_service.cc:72: }  // namespace
> On 2013/10/16 12:45:38, Bernhard Bauer wrote:
> > Nit: Everything in here is const, which automatically gets internal linkage,
> so
> > the anonymous namespace isn't really necessary (and neither is the `static`
> two
> > lines above).
> 
> I'm happy to clean this up in a follow-up CL, I agree that we should remove
the
> static two lines above as its redundant with the unnamed namespace, but I like
> keeping the unnamed namespace itself to be explicit about the internal scope
of
> those variables (such is the style in most files in Chromium AFAIK).
> 
> WDYT?

Yeah, that's fine with me.

[commit-bot: I haz the power, 2013-10-16 16:47:30]

Change committed as 228934