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..995dd5de0e1ec9d662b56cac0cefb46e30b92d47 100644
--- a/chrome/browser/prefs/session_startup_pref.cc
+++ b/chrome/browser/prefs/session_startup_pref.cc
@@ -132,14 +132,29 @@ SessionStartupPref SessionStartupPref::GetStartupPref(PrefService* 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) {
+  // Migrate from "Open the home page" to "Open the following URLs". If the
+  // user had the "Open the home page" option selected, switch them to "Open
+  // the following URLs" and set the list of URLs to a one-item list containing
+  // their homepage. If the kRestoreOnStartup pref is blank, it could be a user
+  // coming from Chrome 18 or lower, who had "open the homepage" selected
+  // (since that was the default) or it could be a user who originally
+  // installed Chrome 19 or higher. We distinguish between these cases using
+  // the kHomePageIsNewTabPage pref: A Chrome 18 user wanting to open a
+  // homepage other than the NTP on startup would have set
+  // kHomePageIsNewTabPage to false. If kHomePageIsNewTabPage is true, there is
+  // no need to do the migration; the new default behavior is to open the NTP.
+  if (pref.type == SessionStartupPref::HOMEPAGE ||
+      (!prefs->HasPrefPath(prefs::kRestoreOnStartup) &&
+       !prefs->GetBoolean(prefs::kHomePageIsNewTabPage))) {

[Mattias Nissler (ping if slow), 2012/04/16 09:21:46]

This is likely to break in case there is policy present that affects these
prefs. For example, if policy forces prefs::kRestoreOnStartup to HOMEPAGE,
you'll run this migration code every time, because the
SetInteger(prefs::kRestoreOnStartup) below will be masked by policy.

     prefs->SetInteger(prefs::kRestoreOnStartup, kPrefValueURLs);
     pref.type = SessionStartupPref::URLS;
     SetNewURLList(prefs);
+  } else if (!prefs->HasPrefPath(prefs::kRestoreOnStartup)) {

[Mattias Nissler (ping if slow), 2012/04/16 09:21:46]

Setting a pref to the default is bad. This breaks situations where the user
hasn't made any decision and the admin wants to change the default through
recommended policy (which is lower priority than user prefs, so your forced
default will mask the admin-provided default).

+    // If we didn't do the migration, but the kRestoreOnStartup pref is blank,
+    // then set it explicitly to the default value, so that we won't do the
+    // migration the next time this function is called.
+    prefs->SetInteger(prefs::kRestoreOnStartup,
+                      TypeToPrefValue(GetDefaultStartupType()));
   }
 
   // Always load the urls, even if the pref type isn't URLS. This way the