Move kInstallDate from chrome/common/pref_names.h to components/metrics/metrics_pref_names.h

Move kInstallDate from chrome/common/pref_names.h to components/metrics/metrics_pref_names.h

This is a precursor CL to having MetricsStateManager be able to recover missing client_ids and restore old installation dates.

BUG=391338
TBR=sky@chromium.org

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

[gab, 2014-07-07 22:27:04]

Ilya PTAL at this in the context of https://codereview.chromium.org/372473004/

Thanks!
Gab

[Ilya Sherman, 2014-07-08 03:26:33]

https://codereview.chromium.org/370813003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/370813003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_metrics_service_client.cc:147:
registry->RegisterInt64Pref(metrics::prefs::kInstallDate, 0);
It would probably be more appropriate to move this into the metrics component as
well; likewise, the GetInstallDate() call below.

https://codereview.chromium.org/370813003/diff/1/chrome/browser/ui/autofill/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/370813003/diff/1/chrome/browser/ui/autofill/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2840:
->GetMetricsService()
nit: "g_browser_process->metrics_service()"

https://codereview.chromium.org/370813003/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/370813003/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1400: // opt-in services such as Wallet and UMA.
nit: Please update this comment.

[Ilya Sherman, 2014-07-08 03:26:59]

By the way, thanks for splitting off these precursor CLs from the main one -- it
definitely makes things easier to review!

[gab, 2014-07-08 17:41:02]

Thanks, PTAL.

https://codereview.chromium.org/370813003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/370813003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_metrics_service_client.cc:147:
registry->RegisterInt64Pref(metrics::prefs::kInstallDate, 0);
On 2014/07/08 03:26:33, Ilya Sherman wrote:
> It would probably be more appropriate to move this into the metrics component
as
> well; likewise, the GetInstallDate() call below.

Ok, that makes it such that MetricsLog::RecordEnvironment() now has to take an
|install_date| param (since it's under the MetricsService and I don't think we
want to introduce a loop just to make GetInstallDate() accessible).

But I think that's okay.. up to you.

https://codereview.chromium.org/370813003/diff/1/chrome/browser/ui/autofill/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/370813003/diff/1/chrome/browser/ui/autofill/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2840:
->GetMetricsService()
On 2014/07/08 03:26:33, Ilya Sherman wrote:
> nit: "g_browser_process->metrics_service()"

Done.

https://codereview.chromium.org/370813003/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/370813003/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1400: // opt-in services such as Wallet and UMA.
On 2014/07/08 03:26:33, Ilya Sherman wrote:
> nit: Please update this comment.

Done.

[gab, 2014-07-08 17:41:37]

On 2014/07/08 03:26:59, Ilya Sherman wrote:
> By the way, thanks for splitting off these precursor CLs from the main one --
it
> definitely makes things easier to review!

NP, only way to draw a clear picture IMO :-).

[Ilya Sherman, 2014-07-08 23:35:30]

LGTM, thanks.

[gab, 2014-07-08 23:51:06]

Thanks,

TBR sky for refactoring side-effects on:
chrome/browser/chrome_browser_field_trials.cc
chrome/browser/chrome_browser_main.cc
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc

[gab, 2014-07-08 23:51:15]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-07-08 23:52:10]

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

[commit-bot: I haz the power, 2014-07-09 05:42:44]

Change committed as 281960

[Alexei Svitkine (slow), 2014-07-14 17:00:53]

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:946: base::Time::Now().ToTimeT());
I don't like the fact that we're recording install date directly here and then
accessing it from MetricsService.

I think it would be better to move the two together. Can you create a follow-up
CL that adds a static MetricsService::InitInstallDate() method with this code
and call it from here? This way, the setting and querying install date can be
done entirely in the metrics component.

[gab, 2014-07-14 19:38:13]

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:946: base::Time::Now().ToTimeT());
On 2014/07/14 17:00:53, Alexei Svitkine wrote:
> I don't like the fact that we're recording install date directly here and then
> accessing it from MetricsService.
> 
> I think it would be better to move the two together. Can you create a
follow-up
> CL that adds a static MetricsService::InitInstallDate() method with this code
> and call it from here? This way, the setting and querying install date can be
> done entirely in the metrics component.

SGTM, in fact how about we just do this automatically when MetricsService
initializes? (i.e. no need to tell it from outside that it's time to init the
install date... it should just do it as soon as it sees it's missing on startup)

[Alexei Svitkine (slow), 2014-07-14 19:40:38]

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:946: base::Time::Now().ToTimeT());
On 2014/07/14 19:38:13, gab wrote:
> On 2014/07/14 17:00:53, Alexei Svitkine wrote:
> > I don't like the fact that we're recording install date directly here and
then
> > accessing it from MetricsService.
> > 
> > I think it would be better to move the two together. Can you create a
> follow-up
> > CL that adds a static MetricsService::InitInstallDate() method with this
code
> > and call it from here? This way, the setting and querying install date can
be
> > done entirely in the metrics component.
> 
> SGTM, in fact how about we just do this automatically when MetricsService
> initializes? (i.e. no need to tell it from outside that it's time to init the
> install date... it should just do it as soon as it sees it's missing on
startup)

That sounds even better assuming it's safe to do (i.e. nothing else uses that
value between this point and when MetricsService is initialized.)

