Pass the previous and the current PolicyMap to Observers of the PolicyService.

BUG=124374
TEST=PolicyServiceTest.NotifyObservers is green


Review URL: http://codereview.chromium.org/10174003

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

5 files changed, 71 insertions, 38 deletions

diff --git a/chrome/browser/policy/configuration_policy_pref_store.cc b/chrome/browser/policy/configuration_policy_pref_store.cc
index a4d0c02..00d7c9d 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store.cc
@@ -83,7 +83,9 @@ ConfigurationPolicyPrefStore::GetValue(const std::string& key,
 
 void ConfigurationPolicyPrefStore::OnPolicyUpdated(
     PolicyDomain domain,
-    const std::string& component_id) {
+    const std::string& component_id,
+    const PolicyMap& previous,
+    const PolicyMap& current) {
   DCHECK_EQ(POLICY_DOMAIN_CHROME, domain);
   DCHECK_EQ("", component_id);
   Refresh();
diff --git a/chrome/browser/policy/configuration_policy_pref_store.h b/chrome/browser/policy/configuration_policy_pref_store.h
index 93c7bd5..bbd13be 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.h
+++ b/chrome/browser/policy/configuration_policy_pref_store.h
@@ -42,7 +42,9 @@ class ConfigurationPolicyPrefStore
 
   // PolicyService::Observer methods:
   virtual void OnPolicyUpdated(PolicyDomain domain,
-                               const std::string& component_id) OVERRIDE;
+                               const std::string& component_id,
+                               const PolicyMap& previous,
+                               const PolicyMap& current) OVERRIDE;
   virtual void OnPolicyServiceInitialized() OVERRIDE;
 
   // Creates a ConfigurationPolicyPrefStore that only provides policies that
diff --git a/chrome/browser/policy/policy_service.h b/chrome/browser/policy/policy_service.h
index 124c41c..5b5f3f3 100644
--- a/chrome/browser/policy/policy_service.h
+++ b/chrome/browser/policy/policy_service.h
@@ -34,9 +34,12 @@ class PolicyService {
 
     // Invoked whenever policies for the |domain|, |component_id| namespace are
     // modified. This is only invoked for changes that happen after AddObserver
-    // is called.
+    // is called. |previous| contains the values of the policies before the
+    // update, and |current| contains the current values.
     virtual void OnPolicyUpdated(PolicyDomain domain,
-                                 const std::string& component_id) = 0;
+                                 const std::string& component_id,
+                                 const PolicyMap& previous,
+                                 const PolicyMap& current) = 0;
 
     // Invoked at most once, when the PolicyService becomes ready. If
     // IsInitializationComplete() is false, then this will be invoked once all
diff --git a/chrome/browser/policy/policy_service_impl.cc b/chrome/browser/policy/policy_service_impl.cc
index 5083a0c..a911166 100644
--- a/chrome/browser/policy/policy_service_impl.cc
+++ b/chrome/browser/policy/policy_service_impl.cc
@@ -125,18 +125,21 @@ void PolicyServiceImpl::MaybeCleanup(const PolicyNamespace& ns) {
 void PolicyServiceImpl::MergeAndTriggerUpdates() {
   // TODO(joaodasilva): do this for each namespace once the providers also
   // provide policy for more namespaces.
-  PolicyMap new_policies;
+  PolicyMap policies;
   for (ProviderList::iterator it = providers_.begin();
        it != providers_.end(); ++it) {
-    new_policies.MergeFrom((*it)->policies);
+    policies.MergeFrom((*it)->policies);
   }
 
   Entry* entry = GetOrCreate(std::make_pair(POLICY_DOMAIN_CHROME, ""));
-  if (!new_policies.Equals(entry->policies)) {
-    entry->policies.Swap(&new_policies);
-    FOR_EACH_OBSERVER(PolicyService::Observer,
-                      entry->observers,
-                      OnPolicyUpdated(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));
   }
 
   // Check if all providers became initialized just now, if they weren't before.
diff --git a/chrome/browser/policy/policy_service_unittest.cc b/chrome/browser/policy/policy_service_unittest.cc
index ad99a7b..9fa0de5 100644
--- a/chrome/browser/policy/policy_service_unittest.cc
+++ b/chrome/browser/policy/policy_service_unittest.cc
@@ -21,9 +21,20 @@ namespace {
 class MockPolicyServiceObserver : public PolicyService::Observer {
  public:
   virtual ~MockPolicyServiceObserver() {}
-  MOCK_METHOD2(OnPolicyUpdated, void(PolicyDomain, const std::string&));
+  MOCK_METHOD4(OnPolicyUpdated, void(PolicyDomain,
+                                     const std::string&,
+                                     const PolicyMap& previous,
+                                     const PolicyMap& current));
 };
 
+// Helper to compare the arguments to an EXPECT_CALL of OnPolicyUpdated() with
+// their expected values.
+MATCHER_P(PolicyEquals, expected, "") {
+  return arg.Equals(*expected);
+}
+
+}  // namespace
+
 class PolicyServiceTest : public testing::Test {
  public:
   PolicyServiceTest() {}
@@ -73,57 +84,71 @@ TEST_F(PolicyServiceTest, LoadsPoliciesBeforeProvidersRefresh) {
 }
 
 TEST_F(PolicyServiceTest, NotifyObservers) {
-  PolicyMap expected;
-  expected.Set("pre", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
-               base::Value::CreateIntegerValue(13));
-
-  expected.Set("aaa", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
-               base::Value::CreateIntegerValue(123));
+  PolicyMap expectedPrevious;
+  expectedPrevious.Set("pre", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+                       base::Value::CreateIntegerValue(13));
+
+  PolicyMap expectedCurrent;
+  expectedCurrent.CopyFrom(expectedPrevious);
+  expectedCurrent.Set("aaa", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+                      base::Value::CreateIntegerValue(123));
   provider0_.AddMandatoryPolicy("aaa", base::Value::CreateIntegerValue(123));
-  EXPECT_CALL(observer_, OnPolicyUpdated(POLICY_DOMAIN_CHROME, "")).Times(1);
+  EXPECT_CALL(observer_, OnPolicyUpdated(POLICY_DOMAIN_CHROME, "",
+                                         PolicyEquals(&expectedPrevious),
+                                         PolicyEquals(&expectedCurrent)));
   provider0_.RefreshPolicies();
   Mock::VerifyAndClearExpectations(&observer_);
-  EXPECT_TRUE(VerifyPolicies(POLICY_DOMAIN_CHROME, "", expected));
+
   // No changes.
-  EXPECT_CALL(observer_, OnPolicyUpdated(_, _)).Times(0);
+  EXPECT_CALL(observer_, OnPolicyUpdated(_, _, _, _)).Times(0);
   provider0_.RefreshPolicies();
   Mock::VerifyAndClearExpectations(&observer_);
-  EXPECT_TRUE(VerifyPolicies(POLICY_DOMAIN_CHROME, "", expected));
+  EXPECT_TRUE(VerifyPolicies(POLICY_DOMAIN_CHROME, "", expectedCurrent));
+
   // New policy.
-  expected.Set("bbb", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
-               base::Value::CreateIntegerValue(456));
+  expectedPrevious.CopyFrom(expectedCurrent);
+  expectedCurrent.Set("bbb", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+                      base::Value::CreateIntegerValue(456));
   provider0_.AddMandatoryPolicy("bbb", base::Value::CreateIntegerValue(456));
-  EXPECT_CALL(observer_, OnPolicyUpdated(POLICY_DOMAIN_CHROME, "")).Times(1);
+  EXPECT_CALL(observer_, OnPolicyUpdated(POLICY_DOMAIN_CHROME, "",
+                                         PolicyEquals(&expectedPrevious),
+                                         PolicyEquals(&expectedCurrent)));
   provider0_.RefreshPolicies();
   Mock::VerifyAndClearExpectations(&observer_);
-  EXPECT_TRUE(VerifyPolicies(POLICY_DOMAIN_CHROME, "", expected));
+
   // Removed policy.
-  expected.Erase("bbb");
+  expectedPrevious.CopyFrom(expectedCurrent);
+  expectedCurrent.Erase("bbb");
   provider0_.RemovePolicy("bbb");
-  EXPECT_CALL(observer_, OnPolicyUpdated(POLICY_DOMAIN_CHROME, "")).Times(1);
+  EXPECT_CALL(observer_, OnPolicyUpdated(POLICY_DOMAIN_CHROME, "",
+                                         PolicyEquals(&expectedPrevious),
+                                         PolicyEquals(&expectedCurrent)));
   provider0_.RefreshPolicies();
   Mock::VerifyAndClearExpectations(&observer_);
-  EXPECT_TRUE(VerifyPolicies(POLICY_DOMAIN_CHROME, "", expected));
+
   // Changed policy.
-  expected.Set("aaa", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
-               base::Value::CreateIntegerValue(789));
+  expectedPrevious.CopyFrom(expectedCurrent);
+  expectedCurrent.Set("aaa", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+                      base::Value::CreateIntegerValue(789));
   provider0_.AddMandatoryPolicy("aaa", base::Value::CreateIntegerValue(789));
-  EXPECT_CALL(observer_, OnPolicyUpdated(POLICY_DOMAIN_CHROME, "")).Times(1);
+  EXPECT_CALL(observer_, OnPolicyUpdated(POLICY_DOMAIN_CHROME, "",
+                                         PolicyEquals(&expectedPrevious),
+                                         PolicyEquals(&expectedCurrent)));
   provider0_.RefreshPolicies();
   Mock::VerifyAndClearExpectations(&observer_);
-  EXPECT_TRUE(VerifyPolicies(POLICY_DOMAIN_CHROME, "", expected));
+
   // No changes again.
-  EXPECT_CALL(observer_, OnPolicyUpdated(_, _)).Times(0);
+  EXPECT_CALL(observer_, OnPolicyUpdated(_, _, _, _)).Times(0);
   provider0_.RefreshPolicies();
   Mock::VerifyAndClearExpectations(&observer_);
-  EXPECT_TRUE(VerifyPolicies(POLICY_DOMAIN_CHROME, "", expected));
+  EXPECT_TRUE(VerifyPolicies(POLICY_DOMAIN_CHROME, "", expectedCurrent));
 }
 
 TEST_F(PolicyServiceTest, Priorities) {
   PolicyMap expected;
   expected.Set("pre", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
                base::Value::CreateIntegerValue(13));
-  EXPECT_CALL(observer_, OnPolicyUpdated(_, _)).Times(AnyNumber());
+  EXPECT_CALL(observer_, OnPolicyUpdated(_, _, _, _)).Times(AnyNumber());
 
   expected.Set("aaa", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
                base::Value::CreateIntegerValue(0));
@@ -148,6 +173,4 @@ TEST_F(PolicyServiceTest, Priorities) {
   EXPECT_TRUE(VerifyPolicies(POLICY_DOMAIN_CHROME, "", expected));
 }
 
-}  // namespace
-
 }  // namespace policy