Move user cloud policy to BrowserProcess.

chrome/browser/policy/cloud_policy_provider.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/cloud_policy_provider.h"
+
+#include <set>
+
+#include "base/values.h"
+#include "chrome/browser/policy/configuration_policy_pref_store.h"
+#include "chrome/browser/policy/policy_notifier.h"
+
+namespace policy {
+
+CloudPolicyProvider::CloudPolicyProvider(
+    const ConfigurationPolicyProvider::PolicyDefinitionList* policy_list,
+    CloudPolicyCacheBase::PolicyLevel level)
+  : ConfigurationPolicyProvider(policy_list),
+    cache_(NULL),
+    level_(level) {}
+
+CloudPolicyProvider::~CloudPolicyProvider() {
+  FOR_EACH_OBSERVER(ConfigurationPolicyProvider::Observer,
+                    observer_list_, OnProviderGoingAway());
+  if (cache_)
+    cache_->RemoveObserver(this);
+}
+
+void CloudPolicyProvider::set_cache(CloudPolicyCacheBase* cache) {
+  cache_ = cache;
+  cache_->AddObserver(this);

[Joao da Silva, 2011/05/31 14:50:23]

If this can replace a previous cache, shouldn't it remove |this| as an observer
of the previous cache too? I see OnCacheGoingAway() below; if it is expected
that cache_ is always NULL here, add a DCHECK(cache_ != NULL).

[sfeuz, 2011/06/03 08:30:35]

Good point.
However in the new design caches can't be removed externally but only go away on
OnCacheGoingAway. I think it is not necessary to add a RemoveCache-method.

+}
+
+bool CloudPolicyProvider::Provide(ConfigurationPolicyStoreInterface* store) {
+  if (!cache_)
+    return false;
+  if (level_ == CloudPolicyCacheBase::POLICY_LEVEL_MANDATORY)
+    ApplyPolicyMap(cache_->mandatory_policy(), store);
+  else if (level_ == CloudPolicyCacheBase::POLICY_LEVEL_RECOMMENDED)
+    ApplyPolicyMap(cache_->recommended_policy(), store);
+  return true;
+}
+
+bool CloudPolicyProvider::IsInitializationComplete() const {
+  return cache_ && cache_->initialization_complete();

[Joao da Silva, 2011/05/31 14:50:23]

It would be nicer to have consistent naming for this method :-)

[sfeuz, 2011/06/03 08:30:35]

I followed the convention that initialization_complete() is a simple getter for
|initialization_complete|. And IsInitializationComplete is given by
ConfigurationPolicyProvider.
If you feel strongly about it then I am happy to change it :-)

