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..53c0c868f0364b10b107856e77c689694e0c660c 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_PERFORMED,
+  STARTUP_URLS_MIGRATION_METRICS_NOT_PRESENT,
+  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,55 @@ 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)) {
+    // Record the absence of the migration timestamp, this will get overwritten
+    // below if migration occurs now.
+    StartupURLsMigrationMetrics metrics_result =
+        STARTUP_URLS_MIGRATION_METRICS_NOT_PRESENT;
+
+    // Seems like we never migrated, do it if necessary.
+    if (!prefs->GetUserPrefValue(prefs::kURLsToRestoreOnStartup)) {
+      if (old_startup_urls && !old_startup_urls->empty()) {

[Bernhard Bauer, 2013/10/15 22:03:52]

|old_startup_urls| is guaranteed not to be NULL.

[robertshield, 2013/10/16 01:23:27]

That does not line up with my reading of PrefService::GetList in
pref_service.cc. Am I misreading?

[Bernhard Bauer, 2013/10/16 15:13:33]

Well, it's guaranteed not to be NULL in the absence of bugs (as in, running into
a NOTREACHED() means a bug). Admittedly, the code in pref_service.cc that does
`NOTREACHED(); return NULL;` should not do that. If we would change it to a pure
DCHECK, it would make the contract clearer.

[robertshield, 2013/10/16 15:50:08]

On line 308
(https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_se...),
GetList returns NULL if the value type is not of type list. 

The value type is (afaict) ultimately derived from what is in the preferences
file, which is user-controlled data and should not be trusted to be intact.
Having a corrupted preferences file looks like it would cause NULL to be
returned and we should not crash in release builds if this happens. 

+        prefs->Set(prefs::kURLsToRestoreOnStartup, *old_startup_urls);
+        prefs->Set(prefs::kURLsToRestoreOnStartupOld, base::ListValue());

[Bernhard Bauer, 2013/10/15 22:03:52]

You can use prefs->ClearPref().

[robertshield, 2013/10/16 01:23:27]

Done.

+      }
+      metrics_result = STARTUP_URLS_MIGRATION_METRICS_PERFORMED;
+    }
+
+    UMA_HISTOGRAM_ENUMERATION(
+          "Settings.StartupURLsMigration",
+          metrics_result,
+          STARTUP_URLS_MIGRATION_METRICS_MAX);
+
+    prefs->SetInt64(prefs::kRestoreStartupURLsMigrated,
+                    base::Time::Now().ToInternalValue());
+  } else if (old_startup_urls && !old_startup_urls->empty()) {
+    // Migration needs to be reset.
+    prefs->Set(prefs::kURLsToRestoreOnStartupOld, base::ListValue());

[Bernhard Bauer, 2013/10/15 22:03:52]

It seems like you are doing a lot of similar things in the two branches here
(clear the old value, set the new migration time). Would it simplify things if
you merged the two branches?

[robertshield, 2013/10/16 01:23:27]

Played around with the logic a bit. I extracted the histogram logic (which helps
with code size, as Alexei pointed out UMA macros expand to mucho code) but
wasn't able to make this any shorter without making it more convoluted. Let me
know if you have a better factorization in mind.

+    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);

[Bernhard Bauer, 2013/10/15 22:03:52]

Nit: DCHECK_GT(now, last_migration_time) gives nicer messages in case of a check
failure.

[robertshield, 2013/10/16 01:23:27]

Sadly, DCHECK_GT doesn't work with base::Time instances. Seems there's a missing
operator<< or some such. This said, I'll remove the DCHECK based on your next
comment.

+    if (now < last_migration_time)

[Bernhard Bauer, 2013/10/15 22:03:52]

This looks like you're handling a DCHECK failure? You're not supposed to do
that, see
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-.

[robertshield, 2013/10/16 01:23:27]

Removing the DCHECK, there's no reason for it to be there.

+      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;