Revert 133740 - Fix homepage migration for users who never changed their settings.

This caused chromium os tests to fail, http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/1981/steps/unit_tests/logs/DefaultMigrationHomepageIsNTP

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

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

TBR=tbreisacher@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10173007

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

chrome/browser/prefs/pref_service.cc

@@ -684,31 +684,6 @@ 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,10 +267,6 @@ 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,13 +35,10 @@ int TypeToPrefValue(SessionStartupPref::Type type) {
 }
 
 void SetNewURLList(PrefService* prefs) {
-  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);
-  }
+  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) {
@@ -64,9 +61,6 @@ void SessionStartupPref::RegisterUserPrefs(PrefService* prefs) {
                              PrefService::SYNCABLE_PREF);
   prefs->RegisterListPref(prefs::kURLsToRestoreOnStartup,
                           PrefService::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kRestoreOnStartupMigrated,
-                             false,
-                             PrefService::UNSYNCABLE_PREF);
 }
 
 // static
@@ -126,12 +120,19 @@ 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);
@@ -140,48 +141,6 @@ 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,11 +53,6 @@ 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,7 +14,6 @@ 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_;
@@ -58,43 +57,6 @@ 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/");
 
@@ -103,31 +65,10 @@ 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,7 +151,8 @@ void ProtectedPrefsWatcher::Observe(
 }
 
 void ProtectedPrefsWatcher::EnsurePrefsMigration() {
-  SessionStartupPref::MigrateIfNecessary(profile_->GetPrefs());
+  // Force SessionStartupPref migration, if necessary.
+  SessionStartupPref::GetStartupPref(profile_);
 }
 
 bool ProtectedPrefsWatcher::UpdateCachedPrefs() {

chrome/browser/ui/browser_init.cc

@@ -1194,9 +1194,10 @@ Browser* BrowserInit::LaunchWithProfile::ProcessSpecifiedURLs(
     UrlsToTabs(urls, &tabs);
 
   } else if (pref.type == SessionStartupPref::HOMEPAGE) {
-    // 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 the user had 'homepage' selected, we should have migrated them to
+    // 'URLs' instead.
+    DLOG(ERROR) << "pref.type == HOMEPAGE";
+    NOTREACHED();
   }
 
   if (tabs.empty())

chrome/common/pref_names.cc

@@ -43,25 +43,18 @@ const char kIsGooglePlusUser[] = "is_google_plus_user";
 const char kSessionExitedCleanly[] = "profile.exited_cleanly";
 
 // An integer pref. Holds one of several values:
-// 0: (deprecated) open the homepage on startup
+// 0: (or empty) don't do anything special 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,7 +25,6 @@ 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,30 +151,21 @@ class BaseProtectorTest(pyauto.PyUITest):
     prefs['backup']['_signature'] = 'INVALID'
     self._WritePreferences(prefs)
 
-  def _ChangeSessionStartupPrefs(self, startup_type=None, startup_urls=None,
-                                 homepage=None, delete_migrated_pref=False):
+  def _ChangeSessionStartupPrefs(self, startup_type, startup_urls=None,
+                                 homepage=None):
     """Changes the session startup type and the list of URLs to load on startup.
 
     Args:
-      startup_type: int with one of _SESSION_STARTUP_* values. If startup_type
-          is None, then it deletes the preference.
+      startup_type: int with one of _SESSION_STARTUP_* values.
       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()
-    if startup_type is None:
-      del prefs['session']['restore_on_startup']
-    else:
-      prefs['session']['restore_on_startup'] = startup_type
-
+    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):
@@ -461,7 +452,7 @@ class ProtectorSessionStartupTest(BaseProtectorTest):
     # No longer showing the change.
     self.assertFalse(self.GetProtectorState()['showing_change'])
 
-  def testSessionStartupPrefMigrationFromHomepage(self):
+  def testSessionStartupPrefMigration(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)
@@ -472,8 +463,7 @@ class ProtectorSessionStartupTest(BaseProtectorTest):
         clear_profile=False,
         pre_launch_hook=lambda: self._ChangeSessionStartupPrefs(
             self._SESSION_STARTUP_HOMEPAGE,
-            homepage=new_homepage,
-            delete_migrated_pref=True))
+            homepage=new_homepage))
     # The change must be detected by Protector.
     self.assertTrue(self.GetProtectorState()['showing_change'])
     # Protector must restore old preference values.
@@ -491,63 +481,6 @@ 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/']