Don't refresh policy immediately when fetched already.

Avoid an immediate policy fetch in CloudPolicyRefreshScheduler for the
case when the corresponding CloudPolicyClient has already downloaded
policy before the refresh scheduler was instantiated.

BUG=chromium:108928
TEST=unit tests


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

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

2 files changed, 31 insertions, 5 deletions

diff --git a/chrome/browser/policy/cloud_policy_refresh_scheduler.cc b/chrome/browser/policy/cloud_policy_refresh_scheduler.cc
index dc19b5d..da011df 100644
--- a/chrome/browser/policy/cloud_policy_refresh_scheduler.cc
+++ b/chrome/browser/policy/cloud_policy_refresh_scheduler.cc
@@ -116,8 +116,21 @@ void CloudPolicyRefreshScheduler::OnIPAddressChanged() {
 }
 
 void CloudPolicyRefreshScheduler::UpdateLastRefreshFromPolicy() {
-  if (store_->has_policy() && !store_->is_managed() &&
-      last_refresh_.is_null()) {
+  if (!last_refresh_.is_null())
+    return;
+
+  // If the client has already fetched policy, assume that happened recently. If
+  // that assumption ever breaks, the proper thing to do probably is to move the
+  // |last_refresh_| bookkeeping to CloudPolicyClient.
+  if (client_->policy()) {
+    last_refresh_ = base::Time::NowFromSystemTime();
+    return;
+  }
+
+  // If there is a cached non-managed response, make sure to only re-query the
+  // server after kUnmanagedRefreshDelayMs. NB: For existing policy, an
+  // immediate refresh is intentional.
+  if (store_->has_policy() && !store_->is_managed()) {
     last_refresh_ =
         base::Time::UnixEpoch() +
         base::TimeDelta::FromMilliseconds(store_->policy()->timestamp());
diff --git a/chrome/browser/policy/cloud_policy_refresh_scheduler_unittest.cc b/chrome/browser/policy/cloud_policy_refresh_scheduler_unittest.cc
index c741e18..235d9efd 100644
--- a/chrome/browser/policy/cloud_policy_refresh_scheduler_unittest.cc
+++ b/chrome/browser/policy/cloud_policy_refresh_scheduler_unittest.cc
@@ -129,7 +129,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InitialRefreshUnmanaged) {
   last_callback_.Run();
 }
 
-TEST_F(CloudPolicyRefreshSchedulerTest, InitialRefreshManaged) {
+TEST_F(CloudPolicyRefreshSchedulerTest, InitialRefreshManagedNotYetFetched) {
   scoped_ptr<CloudPolicyRefreshScheduler> scheduler(CreateRefreshScheduler());
   EXPECT_EQ(last_delay_, base::TimeDelta());
   ASSERT_FALSE(last_callback_.is_null());
@@ -137,6 +137,16 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InitialRefreshManaged) {
   last_callback_.Run();
 }
 
+TEST_F(CloudPolicyRefreshSchedulerTest, InitialRefreshManagedAlreadyFetched) {
+  last_refresh_ = base::Time::NowFromSystemTime();
+  client_.SetPolicy(em::PolicyFetchResponse());
+  scoped_ptr<CloudPolicyRefreshScheduler> scheduler(CreateRefreshScheduler());
+  CheckTiming(kPolicyRefreshRate);
+  ASSERT_FALSE(last_callback_.is_null());
+  EXPECT_CALL(client_, FetchPolicy()).Times(1);
+  last_callback_.Run();
+}
+
 TEST_F(CloudPolicyRefreshSchedulerTest, Unregistered) {
   client_.SetDMToken("");
   EXPECT_CALL(*task_runner_, PostDelayedTask(_, _, _)).Times(0);
@@ -256,11 +266,11 @@ class CloudPolicyRefreshSchedulerClientErrorTest
 
 TEST_P(CloudPolicyRefreshSchedulerClientErrorTest, OnClientError) {
   client_.SetStatus(GetParam().client_error);
+  last_delay_ = base::TimeDelta();
+  last_callback_.Reset();
 
   // See whether the error triggers the right refresh delay.
   int64 expected_delay_ms = GetParam().expected_delay_ms;
-  if (expected_delay_ms == -1)
-    EXPECT_CALL(*task_runner_, PostDelayedTask(_, _, _)).Times(0);
   client_.NotifyClientError();
   if (expected_delay_ms >= 0) {
     CheckTiming(expected_delay_ms);
@@ -275,6 +285,9 @@ TEST_P(CloudPolicyRefreshSchedulerClientErrorTest, OnClientError) {
                            GetParam().expected_delay_ms));
     } while (GetParam().backoff_factor > 1 &&
              expected_delay_ms <= kPolicyRefreshRate);
+  } else {
+    EXPECT_EQ(base::TimeDelta(), last_delay_);
+    EXPECT_TRUE(last_callback_.is_null());
   }
 }