Implement a policy to specify which pages to restore at startup. The admin can choose no restore, last opened windows, or specific pages to be opened.

xib changes: Added an enabled binding to the policy-controlled "Restore on Startup" radiobox group in the "Basic" preference pane.  They can be disabled when their value is overridden by policy. The methods for the bindings are in the preference window controller.

BUG=49297
TEST=ConfigurationPolicyPrefStoreTest.* and ConfigurationPolicyProviderWinTest.*  Manual tests: Set the policies externally.  Check that appropriate restore action is done at startup.  Check that the Restore on Startup buttons are disabled.

Review URL: http://codereview.chromium.org/3115021
Patch from Jean-Luc Brouillet <jeanluc@google.com>.

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

14 files changed, 204 insertions, 80 deletions

diff --git a/chrome/browser/cocoa/preferences_window_controller.h b/chrome/browser/cocoa/preferences_window_controller.h
index 8cfd9e8..5d2f6f8 100644
--- a/chrome/browser/cocoa/preferences_window_controller.h
+++ b/chrome/browser/cocoa/preferences_window_controller.h
@@ -129,6 +129,8 @@ class ProfileSyncService;
   IBOutlet NSButton* enableLoggingCheckbox_;
   scoped_ptr<PrefSetObserver> proxyPrefs_;
   BOOL proxiesConfigureButtonEnabled_;
+  BOOL restoreButtonsEnabled_;
+  BOOL restoreURLsEnabled_;
 }
 
 // Designated initializer. |profile| should not be NULL.
@@ -140,6 +142,9 @@ class ProfileSyncService;
 // Switch to the given preference page.
 - (void)switchToPage:(OptionsPage)page animate:(BOOL)animate;
 
+// Enables or disables the restoreOnStartup elements
+- (void) setEnabledStateOfRestoreOnStartup;
+
 // IBAction methods for responding to user actions.
 
 // Basics panel
@@ -179,7 +184,8 @@ class ProfileSyncService;
 @property (assign, nonatomic) BOOL showHomeButtonEnabled;
 @property (assign, nonatomic) BOOL autoFillSettingsButtonEnabled;
 @property (assign, nonatomic) BOOL proxiesConfigureButtonEnabled;
-
+@property (assign, nonatomic) BOOL restoreButtonsEnabled;
+@property (assign, nonatomic) BOOL restoreURLsEnabled;
 @end
 
 @interface PreferencesWindowController(Testing)
diff --git a/chrome/browser/cocoa/preferences_window_controller.mm b/chrome/browser/cocoa/preferences_window_controller.mm
index 272a3b2..ffd6c2c 100644
--- a/chrome/browser/cocoa/preferences_window_controller.mm
+++ b/chrome/browser/cocoa/preferences_window_controller.mm
@@ -421,6 +421,8 @@ class ManagedPrefsBannerState : public ManagedPrefsBannerBase {
 @synthesize showHomeButtonEnabled = showHomeButtonEnabled_;
 @synthesize autoFillSettingsButtonEnabled = autoFillSettingsButtonEnabled_;
 @synthesize proxiesConfigureButtonEnabled = proxiesConfigureButtonEnabled_;
+@synthesize restoreButtonsEnabled = restoreButtonsEnabled_;
+@synthesize restoreURLsEnabled = restoreURLsEnabled_;
 
 - (id)initWithProfile:(Profile*)profile initialPage:(OptionsPage)initialPage {
   DCHECK(profile);
@@ -498,8 +500,10 @@ class ManagedPrefsBannerState : public ManagedPrefsBannerBase {
         PrefSetObserver::CreateProxyPrefSetObserver(prefs_, observer_.get()));
     [self setProxiesConfigureButtonEnabled:!proxyPrefs_->IsManaged()];
 
-    // Initialize show home button checkbox enabled state.
+    // Initialize the enabled state of the show home button and
+    // restore on startup elements.
     [self setShowHomeButtonEnabled:!showHomeButton_.IsManaged()];
+    [self setEnabledStateOfRestoreOnStartup];
   }
   return self;
 }