+}
+
+void CloudPolicyProvider::AddObserver(
+    ConfigurationPolicyProvider::Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+void CloudPolicyProvider::RemoveObserver(
+    ConfigurationPolicyProvider::Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
+void CloudPolicyProvider::OnCacheUpdate() {
+  FOR_EACH_OBSERVER(ConfigurationPolicyProvider::Observer,
+                    observer_list_, OnUpdatePolicy());
+}
+
+void CloudPolicyProvider::OnCacheGoingAway() {
+  cache_->RemoveObserver(this);
+  cache_ = NULL;
+}
+
+PolicyMap* CloudPolicyProvider::policy_map() {

[Joao da Silva, 2011/05/31 14:50:23]

Return const here?

[sfeuz, 2011/06/03 08:30:35]

Obsolete.

+  if (!cache_)
+    return NULL;
+  if (level_ == CloudPolicyCacheBase::POLICY_LEVEL_MANDATORY)
+    return cache_->mandatory_policy();
+  else if (level_ == CloudPolicyCacheBase::POLICY_LEVEL_RECOMMENDED)
+    return cache_->recommended_policy();
+  NOTREACHED();
+  return NULL;
+}
+
+CombiningCloudPolicyProvider::CombiningCloudPolicyProvider(
+   const ConfigurationPolicyProvider::PolicyDefinitionList* policy_list)
+  : ConfigurationPolicyProvider(policy_list) {}
+
+CombiningCloudPolicyProvider::~CombiningCloudPolicyProvider() {
+  FOR_EACH_OBSERVER(ConfigurationPolicyProvider::Observer,
+                    observer_list_, OnProviderGoingAway());
+}
+
+void CombiningCloudPolicyProvider::AddCloudPolicyProvider(
+    CloudPolicyProvider* cloud_policy_provider) {
+  cloud_policy_providers_.push_back(
+      new CloudPolicyProviderWithObserver(this,
+                                          cloud_policy_provider));
+}
+
+bool CombiningCloudPolicyProvider::Provide(
+    ConfigurationPolicyStoreInterface* store) {
+  typedef std::set<ConfigurationPolicyType> PolicySet;
+  PolicySet already_applied;
+  PolicySet newly_applied;
+  for (ListType::iterator i = cloud_policy_providers_.begin();
+       i != cloud_policy_providers_.end(); ++i) {
+    if ((*i)->cloud_policy_provider() &&
+        (*i)->cloud_policy_provider()->IsInitializationComplete()) {

[Joao da Silva, 2011/05/31 14:50:23]

How about this to save one level of nesting and some typing:

CloudPolicyProviderWithObserver* provider = (*i)->cloud_policy_provider();
if (!provider || !provider->IsInitializationComplete())
  continue;

[sfeuz, 2011/06/03 08:30:35]

Obsolete.

+      PolicyMap* policy_map = (*i)->cloud_policy_provider()->policy_map();
+      if (policy_map) {

[Joao da Silva, 2011/05/31 14:50:23]

Same:
if (!policy_map)
  continue;

[sfeuz, 2011/06/03 08:30:35]

Obsolete.

+        const PolicyDefinitionList* policy_list(policy_definition_list());

[Joao da Silva, 2011/05/31 14:50:23]

This can be moved outside the loop.

[sfeuz, 2011/06/03 08:30:35]

Obsolete.

+        for (const PolicyDefinitionList::Entry* j = policy_list->begin;
+             j != policy_list->end; ++j) {
+          // Already applied by a CloudProvider which takes precedence.
+          if (already_applied.find(j->policy_type)!=already_applied.end())

[Joao da Silva, 2011/05/31 14:50:23]

Nit: spaces around != ?

[sfeuz, 2011/06/03 08:30:35]

Obsolete.

+            continue;
+          const Value* value = policy_map->Get(j->policy_type);
+          if (value && value->IsType(j->value_type)) {
+            newly_applied.insert(j->policy_type);
+            store->Apply(j->policy_type, value->DeepCopy());
+          }
+        }
+        // Update the already_applied PolicySet
+        for (PolicySet::iterator policy = newly_applied.begin();
+             policy != newly_applied.end(); ++policy) {
+          already_applied.insert(*policy);
+          // In case that one Proxy Policy got applied, we want to set all of
+          // them as applied. We need that in order not to end up with an
+          // inconsistend Proxy-state in case that the different Policy-related
+          // Policies got set by different CloudPolicyProviders.
+          if (*policy == kPolicyProxyMode ||
+              *policy == kPolicyProxyServerMode ||
+              *policy == kPolicyProxyServer ||
+              *policy == kPolicyProxyPacUrl ||
+              *policy == kPolicyProxyBypassList) {
+            already_applied.insert(kPolicyProxyMode);
+            already_applied.insert(kPolicyProxyServerMode);
+            already_applied.insert(kPolicyProxyServer);
+            already_applied.insert(kPolicyProxyPacUrl);
+            already_applied.insert(kPolicyProxyBypassList);
+          }
+        }
+      }
+    }
+  }

[Mattias Nissler (ping if slow), 2011/05/31 14:14:19]

6 levels of nesting? Compared to the existing merging code, this is super-hard
to follow. How about implementing a Merge() function that takes two policy maps
and takes care of handling the necessary grouping?

[Joao da Silva, 2011/05/31 14:50:23]

We should discuss this code.

The original only merged the proxy settings, but this code will merge anything
that hasn't been set by the user policy. Other policies might also require
special handling (for example, DefaultSearchProvider*).

[sfeuz, 2011/06/03 08:30:35]

I think we can not do much better than adding a special case for
DefaultSearchProvider.
Or maybe a generic Grouping-Mechanism to only apply them in groups but probably
overkill for 2 groups?

+  // Return true if we could apply at least one policy.
+  return !already_applied.empty();
+}
+
+bool CombiningCloudPolicyProvider::IsInitializationComplete() const {
+  bool initialization_complete = false;
+  for (ListType::const_iterator i = cloud_policy_providers_.begin();
+       i != cloud_policy_providers_.end(); ++i) {
+    initialization_complete = initialization_complete ||

[Joao da Silva, 2011/05/31 14:50:23]

This means that initialization is complete once *any* of the providers is
initialized. Is this what is intended? Or did you intend to return true when
*all* the providers are initialized?

In that case, you could just return false during the loop if one provider is not
initialized. If all are initialized and none returns from the loop, just return
true after the loop.

[sfeuz, 2011/06/03 08:30:35]

See comments above about what to return here.

+      ((*i)->cloud_policy_provider() &&
+       (*i)->cloud_policy_provider()->IsInitializationComplete());
+  }
+  return initialization_complete;
+}
+
+void CombiningCloudPolicyProvider::AddObserver(
+    ConfigurationPolicyProvider::Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void CombiningCloudPolicyProvider::RemoveObserver(
+    ConfigurationPolicyProvider::Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
+// Note: we are triggering updates even though the change might not actually be
+// visible to the outside.
+void CombiningCloudPolicyProvider::OnUpdatePolicy(
+    CloudPolicyProvider* cloud_policy_provider) {
+  FOR_EACH_OBSERVER(ConfigurationPolicyProvider::Observer,
+                    observer_list_, OnUpdatePolicy());
+}
+
+// Removes the going-away CloudPolicyProvider from |cloud_policy_providers_|.
+void CombiningCloudPolicyProvider::OnProviderGoingAway(
+    CloudPolicyProvider* cloud_policy_provider) {
+  ListType::iterator i;
+  for (i = cloud_policy_providers_.begin();
+       i != cloud_policy_providers_.end(); ++i) {
+    if ((*i)->cloud_policy_provider() == cloud_policy_provider) {
+      cloud_policy_providers_.erase(i);
+      return;
+    }
+  }
+}
+
+}  // namespace policy