Simplify remoting::PolicyWatcher's cross-platform core.

Rather than maintain two separate lists of Boolean and string policies, and having their defaults for missing and type-mismatched values specified in a member function, PolicyWatcher now constructs DictionaryValues for these when constructed, and uses the values in the default dictionary to drive policy processing operations.

BUG=159891


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

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

4 files changed, 87 insertions, 139 deletions

diff --git a/remoting/host/policy_hack/policy_watcher.cc b/remoting/host/policy_hack/policy_watcher.cc
index 37c9009..40913ae 100644
--- a/remoting/host/policy_hack/policy_watcher.cc
+++ b/remoting/host/policy_hack/policy_watcher.cc
@@ -21,89 +21,36 @@ namespace remoting {
 namespace policy_hack {
 
 namespace {
+
 // The time interval for rechecking policy. This is our fallback in case the
 // delegate never reports a change to the ReloadObserver.
 const int kFallbackReloadDelayMinutes = 15;
 
-// Gets a boolean from a dictionary, or returns a default value if the boolean
-// couldn't be read.
-bool GetBooleanOrDefault(const base::DictionaryValue* dict, const char* key,
-                         bool default_if_value_missing,
-                         bool default_if_value_not_boolean) {
-  if (!dict->HasKey(key)) {
-    return default_if_value_missing;
-  }
-  const base::Value* value;
-  if (dict->Get(key, &value) && value->IsType(base::Value::TYPE_BOOLEAN)) {
-    bool bool_value;
-    CHECK(value->GetAsBoolean(&bool_value));
-    return bool_value;
-  }
-  return default_if_value_not_boolean;
-}
-
-// Gets a string from a dictionary, or returns a default value if the string
-// couldn't be read.
-std::string GetStringOrDefault(const base::DictionaryValue* dict,
-                               const char* key,
-                               const std::string& default_if_value_missing,
-                               const std::string& default_if_value_not_string) {
-  if (!dict->HasKey(key)) {
-    return default_if_value_missing;
-  }
-  const base::Value* value;
-  if (dict->Get(key, &value) && value->IsType(base::Value::TYPE_STRING)) {
-    std::string string_value;
-    CHECK(value->GetAsString(&string_value));
-    return string_value;
-  }
-  return default_if_value_not_string;
-}
-
-// Copies a boolean from one dictionary to another, using a default value
-// if the boolean couldn't be read from the first dictionary.
-void CopyBooleanOrDefault(base::DictionaryValue* to,
-                          const base::DictionaryValue* from, const char* key,
-                          bool default_if_value_missing,
-                          bool default_if_value_not_boolean) {
-  to->Set(key, base::Value::CreateBooleanValue(
-      GetBooleanOrDefault(from, key, default_if_value_missing,
-                          default_if_value_not_boolean)));
-}
+// Copies all policy values from one dictionary to another, using values from
+// |default| if they are not set in |from|, or values from |bad_type_values| if
+// the value in |from| has the wrong type.
+scoped_ptr<base::DictionaryValue> CopyGoodValuesAndAddDefaults(
+    const base::DictionaryValue* from,
+    const base::DictionaryValue* default_values,
+    const base::DictionaryValue* bad_type_values) {
+  scoped_ptr<base::DictionaryValue> to(default_values->DeepCopy());
+  for (base::DictionaryValue::Iterator i(*default_values);
+       i.HasNext(); i.Advance()) {
+
+    const base::Value* value = NULL;
+
+    // If the policy isn't in |from|, use the default.
+    if (!from->Get(i.key(), &value)) {
+      continue;
+    }
 
-// Copies a string from one dictionary to another, using a default value
-// if the string couldn't be read from the first dictionary.
-void CopyStringOrDefault(base::DictionaryValue* to,
-                         const base::DictionaryValue* from, const char* key,
-                         const std::string& default_if_value_missing,
-                         const std::string& default_if_value_not_string) {
-  to->Set(key, base::Value::CreateStringValue(
-      GetStringOrDefault(from, key, default_if_value_missing,
-                         default_if_value_not_string)));
-}
+    // If the policy is the wrong type, use the value from |bad_type_values|.
+    if (!value->IsType(i.value().GetType())) {
+      CHECK(bad_type_values->Get(i.key(), &value));
+    }
 
-// Copies all policy values from one dictionary to another, using default values
-// when necessary.
-scoped_ptr<base::DictionaryValue> AddDefaultValuesWhenNecessary(
-    const base::DictionaryValue* from) {
-  scoped_ptr<base::DictionaryValue> to(new base::DictionaryValue());
-  CopyBooleanOrDefault(to.get(), from,
-                       PolicyWatcher::kNatPolicyName, true, false);
-  CopyBooleanOrDefault(to.get(), from,
-                       PolicyWatcher::kHostRequireTwoFactorPolicyName,
-                       false, false);
-  CopyStringOrDefault(to.get(), from,
-                      PolicyWatcher::kHostDomainPolicyName, "", "");
-  CopyBooleanOrDefault(to.get(), from,
-                       PolicyWatcher::kHostMatchUsernamePolicyName,
-                       false, false);
-  CopyStringOrDefault(to.get(), from,
-                      PolicyWatcher::kHostTalkGadgetPrefixPolicyName,
-                      kDefaultHostTalkGadgetPrefix,
-                      kDefaultHostTalkGadgetPrefix);
-  CopyBooleanOrDefault(to.get(), from,
-                       PolicyWatcher::kHostRequireCurtainPolicyName,
-                       false, false);
+    to->Set(i.key(), value->DeepCopy());
+  }
 
   return to.Pass();
 }
@@ -128,29 +75,25 @@ const char PolicyWatcher::kHostTalkGadgetPrefixPolicyName[] =
 const char PolicyWatcher::kHostRequireCurtainPolicyName[] =
     "RemoteAccessHostRequireCurtain";
 
-const char* const PolicyWatcher::kBooleanPolicyNames[] =
-    { PolicyWatcher::kNatPolicyName,
-      PolicyWatcher::kHostRequireTwoFactorPolicyName,
-      PolicyWatcher::kHostMatchUsernamePolicyName,
-      PolicyWatcher::kHostRequireCurtainPolicyName
-    };
-
-const int PolicyWatcher::kBooleanPolicyNamesNum =
-    arraysize(kBooleanPolicyNames);
-
-const char* const PolicyWatcher::kStringPolicyNames[] =
-    { PolicyWatcher::kHostDomainPolicyName,
-      PolicyWatcher::kHostTalkGadgetPrefixPolicyName
-    };
-
-const int PolicyWatcher::kStringPolicyNamesNum =
-    arraysize(kStringPolicyNames);
-
 PolicyWatcher::PolicyWatcher(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : task_runner_(task_runner),
       old_policies_(new base::DictionaryValue()),
+      default_values_(new base::DictionaryValue()),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
+  // Initialize the default values for each policy.
+  default_values_->SetBoolean(kNatPolicyName, true);
+  default_values_->SetBoolean(kHostRequireTwoFactorPolicyName, false);
+  default_values_->SetBoolean(kHostRequireCurtainPolicyName, false);
+  default_values_->SetBoolean(kHostMatchUsernamePolicyName, false);
+  default_values_->SetString(kHostDomainPolicyName, "");
+  default_values_->SetString(kHostTalkGadgetPrefixPolicyName,
+                               kDefaultHostTalkGadgetPrefix);
+
+  // Initialize the fall-back values to use for unreadable policies.
+  // For most policies these match the defaults.
+  bad_type_values_.reset(default_values_->DeepCopy());
+  bad_type_values_->SetBoolean(kNatPolicyName, false);
 }
 
 PolicyWatcher::~PolicyWatcher() {
@@ -198,6 +141,10 @@ void PolicyWatcher::ScheduleReloadTask(const base::TimeDelta& delay) {
       delay);
 }
 
+const base::DictionaryValue& PolicyWatcher::Defaults() const {
+  return *default_values_;
+}
+
 bool PolicyWatcher::OnPolicyWatcherThread() const {
   return task_runner_->BelongsToCurrentThread();
 }
@@ -208,7 +155,8 @@ void PolicyWatcher::UpdatePolicies(
 
   // Use default values for any missing policies.
   scoped_ptr<base::DictionaryValue> new_policies =
-      AddDefaultValuesWhenNecessary(new_policies_raw);
+      CopyGoodValuesAndAddDefaults(
+          new_policies_raw, default_values_.get(), bad_type_values_.get());
 
   // Find the changed policies.
   scoped_ptr<base::DictionaryValue> changed_policies(
diff --git a/remoting/host/policy_hack/policy_watcher.h b/remoting/host/policy_hack/policy_watcher.h
index 95217dc..5a8f14f 100644
--- a/remoting/host/policy_hack/policy_watcher.h
+++ b/remoting/host/policy_hack/policy_watcher.h
@@ -81,13 +81,8 @@ class PolicyWatcher {
   void ScheduleFallbackReloadTask();
   void ScheduleReloadTask(const base::TimeDelta& delay);
 
-  // The names of policies with boolean values.
-  static const char* const kBooleanPolicyNames[];
-  static const int kBooleanPolicyNamesNum;
-
-  // The names of policies with string values.
-  static const char* const kStringPolicyNames[];
-  static const int kStringPolicyNamesNum;
+  // Returns a DictionaryValue containing the default values for each policy.
+  const base::DictionaryValue& Defaults() const;
 
  private:
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
@@ -95,6 +90,8 @@ class PolicyWatcher {
   PolicyCallback policy_callback_;
 
   scoped_ptr<base::DictionaryValue> old_policies_;
+  scoped_ptr<base::DictionaryValue> default_values_;
+  scoped_ptr<base::DictionaryValue> bad_type_values_;
 
   // Allows us to cancel any inflight FileWatcher events or scheduled reloads.
   base::WeakPtrFactory<PolicyWatcher> weak_factory_;
diff --git a/remoting/host/policy_hack/policy_watcher_mac.mm b/remoting/host/policy_hack/policy_watcher_mac.mm
index d0dd473..793c48b 100644
--- a/remoting/host/policy_hack/policy_watcher_mac.mm
+++ b/remoting/host/policy_hack/policy_watcher_mac.mm
@@ -45,34 +45,35 @@ class PolicyWatcherMac : public PolicyWatcher {
 
     CFStringRef policy_bundle_id = CFSTR("com.google.Chrome");
     if (CFPreferencesAppSynchronize(policy_bundle_id)) {
-      for (int i = 0; i < kBooleanPolicyNamesNum; ++i) {
-        const char* policy_name = kBooleanPolicyNames[i];
+      for (base::DictionaryValue::Iterator i(Defaults());
+           i.HasNext(); i.Advance()) {
+        const std::string& policy_name = i.key();
         base::mac::ScopedCFTypeRef<CFStringRef> policy_key(
             base::SysUTF8ToCFStringRef(policy_name));
-        Boolean valid = false;
-        bool allowed = CFPreferencesGetAppBooleanValue(policy_key,
+
+        if (i.value().GetType() == base::DictionaryValue::TYPE_BOOLEAN) {
+          Boolean valid = false;
+          bool value = CFPreferencesGetAppBooleanValue(policy_key,
                                                        policy_bundle_id,
                                                        &valid);
-        if (valid) {
-          policy.SetBoolean(policy_name, allowed);
+          if (valid) {
+            policy.SetBoolean(policy_name, value);
+          }
         }
-      }
-      for (int i = 0; i < kStringPolicyNamesNum; ++i) {
-        const char* policy_name = kStringPolicyNames[i];
-        base::mac::ScopedCFTypeRef<CFStringRef> policy_key(
-            base::SysUTF8ToCFStringRef(policy_name));
-        base::mac::ScopedCFTypeRef<CFPropertyListRef> property_list(
-            CFPreferencesCopyAppValue(policy_key, policy_bundle_id));
-        if (property_list.get() != NULL) {
-          CFStringRef policy_value = base::mac::CFCast<CFStringRef>(
-              property_list.get());
-          if (policy_value != NULL) {
-            policy.SetString(policy_name,
-                             base::SysCFStringRefToUTF8(policy_value));
-          } else {
-            LOG(WARNING) << "Policy " << policy_name << ": value not a string.";
+
+        if (i.value().GetType() == base::DictionaryValue::TYPE_STRING) {
+          base::mac::ScopedCFTypeRef<CFPropertyListRef> property_list(
+              CFPreferencesCopyAppValue(policy_key, policy_bundle_id));
+          if (property_list.get() != NULL) {
+            CFStringRef policy_value = base::mac::CFCast<CFStringRef>(
+                property_list.get());
+            if (policy_value != NULL) {
+              policy.SetString(policy_name,
+                               base::SysCFStringRefToUTF8(policy_value));
+            }
           }
         }
+
       }
     }
 
diff --git a/remoting/host/policy_hack/policy_watcher_win.cc b/remoting/host/policy_hack/policy_watcher_win.cc
index a333911..94d71f1 100644
--- a/remoting/host/policy_hack/policy_watcher_win.cc
+++ b/remoting/host/policy_hack/policy_watcher_win.cc
@@ -168,24 +168,26 @@ class PolicyWatcherWin :
     return ret;
   }
 
-  base::DictionaryValue* Load() {
-    base::DictionaryValue* policy = new base::DictionaryValue();
-
-    for (int i = 0; i < kBooleanPolicyNamesNum; ++i) {
-      const char* policy_name = kBooleanPolicyNames[i];
-      bool bool_value;
-      if (GetRegistryPolicyBoolean(policy_name, &bool_value)) {
-        policy->SetBoolean(policy_name, bool_value);
+  scoped_ptr<base::DictionaryValue> Load() {
+    scoped_ptr<base::DictionaryValue> policy(new base::DictionaryValue());
+
+    for (base::DictionaryValue::Iterator i(Defaults());
+         i.HasNext(); i.Advance()) {
+      const std::string& policy_name = i.key();
+      if (i.value().GetType() == base::DictionaryValue::TYPE_BOOLEAN) {
+        bool bool_value;
+        if (GetRegistryPolicyBoolean(policy_name, &bool_value)) {
+          policy->SetBoolean(policy_name, bool_value);
+        }
       }
-    }
-    for (int i = 0; i < kStringPolicyNamesNum; ++i) {
-      const char* policy_name = kStringPolicyNames[i];
-      std::string string_value;
-      if (GetRegistryPolicyString(policy_name, &string_value)) {
-        policy->SetString(policy_name, string_value);
+      if (i.value().GetType() == base::DictionaryValue::TYPE_STRING) {
+        std::string string_value;
+        if (GetRegistryPolicyString(policy_name, &string_value)) {
+          policy->SetString(policy_name, string_value);
+        }
       }
     }
-    return policy;
+    return policy.Pass();
   }
 
   // Post a reload notification and update the watch machinery.