Migrate startup URLs pref.

chrome/browser/prefs/session_startup_pref.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/prefs/session_startup_pref.h"
 
 #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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/net/url_fixer_upper.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 
 #if defined(OS_MACOSX)
 #include "chrome/browser/ui/cocoa/window_restore_utils.h"
 #endif
 
 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) {
     case SessionStartupPref::LAST: return SessionStartupPref::kPrefValueLast;
     case SessionStartupPref::URLS: return SessionStartupPref::kPrefValueURLs;
     default:                       return SessionStartupPref::kPrefValueNewTab;
   }
 }
 
 void SetNewURLList(PrefService* prefs) {
@@ skipping 21 matching lines @@
 
 // static
 void SessionStartupPref::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterIntegerPref(
       prefs::kRestoreOnStartup,
       TypeToPrefValue(GetDefaultStartupType()),
       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
 SessionStartupPref::Type SessionStartupPref::GetDefaultStartupType() {
 #if defined(OS_CHROMEOS)
   return SessionStartupPref::LAST;
 #else
   return SessionStartupPref::DEFAULT;
 #endif
 }
@@ skipping 50 matching lines @@
   const ListValue* url_list = prefs->GetList(prefs::kURLsToRestoreOnStartup);
   URLListToPref(url_list, &pref);
 
   return pref;
 }
 
 // static
 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;
     if (homepage_is_new_tab_page_value) {
       if (!homepage_is_new_tab_page_value->GetAsBoolean(
               &homepage_is_new_tab_page))
         NOTREACHED();
     }
 
     const base::Value* restore_on_startup_value =
@@ skipping 81 matching lines @@
     case kPrefValueLast:     return SessionStartupPref::LAST;
     case kPrefValueURLs:     return SessionStartupPref::URLS;
     case kPrefValueHomePage: return SessionStartupPref::HOMEPAGE;
     default:                 return SessionStartupPref::DEFAULT;
   }
 }
 
 SessionStartupPref::SessionStartupPref(Type type) : type(type) {}
 
 SessionStartupPref::~SessionStartupPref() {}