Fix homepage migration for users who never changed their settings.

BUG=122936
TEST=Upgrade from 18 to 19
TBR=ben@chromium.org

Review URL: https://chromiumcodereview.appspot.com/10049005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@133740 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/prefs/pref_service.cc

@@ -684,6 +684,31 @@ const DictionaryValue* PrefService::GetDictionary(const char* path) const {
   return static_cast<const DictionaryValue*>(value);
 }
 
+const base::Value* PrefService::GetUserPrefValue(const char* path) const {
+  DCHECK(CalledOnValidThread());
+
+  const Preference* pref = FindPreference(path);
+  if (!pref) {
+    NOTREACHED() << "Trying to get an unregistered pref: " << path;
+    return NULL;
+  }
+
+  // Look for an existing preference in the user store. If it doesn't
+  // exist, return NULL.
+  base::Value* value = NULL;
+  if (user_pref_store_->GetMutableValue(path, &value) !=
+          PersistentPrefStore::READ_OK) {
+    return NULL;
+  }
+
+  if (!value->IsType(pref->GetType())) {
+    NOTREACHED() << "Pref value type doesn't match registered type.";
+    return NULL;
+  }
+
+  return value;
+}
+
 const ListValue* PrefService::GetList(const char* path) const {
   DCHECK(CalledOnValidThread());
 

chrome/browser/prefs/pref_service.h

@@ -267,6 +267,10 @@ class PrefService : public base::NonThreadSafe {
   const base::DictionaryValue* GetDictionary(const char* path) const;
   const base::ListValue* GetList(const char* path) const;
 
+  // Returns the value of the given preference, from the user pref store. If
+  // the preference is not set in the user pref store, returns NULL.
+  const base::Value* GetUserPrefValue(const char* path) const;
+
   // Removes a user pref and restores the pref to its default value.
   void ClearPref(const char* path);
 

chrome/browser/prefs/session_startup_pref.cc

@@ -35,10 +35,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)) {
+    base::ListValue new_url_pref_list;
+    base::StringValue* home_page =
+        new base::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) {
@@ -61,6 +64,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
@@ -120,19 +126,12 @@ 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);
-  }
-
   // Always load the urls, even if the pref type isn't URLS. This way the
   // preferences panels can show the user their last choice.
   const ListValue* url_list = prefs->GetList(prefs::kURLsToRestoreOnStartup);
@@ -141,6 +140,48 @@ SessionStartupPref SessionStartupPref::GetStartupPref(PrefService* prefs) {
   return pref;
 }
 
