Update RestoreOnStartupPolicyHandler to translate to the correct preferences

chrome/browser/policy/configuration_policy_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/policy/configuration_policy_handler.h"
 
 #include <string>
 
 #include "base/file_path.h"
 #include "base/json/json_writer.h"
@@ skipping 946 matching lines @@
 
   if (setting != CONTENT_SETTING_DEFAULT) {
     prefs->SetValue(prefs::kManagedDefaultJavaScriptSetting,
                     Value::CreateIntegerValue(setting));
   }
 }
 
 // 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) {
+    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* 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);
+          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));
       }
     }
   }
   return true;
 }
 
 }  // namespace policy