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

chrome/browser/chrome_browser_main.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chrome_browser_main.h"
 
 #include <set>
 #include <string>
 #include <vector>
 
@@ skipping 87 matching lines @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/crash_keys.h"
 #include "chrome/common/env_vars.h"
 #include "chrome/common/logging_chrome.h"
 #include "chrome/common/net/net_resource_provider.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/profiling.h"
 #include "chrome/installer/util/google_update_settings.h"
 #include "components/google/core/browser/google_util.h"
 #include "components/language_usage_metrics/language_usage_metrics.h"
+#include "components/metrics/metrics_pref_names.h"
 #include "components/metrics/metrics_service.h"
 #include "components/nacl/browser/nacl_browser.h"
 #include "components/nacl/browser/nacl_process_host.h"
 #include "components/rappor/rappor_service.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "components/startup_metric_utils/startup_metric_utils.h"
 #include "components/translate/core/browser/translate_download_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ skipping 814 matching lines @@
   base::debug::SetCrashKeyValue(crash_keys::kChannel,
       chrome::VersionInfo::GetVersionStringModifier());
 #endif
 
   // Initialize tracking synchronizer system.
   tracking_synchronizer_ = new chrome_browser_metrics::TrackingSynchronizer();
 
   // Now that all preferences have been registered, set the install date
   // for the uninstall metrics if this is our first run. This only actually
   // gets used if the user has metrics reporting enabled at uninstall time.
-  int64 install_date = local_state_->GetInt64(prefs::kInstallDate);
-  if (install_date == 0)
-    local_state_->SetInt64(prefs::kInstallDate, base::Time::Now().ToTimeT());
+  int64 install_date = local_state_->GetInt64(metrics::prefs::kInstallDate);
+  if (install_date == 0) {
+    local_state_->SetInt64(metrics::prefs::kInstallDate,
+                           base::Time::Now().ToTimeT());

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

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]

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]

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]

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]

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]

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

+  }
 
 #if defined(OS_MACOSX)
   // Get the Keychain API to register for distributed notifications on the main
   // thread, which has a proper CFRunloop, instead of later on the I/O thread,
   // which doesn't. This ensures those notifications will get delivered
   // properly. See issue 37766.
   // (Note that the callback mask here is empty. I don't want to register for
   // any callbacks, I just want to initialize the mechanism.)
   SecKeychainAddCallback(&KeychainCallback, 0, NULL);
 #endif
@@ skipping 701 matching lines @@
   chromeos::CrosSettings::Shutdown();
 #endif
 #endif
 }
 
 // Public members:
 
 void ChromeBrowserMainParts::AddParts(ChromeBrowserMainExtraParts* parts) {
   chrome_extra_parts_.push_back(parts);
 }