Move user cloud policy to BrowserProcess.

chrome/browser/policy/cloud_policy_provider.h

+// 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.
+
+#ifndef CHROME_BROWSER_POLICY_CLOUD_POLICY_PROVIDER_H_
+#define CHROME_BROWSER_POLICY_CLOUD_POLICY_PROVIDER_H_
+#pragma once
+
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "base/observer_list.h"
+#include "chrome/browser/policy/cloud_policy_cache_base.h"
+#include "chrome/browser/policy/configuration_policy_provider.h"
+#include "chrome/browser/policy/policy_map.h"
+
+namespace policy {
+
+// A thin wrapper around CloudPolicyCacheBase. Proxies the notifications and
+// delegates the actions to the underlying |cache_|. Also exposes the PolicyMap
+// of |cache_|.
+// The |cache_| is kept as a weak reference and can be exchanged at any point
+// destroying the CloudPolicyProvider instance.

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

destroying is not exactly exchanging the |cache_| pointer, is it? Also, the
set_cache() call below suggests that you can replace the backend without
destroying anything.

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

I meant to write "[...] at any point WITHOUT destroying the CloudPolicyProvider
instance."

+class CloudPolicyProvider : public ConfigurationPolicyProvider,
+                            public CloudPolicyCacheBase::Observer {
+ public:
+  CloudPolicyProvider(const PolicyDefinitionList* policy_list,
+                      CloudPolicyCacheBase::PolicyLevel level);
+  virtual ~CloudPolicyProvider();
+
+  // ConfigurationPolicyProvider implementation.
+  virtual bool Provide(ConfigurationPolicyStoreInterface* store) OVERRIDE;
+  virtual bool IsInitializationComplete() const OVERRIDE;
+  virtual void AddObserver(ConfigurationPolicyProvider::Observer* observer)
+      OVERRIDE;
+  virtual void RemoveObserver(ConfigurationPolicyProvider::Observer* observer)
+      OVERRIDE;
+
+  // CloudPolicyCacheBase::Observer implementation.
+  virtual void OnCacheUpdate() OVERRIDE;
+  virtual void OnCacheGoingAway() OVERRIDE;
+
+  // Exposes |policy_map| of the underlying |cache_|.
+  PolicyMap* policy_map();

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

It feels wrong that you both expose the policy map and still implement the
ConfigurationPolicyProvider interface, which are both ways of getting at the
same information. To me, it seems like this is an indicator of the layering
becoming convoluted where there's actually no reason for that.

Here is my suggestion: Have CloudPolicyCacheBase implement whatever interface
you need to get to the actual policy data. Implement a cloud policy provider
that does the necessary merging and accesses whatever set of CloudPolicyCaches
it needs to talk to.

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

Should this be const?

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

Done. As we discussed offline.

+
+  // Sets another backend.
+  void set_cache(CloudPolicyCacheBase* cache);
+
+ private:
+  // The underlying policy cache. Can be NULL if currently none is present.
+  CloudPolicyCacheBase* cache_;
+  // Policy level this provider will handle.
+  CloudPolicyCacheBase::PolicyLevel level_;
+
+  // Provider observers that are registered with this provider.
+  ObserverList<ConfigurationPolicyProvider::Observer, true> observer_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(CloudPolicyProvider);
+};
+
+// Combines multiple CloudPolicyProviders and applies them in the given order.
+class CombiningCloudPolicyProvider : public ConfigurationPolicyProvider {
+ public:
+  explicit CombiningCloudPolicyProvider(
+      const PolicyDefinitionList* policy_list);
+  virtual ~CombiningCloudPolicyProvider();
+
+  // ConfigurationPolicyProvider implementation.
+  // Applies policy by applying the policies of the underlying
+  // CloudPolicyProviders in |cloud_policy_providers_| in the
+  // order they appear there. Early elements in |cloud_policy_providers_| take
+  // precedence. Handles special case for Proxy policy by marking all
+  // Proxy-related policies as applied as soon as one of them is applied.
+  // Returns true if we could apply at least one policy.

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

That seems wrong. It's totally OK for no policy to existing, why would we want
to return false then?

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

I agree. Just returning true is probably the right way to go. Does the same
apply for the initialization-query? I.e. should we always return true in
IsInitializationComplete()?

+  bool Provide(ConfigurationPolicyStoreInterface* store) OVERRIDE;

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

virtual

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

Done.

+
+  // Returns true if at least one CloudPolicyProvider in
+  // |cloud_policy_providers_| is initialized.
+  bool IsInitializationComplete() const OVERRIDE;

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

virtual

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

I'm not sure this is the correct behavior, instead of returning true when *all*
the cloud providers are initialized. Any reason for this?

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

Done.

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

I now believe the correct behaviour is to always return true, with the reasoning
that the Provider as is is, indeed, initialized and ready to serve what is
there. Why should we wait for all caches? They will issue an update once they
are ready anyways.

@mnissler: WDYT? 

+  void AddObserver(ConfigurationPolicyProvider::Observer* observer) OVERRIDE;

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

virtual

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

Done.

+  void RemoveObserver(ConfigurationPolicyProvider::Observer* observer) OVERRIDE;

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

virtual

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

Done.

+
+  // Callbacks for CloudPolicyProviderWithObserver.
+  void OnUpdatePolicy(CloudPolicyProvider* cloud_policy_provider);
+  void OnProviderGoingAway(CloudPolicyProvider* cloud_policy_provider);
+
+  // Adds a new CloudPolicyProvider to the end of |cloud_policy_providers_|.
+  // Does not take ownership of |cloud_policy_provider|.
+  void AddCloudPolicyProvider(CloudPolicyProvider* cloud_policy_provider);
+
+ private:
+  // Wrapper around a CloudPolicyProvider to include the source in the
+  // callbacks. We need that to figure out which element in
+  // |cloud_policy_providers_| to remove if a CloudPolicyProvider calls
+  // OnProviderGoingAway.

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

It seems like a adding an appropriate parameter to the
CloudPolicyProvider::Observer interface would be not to hard and achieve the
same thing, no?

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

Added the parameter to the cache observer interface.

+  class CloudPolicyProviderWithObserver :
+      public ConfigurationPolicyProvider::Observer {
+   public:
+    CloudPolicyProviderWithObserver(
+        CombiningCloudPolicyProvider* combining_cloud_policy_provider,
+        CloudPolicyProvider* cloud_policy_provider) :
+        combining_cloud_policy_provider_(combining_cloud_policy_provider),
+        cloud_policy_provider_(cloud_policy_provider) {
+      DCHECK(combining_cloud_policy_provider_ && cloud_policy_provider_);
+      cloud_policy_provider_->AddObserver(this);
+    }
+    ~CloudPolicyProviderWithObserver() {
+      if (cloud_policy_provider_) {
+        cloud_policy_provider_->RemoveObserver(this);
+        cloud_policy_provider_ = NULL;
+      }
+    }
+    virtual void OnUpdatePolicy() {
+      combining_cloud_policy_provider_->OnUpdatePolicy(
+          cloud_policy_provider_);
+    }
+    virtual void OnProviderGoingAway() {
+      combining_cloud_policy_provider_->OnProviderGoingAway(
+          cloud_policy_provider_);
+      // Normally our dtor is called on removal from |cloud_policy_providers_|,
+      // but just in case we are still active remove us as Observer.
+      if (cloud_policy_provider_) {
+        cloud_policy_provider_->RemoveObserver(this);
+        cloud_policy_provider_ = NULL;
+      }
+    }
+    CloudPolicyProvider* cloud_policy_provider() const {
+      return cloud_policy_provider_;
+    }
+
+   private:
+    CombiningCloudPolicyProvider* combining_cloud_policy_provider_;
+    CloudPolicyProvider* cloud_policy_provider_;
+
+    DISALLOW_COPY_AND_ASSIGN(CloudPolicyProviderWithObserver);
+  };
+
+  // CloudPolicyProviders which are combined by this instance of
+  // CombiningCoudPolicyProvider. Order dependant.

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

"Order dependant" conveys to me: There is something important about the ordering
here. Unfortunately, you leave it to the reader to figure out what that is.
That's really inconvenient, given that it's easy for you to just spell it out :)

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

Was explained in Provide.

+  typedef ScopedVector<CloudPolicyProviderWithObserver> ListType;
+  ListType cloud_policy_providers_;
+
+  // Provider observers that are registered with this provider.
+  ObserverList<ConfigurationPolicyProvider::Observer, true> observer_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(CombiningCloudPolicyProvider);
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_POLICY_CLOUD_POLICY_PROVIDER_H_