[gab, 2014-07-14 19:52:37]

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:946: base::Time::Now().ToTimeT());
On 2014/07/14 19:40:38, Alexei Svitkine wrote:
> On 2014/07/14 19:38:13, gab wrote:
> > On 2014/07/14 17:00:53, Alexei Svitkine wrote:
> > > I don't like the fact that we're recording install date directly here and
> then
> > > accessing it from MetricsService.
> > > 
> > > I think it would be better to move the two together. Can you create a
> > follow-up
> > > CL that adds a static MetricsService::InitInstallDate() method with this
> code
> > > and call it from here? This way, the setting and querying install date can
> be
> > > done entirely in the metrics component.
> > 
> > SGTM, in fact how about we just do this automatically when MetricsService
> > initializes? (i.e. no need to tell it from outside that it's time to init
the
> > install date... it should just do it as soon as it sees it's missing on
> startup)
> 
> That sounds even better assuming it's safe to do (i.e. nothing else uses that
> value between this point and when MetricsService is initialized.)

Well that's the same (slightly earlier in fact) as calling
MetricsService::InitInstallDate() as you originally suggested (unless you meant
to make that a static method that can be called without actually having a
service which sounds weird to me...).

The only potential early getter of this pref is
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
is g_browser_process->metrics_service() up then? Can we use GetInstallDate()
there?

[Alexei Svitkine (slow), 2014-07-15 15:49:33]

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:946: base::Time::Now().ToTimeT());
On 2014/07/14 19:52:37, gab wrote:
> On 2014/07/14 19:40:38, Alexei Svitkine wrote:
> > On 2014/07/14 19:38:13, gab wrote:
> > > On 2014/07/14 17:00:53, Alexei Svitkine wrote:
> > > > I don't like the fact that we're recording install date directly here
and
> > then
> > > > accessing it from MetricsService.
> > > > 
> > > > I think it would be better to move the two together. Can you create a
> > > follow-up
> > > > CL that adds a static MetricsService::InitInstallDate() method with this
> > code
> > > > and call it from here? This way, the setting and querying install date
can
> > be
> > > > done entirely in the metrics component.
> > > 
> > > SGTM, in fact how about we just do this automatically when MetricsService
> > > initializes? (i.e. no need to tell it from outside that it's time to init
> the
> > > install date... it should just do it as soon as it sees it's missing on
> > startup)
> > 
> > That sounds even better assuming it's safe to do (i.e. nothing else uses
that
> > value between this point and when MetricsService is initialized.)
> 
> Well that's the same (slightly earlier in fact) as calling
> MetricsService::InitInstallDate() as you originally suggested (unless you
meant
> to make that a static method that can be called without actually having a
> service which sounds weird to me...).
> 
> The only potential early getter of this pref is
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> is g_browser_process->metrics_service() up then? Can we use GetInstallDate()
> there?

I was originally suggesting it to be a static method called from here. While
weird, it would still be a strict improvement over the current code.

But looking at the code you linked, I think it should be fine to put it into the
ctor of MetricsService and to expose a getter from it to retrieve. The linked
code runs after MetricsService has been created, so it should be fine to pass as
a param to the SetupFieldTrials method from:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

[gab, 2014-07-15 22:13:15]

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/370813003/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:946: base::Time::Now().ToTimeT());
On 2014/07/15 15:49:33, Alexei Svitkine wrote:
> On 2014/07/14 19:52:37, gab wrote:
> > On 2014/07/14 19:40:38, Alexei Svitkine wrote:
> > > On 2014/07/14 19:38:13, gab wrote:
> > > > On 2014/07/14 17:00:53, Alexei Svitkine wrote:
> > > > > I don't like the fact that we're recording install date directly here
> and
> > > then
> > > > > accessing it from MetricsService.
> > > > > 
> > > > > I think it would be better to move the two together. Can you create a
> > > > follow-up
> > > > > CL that adds a static MetricsService::InitInstallDate() method with
this
> > > code
> > > > > and call it from here? This way, the setting and querying install date
> can
> > > be
> > > > > done entirely in the metrics component.
> > > > 
> > > > SGTM, in fact how about we just do this automatically when
MetricsService
> > > > initializes? (i.e. no need to tell it from outside that it's time to
init
> > the
> > > > install date... it should just do it as soon as it sees it's missing on
> > > startup)
> > > 
> > > That sounds even better assuming it's safe to do (i.e. nothing else uses
> that
> > > value between this point and when MetricsService is initialized.)
> > 
> > Well that's the same (slightly earlier in fact) as calling
> > MetricsService::InitInstallDate() as you originally suggested (unless you
> meant
> > to make that a static method that can be called without actually having a
> > service which sounds weird to me...).
> > 
> > The only potential early getter of this pref is
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> > is g_browser_process->metrics_service() up then? Can we use GetInstallDate()
> > there?
> 
> I was originally suggesting it to be a static method called from here. While
> weird, it would still be a strict improvement over the current code.
> 
> But looking at the code you linked, I think it should be fine to put it into
the
> ctor of MetricsService and to expose a getter from it to retrieve. The linked
> code runs after MetricsService has been created, so it should be fine to pass
as
> a param to the SetupFieldTrials method from:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

Follow-up CL posted @ https://codereview.chromium.org/396753005/