@@ -830,7 +834,7 @@ class ManagedPrefsBannerState : public ManagedPrefsBannerBase {
   if ([key isEqualToString:@"isHomepageURLEnabled"]) {
     paths = [paths setByAddingObject:@"newTabPageIsHomePageIndex"];
     paths = [paths setByAddingObject:@"homepageURL"];
-  } else if ([key isEqualToString:@"enableRestoreButtons"]) {
+  } else if ([key isEqualToString:@"restoreURLsEnabled"]) {
     paths = [paths setByAddingObject:@"restoreOnStartupIndex"];
   } else if ([key isEqualToString:@"isHomepageChoiceEnabled"]) {
     paths = [paths setByAddingObject:@"newTabPageIsHomePageIndex"];
@@ -891,10 +895,12 @@ class ManagedPrefsBannerState : public ManagedPrefsBannerBase {
     const SessionStartupPref startupPref =
         SessionStartupPref::GetStartupPref(prefs_);
     [self setRestoreOnStartupIndex:startupPref.type];
+    [self setEnabledStateOfRestoreOnStartup];
   }
 
   if (*prefName == prefs::kURLsToRestoreOnStartup) {
     [customPagesSource_ reloadURLs];
+    [self setEnabledStateOfRestoreOnStartup];
   }
 
   if (*prefName == prefs::kHomePageIsNewTabPage) {
@@ -950,10 +956,13 @@ class ManagedPrefsBannerState : public ManagedPrefsBannerBase {
   [self saveSessionStartupWithType:startupType];
 }
 
-// Returns whether or not the +/-/Current buttons should be enabled, based on
-// the current pref value for the startup urls.
-- (BOOL)enableRestoreButtons {
-  return [self restoreOnStartupIndex] == SessionStartupPref::URLS;
+// Enables or disables the restoreOnStartup elements
+- (void) setEnabledStateOfRestoreOnStartup {
+  const SessionStartupPref startupPref =
+      SessionStartupPref::GetStartupPref(prefs_);
+  [self setRestoreButtonsEnabled:!SessionStartupPref::TypeIsManaged(prefs_)];
+  [self setRestoreURLsEnabled:!SessionStartupPref::URLsAreManaged(prefs_) &&
+      [self restoreOnStartupIndex] == SessionStartupPref::URLS];
 }
 
 // Getter for the |customPagesSource| property for bindings.
diff --git a/chrome/browser/custom_home_pages_table_model.cc b/chrome/browser/custom_home_pages_table_model.cc
index 585b41d0..a3c58bb 100644
--- a/chrome/browser/custom_home_pages_table_model.cc
+++ b/chrome/browser/custom_home_pages_table_model.cc
@@ -57,6 +57,8 @@ void CustomHomePagesTableModel::SetURLs(const std::vector<GURL>& urls) {
   entries_.resize(urls.size());
   for (size_t i = 0; i < urls.size(); ++i) {
     entries_[i].url = urls[i];
+    entries_[i].title.erase();
+    entries_[i].icon.reset();
     LoadTitleAndFavIcon(&(entries_[i]));
   }
   // Complete change, so tell the view to just rebuild itself.
diff --git a/chrome/browser/gtk/options/general_page_gtk.cc b/chrome/browser/gtk/options/general_page_gtk.cc
index 18f5604..2636398 100644
--- a/chrome/browser/gtk/options/general_page_gtk.cc
+++ b/chrome/browser/gtk/options/general_page_gtk.cc
@@ -122,35 +122,37 @@ GeneralPageGtk::~GeneralPageGtk() {
 
 void GeneralPageGtk::NotifyPrefChanged(const std::string* pref_name) {
   initializing_ = true;
-  if (!pref_name || *pref_name == prefs::kRestoreOnStartup) {
+  if (!pref_name ||
+      *pref_name == prefs::kRestoreOnStartup ||
+      *pref_name == prefs::kURLsToRestoreOnStartup) {
     PrefService* prefs = profile()->GetPrefs();
     const SessionStartupPref startup_pref =
         SessionStartupPref::GetStartupPref(prefs);
+    bool radio_buttons_enabled = !SessionStartupPref::TypeIsManaged(prefs);
+    bool restore_urls_enabled = !SessionStartupPref::URLsAreManaged(prefs);
     switch (startup_pref.type) {
     case SessionStartupPref::DEFAULT:
       gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(startup_homepage_radio_),
                                    TRUE);
-      EnableCustomHomepagesControls(false);
+      restore_urls_enabled = false;
       break;
 
     case SessionStartupPref::LAST:
       gtk_toggle_button_set_active(
           GTK_TOGGLE_BUTTON(startup_last_session_radio_), TRUE);
-      EnableCustomHomepagesControls(false);
+      restore_urls_enabled = false;
       break;
 
     case SessionStartupPref::URLS:
       gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(startup_custom_radio_),
                                    TRUE);
-      EnableCustomHomepagesControls(true);
       break;
     }
-  }
-
-  if (!pref_name || *pref_name == prefs::kURLsToRestoreOnStartup) {
-    PrefService* prefs = profile()->GetPrefs();
-    const SessionStartupPref startup_pref =
-        SessionStartupPref::GetStartupPref(prefs);
+    gtk_widget_set_sensitive(startup_homepage_radio_, radio_buttons_enabled);
+    gtk_widget_set_sensitive(startup_last_session_radio_,
+                             radio_buttons_enabled);
+    gtk_widget_set_sensitive(startup_custom_radio_, radio_buttons_enabled);
+    EnableCustomHomepagesControls(restore_urls_enabled);
     startup_custom_pages_table_model_->SetURLs(startup_pref.urls);
   }
 
@@ -547,6 +549,12 @@ void GeneralPageGtk::SetCustomUrlListFromCurrentPages() {
 }
 
 void GeneralPageGtk::OnAddCustomUrl(const GURL& url) {
+  // The restore URLs policy might have become managed while the dialog is
+  // displayed.  While the model makes sure that no changes are made in this
+  // condition, we should still avoid the rest of the method otherwise
+  // graphic elements will become enabled.
+  if (SessionStartupPref::URLsAreManaged(profile()->GetPrefs()))
+    return;
   std::set<int> indices;
   gtk_tree::GetSelectedIndices(startup_custom_pages_selection_, &indices);
   int index;
diff --git a/chrome/browser/policy/configuration_policy_pref_store.cc b/chrome/browser/policy/configuration_policy_pref_store.cc
index b702ca8..b78a56b 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store.cc
@@ -91,6 +91,10 @@ const ConfigurationPolicyPrefStore::PolicyToPreferenceMapEntry
   { Value::TYPE_STRING, kPolicyHomePage,  prefs::kHomePage },
   { Value::TYPE_BOOLEAN, kPolicyHomepageIsNewTabPage,
       prefs::kHomePageIsNewTabPage },
+  { Value::TYPE_INTEGER, kPolicyRestoreOnStartup,
+      prefs::kRestoreOnStartup},
+  { Value::TYPE_LIST, kPolicyURLsToRestoreOnStartup,
+      prefs::kURLsToRestoreOnStartup },
   { Value::TYPE_BOOLEAN, kPolicyAlternateErrorPagesEnabled,
       prefs::kAlternateErrorPagesEnabled },
   { Value::TYPE_BOOLEAN, kPolicySearchSuggestEnabled,
diff --git a/chrome/browser/policy/configuration_policy_pref_store_unittest.cc b/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
index 3f74266..986cf4d 100644
--- a/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
@@ -19,6 +19,17 @@ class ConfigurationPolicyPrefStoreTest : public testing::Test {
       ConfigurationPolicyStore::PolicyType type,
       const char* policy_value);
 
+  // The following three methods test a policy which controls a preference
+  // that is a list of strings.
+  // Checks that by default, it's an empty list.
+  void TestListPolicyGetDefault(const char* pref_name);
+  // Checks that values can be set.
+  void TestListPolicySetValue(const char* pref_name,
+                              ConfigurationPolicyStore::PolicyType type);
+  // A wrapper that calls the above two methods.
+  void TestListPolicy(const char* pref_name,
+                      ConfigurationPolicyStore::PolicyType type);
+
   // The following three methods test a policy which controls a string
   // preference.
   // Checks that by default, it's an empty string.
@@ -60,6 +71,42 @@ void ConfigurationPolicyPrefStoreTest::ApplyStringPolicyValue(
   store->Apply(type, Value::CreateStringValue(policy_value));
 }
 
+void ConfigurationPolicyPrefStoreTest::TestListPolicyGetDefault(
+    const char* pref_name) {
+  ConfigurationPolicyPrefStore store(NULL, NULL);
+  ListValue* list = NULL;
+  EXPECT_FALSE(store.prefs()->GetList(pref_name, &list));
+}
+
+void ConfigurationPolicyPrefStoreTest::TestListPolicySetValue(
+    const char* pref_name, ConfigurationPolicyStore::PolicyType type) {
+  ConfigurationPolicyPrefStore store(NULL, NULL);
+  ListValue* in_value = new ListValue();
+  in_value->Append(Value::CreateStringValue("test1"));
+  in_value->Append(Value::CreateStringValue("test2,"));
+  store.Apply(type, in_value);
+  ListValue* list = NULL;
+  EXPECT_TRUE(store.prefs()->GetList(pref_name, &list));
+  ListValue::const_iterator current(list->begin());
+  ListValue::const_iterator end(list->end());
+  ASSERT_TRUE(current != end);
+  std::string value;
+  (*current)->GetAsString(&value);
+  EXPECT_EQ("test1", value);
+  ++current;
+  ASSERT_TRUE(current != end);
+  (*current)->GetAsString(&value);
+  EXPECT_EQ("test2,", value);
+  ++current;
+  EXPECT_TRUE(current == end);
+}
+
+void ConfigurationPolicyPrefStoreTest::TestListPolicy(
+    const char* pref_name, ConfigurationPolicyStore::PolicyType type) {
+  TestListPolicyGetDefault(pref_name);
+  TestListPolicySetValue(pref_name, type);
+}
+
 void ConfigurationPolicyPrefStoreTest::TestStringPolicyGetDefault(
     const char* pref_name) {
   ConfigurationPolicyPrefStore store(NULL, NULL);
@@ -147,6 +194,13 @@ TEST_F(ConfigurationPolicyPrefStoreTest, TestPolicyHomepageIsNewTabPage) {
       ConfigurationPolicyPrefStore::kPolicyHomepageIsNewTabPage);
 }
 
+TEST_F(ConfigurationPolicyPrefStoreTest, TestPolicyRestoreOnStartup) {
+  TestIntegerPolicy(prefs::kRestoreOnStartup,
+      ConfigurationPolicyPrefStore::kPolicyRestoreOnStartup);
+  TestListPolicy(prefs::kURLsToRestoreOnStartup,
+      ConfigurationPolicyPrefStore::kPolicyURLsToRestoreOnStartup);
+}
+
 TEST_F(ConfigurationPolicyPrefStoreTest, TestPolicyAlternateErrorPagesEnabled) {
   TestBooleanPolicy(prefs::kAlternateErrorPagesEnabled,
       ConfigurationPolicyStore::kPolicyAlternateErrorPagesEnabled);
diff --git a/chrome/browser/policy/configuration_policy_provider.cc b/chrome/browser/policy/configuration_policy_provider.cc
index d24196b..342c011 100644
--- a/chrome/browser/policy/configuration_policy_provider.cc
+++ b/chrome/browser/policy/configuration_policy_provider.cc
@@ -24,6 +24,10 @@ const InternalPolicyValueMapEntry kPolicyValueMap[] = {
       Value::TYPE_STRING, policy::key::kHomepageLocation },
   { ConfigurationPolicyStore::kPolicyHomepageIsNewTabPage,
       Value::TYPE_BOOLEAN, policy::key::kHomepageIsNewTabPage },
+  { ConfigurationPolicyStore::kPolicyRestoreOnStartup,
+      Value::TYPE_INTEGER, policy::key::kRestoreOnStartup },
+  { ConfigurationPolicyStore::kPolicyURLsToRestoreOnStartup,
+      Value::TYPE_LIST, policy::key::kURLsToRestoreOnStartup },
   { ConfigurationPolicyStore::kPolicyProxyServerMode,
       Value::TYPE_INTEGER, policy::key::kProxyServerMode },
   { ConfigurationPolicyStore::kPolicyProxyServer,
diff --git a/chrome/browser/policy/configuration_policy_provider_win.cc b/chrome/browser/policy/configuration_policy_provider_win.cc
index 2e5904e..5d79da6 100644
--- a/chrome/browser/policy/configuration_policy_provider_win.cc
+++ b/chrome/browser/policy/configuration_policy_provider_win.cc
@@ -58,31 +58,29 @@ ConfigurationPolicyProviderWin::ConfigurationPolicyProviderWin() {
 }
 
 bool ConfigurationPolicyProviderWin::GetRegistryPolicyString(
-    const string16& name, int index, string16* result) {
-  DWORD value_size = 0;
-  DWORD key_type = 0;
-  scoped_array<uint8> buffer;
+    const string16& name, string16* result) {
+  string16 path = string16(policy::kRegistrySubKey);
   RegKey policy_key;
-  string16 location = string16(policy::kRegistrySubKey);
-  string16 value_name = name;
-
-  if (index > 0) {
-    // This is a list value, treat |name| as a subkey.
-    location += ASCIIToUTF16("\\") + name;
-    value_name = base::IntToString16(index);
-  }
-
   // First try the global policy.
-  if (!policy_key.Open(HKEY_LOCAL_MACHINE, location.c_str()) ||
-      !policy_key.ReadValue(value_name.c_str(), 0, &value_size, &key_type)) {
+  if (policy_key.Open(HKEY_LOCAL_MACHINE, path.c_str())) {
+    if (ReadRegistryStringValue(&policy_key, name, result))
+      return true;
     policy_key.Close();
-    // Fall back on user-specific policy.
-    if (!policy_key.Open(HKEY_CURRENT_USER, location.c_str()) ||
-        !policy_key.ReadValue(value_name.c_str(), 0, &value_size, &key_type)) {
-      return false;
-    }
   }
+  // Fall back on user-specific policy.
+  if (!policy_key.Open(HKEY_CURRENT_USER, path.c_str()))
+    return false;
+  return ReadRegistryStringValue(&policy_key, name, result);
+}
 
+bool ConfigurationPolicyProviderWin::ReadRegistryStringValue(
+    RegKey* key, const string16& name, string16* result) {
+  DWORD value_size = 0;
+  DWORD key_type = 0;
+  scoped_array<uint8> buffer;
+
+  if (!key->ReadValue(name.c_str(), 0, &value_size, &key_type))
+    return false;
   if (key_type != REG_SZ)
     return false;
 
@@ -92,18 +90,28 @@ bool ConfigurationPolicyProviderWin::GetRegistryPolicyString(
   // the 0-termination.
   buffer.reset(new uint8[value_size + 2]);
   memset(buffer.get(), 0, value_size + 2);
-  policy_key.ReadValue(value_name.c_str(), buffer.get(), &value_size);
+  key->ReadValue(name.c_str(), buffer.get(), &value_size);
   result->assign(reinterpret_cast<const wchar_t*>(buffer.get()));
   return true;
 }
 
-
 bool ConfigurationPolicyProviderWin::GetRegistryPolicyStringList(
     const string16& key, ListValue* result) {
-  int index = 0;
+  string16 path = string16(policy::kRegistrySubKey);
+  path += ASCIIToUTF16("\\") + key;
+  RegKey policy_key;
+  if (!policy_key.Open(HKEY_LOCAL_MACHINE, path.c_str())) {
+    policy_key.Close();
+    // Fall back on user-specific policy.
+    if (!policy_key.Open(HKEY_CURRENT_USER, path.c_str()))
+      return false;
+  }
   string16 policy_string;
-  while (GetRegistryPolicyString(key, ++index,  &policy_string))
+  int index = 0;
+  while (ReadRegistryStringValue(&policy_key, base::IntToString16(++index),
+                                 &policy_string)) {
     result->Append(Value::CreateStringValue(policy_string));
+  }
   return true;
 }
 
@@ -151,7 +159,7 @@ bool ConfigurationPolicyProviderWin::Provide(
     switch (current->value_type) {
       case Value::TYPE_STRING: {
         std::wstring string_value;
-        if (GetRegistryPolicyString(name.c_str(), -1, &string_value)) {
+        if (GetRegistryPolicyString(name.c_str(), &string_value)) {
           store->Apply(current->policy_type,
                        Value::CreateStringValue(string_value));
         }
diff --git a/chrome/browser/policy/configuration_policy_provider_win.h b/chrome/browser/policy/configuration_policy_provider_win.h
index e197e9d..59a2770 100644
--- a/chrome/browser/policy/configuration_policy_provider_win.h
+++ b/chrome/browser/policy/configuration_policy_provider_win.h
@@ -12,6 +12,8 @@
 #include "chrome/browser/policy/configuration_policy_store.h"
 #include "chrome/browser/policy/configuration_policy_provider.h"
 
+class RegKey;
+
 // An implementation of |ConfigurationPolicyProvider| using the
 // mechanism provided by Windows Groups Policy. Policy decisions are
 // stored as values in a special section of the Windows Registry.
@@ -54,15 +56,12 @@ class ConfigurationPolicyProviderWin : public ConfigurationPolicyProvider {
   // Methods to perform type-specific policy lookups in the registry.
   // HKLM is checked first, then HKCU.
 
-  // Reads a string registry value |name| and puts the resulting string in
-  // |result|. If |index| > 0, |name| is the name of a subkey and the value
-  // read is named |index|. Note: A subkey is used for lists to work around
-  // a problem with the Group Policy Editor, where one list value overwrites
-  // another if they appear under the same key (even if they have different
-  // names).
-  bool GetRegistryPolicyString(const string16& name,
-                               int index,
+  // Reads a string registry value |name| at the specified |key| and puts the
+  // resulting string in |result|.
+  bool ReadRegistryStringValue(RegKey* key, const string16& name,
                                string16* result);
+
+  bool GetRegistryPolicyString(const string16& name, string16* result);
   // Gets a list value contained under |key| one level below the policy root.
   bool GetRegistryPolicyStringList(const string16& key, ListValue* result);
   bool GetRegistryPolicyBoolean(const string16& value_name, bool* result);
diff --git a/chrome/browser/policy/configuration_policy_store.h b/chrome/browser/policy/configuration_policy_store.h
index e74fe6c..2612c4d 100644
--- a/chrome/browser/policy/configuration_policy_store.h
+++ b/chrome/browser/policy/configuration_policy_store.h
@@ -20,6 +20,8 @@ class ConfigurationPolicyStore {
   enum PolicyType {
     kPolicyHomePage,
     kPolicyHomepageIsNewTabPage,
+    kPolicyRestoreOnStartup,
+    kPolicyURLsToRestoreOnStartup,
     kPolicyProxyServerMode,
     kPolicyProxyServer,
     kPolicyProxyPacUrl,
diff --git a/chrome/browser/policy/managed_prefs_banner_base.cc b/chrome/browser/policy/managed_prefs_banner_base.cc
index fc334f8..b9d4d83 100644
--- a/chrome/browser/policy/managed_prefs_banner_base.cc
+++ b/chrome/browser/policy/managed_prefs_banner_base.cc
@@ -52,6 +52,8 @@ void ManagedPrefsBannerBase::Init(PrefService* local_state,
       AddUserPref(prefs::kHomePage);
       AddUserPref(prefs::kHomePageIsNewTabPage);
       AddUserPref(prefs::kShowHomeButton);
+      AddUserPref(prefs::kRestoreOnStartup);
+      AddUserPref(prefs::kURLsToRestoreOnStartup);
       break;
     case OPTIONS_PAGE_CONTENT:
       AddUserPref(prefs::kSyncManaged);
diff --git a/chrome/browser/prefs/session_startup_pref.cc b/chrome/browser/prefs/session_startup_pref.cc
index 827af01..87d321b 100644
--- a/chrome/browser/prefs/session_startup_pref.cc
+++ b/chrome/browser/prefs/session_startup_pref.cc
@@ -61,19 +61,23 @@ void SessionStartupPref::SetStartupPref(
 void SessionStartupPref::SetStartupPref(PrefService* prefs,
                                         const SessionStartupPref& pref) {
   DCHECK(prefs);
-  prefs->SetInteger(prefs::kRestoreOnStartup, TypeToPrefValue(pref.type));
-
-  // Always save the URLs, that way the UI can remain consistent even if the
-  // user changes the startup type pref.
-  // Ownership of the ListValue retains with the pref service.
-  ScopedPrefUpdate update(prefs, prefs::kURLsToRestoreOnStartup);
-  ListValue* url_pref_list =
-      prefs->GetMutableList(prefs::kURLsToRestoreOnStartup);
-  DCHECK(url_pref_list);
-  url_pref_list->Clear();
-  for (size_t i = 0; i < pref.urls.size(); ++i) {
-    url_pref_list->Set(static_cast<int>(i),
-                       new StringValue(pref.urls[i].spec()));
+
+  if (!SessionStartupPref::TypeIsManaged(prefs))
+    prefs->SetInteger(prefs::kRestoreOnStartup, TypeToPrefValue(pref.type));
+
+  if (!SessionStartupPref::URLsAreManaged(prefs)) {
+    // Always save the URLs, that way the UI can remain consistent even if the
+    // user changes the startup type pref.
+    // Ownership of the ListValue retains with the pref service.
+    ScopedPrefUpdate update(prefs, prefs::kURLsToRestoreOnStartup);
+    ListValue* url_pref_list =
+        prefs->GetMutableList(prefs::kURLsToRestoreOnStartup);
+    DCHECK(url_pref_list);
+    url_pref_list->Clear();
+    for (size_t i = 0; i < pref.urls.size(); ++i) {
+      url_pref_list->Set(static_cast<int>(i),
+                         new StringValue(pref.urls[i].spec()));
+    }
   }
 }
 
@@ -102,6 +106,24 @@ SessionStartupPref SessionStartupPref::GetStartupPref(PrefService* prefs) {
         pref.urls.push_back(GURL(url_text));
     }
   }
-
   return pref;
 }
+
+// static
+bool SessionStartupPref::TypeIsManaged(PrefService* prefs) {
+  DCHECK(prefs);
+  const PrefService::Preference* pref_restore =
+      prefs->FindPreference(prefs::kRestoreOnStartup);
+  DCHECK(pref_restore);
+  return pref_restore->IsManaged();
+}
+
+// static
+bool SessionStartupPref::URLsAreManaged(PrefService* prefs) {
+  DCHECK(prefs);
+  const PrefService::Preference* pref_urls =
+      prefs->FindPreference(prefs::kURLsToRestoreOnStartup);
+  DCHECK(pref_urls);
+  return pref_urls->IsManaged();
+}
+
diff --git a/chrome/browser/prefs/session_startup_pref.h b/chrome/browser/prefs/session_startup_pref.h
index a2a1660..d7cfaac 100644
--- a/chrome/browser/prefs/session_startup_pref.h
+++ b/chrome/browser/prefs/session_startup_pref.h
@@ -37,6 +37,10 @@ struct SessionStartupPref {
   static SessionStartupPref GetStartupPref(Profile* profile);
   static SessionStartupPref GetStartupPref(PrefService* prefs);
 
+  // Whether the startup type and URLs are managed via policy.
+  static bool TypeIsManaged(PrefService* prefs);
+  static bool URLsAreManaged(PrefService* prefs);
+
   SessionStartupPref() : type(DEFAULT) {}
 
   explicit SessionStartupPref(Type type) : type(type) {}
diff --git a/chrome/browser/views/options/general_page_view.cc b/chrome/browser/views/options/general_page_view.cc
index 0db006c..d8ae49b 100644
--- a/chrome/browser/views/options/general_page_view.cc
+++ b/chrome/browser/views/options/general_page_view.cc
@@ -350,38 +350,33 @@ void GeneralPageView::InitControlLayout() {
 }
 
 void GeneralPageView::NotifyPrefChanged(const std::string* pref_name) {
-  if (!pref_name || *pref_name == prefs::kRestoreOnStartup) {
+  if (!pref_name ||
+      *pref_name == prefs::kRestoreOnStartup ||
+      *pref_name == prefs::kURLsToRestoreOnStartup) {
     PrefService* prefs = profile()->GetPrefs();
     const SessionStartupPref startup_pref =
         SessionStartupPref::GetStartupPref(prefs);
+    bool radio_buttons_enabled = !SessionStartupPref::TypeIsManaged(prefs);
+    bool restore_urls_enabled = !SessionStartupPref::URLsAreManaged(prefs);
     switch (startup_pref.type) {
     case SessionStartupPref::DEFAULT:
       startup_homepage_radio_->SetChecked(true);
-      EnableCustomHomepagesControls(false);
+      restore_urls_enabled = false;
       break;
 
     case SessionStartupPref::LAST:
       startup_last_session_radio_->SetChecked(true);
-      EnableCustomHomepagesControls(false);
+      restore_urls_enabled = false;
       break;
 
     case SessionStartupPref::URLS:
       startup_custom_radio_->SetChecked(true);
-      EnableCustomHomepagesControls(true);
       break;
     }
-  }
-
-  // TODO(beng): Note that the kURLsToRestoreOnStartup pref is a mutable list,
-  //             and changes to mutable lists aren't broadcast through the
-  //             observer system, so the second half of this condition will
-  //             never match. Once support for broadcasting such updates is
-  //             added, this will automagically start to work, and this comment
-  //             can be removed.
-  if (!pref_name || *pref_name == prefs::kURLsToRestoreOnStartup) {
-    PrefService* prefs = profile()->GetPrefs();
-    const SessionStartupPref startup_pref =
-        SessionStartupPref::GetStartupPref(prefs);
+    startup_homepage_radio_->SetEnabled(radio_buttons_enabled);
+    startup_last_session_radio_->SetEnabled(radio_buttons_enabled);
+    startup_custom_radio_->SetEnabled(radio_buttons_enabled);
+    EnableCustomHomepagesControls(restore_urls_enabled);
     startup_custom_pages_table_model_->SetURLs(startup_pref.urls);
   }
 
@@ -735,6 +730,11 @@ void GeneralPageView::EnableCustomHomepagesControls(bool enable) {
 void GeneralPageView::AddBookmark(UrlPicker* dialog,
                                   const std::wstring& title,
                                   const GURL& url) {
+  // The restore URLs policy might have become managed while the dialog is
+  // displayed.  While the model makes sure that no changes are made in this
+  // condition, we should still avoid changing the graphic elements.
+  if (SessionStartupPref::URLsAreManaged(profile()->GetPrefs()))
+    return;
   int index = startup_custom_pages_table_->FirstSelectedRow();
   if (index == -1)
     index = startup_custom_pages_table_model_->RowCount();