Update RestoreOnStartupPolicyHandler to translate to the correct preferences

chrome/browser/policy/configuration_policy_handler.cc

Index: chrome/browser/policy/configuration_policy_handler.cc
diff --git a/chrome/browser/policy/configuration_policy_handler.cc b/chrome/browser/policy/configuration_policy_handler.cc
index 0bae4da6b38e7c2ec1ca63c3208773e560bd6c93..8808837bbed387bd1c04e35e7ced2d1538d0ea79 100644
--- a/chrome/browser/policy/configuration_policy_handler.cc
+++ b/chrome/browser/policy/configuration_policy_handler.cc
@@ -964,48 +964,115 @@ void JavascriptPolicyHandler::ApplyPolicySettings(const PolicyMap& policies,
 // RestoreOnStartupPolicyHandler implementation --------------------------------
 
 RestoreOnStartupPolicyHandler::RestoreOnStartupPolicyHandler()
-    : SimplePolicyHandler(key::kRestoreOnStartup,
-                          prefs::kRestoreOnStartup,
-                          Value::TYPE_INTEGER) {
+    : TypeCheckingPolicyHandler(key::kRestoreOnStartup,
+                                Value::TYPE_INTEGER) {
 }
 
 RestoreOnStartupPolicyHandler::~RestoreOnStartupPolicyHandler() {
 }
 
+void RestoreOnStartupPolicyHandler::ApplyPolicySettings(
+    const PolicyMap& policies,
+    PrefValueMap* prefs) {
+  const Value* restore_on_startup_value = policies.GetValue(policy_name());
+  if (restore_on_startup_value) {
+    int restore_on_startup;
+    if (!restore_on_startup_value->GetAsInteger(&restore_on_startup))
+      return;
+
+    if (restore_on_startup == SessionStartupPref::kPrefValueHomePage) {
+      ApplyPolicySettingsFromHomePage(policies, prefs);
+    } else {
+      prefs->SetInteger(prefs::kRestoreOnStartup, restore_on_startup);
+    }
+  }
+}
+
+void RestoreOnStartupPolicyHandler::ApplyPolicySettingsFromHomePage(
+    const PolicyMap& policies,
+    PrefValueMap* prefs) {
+
+  const base::Value* homepage_is_new_tab_page_value =
+      policies.GetValue(key::kHomepageIsNewTabPage);
+  if (!homepage_is_new_tab_page_value) {
+    // The policy is enforcing 'open the homepage on startup' but not
+    // enforcing what the homepage should be. Don't set any prefs.
+    return;
+  }
+
+  bool homepage_is_new_tab_page;
+  if (!homepage_is_new_tab_page_value->GetAsBoolean(&homepage_is_new_tab_page))
+    return;
+
+  if (homepage_is_new_tab_page) {
+    prefs->SetInteger(
+        prefs::kRestoreOnStartup,
+        SessionStartupPref::kPrefValueNewTab);
+  } else {
+    const base::Value* homepage_value =
+        policies.GetValue(key::kHomepageLocation);
+    if (!homepage_value || !homepage_value->IsType(TYPE_STRING)) {

[Tyler Breisacher (Chromium), 2012/04/20 18:39:44]

I noticed that we weren't type-checking this value, since we never extract the
string itself from the Value. (I could extract the string, but I would just have
to wrap it back up into a value again, which is why I did DeepCopy at line 1020
instead.)

I can't decide which one feels clearer, so I'm good with either one:

if (!homepage_value || !homepage_value->IsType(TYPE_STRING)) {
  or
if (!(homepage_value && homepage_value->IsType(TYPE_STRING))) {

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

My personal preference is in favor of the former, which you currently have in
the code.

+      // The policy is enforcing 'open the homepage on startup' but not
+      // enforcing what the homepage should be. Don't set any prefs.
+      return;
+    }
+    ListValue* url_list = new ListValue();
+    url_list->Append(homepage_value->DeepCopy());
+    prefs->SetInteger(
+        prefs::kRestoreOnStartup,
+        SessionStartupPref::kPrefValueURLs);
+    prefs->SetValue(prefs::kURLsToRestoreOnStartup, url_list);
+  }
+}
+
 bool RestoreOnStartupPolicyHandler::CheckPolicySettings(
     const PolicyMap& policies,
     PolicyErrorMap* errors) {
-  if (!SimplePolicyHandler::CheckPolicySettings(policies, errors))
+  if (!TypeCheckingPolicyHandler::CheckPolicySettings(policies, errors))
     return false;
 
-  // If the restore urls at start up policy is set, session cookies are treated
-  // as permanent cookies and site data needed to restore the session is not
-  // cleared so we have to warn the user in that case.
   const base::Value* restore_policy = policies.GetValue(key::kRestoreOnStartup);
 
   if (restore_policy) {
     int restore_value;
-    if (restore_policy->GetAsInteger(&restore_value) &&
-        SessionStartupPref::PrefValueToType(restore_value) ==
-            SessionStartupPref::LAST) {
-
-      const base::Value* cookies_policy =
-          policies.GetValue(key::kCookiesSessionOnlyForUrls);
-      const base::ListValue *cookies_value;
-      if (cookies_policy && cookies_policy->GetAsList(&cookies_value) &&
-          !cookies_value->empty()) {
-        errors->AddError(key::kCookiesSessionOnlyForUrls,
-                         IDS_POLICY_OVERRIDDEN,
-                         key::kRestoreOnStartup);
-      }
-
-      const base::Value* exit_policy =
-          policies.GetValue(key::kClearSiteDataOnExit);
-      bool exit_value;
-      if (exit_policy && exit_policy->GetAsBoolean(&exit_value) && exit_value) {
-        errors->AddError(key::kClearSiteDataOnExit,
-                         IDS_POLICY_OVERRIDDEN,
-                         key::kRestoreOnStartup);
+    if (restore_policy->GetAsInteger(&restore_value)) {
+      switch (restore_value) {
+        case SessionStartupPref::kPrefValueHomePage:
+          // TODO(tbreisacher): AddError indicating this value is deprecated.
+          break;
+        case SessionStartupPref::kPrefValueLast: {

[Tyler Breisacher (Chromium), 2012/04/20 18:39:44]

The braces here are necessary because we're declaring variables inside this
case. The style guide doesn't seem to say where to place the braces, but it
looks like there's plenty of other code doing this:
http://code.google.com/p/chromium/source/search?q=case.*%3A%5Cs%7B

+          // If the "restore last session" policy is set, session cookies are
+          // treated as permanent cookies and site data needed to restore the
+          // session is not cleared so we have to warn the user in that case.
+          const base::Value* cookies_policy =
+              policies.GetValue(key::kCookiesSessionOnlyForUrls);
+          const base::ListValue *cookies_value;
+          if (cookies_policy && cookies_policy->GetAsList(&cookies_value) &&
+              !cookies_value->empty()) {
+            errors->AddError(key::kCookiesSessionOnlyForUrls,
+                             IDS_POLICY_OVERRIDDEN,
+                             key::kRestoreOnStartup);
+          }
+
+          const base::Value* exit_policy =
+              policies.GetValue(key::kClearSiteDataOnExit);
+          bool exit_value;
+          if (exit_policy &&
+              exit_policy->GetAsBoolean(&exit_value) && exit_value) {
+            errors->AddError(key::kClearSiteDataOnExit,
+                             IDS_POLICY_OVERRIDDEN,
+                             key::kRestoreOnStartup);
+          }
+          break;
+        }
+        case SessionStartupPref::kPrefValueURLs:
+        case SessionStartupPref::kPrefValueNewTab:
+          // No error

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

nit: missing period.

+          break;
+        default:
+          errors->AddError(policy_name(),
+                           IDS_POLICY_OUT_OF_RANGE_ERROR,
+                           base::IntToString(restore_value));
       }
     }
   }