+// static
+void SessionStartupPref::MigrateIfNecessary(PrefService* prefs) {
+  DCHECK(prefs);
+
+  if (!prefs->GetBoolean(prefs::kRestoreOnStartupMigrated)) {
+    // 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)
+      homepage_is_new_tab_page_value->GetAsBoolean(&homepage_is_new_tab_page);
+
+    const base::Value* restore_on_startup_value =
+        prefs->GetUserPrefValue(prefs::kRestoreOnStartup);
+    int restore_on_startup = -1;
+    if (restore_on_startup_value)
+      restore_on_startup_value->GetAsInteger(&restore_on_startup);
+
+    // If restore_on_startup has the deprecated value kPrefValueHomePage,
+    // migrate it to open the homepage on startup. If 'homepage is NTP' is set,
+    // that means just opening the NTP. If not, it means opening a one-item URL
+    // list containing the homepage.
+    if (restore_on_startup == kPrefValueHomePage) {
+      if (homepage_is_new_tab_page) {
+        prefs->SetInteger(prefs::kRestoreOnStartup, kPrefValueNewTab);
+      } else {
+        prefs->SetInteger(prefs::kRestoreOnStartup, kPrefValueURLs);
+        SetNewURLList(prefs);
+      }
+    } else if (!restore_on_startup_value && !homepage_is_new_tab_page) {
+      // kRestoreOnStartup was never set by the user, but the homepage was set.
+      // Migrate to the list of URLs. (If restore_on_startup was never set,
+      // and homepage_is_new_tab_page is true, no action is needed. The new
+      // default value is "open the new tab page" which is what we want.)
+      prefs->SetInteger(prefs::kRestoreOnStartup, kPrefValueURLs);
+      SetNewURLList(prefs);
+    }
+
+    prefs->SetBoolean(prefs::kRestoreOnStartupMigrated, true);
+  }
+}
+
 // static
 bool SessionStartupPref::TypeIsManaged(PrefService* prefs) {
   DCHECK(prefs);

chrome/browser/prefs/session_startup_pref.h

@@ -53,6 +53,11 @@ struct SessionStartupPref {
   static SessionStartupPref GetStartupPref(Profile* profile);
   static SessionStartupPref GetStartupPref(PrefService* prefs);
 
+  // If the user had the "restore on startup" property set to the deprecated
+  // "Open the home page" value, this migrates them to a value that will have
+  // the same effect.
+  static void MigrateIfNecessary(PrefService* prefs);
+
   // Whether the startup type and URLs are managed via policy.
   static bool TypeIsManaged(PrefService* prefs);
   static bool URLsAreManaged(PrefService* prefs);

chrome/browser/prefs/session_startup_pref_unittest.cc

@@ -14,6 +14,7 @@ class SessionStartupPrefTest : public testing::Test {
   virtual void SetUp() {
     pref_service_.reset(new TestingPrefService);
     SessionStartupPref::RegisterUserPrefs(pref_service_.get());
+    pref_service_->RegisterBooleanPref(prefs::kHomePageIsNewTabPage, true);
   }
 
   scoped_ptr<TestingPrefService> pref_service_;
@@ -57,6 +58,43 @@ TEST_F(SessionStartupPrefTest, URLListManagedOverridesUser) {
   EXPECT_EQ(3u, result.urls.size());
 }
 
+// Checks to make sure that if the user had previously not selected anything
+// (so that, in effect, the default value "Open the homepage" was selected),
+// their preferences are migrated on upgrade to m19.
+TEST_F(SessionStartupPrefTest, DefaultMigration) {
+  pref_service_->RegisterStringPref(prefs::kHomePage, "http://google.com/");
+  pref_service_->SetString(prefs::kHomePage, "http://chromium.org/");
+  pref_service_->SetBoolean(prefs::kHomePageIsNewTabPage, false);
+
+  EXPECT_FALSE(pref_service_->HasPrefPath(prefs::kRestoreOnStartup));
+
+  SessionStartupPref pref = SessionStartupPref::GetStartupPref(
+      pref_service_.get());
+
+  EXPECT_EQ(SessionStartupPref::URLS, pref.type);
+  EXPECT_EQ(1U, pref.urls.size());
+  EXPECT_EQ(GURL("http://chromium.org/"), pref.urls[0]);
+}
+
+// Checks to make sure that if the user had previously not selected anything
+// (so that, in effect, the default value "Open the homepage" was selected),
+// and the NTP is being used for the homepage, their preferences are migrated
+// to "Open the New Tab Page" on upgrade to M19.
+TEST_F(SessionStartupPrefTest, DefaultMigrationHomepageIsNTP) {
+  pref_service_->RegisterStringPref(prefs::kHomePage, "http://google.com/");
+  pref_service_->SetString(prefs::kHomePage, "http://chromium.org/");
+  pref_service_->SetBoolean(prefs::kHomePageIsNewTabPage, true);
+
+  EXPECT_FALSE(pref_service_->HasPrefPath(prefs::kRestoreOnStartup));
+
+  SessionStartupPref pref = SessionStartupPref::GetStartupPref(
+      pref_service_.get());
+
+  EXPECT_EQ(SessionStartupPref::DEFAULT, pref.type);
+}
+
+// Checks to make sure that if the user had previously selected "Open the
+// "homepage", their preferences are migrated on upgrade to M19.
 TEST_F(SessionStartupPrefTest, HomePageMigration) {
   pref_service_->RegisterStringPref(prefs::kHomePage, "http://google.com/");
 
@@ -65,10 +103,31 @@ TEST_F(SessionStartupPrefTest, HomePageMigration) {
   // using the pref service directly.
   pref_service_->SetInteger(prefs::kRestoreOnStartup, /*kPrefValueHomePage*/ 0);
   pref_service_->SetString(prefs::kHomePage, "http://chromium.org/");
+  pref_service_->SetBoolean(prefs::kHomePageIsNewTabPage, false);
 
   SessionStartupPref pref = SessionStartupPref::GetStartupPref(
       pref_service_.get());
+
   EXPECT_EQ(SessionStartupPref::URLS, pref.type);
   EXPECT_EQ(1U, pref.urls.size());
   EXPECT_EQ(GURL("http://chromium.org/"), pref.urls[0]);
 }
+
+// Checks to make sure that if the user had previously selected "Open the
+// "homepage", and the NTP is being used for the homepage, their preferences
+// are migrated on upgrade to M19.
+TEST_F(SessionStartupPrefTest, HomePageMigrationHomepageIsNTP) {
+  pref_service_->RegisterStringPref(prefs::kHomePage, "http://google.com/");
+
+  // By design, it's impossible to set the 'restore on startup' pref to 0
+  // ("open the homepage") using SessionStartupPref::SetStartupPref(), so set it
+  // using the pref service directly.
+  pref_service_->SetInteger(prefs::kRestoreOnStartup, /*kPrefValueHomePage*/ 0);
+  pref_service_->SetString(prefs::kHomePage, "http://chromium.org/");
+  pref_service_->SetBoolean(prefs::kHomePageIsNewTabPage, true);
+
+  SessionStartupPref pref = SessionStartupPref::GetStartupPref(
+      pref_service_.get());
+
+  EXPECT_EQ(SessionStartupPref::DEFAULT, pref.type);
+}

chrome/browser/protector/protected_prefs_watcher.cc

@@ -151,8 +151,7 @@ void ProtectedPrefsWatcher::Observe(
 }
 
 void ProtectedPrefsWatcher::EnsurePrefsMigration() {
-  // Force SessionStartupPref migration, if necessary.
-  SessionStartupPref::GetStartupPref(profile_);
+  SessionStartupPref::MigrateIfNecessary(profile_->GetPrefs());
 }
 
 bool ProtectedPrefsWatcher::UpdateCachedPrefs() {

chrome/browser/ui/browser_init.cc

@@ -1194,10 +1194,9 @@ Browser* BrowserInit::LaunchWithProfile::ProcessSpecifiedURLs(
     UrlsToTabs(urls, &tabs);
 
   } else if (pref.type == SessionStartupPref::HOMEPAGE) {
-    // If the user had 'homepage' selected, we should have migrated them to
-    // 'URLs' instead.
-    DLOG(ERROR) << "pref.type == HOMEPAGE";
-    NOTREACHED();
+    // If 'homepage' selected, either by the user or by a policy, we should
+    // have migrated them to another value.
+    NOTREACHED() << "SessionStartupPref has deprecated type HOMEPAGE";
   }
 
   if (tabs.empty())

chrome/common/pref_names.cc

@@ -43,18 +43,25 @@ const char kIsGooglePlusUser[] = "is_google_plus_user";
 const char kSessionExitedCleanly[] = "profile.exited_cleanly";
 
 // An integer pref. Holds one of several values:
-// 0: (or empty) don't do anything special on startup.
+// 0: (deprecated) open the homepage on startup
 // 1: restore the last session.
 // 2: this was used to indicate a specific session should be restored. It is
 //    no longer used, but saved to avoid conflict with old preferences.
 // 3: unused, previously indicated the user wants to restore a saved session.
 // 4: restore the URLs defined in kURLsToRestoreOnStartup.
+// 5: open the New Tab Page on startup (this is the default value).
 const char kRestoreOnStartup[] = "session.restore_on_startup";
 
 // The URLs to restore on startup or when the home button is pressed. The URLs
 // are only restored on startup if kRestoreOnStartup is 4.
 const char kURLsToRestoreOnStartup[] = "session.urls_to_restore_on_startup";
 
+// A preference to keep track of whether we have already checked whether we
+// need to migrate the user from kRestoreOnStartup=0 to kRestoreOnStartup=4.
+// We only need to do this check once, on upgrade from m18 or lower to m19 or
+// higher.
+const char kRestoreOnStartupMigrated[] = "session.restore_on_startup_migrated";
+
 // The application locale.
 // For OS_CHROMEOS we maintain kApplicationLocale property in both local state
 // and user's profile.  Global property determines locale of login screen,

chrome/common/pref_names.h

@@ -25,6 +25,7 @@ extern const char kIsGooglePlusUser[];
 extern const char kSessionExitedCleanly[];
 extern const char kRestoreOnStartup[];
 extern const char kURLsToRestoreOnStartup[];
+extern const char kRestoreOnStartupMigrated[];
 
 // For OS_CHROMEOS we maintain kApplicationLocale property in both local state
 // and user's profile.  Global property determines locale of login screen,

chrome/test/functional/protector.py

@@ -151,21 +151,30 @@ class BaseProtectorTest(pyauto.PyUITest):
     prefs['backup']['_signature'] = 'INVALID'
     self._WritePreferences(prefs)
 
-  def _ChangeSessionStartupPrefs(self, startup_type, startup_urls=None,
-                                 homepage=None):
+  def _ChangeSessionStartupPrefs(self, startup_type=None, startup_urls=None,
+                                 homepage=None, delete_migrated_pref=False):
     """Changes the session startup type and the list of URLs to load on startup.
 
     Args:
-      startup_type: int with one of _SESSION_STARTUP_* values.
+      startup_type: int with one of _SESSION_STARTUP_* values. If startup_type
+          is None, then it deletes the preference.
       startup_urls: list(str) with a list of URLs; if None, is left unchanged.
       homepage: unless None, the new value for homepage.
+      delete_migrated_pref: Whether we should delete the preference which says
+          we've already migrated the startup_type preference.
     """
     prefs = self._LoadPreferences()
-    prefs['session']['restore_on_startup'] = startup_type
+    if startup_type is None:
+      del prefs['session']['restore_on_startup']
+    else:
+      prefs['session']['restore_on_startup'] = startup_type
+
     if startup_urls is not None:
       prefs['session']['urls_to_restore_on_startup'] = startup_urls
     if homepage is not None:
       prefs['homepage'] = homepage
+    if delete_migrated_pref:
+      del prefs['session']['restore_on_startup_migrated']
     self._WritePreferences(prefs)
 
   def _ChangePinnedTabsPrefs(self, pinned_tabs):
@@ -452,7 +461,7 @@ class ProtectorSessionStartupTest(BaseProtectorTest):
     # No longer showing the change.
     self.assertFalse(self.GetProtectorState()['showing_change'])
 
-  def testSessionStartupPrefMigration(self):
+  def testSessionStartupPrefMigrationFromHomepage(self):
     """Test migration from old session.restore_on_startup values (homepage)."""
     # Set startup prefs to restoring last open tabs.
     self.SetPrefs(pyauto.kRestoreOnStartup, self._SESSION_STARTUP_LAST)
@@ -463,7 +472,8 @@ class ProtectorSessionStartupTest(BaseProtectorTest):
         clear_profile=False,
         pre_launch_hook=lambda: self._ChangeSessionStartupPrefs(
             self._SESSION_STARTUP_HOMEPAGE,
-            homepage=new_homepage))
+            homepage=new_homepage,
+            delete_migrated_pref=True))
     # The change must be detected by Protector.
     self.assertTrue(self.GetProtectorState()['showing_change'])
     # Protector must restore old preference values.
@@ -481,6 +491,63 @@ class ProtectorSessionStartupTest(BaseProtectorTest):
     # No longer showing the change.
     self.assertFalse(self.GetProtectorState()['showing_change'])
 
+  def testSessionStartupPrefMigrationFromBlank(self):
+    """Test migration from session.restore_on_startup being blank, as it would
+    be for a user who had m18 or lower, and never changed that preference.
+    """
+    # Set startup prefs to restoring last open tabs.
+    self.SetPrefs(pyauto.kRestoreOnStartup, self._SESSION_STARTUP_LAST)
+    self.SetPrefs(pyauto.kURLsToRestoreOnStartup, [])
+    # Set the homepage.
+    new_homepage = 'http://www.google.com/'
+    self.SetPrefs(pyauto.kHomePageIsNewTabPage, False)
+    self.SetPrefs(pyauto.kHomePage, new_homepage)
+    # Restart browser, clearing the 'restore on startup' pref, to simulate a
+    # user coming from m18 and having left it on the default value.
+    self.RestartBrowser(
+        clear_profile=False,
+        pre_launch_hook=lambda: self._ChangeSessionStartupPrefs(
+            startup_type=None,
+            delete_migrated_pref=True))
+    # The change must be detected by Protector.
+    self.assertTrue(self.GetProtectorState()['showing_change'])
+    # Protector must restore old preference values.
+    self.assertEqual(self._SESSION_STARTUP_LAST,
+                     self.GetPrefsInfo().Prefs(pyauto.kRestoreOnStartup))
+    self.assertEqual([],
+                     self.GetPrefsInfo().Prefs(pyauto.kURLsToRestoreOnStartup))
+    self.ApplyProtectorChange()
+    # Now the new preference values are active.
+    self.assertEqual(self._SESSION_STARTUP_URLS,
+                     self.GetPrefsInfo().Prefs(pyauto.kRestoreOnStartup))
+    # Homepage migrated to the list of startup URLs.
+    self.assertEqual([new_homepage],
+                     self.GetPrefsInfo().Prefs(pyauto.kURLsToRestoreOnStartup))
+    # No longer showing the change.
+    self.assertFalse(self.GetProtectorState()['showing_change'])
+
+  def testSessionStartupPrefNoMigrationOnHomepageChange(self):
+    """Test that when the user modifies their homepage in m19+, we don't do the
+    preference migration.
+    """
+    # Initially, the default value is selected for kRestoreOnStartup.
+    self.assertEqual(self._SESSION_STARTUP_NTP,
+                     self.GetPrefsInfo().Prefs(pyauto.kRestoreOnStartup))
+    # Set the homepage, but leave kRestoreOnStartup unchanged.
+    new_homepage = 'http://www.google.com/'
+    self.SetPrefs(pyauto.kHomePageIsNewTabPage, False)
+    self.SetPrefs(pyauto.kHomePage, new_homepage)
+    # Restart browser.
+    self.RestartBrowser(clear_profile=False)
+    # Now the new preference values are active.
+    self.assertEqual(self._SESSION_STARTUP_NTP,
+                     self.GetPrefsInfo().Prefs(pyauto.kRestoreOnStartup))
+    # kURLsToRestoreOnStartup pref is unchanged.
+    self.assertEqual([],
+                     self.GetPrefsInfo().Prefs(pyauto.kURLsToRestoreOnStartup))
+    # No longer showing the change.
+    self.assertFalse(self.GetProtectorState()['showing_change'])
+
   def testDetectPinnedTabsChangeAndApply(self):
     """Test for detecting and applying a change to pinned tabs."""
     pinned_urls = ['chrome://version/', 'chrome://credits/']