Fix homepage migration for users who never changed their settings.

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 447df6523efc57b16e18fcdbf7e666bb4dc04e77..1d4aad838ea420d625ee81ae94c8cf7d532c59ac 100644
--- a/chrome/browser/prefs/session_startup_pref.cc
+++ b/chrome/browser/prefs/session_startup_pref.cc
@@ -42,10 +42,13 @@ int TypeToPrefValue(SessionStartupPref::Type type) {
 }
 
 void SetNewURLList(PrefService* prefs) {
-  ListValue new_url_pref_list;
-  StringValue* home_page = new StringValue(prefs->GetString(prefs::kHomePage));
-  new_url_pref_list.Append(home_page);
-  prefs->Set(prefs::kURLsToRestoreOnStartup, new_url_pref_list);
+  if (prefs->IsUserModifiablePreference(prefs::kURLsToRestoreOnStartup)) {
+    ListValue new_url_pref_list;
+    StringValue* home_page =
+        new StringValue(prefs->GetString(prefs::kHomePage));
+    new_url_pref_list.Append(home_page);
+    prefs->Set(prefs::kURLsToRestoreOnStartup, new_url_pref_list);
+  }
 }
 
 void URLListToPref(const base::ListValue* url_list, SessionStartupPref* pref) {
@@ -70,6 +73,9 @@ void SessionStartupPref::RegisterUserPrefs(PrefService* prefs) {
                              PrefService::SYNCABLE_PREF);
   prefs->RegisterListPref(prefs::kURLsToRestoreOnStartup,
                           PrefService::SYNCABLE_PREF);
+  prefs->RegisterBooleanPref(prefs::kRestoreOnStartupMigrated,
+                             false,
+                             PrefService::UNSYNCABLE_PREF);
 }
 
 // static
@@ -129,17 +135,17 @@ SessionStartupPref SessionStartupPref::GetStartupPref(Profile* profile) {
 // static
 SessionStartupPref SessionStartupPref::GetStartupPref(PrefService* prefs) {
   DCHECK(prefs);
+
+  MigrateIfNecessary(prefs);
+
   SessionStartupPref pref(
       PrefValueToType(prefs->GetInteger(prefs::kRestoreOnStartup)));
 
-  // Migrate from "Open the home page" to "Open the following URLs". If the user
-  // had the "Open the homepage" option selected, then we need to switch them to
-  // "Open the following URLs" and set the list of URLs to a list containing
-  // just the user's homepage.
-  if (pref.type == SessionStartupPref::HOMEPAGE) {
-    prefs->SetInteger(prefs::kRestoreOnStartup, kPrefValueURLs);
-    pref.type = SessionStartupPref::URLS;
-    SetNewURLList(prefs);
+  if (pref.type == HOMEPAGE) {
+    // We've already migrated, so the only way this can happen is if
+    // kRestoreOnStartup is controlled by a policy. In that case, act as though
+    // URLS was selected.

[Mattias Nissler (ping if slow), 2012/04/18 11:40:54]

We should change RestoreOnStartupPolicyHandler in
configuration_policy_handler.cc to translate policy to the correct pref then. OK
to do in a follow-up CL though. Can you file a bug and put a reference here?

[Tyler Breisacher (Chromium), 2012/04/18 18:26:03]

http://crbug.com/124027

The patch I just uploaded, I wrote as though that had already been fixed. This
makes it a lot easier to write, of course, but if the "RestoreOnStartup" policy
is set to 0, we hit a NOTREACHED() in browser_init.cc

So it might be better to fix the policy-preference mapping first, and then come
back to this CL.

[Mattias Nissler (ping if slow), 2012/04/19 13:23:17]

Thanks for fixing the policy!

+    pref.type = URLS;
   }
 
   // Always load the urls, even if the pref type isn't URLS. This way the
@@ -151,6 +157,30 @@ SessionStartupPref SessionStartupPref::GetStartupPref(PrefService* prefs) {
 }
 
 // static
+void SessionStartupPref::MigrateIfNecessary(PrefService* prefs) {
+  DCHECK(prefs);
+
+  if (!prefs->GetBoolean(prefs::kRestoreOnStartupMigrated)) {
+
+    if (prefs->GetInteger(prefs::kRestoreOnStartup) == kPrefValueHomePage ||
+        (!prefs->HasPrefPath(prefs::kRestoreOnStartup) &&
+         !prefs->GetBoolean(prefs::kHomePageIsNewTabPage))) {
+
+      if (prefs->GetBoolean(prefs::kHomePageIsNewTabPage)) {

[Mattias Nissler (ping if slow), 2012/04/18 11:40:54]

This still breaks if prefs::kHomePageIsNewTabPage is under policy control, in
which case you write the policy-enforced value to user prefs.

[Tyler Breisacher (Chromium), 2012/04/18 18:26:03]

I don't think we change the value of kHomePageIsNewTabPage during the migration.

[Mattias Nissler (ping if slow), 2012/04/19 13:23:17]

What I meant was that the pref value you get in line 169 could actually be not
what the user configured, but what policy enforced. In that case you make a
wrong decision, since you're really meaning to migrate user choice, and not
policy-adjusted user choice.

+        if (prefs->IsUserModifiablePreference(prefs::kRestoreOnStartup))
+          prefs->SetInteger(prefs::kRestoreOnStartup, kPrefValueNewTab);
+      } else {
+        if (prefs->IsUserModifiablePreference(prefs::kRestoreOnStartup))
+          prefs->SetInteger(prefs::kRestoreOnStartup, kPrefValueURLs);
+        SetNewURLList(prefs);
+      }
+    }
+
+    prefs->SetBoolean(prefs::kRestoreOnStartupMigrated, true);
+  }
+}
+
+// static
 bool SessionStartupPref::TypeIsManaged(PrefService* prefs) {
   DCHECK(prefs);
   const PrefService::Preference* pref_restore =