Migrate startup URLs pref.

chrome/browser/prefs/session_startup_pref.cc

Index: chrome/browser/prefs/session_startup_pref.cc
diff --git a/chrome/browser/prefs/session_startup_pref.cc b/chrome/browser/prefs/session_startup_pref.cc
index 478790a157d1c44ab61dc8fc84c2fae754dba090..faa0845aec9f73857c0824b9e5c56ae5a72589e1 100644
--- a/chrome/browser/prefs/session_startup_pref.cc
+++ b/chrome/browser/prefs/session_startup_pref.cc
@@ -6,7 +6,9 @@
 
 #include <string>
 
+#include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
+#include "base/time/time.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
@@ -21,6 +23,13 @@
 
 namespace {
 
+enum StartupURLsMigrationMetrics {
+  STARTUP_URLS_MIGRATION_METRICS_STARTED,

[Alexei Svitkine (slow), 2013/10/15 14:40:55]

I'm not sure _STARTED is accurate here. Perhaps _PERFORMED would be a better
choice?

[robertshield, 2013/10/15 18:00:44]

Done.

+  STARTUP_URLS_MIGRATION_METRICS_DELETED,
+  STARTUP_URLS_MIGRATION_METRICS_RESET,
+  STARTUP_URLS_MIGRATION_METRICS_MAX,
+};
+
 // Converts a SessionStartupPref::Type to an integer written to prefs.
 int TypeToPrefValue(SessionStartupPref::Type type) {
   switch (type) {
@@ -62,10 +71,16 @@ void SessionStartupPref::RegisterProfilePrefs(
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
   registry->RegisterListPref(prefs::kURLsToRestoreOnStartup,
                              user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterListPref(prefs::kURLsToRestoreOnStartupOld,
+                             user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
   registry->RegisterBooleanPref(
       prefs::kRestoreOnStartupMigrated,
       false,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterInt64Pref(
+      prefs::kRestoreStartupURLsMigrated,
+      false,
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
 // static
@@ -136,8 +151,53 @@ SessionStartupPref SessionStartupPref::GetStartupPref(PrefService* prefs) {
 void SessionStartupPref::MigrateIfNecessary(PrefService* prefs) {
   DCHECK(prefs);
 
+  // Check if we need to migrate the old version of the startup URLs preference
+  // to the new name, and also send metrics about the migration.
+  const base::ListValue* old_startup_urls =
+      prefs->GetList(prefs::kURLsToRestoreOnStartupOld);
+  if (!prefs->GetUserPrefValue(prefs::kRestoreStartupURLsMigrated)) {
+    // Seems like we never migrated, do it if necessary.
+    if (!prefs->GetUserPrefValue(prefs::kURLsToRestoreOnStartup)) {
+      if (old_startup_urls && old_startup_urls->GetSize() > 0) {

[Alexei Svitkine (slow), 2013/10/15 14:40:55]

Nit: use !old_startup_urls->empty()

[robertshield, 2013/10/15 18:00:44]

Done.

+        prefs->Set(prefs::kURLsToRestoreOnStartup, *old_startup_urls);
+        prefs->Set(prefs::kURLsToRestoreOnStartupOld, base::ListValue());
+      }
+      UMA_HISTOGRAM_ENUMERATION(
+          "Settings.StartupURLsMigration",
+          STARTUP_URLS_MIGRATION_METRICS_STARTED,

[Alexei Svitkine (slow), 2013/10/15 14:40:55]

Nit: Can you move the UMA macro below outside the if and make a local var that's
the enum value that you'll log. This way, you won't need to repeat the macro two
times.

Alternatively, make a small function in the anon namespace that takes the enum
value as a param and logs the histogram. Then, you can also call it for the
STARTUP_URLS_MIGRATION_METRICS_RESET case below.

Context: Each histogram macro expands to a large-ish block of code, so it's
better to avoid repeating them (i.e. better for code size, etc).

[robertshield, 2013/10/15 18:00:44]

Done.

+          STARTUP_URLS_MIGRATION_METRICS_MAX);
+    } else {
+      // We migrated but the migration data got deleted somehow.

[Alexei Svitkine (slow), 2013/10/15 14:40:55]

Wouldn't this case be quite normal if there were simply no startup urls in prefs
but the migration code hadn't run yet? If so, perhaps update the comment / enum
name (e.g. to _NO_URLS_TO_MIGRATE) for example.

[robertshield, 2013/10/15 18:00:44]

I believe you are correct. Updated the name to
STARTUP_URLS_MIGRATION_METRICS_NOT_PRESENT and updated the value in
histograms.xml as well.

+      UMA_HISTOGRAM_ENUMERATION(
+          "Settings.StartupURLsMigration",
+          STARTUP_URLS_MIGRATION_METRICS_DELETED,
+          STARTUP_URLS_MIGRATION_METRICS_MAX);
+    }
+    prefs->SetInt64(prefs::kRestoreStartupURLsMigrated,
+                    base::Time::Now().ToInternalValue());
+  } else if (old_startup_urls && old_startup_urls->GetSize() > 0) {

[Alexei Svitkine (slow), 2013/10/15 14:40:55]

Nit: old_startup_urls->empty()

[robertshield, 2013/10/15 18:00:44]

Done.

+    // Migration needs to be reset.
+    prefs->Set(prefs::kURLsToRestoreOnStartupOld, base::ListValue());
+    base::Time last_migration_time = base::Time::FromInternalValue(
+        prefs->GetInt64(prefs::kRestoreStartupURLsMigrated));
+    base::Time now = base::Time::Now();
+    prefs->SetInt64(prefs::kRestoreStartupURLsMigrated, now.ToInternalValue());
+    DCHECK(now > last_migration_time);

[Alexei Svitkine (slow), 2013/10/15 14:40:55]

Can DCHECK_GT() work here?

[robertshield, 2013/10/15 18:00:44]

Sadly, no. I tried and it doesn't work with base::Time due to a lack of an
operator<< implementation.

+    if (now < last_migration_time)
+      last_migration_time = now;
+    HISTOGRAM_CUSTOM_TIMES("Settings.StartupURLsResetTime",
+                           now - last_migration_time,
+                           base::TimeDelta::FromDays(0),
+                           base::TimeDelta::FromDays(7),
+                           50);
+    UMA_HISTOGRAM_ENUMERATION(
+        "Settings.StartupURLsMigration",
+        STARTUP_URLS_MIGRATION_METRICS_RESET,
+        STARTUP_URLS_MIGRATION_METRICS_MAX);
+  }
+
   if (!prefs->GetBoolean(prefs::kRestoreOnStartupMigrated)) {
-    // Read existing values
+    // Read existing values.
     const base::Value* homepage_is_new_tab_page_value =
         prefs->GetUserPrefValue(prefs::kHomePageIsNewTabPage);
     bool homepage_is_new_tab_page = true;