ConfigurationPolicyPrefStore refactoring to surface error messages.

chrome/browser/policy/configuration_policy_handler.cc

+// Copyright (c) 2011 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 "base/file_path.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/string16.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/download/download_util.h"
+#include "chrome/browser/policy/configuration_policy_handler_list.h"
+#include "chrome/browser/policy/configuration_policy_pref_store.h"
+#include "chrome/browser/policy/configuration_policy_reader.h"
+#include "chrome/browser/policy/policy_path_parser.h"
+#include "chrome/browser/prefs/proxy_config_dictionary.h"
+#include "chrome/browser/prefs/proxy_prefs.h"
+#include "chrome/browser/search_engines/search_terms_data.h"
+#include "chrome/browser/search_engines/template_url.h"
+#include "chrome/common/pref_names.h"
+#include "grit/generated_resources.h"
+
+namespace policy {
+
+// TypeCheckingPolicyHandler
+TypeCheckingPolicyHandler::TypeCheckingPolicyHandler(
+    ConfigurationPolicyType policy,
+    Value::Type value_type)
+    : policy_type_(policy),
+      value_type_(value_type) {
+}
+
+TypeCheckingPolicyHandler::~TypeCheckingPolicyHandler() {
+}
+
+ConfigurationPolicyType TypeCheckingPolicyHandler::policy_type() const {
+  return policy_type_;
+}
+
+bool TypeCheckingPolicyHandler::CheckPolicySettings(const PolicyMap* policies,
+                                                    PolicyErrorMap* errors) {
+  const Value* value = policies->Get(policy_type_);
+  if (value && value_type_ != value->GetType()) {
+    errors->AddError(policy_type_,
+                     IDS_POLICY_TYPE_ERROR, ValueTypeToString(value_type_));
+    return false;
+  }
+  return true;
+}
+
+void TypeCheckingPolicyHandler::ApplyPolicySettings(const PolicyMap* policies,
+                                                    PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  const Value* value = policies->Get(policy_type());
+  if (value) {
+    if (CheckPolicySettings(policies, &errors)) {

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

so you now have the Check-and-report-errors-before-applying here in
TypeCheckingPolicyHandler, but isn't this something we'd want for any handler?
Thus, would it actually make sense to put this logic into the caller, i.e.
ConfigurationPolicyPrefStore?

+      ApplyPolicyValue(prefs, value);
+    } else {
+      LOG(WARNING) << "Policy: Mismatch in provided and expected policy value  "

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

Why not just GenerateLogMessages(&errors)

[simo, 2011/09/29 09:33:08]

Because the message in |errors| doesn't contain the actual type the value had
because on about:policy, that is apparent. If we don't need it in the log
message either, I will just change it to GeneratelogMessages(&errors).

[Mattias Nissler (ping if slow), 2011/09/30 09:01:33]

I'm all for GenerateLogMessages then :)

+                   << "for " << PolicyStatus::GetPolicyName(policy_type_)
+                   << ". expected = " << value_type_ << ", actual = "
+                   << value->GetType();
+    }
+  }
+}
+
+std::string TypeCheckingPolicyHandler::ValueTypeToString(Value::Type type) {
+  static const char* strings[] = { "null",
+                                   "boolean",
+                                   "integer",
+                                   "double",
+                                   "string",
+                                   "binary",
+                                   "dictionary",
+                                   "list" };
+  DCHECK(static_cast<size_t>(type) < arraysize(strings));
+  return std::string(strings[type]);
+}
+
+// SimplePolicyHandler
+SimplePolicyHandler::SimplePolicyHandler(
+    ConfigurationPolicyType policy,
+    Value::Type value_type,
+    const char* pref_path)
+    : TypeCheckingPolicyHandler(policy, value_type),
+      pref_path_(pref_path) {
+}
+
+SimplePolicyHandler::~SimplePolicyHandler() {
+}
+
+void SimplePolicyHandler::ApplyPolicyValue(PrefValueMap* prefs,
+                                           const Value* value) {
+  prefs->SetValue(pref_path_, value->DeepCopy());
+}
+
+// SyncPolicyHandler
+SyncPolicyHandler::SyncPolicyHandler()
+    : TypeCheckingPolicyHandler(kPolicySyncDisabled,
+                                Value::TYPE_BOOLEAN) {
+}
+
+SyncPolicyHandler::~SyncPolicyHandler() {
+}
+
+void SyncPolicyHandler::ApplyPolicyValue(PrefValueMap* prefs,
+                                         const Value* value) {
+  bool disable_sync;
+  DCHECK(value->GetAsBoolean(&disable_sync));

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

DCHECK compiles to nothing in release builds, so you'd lose the GetAsBoolean :)
Also, we should guard against setting anything in this case, so please make the
GetAsBoolean() return value part of the condition below.

[simo, 2011/09/29 09:33:08]

Done.

+  if (disable_sync)
+    prefs->SetValue(prefs::kSyncManaged, value->DeepCopy());
+}
+
+// AutofillPolicyHandler
+AutofillPolicyHandler::AutofillPolicyHandler()
+    : TypeCheckingPolicyHandler(kPolicyAutoFillEnabled,
+                                Value::TYPE_BOOLEAN) {
+}
+
+AutofillPolicyHandler::~AutofillPolicyHandler() {
+}
+
+void AutofillPolicyHandler::ApplyPolicyValue(PrefValueMap* prefs,
+                                             const Value* value) {
+  bool auto_fill_enabled;
+  DCHECK(value->GetAsBoolean(&auto_fill_enabled));

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

same as above

[simo, 2011/09/29 09:33:08]

Done.

+  if (!auto_fill_enabled) {
+    prefs->SetValue(prefs::kAutofillEnabled,
+                    Value::CreateBooleanValue(false));
+  }
+}
+
+// DownloadDirPolicyHandler
+DownloadDirPolicyHandler::DownloadDirPolicyHandler()
+    : TypeCheckingPolicyHandler(kPolicyDownloadDirectory,
+                                Value::TYPE_STRING) {
+}
+
+DownloadDirPolicyHandler::~DownloadDirPolicyHandler() {
+}
+
+bool DownloadDirPolicyHandler::CheckPolicySettings(const PolicyMap* policies,
+                                                   PolicyErrorMap* errors) {
+  // This policy is ignored on ChromeOS because the download path there is fixed
+  // and can not be configured by the user.
+#if !defined(OS_CHROMEOS)

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

It seems like this handler is only useful if we're not on ChromeOS. Maybe we can
just not instantiate the handler for OS_CHROMEOS and remove the guards here (and
below)?

[simo, 2011/09/29 09:33:08]

Done.

+  return TypeCheckingPolicyHandler::CheckPolicySettings(policies, errors);
+#endif  // defined(OS_CHROMEOS)
+  return true;
+}
+
+void DownloadDirPolicyHandler::ApplyPolicyValue(PrefValueMap* prefs,
+                                                const Value* value) {
+#if !defined(OS_CHROMEOS)
+  FilePath::StringType string_value;
+  DCHECK(value->GetAsString(&string_value));

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

ditto

[simo, 2011/09/29 09:33:08]

Done.

+  FilePath::StringType expanded_value =
+      policy::path_parser::ExpandPathVariables(string_value);
+  // Leaving the policy empty would revert to the default download location
+  // else we would point in an undefined location. We do this after the
+  // path expansion because it might lead to an empty string(e.g. for "\"\"").
+  if (expanded_value.empty())
+    expanded_value = download_util::GetDefaultDownloadDirectory().value();
+  prefs->SetValue(prefs::kDownloadDefaultDirectory,
+                  Value::CreateStringValue(expanded_value));
+  prefs->SetValue(prefs::kPromptForDownload,
+                  Value::CreateBooleanValue(false));
+#endif  // !defined(OS_CHROMEOS)
+}
+
+// DiskCacheDirPolicyHandler
+DiskCacheDirPolicyHandler::DiskCacheDirPolicyHandler()
+    : TypeCheckingPolicyHandler(kPolicyDiskCacheDir,
+                                Value::TYPE_STRING) {
+}
+
+DiskCacheDirPolicyHandler::~DiskCacheDirPolicyHandler() {
+}
+
+void DiskCacheDirPolicyHandler::ApplyPolicyValue(PrefValueMap* prefs,
+                                                 const Value* value) {
+  FilePath::StringType string_value;
+  DCHECK(value->GetAsString(&string_value));

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

ditto

[simo, 2011/09/29 09:33:08]

Done.

+  FilePath::StringType expanded_value =
+      policy::path_parser::ExpandPathVariables(string_value);
+  prefs->SetValue(prefs::kDiskCacheDir,
+                  Value::CreateStringValue(expanded_value));
+}
+
+// FileSelectionDialogsHandler
+FileSelectionDialogsHandler::FileSelectionDialogsHandler()
+    : TypeCheckingPolicyHandler(kPolicyAllowFileSelectionDialogs,
+                                Value::TYPE_BOOLEAN) {
+}
+
+FileSelectionDialogsHandler::~FileSelectionDialogsHandler() {
+}
+
+void FileSelectionDialogsHandler::ApplyPolicyValue(PrefValueMap* prefs,
+                                                   const Value* value) {
+  prefs->SetValue(prefs::kAllowFileSelectionDialogs, value->DeepCopy());
+  // If file-selection dialogs are not allowed we forbid the user to be
+  // prompted for the download location, since this would end up in an Infobar
+  // explaining that file-selection dialogs are forbidden anyways.
+  bool allow_file_selection_dialogs = true;
+  DCHECK(value->GetAsBoolean(&allow_file_selection_dialogs));

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

ditto

[simo, 2011/09/29 09:33:08]

Done.

+  if (!allow_file_selection_dialogs) {
+    prefs->SetValue(prefs::kPromptForDownload,
+                    Value::CreateBooleanValue(false));
+  }
+}
+
+// BookmarksPolicyHandler
+BookmarksPolicyHandler::BookmarksPolicyHandler()
+    : TypeCheckingPolicyHandler(kPolicyBookmarkBarEnabled,
+                                Value::TYPE_BOOLEAN) {
+}
+
+BookmarksPolicyHandler::~BookmarksPolicyHandler() {
+}
+
+void BookmarksPolicyHandler::ApplyPolicyValue(PrefValueMap* prefs,
+                                              const Value* value) {
+  prefs->SetValue(prefs::kEnableBookmarkBar, value->DeepCopy());
+  // kShowBookmarkBar is not managed directly by a policy, but when
+  // kEnableBookmarkBar is managed, kShowBookmarkBar should be false so that
+  // the bookmarks bar either is completely disabled or only shows on the NTP.
+  // This also disables the checkbox for this preference in the prefs UI.
+  prefs->SetValue(prefs::kShowBookmarkBar, Value::CreateBooleanValue(false));
+}
+
+namespace {

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

anonymous namespace should go to the top of the file.

[simo, 2011/09/29 09:33:08]

Done.

+
+void GenerateLogMessages(PolicyErrorMap* errors) {
+  for (PolicyErrorMap::const_iterator it = errors->begin();
+       it != errors->end(); ++it) {
+    string16 policy = PolicyStatus::GetPolicyName(it->first);
+    LOG(WARNING) << ASCIIToUTF16("Policy (") + policy + ASCIIToUTF16("): ")

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

no need to convert to UTF16

[simo, 2011/09/29 09:33:08]

I can't get it to compile if I don't because |policy| is UTF16.

+                    + it->second;
+  }
+}
+
+const PolicyToPreferenceMapEntry kDefaultSearchPolicyMap[] = {
+  { Value::TYPE_BOOLEAN, kPolicyDefaultSearchProviderEnabled,
+    prefs::kDefaultSearchProviderEnabled },
+  { Value::TYPE_STRING, kPolicyDefaultSearchProviderName,
+    prefs::kDefaultSearchProviderName },
+  { Value::TYPE_STRING, kPolicyDefaultSearchProviderKeyword,
+    prefs::kDefaultSearchProviderKeyword },
+  { Value::TYPE_STRING, kPolicyDefaultSearchProviderSearchURL,
+    prefs::kDefaultSearchProviderSearchURL },
+  { Value::TYPE_STRING, kPolicyDefaultSearchProviderSuggestURL,
+    prefs::kDefaultSearchProviderSuggestURL },
+  { Value::TYPE_STRING, kPolicyDefaultSearchProviderInstantURL,
+    prefs::kDefaultSearchProviderInstantURL },
+  { Value::TYPE_STRING, kPolicyDefaultSearchProviderIconURL,
+    prefs::kDefaultSearchProviderIconURL },
+  { Value::TYPE_LIST, kPolicyDefaultSearchProviderEncodings,
+    prefs::kDefaultSearchProviderEncodings },
+};
+
+// Implementation of SearchTermsData just for validation.
+class SearchTermsDataForValidation : public SearchTermsData {
+ public:
+  SearchTermsDataForValidation() {}
+
+  // Implementation of SearchTermsData.
+  virtual std::string GoogleBaseURLValue() const {
+    return "http://www.google.com/";
+  }
+  virtual std::string GetApplicationLocale() const {
+    return "en";
+  }
+#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
+  virtual string16 GetRlzParameterValue() const {
+    return string16();
+  }
+#endif
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SearchTermsDataForValidation);
+};
+
+}  // namespace
+
+// IncognitoModePolicyHandler
+IncognitoModePolicyHandler::IncognitoModePolicyHandler() {
+}
+
+IncognitoModePolicyHandler::~IncognitoModePolicyHandler() {
+}
+
+bool IncognitoModePolicyHandler::CheckPolicySettings(const PolicyMap* policies,
+                                                     PolicyErrorMap* errors) {
+  int int_value = IncognitoModePrefs::ENABLED;
+  std::string message;
+  const Value* availability = policies->Get(kPolicyIncognitoModeAvailability);
+  if (availability) {
+    if (availability->GetAsInteger(&int_value)) {
+      IncognitoModePrefs::Availability availability_enum_value;
+      if (!IncognitoModePrefs::IntToAvailability(int_value,
+                                                 &availability_enum_value)) {
+        std::string string_value;
+        std::stringstream out;
+        out << int_value;
+        string_value = out.str();
+        errors->AddError(kPolicyIncognitoModeAvailability,
+                         IDS_POLICY_OUT_OF_RANGE_ERROR, "" + int_value);

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

Wow. I guess you're not aware of base/string_number_conversions.h and the
IntToString that's declared there?

[simo, 2011/09/29 09:33:08]

No I guess I wasn't.

+        return false;
+      }
+    } else {
+      errors->AddError(kPolicyIncognitoModeAvailability,
+                       IDS_POLICY_PARSE_ERROR);
+      return false;
+    }
+  } else {
+    const Value* deprecated_enabled = policies->Get(kPolicyIncognitoEnabled);
+    // If kPolicyIncognitoModeAvailability is not specified, check the obsolete
+    // kPolicyIncognitoEnabled.
+    if (deprecated_enabled &&
+        !deprecated_enabled->IsType(Value::TYPE_BOOLEAN)) {
+      errors->AddError(kPolicyIncognitoEnabled, IDS_POLICY_PARSE_ERROR);
+      return false;
+    }
+  }
+  return true;
+}
+
+void IncognitoModePolicyHandler::ApplyPolicySettings(const PolicyMap* policies,
+                                                     PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  const Value* availability = policies->Get(kPolicyIncognitoModeAvailability);
+  const Value* deprecated_enabled = policies->Get(kPolicyIncognitoEnabled);
+  if (CheckPolicySettings(policies, &errors)) {
+    if (availability) {
+      int int_value = IncognitoModePrefs::ENABLED;
+      IncognitoModePrefs::Availability availability_enum_value;
+      DCHECK(availability->GetAsInteger(&int_value));

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

DCHECK compiles away

[simo, 2011/09/29 09:33:08]

Done.

+      DCHECK(IncognitoModePrefs::IntToAvailability(int_value,
+                                                   &availability_enum_value));
+      prefs->SetValue(prefs::kIncognitoModeAvailability,
+                      Value::CreateIntegerValue(availability_enum_value));
+    } else if (deprecated_enabled) {
+      bool enabled = true;
+      DCHECK(deprecated_enabled->GetAsBoolean(&enabled));

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

ditto

[simo, 2011/09/29 09:33:08]

Done.

+      prefs->SetInteger(prefs::kIncognitoModeAvailability,
+                        enabled ? IncognitoModePrefs::ENABLED :
+                                  IncognitoModePrefs::DISABLED);
+    }
+  } else {
+    GenerateLogMessages(&errors);
+  }
+}
+
+// DefaultSearchEncodingsPolicyHandler
+DefaultSearchEncodingsPolicyHandler::DefaultSearchEncodingsPolicyHandler()
+    : TypeCheckingPolicyHandler(kPolicyDefaultSearchProviderEncodings,
+                                Value::TYPE_LIST) {
+}
+
+DefaultSearchEncodingsPolicyHandler::~DefaultSearchEncodingsPolicyHandler() {
+}
+
+void DefaultSearchEncodingsPolicyHandler::ApplyPolicyValue(
+    PrefValueMap* prefs, const Value* value) {
+  const ListValue* list;
+  DCHECK(value->GetAsList(&list));
+  ListValue::const_iterator iter(list->begin());
+  ListValue::const_iterator end(list->end());
+  std::string encodings;
+  for (; iter != end; ++iter) {
+    std::string s;
+    if ((*iter)->GetAsString(&s)) {
+      if (!encodings.empty())
+        encodings.push_back(';');
+      encodings.append(s);

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

base/string_util.h JoinString()?

[simo, 2011/09/29 09:33:08]

Done.

+    } else {
+      NOTREACHED();
+    }
+  }
+  prefs->SetValue(prefs::kDefaultSearchProviderEncodings,
+                  Value::CreateStringValue(encodings));
+}
+
+// DefaultSearchPolicyHandler
+DefaultSearchPolicyHandler::DefaultSearchPolicyHandler() {
+  for (unsigned int current = 0;
+      current < arraysize(kDefaultSearchPolicyMap); ++current) {

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

identation

[simo, 2011/09/29 09:33:08]

Done.

+    ConfigurationPolicyType policy_type =
+        kDefaultSearchPolicyMap[current].policy_type;
+    if (policy_type != kPolicyDefaultSearchProviderEncodings) {
+      handlers_.push_back(new SimplePolicyHandler(
+                             policy_type,

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

indentation

[simo, 2011/09/29 09:33:08]

Done.

+                             kDefaultSearchPolicyMap[current].value_type,
+                             kDefaultSearchPolicyMap[current].preference_path));
+    } else {
+      handlers_.push_back(new DefaultSearchEncodingsPolicyHandler());

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

if you need to special-case this anyway, why not just remove the entry from
kDefaultSearchPolicyMap?

[simo, 2011/09/29 09:33:08]

Because I need it to be in the map in other methods like 
ClearDefaultSearchPreferences(). I have removed this method since but I'm now
using it in CheckPolicySettings();

+    }
+  }
+}
+
+DefaultSearchPolicyHandler::~DefaultSearchPolicyHandler() {
+  STLDeleteContainerPointers(handlers_.begin(), handlers_.end());
+  handlers_.clear();
+}
+
+bool DefaultSearchPolicyHandler::CheckPolicySettings(const PolicyMap* policies,
+                                                     PolicyErrorMap* errors) {
+  if (!CheckIndividualPolicies(policies, errors))
+    return false;
+
+  if (DefaultSearchProviderIsDisabled(policies)) {
+    int message_id = IDS_POLICY_DEFAULT_SEARCH_DISABLED;
+    errors->AddError(kPolicyDefaultSearchProviderName, message_id);

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

Is it OK to add errors about these policy settings even if they're not present
in policy?

[simo, 2011/09/29 09:33:08]

Ah I didn't notice that. I don't think it is!

+    errors->AddError(kPolicyDefaultSearchProviderSearchURL, message_id);
+    errors->AddError(kPolicyDefaultSearchProviderSuggestURL, message_id);
+    errors->AddError(kPolicyDefaultSearchProviderIconURL, message_id);
+    errors->AddError(kPolicyDefaultSearchProviderEncodings, message_id);
+    errors->AddError(kPolicyDefaultSearchProviderKeyword, message_id);
+    errors->AddError(kPolicyDefaultSearchProviderInstantURL, message_id);
+    return false;
+  }
+
+  const Value* search_url =
+      policies->Get(kPolicyDefaultSearchProviderSearchURL);
+  if (!search_url) {
+    errors->AddError(kPolicyDefaultSearchProviderSearchURL,

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

Am I missing something or are we adding this error unconditionally if the policy
is not specified?

[simo, 2011/09/29 09:33:08]

Oh right, I should check whether any other default search policies are actually
set.

+                     IDS_POLICY_NOT_SPECIFIED_ERROR);
+    return false;
+  }
+  if (DefaultSearchURLIsPresentAndValid(policies)) {

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

We're adding an error if the URL is present and valid? That doesn't seem right
to me.

[simo, 2011/09/29 09:33:08]

Oh, that was a mistake.

+    errors->AddError(kPolicyDefaultSearchProviderSearchURL,
+                     IDS_POLICY_INVALID_ERROR);
+    return false;
+  }
+  return true;
+}
+
+void DefaultSearchPolicyHandler::ApplyPolicySettings(const PolicyMap* policies,
+                                                     PrefValueMap* prefs) {
+  if (DefaultSearchProviderIsDisabled(policies)) {
+    // If default search is disabled, we ignore the other fields.
+    prefs->SetString(prefs::kDefaultSearchProviderName, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderSearchURL, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderSuggestURL, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderIconURL, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderEncodings, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderKeyword, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderInstantURL, std::string());
+    return;
+  }
+
+  const Value* search_url =
+      policies->Get(kPolicyDefaultSearchProviderSearchURL);
+  // The search URL is required.
+  if (!search_url)
+    return;
+
+  // Apply all default search policies.

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

Shouldn't this block be moved into the if-block starting at 469? We shouldn't
apply anything if the URL is not valid.

[simo, 2011/09/29 09:33:08]

Done.

+  PolicyErrorMap errors;
+  PrefValueMap temp_prefs;
+  HandlerList::const_iterator handler = handlers_.begin();
+  for ( ; handler != handlers_.end(); ++handler) {

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

No curlies required.

[simo, 2011/09/29 09:33:08]

Done.

+    (*handler)->ApplyPolicySettings(policies, &temp_prefs);
+  }
+
+  // The other entries are optional.  Just make sure that they are all
+  // specified via policy, so that we don't use regular prefs.
+  if (DefaultSearchURLIsPresentAndValid(policies)) {
+    EnsureStringPrefExists(
+        &temp_prefs, prefs::kDefaultSearchProviderSuggestURL);
+    EnsureStringPrefExists(&temp_prefs, prefs::kDefaultSearchProviderIconURL);
+    EnsureStringPrefExists(&temp_prefs, prefs::kDefaultSearchProviderEncodings);
+    EnsureStringPrefExists(&temp_prefs, prefs::kDefaultSearchProviderKeyword);
+    EnsureStringPrefExists(
+        &temp_prefs, prefs::kDefaultSearchProviderInstantURL);
+
+    // For the name, default to the host if not specified.
+    std::string name;
+    if (!temp_prefs.GetString(prefs::kDefaultSearchProviderName, &name) ||
+        name.empty()) {
+      const Value* search_url =
+          policies->Get(kPolicyDefaultSearchProviderSearchURL);

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

search_url is already read above. reuse?

[simo, 2011/09/29 09:33:08]

Done.

+      std::string search_url_string;
+      DCHECK(search_url->GetAsString(&search_url_string));

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

DCHECK compiles away.

[simo, 2011/09/29 09:33:08]

Done.

+      temp_prefs.SetString(prefs::kDefaultSearchProviderName,
+                       GURL(search_url_string).host());

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

indentation

[simo, 2011/09/29 09:33:08]

Done.

+    }
+
+    // And clear the IDs since these are not specified via policy.
+    temp_prefs.SetString(prefs::kDefaultSearchProviderID, std::string());
+    temp_prefs.SetString(prefs::kDefaultSearchProviderPrepopulateID,
+                     std::string());

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

indentation

[simo, 2011/09/29 09:33:08]

Done.

+    CopyTempPrefsToRealPrefs(&temp_prefs, prefs);
+    return;
+  }
+
+  // Required entries are not there.  Remove any related entries.
+  ClearDefaultSearchPreferences(&temp_prefs);
+  CopyTempPrefsToRealPrefs(&temp_prefs, prefs);

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

Wait. clear-then-copy?  Am I missing something or is this a very elaborate way
of doing nothing?

[simo, 2011/09/29 09:33:08]

Oh right, of course. I obviously wasn't thinking.

+}
+
+bool DefaultSearchPolicyHandler::CheckIndividualPolicies(
+    const PolicyMap* policies, PolicyErrorMap* errors) {
+  HandlerList::const_iterator handler = handlers_.begin();
+  for ( ; handler != handlers_.end(); ++handler) {
+    if (!(*handler)->CheckPolicySettings(policies, errors))
+      return false;

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

I think we shouldn't abort here, since we want to collect error messages for all
policies and not just the first one  failing.

[simo, 2011/09/29 09:33:08]

Done.

+  }
+  return true;
+}
+
+bool DefaultSearchPolicyHandler::DefaultSearchProviderIsDisabled(
+    const PolicyMap* policies) {
+  const Value* provider_enabled =
+      policies->Get(kPolicyDefaultSearchProviderEnabled);
+  bool enabled = true;
+  return provider_enabled &&
+         provider_enabled->GetAsBoolean(&enabled) &&
+         !enabled;
+}
+
+bool DefaultSearchPolicyHandler::DefaultSearchURLIsPresentAndValid(
+    const PolicyMap* policies) {
+  const Value* search_url =
+      policies->Get(kPolicyDefaultSearchProviderSearchURL);
+  std::string search_url_string;
+  if (search_url) {

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

you can as well invert the checks and put early returns, so you don't get into
indentation problems in 535.

[simo, 2011/09/29 09:33:08]

Done.

+    std::string search_url_string;
+    if (search_url->GetAsString(&search_url_string)) {
+      SearchTermsDataForValidation search_terms_data;
+      const TemplateURLRef search_url_ref(search_url_string, 0, 0);
+      // It must support replacement (which implies it is valid).
+      return
+          search_url_ref.SupportsReplacementUsingTermsData(search_terms_data);
+    }
+  }
+  return false;
+}
+
+void DefaultSearchPolicyHandler::EnsureStringPrefExists(PrefValueMap* prefs,
+    const std::string& path) {

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

function parameters must line up

[simo, 2011/09/29 09:33:08]

Done.

+  std::string value;
+  if (!prefs->GetString(path, &value))
+    prefs->SetString(path, value);
+}
+
+void DefaultSearchPolicyHandler::CopyTempPrefsToRealPrefs(

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

Can't we just implement a MergeFrom() in PrefValueMap?

[simo, 2011/09/29 09:33:08]

Done.

+    PrefValueMap* temp_prefs, PrefValueMap* prefs) {
+
+  for (unsigned int current = 0;
+      current < arraysize(kDefaultSearchPolicyMap); ++current) {
+    Value* value;
+    const char* pref_path = kDefaultSearchPolicyMap[current].preference_path;
+    if (temp_prefs->GetValue(pref_path, &value))
+        prefs->SetValue(pref_path, value->DeepCopy());
+  }
+}
+
+void DefaultSearchPolicyHandler::ClearDefaultSearchPreferences(
+    PrefValueMap* prefs) {
+

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

remove extra newline

[simo, 2011/09/29 09:33:08]

Done.

+  for (unsigned int current = 0;
+      current < arraysize(kDefaultSearchPolicyMap); ++current) {

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

+1 space indentation

[simo, 2011/09/29 09:33:08]

Done.

+    prefs->RemoveValue(kDefaultSearchPolicyMap[current].preference_path);
+  }
+}
+
+// ProxyPolicyHandler
+ProxyPolicyHandler::ProxyPolicyHandler() {
+}
+
+ProxyPolicyHandler::~ProxyPolicyHandler() {
+}
+
+bool ProxyPolicyHandler::CheckPolicySettings(const PolicyMap* policies,
+                                             PolicyErrorMap* errors) {
+  const Value* mode = GetProxyPolicyValue(policies, kPolicyProxyMode);
+  const Value* server = GetProxyPolicyValue(policies, kPolicyProxyServer);
+  const Value* server_mode =
+      GetProxyPolicyValue(policies, kPolicyProxyServerMode);
+  const Value* pac_url = GetProxyPolicyValue(policies, kPolicyProxyPacUrl);
+  const Value* bypass_list =
+      GetProxyPolicyValue(policies, kPolicyProxyBypassList);
+
+
+  if ((server || pac_url || bypass_list) && !(mode || server_mode)) {
+    errors->AddError(kPolicyProxyMode,
+                     IDS_POLICY_PROXY_MODE_NOT_SPECIFIED_ERROR);
+    return false;
+  }
+
+  std::string mode_value;
+  if (!CheckProxyModeAndServerMode(policies, errors, &mode_value))
+    return false;
+
+  // If neither ProxyMode nor ProxyServerMode are specified, mode_value will be
+  // empty and the proxy shouldn't be configured at all.
+  if (mode_value.empty())
+    return true;
+
+  if (mode_value == ProxyPrefs::kDirectProxyModeName) {

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

Hm, maybe we can specify all these checks here in a more declarative way? One
idea would be to make a table with each entry saying whether |server|,
|pac_url|, |bypass_list| are valid for a given |mode_value| and which
|message_id| to set if invalid policies are specified? Then you could loop over
the table entries and do handle everything in one block.

[simo, 2011/09/29 09:33:08]

I like that, I've done it :) 

+    if (server || pac_url || bypass_list) {
+      int message_id = IDS_POLICY_PROXY_MODE_DISABLED_ERROR;
+      if (server)
+        errors->AddError(kPolicyProxyServer, message_id);
+      if (bypass_list)
+        errors->AddError(kPolicyProxyBypassList, message_id);
+      if (pac_url)
+        errors->AddError(kPolicyProxyPacUrl, message_id);
+      return false;
+    }
+  } else if (mode_value == ProxyPrefs::kAutoDetectProxyModeName) {
+    if (server || bypass_list || pac_url) {
+      int message_id = IDS_POLICY_PROXY_MODE_AUTO_DETECT_ERROR;
+      if (server)
+        errors->AddError(kPolicyProxyServer, message_id);
+      if (bypass_list)
+        errors->AddError(kPolicyProxyBypassList, message_id);
+      if (pac_url)
+        errors->AddError(kPolicyProxyPacUrl, message_id);
+      return false;
+    }
+  } else if (mode_value == ProxyPrefs::kPacScriptProxyModeName) {
+    if (server || bypass_list) {
+      int message_id = IDS_POLICY_PROXY_MODE_PAC_URL_ERROR;
+      if (server)
+        errors->AddError(kPolicyProxyServer, message_id);
+      if (bypass_list)
+        errors->AddError(kPolicyProxyBypassList, message_id);
+      return false;
+    }
+  } else if (mode_value == ProxyPrefs::kFixedServersProxyModeName) {
+    if (pac_url) {
+      errors->AddError(kPolicyProxyPacUrl,
+                       IDS_POLICY_PROXY_MODE_FIXED_SERVERS_ERROR);
+      return false;
+    }
+  } else if (mode_value == ProxyPrefs::kSystemProxyModeName) {
+    if (server || pac_url || bypass_list) {
+      int message_id = IDS_POLICY_PROXY_MODE_SYSTEM_ERROR;
+      if (server)
+        errors->AddError(kPolicyProxyServer, message_id);
+      if (bypass_list)
+        errors->AddError(kPolicyProxyBypassList, message_id);
+      if (pac_url)
+        errors->AddError(kPolicyProxyPacUrl, message_id);
+      return false;
+    }
+  } else {
+    if (mode && server_mode)
+      errors->AddError(kPolicyProxyServerMode, IDS_POLICY_INVALID_ERROR);
+    else
+      errors->AddError(kPolicyProxyMode, IDS_POLICY_INVALID_ERROR);
+    return false;
+  }
+  return true;
+}
+
+void ProxyPolicyHandler::ApplyPolicySettings(const PolicyMap* policies,
+                                             PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  if (!CheckPolicySettings(policies, &errors)) {
+    GenerateLogMessages(&errors);
+    return;
+  }
+
+  const Value* mode = GetProxyPolicyValue(policies, kPolicyProxyMode);
+  const Value* server = GetProxyPolicyValue(policies, kPolicyProxyServer);
+  const Value* server_mode =
+      GetProxyPolicyValue(policies, kPolicyProxyServerMode);
+  const Value* pac_url = GetProxyPolicyValue(policies, kPolicyProxyPacUrl);
+  const Value* bypass_list =
+      GetProxyPolicyValue(policies, kPolicyProxyBypassList);
+

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

The following seems to be the same code as CheckProxyModeAndServerMode. Share?

[simo, 2011/09/29 09:33:08]

In CheckProxyModeAndServerMode the |mode_value| is a string whereas here it's
using an enum value for |proxy_mode|. I'm not sure how to unify these two.

+  ProxyPrefs::ProxyMode proxy_mode;
+  if (mode) {
+    std::string string_mode;
+    DCHECK(mode->GetAsString(&string_mode));

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

compiles away

[simo, 2011/09/29 09:33:08]

Done.

+    ProxyPrefs::StringToProxyMode(string_mode, &proxy_mode);
+  } else if (server_mode) {
+    int int_mode = 0;
+    DCHECK(server_mode->GetAsInteger(&int_mode));

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

ditto

[simo, 2011/09/29 09:33:08]

Done.

+    switch (int_mode) {
+      case kPolicyNoProxyServerMode:
+        proxy_mode = ProxyPrefs::MODE_DIRECT;
+        break;
+      case kPolicyAutoDetectProxyServerMode:
+        proxy_mode = ProxyPrefs::MODE_AUTO_DETECT;
+        break;
+      case kPolicyManuallyConfiguredProxyServerMode:
+        proxy_mode = ProxyPrefs::MODE_FIXED_SERVERS;
+        if (pac_url)
+          proxy_mode = ProxyPrefs::MODE_PAC_SCRIPT;
+        break;
+      case kPolicyUseSystemProxyServerMode:
+        proxy_mode = ProxyPrefs::MODE_SYSTEM;
+        break;
+      default:
+        proxy_mode = ProxyPrefs::MODE_DIRECT;
+        NOTREACHED();
+    }
+  } else {
+    return;
+  }

[Mattias Nissler (ping if slow), 2011/09/26 13:30:48]

I'd put a newline here.

[simo, 2011/09/29 09:33:08]

Done.

+  switch (proxy_mode) {
+    case ProxyPrefs::MODE_DIRECT:
+      prefs->SetValue(prefs::kProxy, ProxyConfigDictionary::CreateDirect());
+      break;
+    case ProxyPrefs::MODE_AUTO_DETECT:
+      prefs->SetValue(prefs::kProxy, ProxyConfigDictionary::CreateAutoDetect());
+      break;
+    case ProxyPrefs::MODE_PAC_SCRIPT: {
+      std::string pac_url_string;
+      DCHECK(pac_url->GetAsString(&pac_url_string));
+      prefs->SetValue(prefs::kProxy,
+          ProxyConfigDictionary::CreatePacScript(pac_url_string, false));
+      break;
+    }
+    case ProxyPrefs::MODE_FIXED_SERVERS: {
+      std::string proxy_server;
+      DCHECK(server->GetAsString(&proxy_server));
+      std::string bypass_list_string;
+      if (bypass_list)
+        DCHECK(bypass_list->GetAsString(&bypass_list_string));
+      prefs->SetValue(prefs::kProxy,
+                      ProxyConfigDictionary::CreateFixedServers(
+                          proxy_server, bypass_list_string));
+      break;
+    }
+    case ProxyPrefs::MODE_SYSTEM:
+      prefs->SetValue(prefs::kProxy,
+                      ProxyConfigDictionary::CreateSystem());
+      break;
+    case ProxyPrefs::kModeCount:
+      NOTREACHED();
+  }
+}
+
+const Value* ProxyPolicyHandler::GetProxyPolicyValue(
+    const PolicyMap* policies, ConfigurationPolicyType policy) {
+  const Value* value = policies->Get(policy);
+  std::string tmp;
+  if (!value ||
+      value->IsType(Value::TYPE_NULL) ||
+      (value->IsType(Value::TYPE_STRING) &&
+      value->GetAsString(&tmp) &&
+      tmp.empty())) {
+    return NULL;
+  }
+  return value;
+}
+
+bool ProxyPolicyHandler::CheckProxyModeAndServerMode(const PolicyMap* policies,
+                                                     PolicyErrorMap* errors,
+                                                     std::string* mode_value) {
+  const Value* mode = GetProxyPolicyValue(policies, kPolicyProxyMode);
+  const Value* server = GetProxyPolicyValue(policies, kPolicyProxyServer);
+  const Value* server_mode =
+      GetProxyPolicyValue(policies, kPolicyProxyServerMode);
+  const Value* pac_url = GetProxyPolicyValue(policies, kPolicyProxyPacUrl);
+  std::string message;
+
+  // If there's a server mode, convert it into a mode.
+  if (mode) {
+    if (server_mode) {
+      errors->AddError(kPolicyProxyMode, IDS_POLICY_PROXY_MODE_IGNORED);
+      LOG(WARNING) << message;
+    }
+    if (!mode->GetAsString(mode_value)) {
+      errors->AddError(kPolicyProxyMode, IDS_POLICY_INVALID_ERROR);
+      LOG(WARNING) << message;
+      return false;
+    }
+
+    ProxyPrefs::ProxyMode mode;
+    if (!ProxyPrefs::StringToProxyMode(*mode_value, &mode)) {
+      errors->AddError(kPolicyProxyMode, IDS_POLICY_NOT_RECOGNIZED_ERROR);
+      return false;
+    }
+
+    if (mode == ProxyPrefs::MODE_PAC_SCRIPT && !pac_url) {
+      errors->AddError(kPolicyProxyPacUrl, IDS_POLICY_NOT_SPECIFIED_ERROR);
+      return false;
+    } else if (mode == ProxyPrefs::MODE_FIXED_SERVERS && !server) {
+      errors->AddError(kPolicyProxyServer, IDS_POLICY_NOT_SPECIFIED_ERROR);
+      return false;
+    }
+  } else if (server_mode) {
+    int server_mode_value;
+    if (!server_mode->GetAsInteger(&server_mode_value)) {
+      errors->AddError(kPolicyProxyServerMode, IDS_POLICY_INVALID_ERROR);
+      LOG(WARNING) << message;
+      return false;
+    }
+
+    switch (server_mode_value) {
+      case kPolicyNoProxyServerMode:
+        *mode_value = ProxyPrefs::kDirectProxyModeName;
+        break;
+      case kPolicyAutoDetectProxyServerMode:
+        *mode_value = ProxyPrefs::kAutoDetectProxyModeName;
+        break;
+      case kPolicyManuallyConfiguredProxyServerMode:
+        if (server && pac_url) {
+          int message_id = IDS_POLICY_PROXY_BOTH_SPECIFIED_ERROR;
+          errors->AddError(kPolicyProxyServer, message_id);
+          errors->AddError(kPolicyProxyPacUrl, message_id);
+          return false;
+        }
+        if (!server && !pac_url) {
+          int message_id = IDS_POLICY_PROXY_NEITHER_SPECIFIED_ERROR;
+          errors->AddError(kPolicyProxyServer, message_id);
+          errors->AddError(kPolicyProxyPacUrl, message_id);
+          return false;
+        }
+        if (pac_url)
+          *mode_value = ProxyPrefs::kPacScriptProxyModeName;
+        else
+          *mode_value = ProxyPrefs::kFixedServersProxyModeName;
+        break;
+      case kPolicyUseSystemProxyServerMode:
+        *mode_value = ProxyPrefs::kSystemProxyModeName;
+        break;
+      default:
+        errors->AddError(kPolicyProxyServer, IDS_POLICY_INVALID_ERROR);
+        return false;
+    }
+  }
+  return true;
+}
+
+}  // namespace policy