Refactored ConfigurationPolicyProvider to provide PolicyBundles instead of PolicyMaps.

chrome/browser/policy/policy_service_impl.cc

Index: chrome/browser/policy/policy_service_impl.cc
diff --git a/chrome/browser/policy/policy_service_impl.cc b/chrome/browser/policy/policy_service_impl.cc
index 961a2a4038ba704a44f4300df99d204e02badacb..d927c657cf1c381bf10f0e9bfa12ecfac031cfe4 100644
--- a/chrome/browser/policy/policy_service_impl.cc
+++ b/chrome/browser/policy/policy_service_impl.cc
@@ -4,78 +4,61 @@
 
 #include "chrome/browser/policy/policy_service_impl.h"
 
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/observer_list.h"
 #include "base/stl_util.h"
-#include "content/public/browser/browser_thread.h"
-
-using content::BrowserThread;
+#include "chrome/browser/policy/policy_map.h"
 
 namespace policy {
 
-struct PolicyServiceImpl::Entry {
-  PolicyMap policies;
-  ObserverList<PolicyService::Observer, true> observers;
-};
-
-struct PolicyServiceImpl::ProviderData {
-  ConfigurationPolicyProvider* provider;
-  ConfigurationPolicyObserverRegistrar registrar;
-  PolicyMap policies;
-  bool refresh_pending;
-};
-
 PolicyServiceImpl::PolicyServiceImpl(const Providers& providers) {
   initialization_complete_ = true;
   for (size_t i = 0; i < providers.size(); ++i) {
     ConfigurationPolicyProvider* provider = providers[i];
-    ProviderData* data = new ProviderData;
-    data->provider = provider;
-    data->registrar.Init(provider, this);
-    data->refresh_pending = false;
-    if (provider->IsInitializationComplete())
-      provider->Provide(&data->policies);
-    else
-      initialization_complete_ = false;
-    providers_.push_back(data);
+    ConfigurationPolicyObserverRegistrar* registrar =
+        new ConfigurationPolicyObserverRegistrar();
+    registrar->Init(provider, this);
+    initialization_complete_ &= provider->IsInitializationComplete();
+    registrars_.push_back(registrar);
   }
   // There are no observers yet, but calls to GetPolicies() should already get
   // the processed policy values.
-  MergeAndTriggerUpdates(false);
+  MergeAndTriggerUpdates();
 }
 
 PolicyServiceImpl::~PolicyServiceImpl() {
-  STLDeleteElements(&providers_);
-  STLDeleteValues(&entries_);
+  STLDeleteElements(&registrars_);
+  STLDeleteValues(&observers_);
 }
 
 void PolicyServiceImpl::AddObserver(PolicyDomain domain,
                                     const std::string& component_id,
                                     PolicyService::Observer* observer) {
-  PolicyNamespace ns = std::make_pair(domain, component_id);
-  GetOrCreate(ns)->observers.AddObserver(observer);
+  PolicyBundle::PolicyNamespace ns(domain, component_id);
+  Observers*& list = observers_[ns];
+  if (!list)
+    list = new Observers();
+  list->AddObserver(observer);
 }
 
 void PolicyServiceImpl::RemoveObserver(PolicyDomain domain,
                                        const std::string& component_id,
                                        PolicyService::Observer* observer) {
-  PolicyNamespace ns = std::make_pair(domain, component_id);
-  EntryMap::const_iterator it = entries_.find(ns);
-  if (it == entries_.end()) {
+  PolicyBundle::PolicyNamespace ns(domain, component_id);
+  ObserverMap::iterator it = observers_.find(ns);
+  if (it == observers_.end()) {
     NOTREACHED();
     return;
   }
-  it->second->observers.RemoveObserver(observer);
-  MaybeCleanup(ns);
+  it->second->RemoveObserver(observer);
+  if (it->second->size() == 0) {
+    delete it->second;
+    observers_.erase(it);
+  }
 }
 
-const PolicyMap* PolicyServiceImpl::GetPolicies(
+const PolicyMap& PolicyServiceImpl::GetPolicies(
     PolicyDomain domain,
     const std::string& component_id) const {
-  PolicyNamespace ns = std::make_pair(domain, component_id);
-  EntryMap::const_iterator it = entries_.find(ns);
-  return it == entries_.end() ? NULL : &it->second->policies;
+  return policy_bundle_.Get(domain, component_id);
 }
 
 bool PolicyServiceImpl::IsInitializationComplete() const {
@@ -86,114 +69,137 @@ void PolicyServiceImpl::RefreshPolicies(const base::Closure& callback) {
   if (!callback.is_null())
     refresh_callbacks_.push_back(callback);
 
-  if (providers_.empty()) {
+  if (registrars_.empty()) {
     // Refresh is immediately complete if there are no providers.
-    MergeAndTriggerUpdates(true);
+    MergeAndTriggerUpdates();
   } else {
     // Some providers might invoke OnUpdatePolicy synchronously while handling
-    // RefreshPolicies. Flag all with refresh_pending before refreshing.
-    ProviderList::iterator it;
-    for (it = providers_.begin(); it != providers_.end(); ++it)
-      (*it)->refresh_pending = true;
-    for (it = providers_.begin(); it != providers_.end(); ++it)
-      (*it)->provider->RefreshPolicies();
+    // RefreshPolicies. Mark all as pending before refreshing.
+    RegistrarList::iterator it;
+    for (it = registrars_.begin(); it != registrars_.end(); ++it)
+      refresh_pending_.insert((*it)->provider());
+    for (it = registrars_.begin(); it != registrars_.end(); ++it)
+      (*it)->provider()->RefreshPolicies();
   }
 }
 
 void PolicyServiceImpl::OnUpdatePolicy(ConfigurationPolicyProvider* provider) {
-  ProviderList::iterator it = GetProviderData(provider);
-  if (it == providers_.end())
+  RegistrarList::iterator it = GetRegistrar(provider);
+  if (it == registrars_.end())
     return;
-  provider->Provide(&(*it)->policies);
-  bool did_refresh = (*it)->refresh_pending;
-  (*it)->refresh_pending = false;
-  MergeAndTriggerUpdates(did_refresh);
+  refresh_pending_.erase(provider);
+  MergeAndTriggerUpdates();
 }
 
 void PolicyServiceImpl::OnProviderGoingAway(
     ConfigurationPolicyProvider* provider) {
-  ProviderList::iterator it = GetProviderData(provider);
-  if (it == providers_.end())
+  RegistrarList::iterator it = GetRegistrar(provider);
+  if (it == registrars_.end())

[Mattias Nissler (ping if slow), 2012/05/14 09:07:21]

NOTREACHED()? Or can this situation actually happen?

[Joao da Silva, 2012/05/14 09:39:17]

GetRegistrar() already has a NOTREACHED() for this situation, but this method
has to return when that happens in release mode.

[Mattias Nissler (ping if slow), 2012/05/14 11:41:24]

OK.

     return;
-  bool did_refresh = (*it)->refresh_pending;
+  refresh_pending_.erase(provider);
   delete *it;
-  providers_.erase(it);
-  MergeAndTriggerUpdates(did_refresh);
+  registrars_.erase(it);
+  MergeAndTriggerUpdates();
 }
 
-PolicyServiceImpl::Entry* PolicyServiceImpl::GetOrCreate(
-    const PolicyNamespace& ns) {
-  Entry*& entry = entries_[ns];
-  if (!entry)
-    entry = new Entry;
-  return entry;
-}
-
-PolicyServiceImpl::ProviderList::iterator PolicyServiceImpl::GetProviderData(
+PolicyServiceImpl::RegistrarList::iterator PolicyServiceImpl::GetRegistrar(
     ConfigurationPolicyProvider* provider) {
-  for (ProviderList::iterator it = providers_.begin();
-       it != providers_.end(); ++it) {
-    if ((*it)->provider == provider)
+  for (RegistrarList::iterator it = registrars_.begin();

[Mattias Nissler (ping if slow), 2012/05/14 09:07:21]

std::find? Also, how about making this a std::map<Provider*, Registrar*>?

[Joao da Silva, 2012/05/14 09:39:17]

std::find() doesn't quite work, because what is being searched for isn't a value
contained in |registrars_| but a value whose ->provider() equals |provider|.
This could be done with std::find_if and a special predicate but I don't think
it'd improve readability.

|registrars_| is also in decreasing order of priority, and it must be preserved
so that merges take the right priorities into account. That's why it isn't a
map.

[Mattias Nissler (ping if slow), 2012/05/14 11:41:24]

I understand now.

+       it != registrars_.end(); ++it) {
+    if ((*it)->provider() == provider)
       return it;
   }
   NOTREACHED();
-  return providers_.end();
+  return registrars_.end();
 }
 
-void PolicyServiceImpl::MaybeCleanup(const PolicyNamespace& ns) {
-  EntryMap::iterator it = entries_.find(ns);
-  if (it != entries_.end() &&
-      it->second->policies.empty() &&
-      it->second->observers.size() == 0) {
-    delete it->second;
-    entries_.erase(it);
+void PolicyServiceImpl::NotifyNamespaceUpdated(
+    const PolicyBundle::PolicyNamespace& ns,
+    const PolicyMap& previous,
+    const PolicyMap& current) {
+  ObserverMap::iterator iterator = observers_.find(ns);
+  if (iterator != observers_.end()) {
+    FOR_EACH_OBSERVER(
+        PolicyService::Observer,
+        *iterator->second,
+        OnPolicyUpdated(ns.first, ns.second, previous, current));
   }
 }
 
-void PolicyServiceImpl::MergeAndTriggerUpdates(bool is_refresh) {
-  // TODO(joaodasilva): do this for each namespace once the providers also
-  // provide policy for more namespaces.
-
-  PolicyMap policies;
-  bool refresh_pending = false;
-  for (ProviderList::iterator it = providers_.begin();
-       it != providers_.end(); ++it) {
-    policies.MergeFrom((*it)->policies);
-    refresh_pending |= (*it)->refresh_pending;
+void PolicyServiceImpl::MergeAndTriggerUpdates() {

[Mattias Nissler (ping if slow), 2012/05/14 09:07:21]

Can we have a unit test for this?

[Joao da Silva, 2012/05/14 09:39:17]

This is covered by unit tests in policy_service_unittest.cc:

- NotifyObservers tests that the right notifications are triggered
- Priorities tests that the merging takes the providers' priorities into account
- RefreshPolicies tests that the refresh callbacks are invoked at the right time

Do you have some other test in mind?

[Mattias Nissler (ping if slow), 2012/05/14 11:41:24]

I was referring to checking whether different namespaces are handled properly.
Can we have a test case for that?

Also, I didn't see these tests when checking because they are in
policy_service_unittest.cc, whereas I had expected
policy_service_impl_unittest.cc. Move? Maybe not in this CL though.

[Joao da Silva, 2012/05/14 12:53:30]

Merging different namespaces within a bundle is tested at
policy_bundle_unittest.cc. I've added a new test that merges policy from more
than one provider and into shared namespaces.
Renamed to policy_service_impl_unittest.cc.

[Mattias Nissler (ping if slow), 2012/05/14 13:59:37]

I actually meant testing the notification generation below.

[Joao da Silva, 2012/05/14 15:26:51]

Ah got it, I misunderstood what you meant. Added a new test for that.

+  // Merge from each provider in their order of priority.
+  PolicyBundle bundle;
+  for (RegistrarList::iterator it = registrars_.begin();
+       it != registrars_.end(); ++it) {
+    bundle.MergeFrom((*it)->provider()->policies());
   }
 
-  Entry* entry = GetOrCreate(std::make_pair(POLICY_DOMAIN_CHROME, ""));
-  if (!policies.Equals(entry->policies)) {
-    // Swap first, so that observers that call GetPolicies() see the current
-    // values.
-    entry->policies.Swap(&policies);
-    FOR_EACH_OBSERVER(
-        PolicyService::Observer,
-        entry->observers,
-        OnPolicyUpdated(POLICY_DOMAIN_CHROME, "", policies, entry->policies));
+  // Swap first, so that observers that call GetPolicies() see the current
+  // values.
+  policy_bundle_.Swap(&bundle);
+
+  // Only notify observers of namespaces that have been modified.
+  const PolicyMap kEmpty;
+  PolicyBundle::const_iterator it_new = policy_bundle_.begin();
+  PolicyBundle::const_iterator end_new = policy_bundle_.end();
+  PolicyBundle::const_iterator it_old = bundle.begin();
+  PolicyBundle::const_iterator end_old = bundle.end();
+  while (it_new != end_new && it_old != end_old) {
+    if (it_new->first < it_old->first) {
+      // A new namespace is available.
+      NotifyNamespaceUpdated(it_new->first, kEmpty, *it_new->second);
+      ++it_new;
+    } else if (it_new->first > it_old->first) {
+      // A previously available namespace is now gone.
+      NotifyNamespaceUpdated(it_old->first, *it_old->second, kEmpty);
+      ++it_old;
+    } else {
+      if (!it_new->second->Equals(*it_old->second)) {
+        // An existing namespace's policies have changed.
+        NotifyNamespaceUpdated(it_new->first, *it_old->second, *it_new->second);
+      }
+      ++it_new;
+      ++it_old;
+    }
   }
 
+  // Send updates for the remaining new namespaces, if any.
+  for (; it_new != end_new; ++it_new)
+    NotifyNamespaceUpdated(it_new->first, kEmpty, *it_new->second);
+
+  // Sends updates for the remaining removed namespaces, if any.
+  for (; it_old != end_old; ++it_old)
+    NotifyNamespaceUpdated(it_old->first, *it_old->second, kEmpty);
+
+  CheckInitializationComplete();
+  CheckRefreshComplete();
+}
+
+void PolicyServiceImpl::CheckInitializationComplete() {
   // Check if all providers became initialized just now, if they weren't before.
   if (!initialization_complete_) {
     initialization_complete_ = true;
-    for (ProviderList::iterator iter = providers_.begin();
-         iter != providers_.end(); ++iter) {
-      if (!(*iter)->provider->IsInitializationComplete()) {
+    for (RegistrarList::iterator iter = registrars_.begin();
+         iter != registrars_.end(); ++iter) {
+      if (!(*iter)->provider()->IsInitializationComplete()) {
         initialization_complete_ = false;
         break;
       }
     }
     if (initialization_complete_) {
-      for (EntryMap::iterator i = entries_.begin(); i != entries_.end(); ++i) {
+      for (ObserverMap::iterator iter = observers_.begin();
+           iter != observers_.end(); ++iter) {
         FOR_EACH_OBSERVER(PolicyService::Observer,
-                          i->second->observers,
+                          *iter->second,
                           OnPolicyServiceInitialized());
       }
     }
   }
+}
 
+void PolicyServiceImpl::CheckRefreshComplete() {
   // Invoke all the callbacks if a refresh has just fully completed.
-  if (is_refresh && !refresh_pending) {
+  if (refresh_pending_.empty() && !refresh_callbacks_.empty()) {
     std::vector<base::Closure> callbacks;
     callbacks.swap(refresh_callbacks_);
     for (size_t i = 0; i < callbacks.size(); ++i)