Refactored device status uploading out of policy fetch infrastructure.

Created StatusUploader class to manage the uploading of device status.

Refactored status uploading functionality in cloud policy infrastructure - the
timing and contents of the device upload are now controlled by StatusUploader,
while CloudPolicyClient exposes an API to create the upload job.

BUG=430908

Review URL: https://codereview.chromium.org/845313008

Cr-Commit-Position: refs/heads/master@{#313535}

chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc

@@ -28,6 +28,7 @@
 #include "chrome/browser/chromeos/policy/enrollment_config.h"
 #include "chrome/browser/chromeos/policy/enterprise_install_attributes.h"
 #include "chrome/browser/chromeos/policy/server_backed_state_keys_broker.h"
+#include "chrome/browser/chromeos/policy/status_uploader.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chrome/browser/policy/device_management_service_configuration.h"
@@ -313,6 +314,9 @@ void BrowserPolicyConnectorChromeOS::RegisterPrefs(
   registry->RegisterIntegerPref(
       prefs::kDevicePolicyRefreshRate,
       CloudPolicyRefreshScheduler::kDefaultRefreshDelayMs);
+  registry->RegisterIntegerPref(
+      prefs::kDeviceStatusUploadRate,
+      StatusUploader::kDefaultUploadDelayMs);
 }
 
 void BrowserPolicyConnectorChromeOS::OnDeviceCloudPolicyManagerConnected() {

chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/chromeos/policy/device_status_collector.h"
 #include "chrome/browser/chromeos/policy/enterprise_install_attributes.h"
 #include "chrome/browser/chromeos/policy/server_backed_state_keys_broker.h"
+#include "chrome/browser/chromeos/policy/status_uploader.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/chromeos_constants.h"
 #include "chromeos/chromeos_switches.h"
@@ -113,6 +114,7 @@ DeviceCloudPolicyManagerChromeOS::DeviceCloudPolicyManagerChromeOS(
           BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO)),
       device_store_(store.Pass()),
       state_keys_broker_(state_keys_broker),
+      task_runner_(task_runner),
       local_state_(nullptr) {
 }
 
@@ -183,6 +185,7 @@ bool DeviceCloudPolicyManagerChromeOS::IsSharkRequisition() const {
 }
 
 void DeviceCloudPolicyManagerChromeOS::Shutdown() {
+  status_uploader_.reset();
   state_keys_update_subscription_.reset();
   CloudPolicyManager::Shutdown();
 }
@@ -225,18 +228,6 @@ void DeviceCloudPolicyManagerChromeOS::StartConnection(
     scoped_ptr<CloudPolicyClient> client_to_connect,
     EnterpriseInstallAttributes* install_attributes) {
   CHECK(!service());
-  // Enable device reporting for enterprise enrolled devices. We want to do this
-  // even if management is currently inactive, in case management is turned
-  // back on in a future policy fetch.
-  if (install_attributes->IsEnterpriseDevice()) {
-    client_to_connect->SetStatusProvider(
-        scoped_ptr<CloudPolicyClient::StatusProvider>(
-            new DeviceStatusCollector(
-                local_state_,
-                chromeos::system::StatisticsProvider::GetInstance(),
-                DeviceStatusCollector::LocationUpdateRequester(),
-                DeviceStatusCollector::VolumeInfoFetcher())));
-  }
 
   // Set state keys here so the first policy fetch submits them to the server.
   if (ForcedReEnrollmentEnabled())
@@ -249,6 +240,12 @@ void DeviceCloudPolicyManagerChromeOS::StartConnection(
   attestation_policy_observer_.reset(
       new chromeos::attestation::AttestationPolicyObserver(client()));
 
+  // Enable device reporting for enterprise enrolled devices. We want to do this
+  // even if management is currently inactive, in case management is turned
+  // back on in a future policy fetch.
+  if (install_attributes->IsEnterpriseDevice())
+    CreateStatusUploader();
+
   NotifyConnected();
 }
 
@@ -265,6 +262,7 @@ void DeviceCloudPolicyManagerChromeOS::Unregister(
 }
 
 void DeviceCloudPolicyManagerChromeOS::Disconnect() {
+  status_uploader_.reset();
   core()->Disconnect();
 
   NotifyDisconnected();
@@ -317,4 +315,17 @@ void DeviceCloudPolicyManagerChromeOS::NotifyDisconnected() {
       Observer, observers_, OnDeviceCloudPolicyManagerDisconnected());
 }
 
+void DeviceCloudPolicyManagerChromeOS::CreateStatusUploader() {
+  status_uploader_.reset(
+      new StatusUploader(
+          local_state_,
+          client(),
+          make_scoped_ptr(new DeviceStatusCollector(
+              local_state_,
+              chromeos::system::StatisticsProvider::GetInstance(),
+              DeviceStatusCollector::LocationUpdateRequester(),
+              DeviceStatusCollector::VolumeInfoFetcher())),
+          task_runner_));
+}
+
 }  // namespace policy

chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h

@@ -35,6 +35,7 @@ namespace policy {
 
 class DeviceCloudPolicyStoreChromeOS;
 class EnterpriseInstallAttributes;
+class StatusUploader;
 
 // CloudPolicyManager specialization for device policy on Chrome OS.
 class DeviceCloudPolicyManagerChromeOS : public CloudPolicyManager {
@@ -102,6 +103,8 @@ class DeviceCloudPolicyManagerChromeOS : public CloudPolicyManager {
     return device_store_.get();
   }
 
+  bool HasStatusUploaderForTest() { return status_uploader_; }
+
  private:
   // Saves the state keys received from |session_manager_client_|.
   void OnStateKeysUpdated();
@@ -112,11 +115,21 @@ class DeviceCloudPolicyManagerChromeOS : public CloudPolicyManager {
   void NotifyConnected();
   void NotifyDisconnected();
 
+  // Factory function to create the StatusUploader.
+  void CreateStatusUploader();
+
   // Points to the same object as the base CloudPolicyManager::store(), but with
   // actual device policy specific type.
   scoped_ptr<DeviceCloudPolicyStoreChromeOS> device_store_;
   ServerBackedStateKeysBroker* state_keys_broker_;
 
+  // Helper object that handles updating the server with our current device
+  // state.
+  scoped_ptr<StatusUploader> status_uploader_;
+
+  // The TaskRunner used to do device status uploads.
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
   ServerBackedStateKeysBroker::Subscription state_keys_update_subscription_;
 
   // PrefService instance to read the policy refresh rate from.

chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc

@@ -83,10 +83,6 @@ class TestingDeviceCloudPolicyManagerChromeOS
       : DeviceCloudPolicyManagerChromeOS(
             store.Pass(), task_runner, state_keys_broker) {}
   ~TestingDeviceCloudPolicyManagerChromeOS() override {}
-
-  bool HasStatusProvider() {
-    return client() ? client()->HasStatusProviderForTest() : false;
-  }
 };
 
 class DeviceCloudPolicyManagerChromeOSTest
@@ -255,8 +251,8 @@ TEST_F(DeviceCloudPolicyManagerChromeOSTest, EnrolledDevice) {
   base::RunLoop().RunUntilIdle();
   Mock::VerifyAndClearExpectations(&device_management_service_);
   ASSERT_TRUE(policy_fetch_job);
-  // Should create a status provider for reporting on enrolled devices.
-  EXPECT_TRUE(manager_->HasStatusProvider());
+  // Should create a status uploader for reporting on enrolled devices.
+  EXPECT_TRUE(manager_->HasStatusUploaderForTest());
   VerifyPolicyPopulated();
 
   manager_->Shutdown();
@@ -294,7 +290,7 @@ TEST_F(DeviceCloudPolicyManagerChromeOSTest, UnmanagedDevice) {
   ASSERT_TRUE(policy_fetch_job);
   // Should create a status provider for reporting on enrolled devices, even
   // those that aren't managed.
-  EXPECT_TRUE(manager_->HasStatusProvider());
+  EXPECT_TRUE(manager_->HasStatusUploaderForTest());
 
   // Switch back to ACTIVE, service the policy fetch and let it propagate.
   device_policy_.policy_data().set_state(em::PolicyData::ACTIVE);
@@ -322,7 +318,7 @@ TEST_F(DeviceCloudPolicyManagerChromeOSTest, ConsumerDevice) {
   ConnectManager();
   EXPECT_TRUE(manager_->policies().Equals(bundle));
   // Should not create a status provider for reporting on consumer devices.
-  EXPECT_FALSE(manager_->HasStatusProvider());
+  EXPECT_FALSE(manager_->HasStatusUploaderForTest());
 
   manager_->Shutdown();
   EXPECT_TRUE(manager_->policies().Equals(bundle));

chrome/browser/chromeos/policy/device_status_collector.cc

@@ -687,10 +687,16 @@ bool DeviceStatusCollector::GetDeviceStatus(
   if (report_hardware_status_)
     GetHardwareStatus(status);
 
-  return true;
+  return (report_activity_times_ ||
+          report_version_info_ ||
+          report_boot_mode_ ||
+          report_location_ ||
+          report_network_interfaces_ ||
+          report_users_ ||
+          report_hardware_status_);
 }
 
-bool DeviceStatusCollector::GetSessionStatus(
+bool DeviceStatusCollector::GetDeviceSessionStatus(
     em::SessionStatusReportRequest* status) {
   return false;
 }

chrome/browser/chromeos/policy/device_status_collector.h

@@ -21,7 +21,6 @@
 #include "base/timer/timer.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chromeos/system/version_loader.h"
-#include "components/policy/core/common/cloud/cloud_policy_client.h"
 #include "content/public/browser/geolocation_provider.h"
 #include "content/public/common/geoposition.h"
 #include "policy/proto/device_management_backend.pb.h"
@@ -45,7 +44,7 @@ class PrefService;
 namespace policy {
 
 // Collects and summarizes the status of an enterprised-managed ChromeOS device.
-class DeviceStatusCollector : public CloudPolicyClient::StatusProvider {
+class DeviceStatusCollector {
  public:
   // TODO(bartfab): Remove this once crbug.com/125931 is addressed and a proper
   // way to mock geolocation exists.
@@ -64,14 +63,23 @@ class DeviceStatusCollector : public CloudPolicyClient::StatusProvider {
       chromeos::system::StatisticsProvider* provider,
       const LocationUpdateRequester& location_update_requester,
       const VolumeInfoFetcher& volume_info_fetcher);
-  ~DeviceStatusCollector() override;
-
-  // CloudPolicyClient::StatusProvider:
-  bool GetDeviceStatus(
-      enterprise_management::DeviceStatusReportRequest* status) override;
-  bool GetSessionStatus(
-      enterprise_management::SessionStatusReportRequest* status) override;
-  void OnSubmittedSuccessfully() override;
+  virtual ~DeviceStatusCollector();
+
+  // Fills in the passed proto with device status information. Will return
+  // false if no status information is filled in (because status reporting
+  // is disabled).
+  virtual bool GetDeviceStatus(
+      enterprise_management::DeviceStatusReportRequest* status);
+
+  // Fills in the passed proto with session status information. Will return
+  // false if no status information is filled in (because status reporting
+  // is disabled, or because the active session is not a kiosk session).
+  virtual bool GetDeviceSessionStatus(
+      enterprise_management::SessionStatusReportRequest* status);
+
+  // Called after the status information has successfully been submitted to
+  // the server.
+  void OnSubmittedSuccessfully();
 
   static void RegisterPrefs(PrefRegistrySimple* registry);
 

chrome/browser/chromeos/policy/status_uploader.cc

+// Copyright (c) 2015 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/chromeos/policy/status_uploader.h"
+
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/location.h"
+#include "chrome/browser/chromeos/policy/device_status_collector.h"
+#include "chrome/common/pref_names.h"
+#include "components/policy/core/common/cloud/cloud_policy_client.h"
+#include "components/policy/core/common/cloud/device_management_service.h"
+
+namespace {
+const int kMinUploadDelayMs = 60 * 1000;  // 60 seconds
+}  // namespace
+
+namespace policy {
+
+const int64 StatusUploader::kDefaultUploadDelayMs =
+    3 * 60 * 60 * 1000;  // 3 hours
+
+StatusUploader::StatusUploader(
+    PrefService* local_state,
+    CloudPolicyClient* client,
+    scoped_ptr<DeviceStatusCollector> collector,
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner)
+    : client_(client),
+      collector_(collector.Pass()),
+      task_runner_(task_runner),
+      weak_factory_(this) {
+  // StatusUploader is currently only created for registered clients, and
+  // it is currently safe to assume that the client will not unregister while
+  // StatusUploader is alive.
+  //
+  // If future changes result in StatusUploader's lifetime extending beyond
+  // unregistration events, then this class should be updated
+  // to skip status uploads for unregistered clients, and to observe the client
+  // and kick off an upload when registration happens.
+  DCHECK(client->is_registered());
+
+  // Listen for changes to the upload delay, and start sending updates to the
+  // server.
+  upload_delay_.reset(new IntegerPrefMember());
+  upload_delay_->Init(
+      prefs::kDeviceStatusUploadRate, local_state,
+      base::Bind(&StatusUploader::ScheduleNextStatusUpload,
+                 base::Unretained(this)));
+  ScheduleNextStatusUpload();
+}
+
+StatusUploader::~StatusUploader() {
+}
+
+void StatusUploader::ScheduleNextStatusUpload() {
+  // Calculate when to fire off the next update (if it should have already
+  // happened, this yields a TimeDelta of 0).
+  base::TimeDelta delta = base::TimeDelta::FromMilliseconds(
+      std::max(kMinUploadDelayMs, upload_delay_->GetValue()));
+  base::TimeDelta delay =
+      std::max((last_upload_ + delta) - base::Time::NowFromSystemTime(),
+               base::TimeDelta());
+  upload_callback_.Reset(base::Bind(&StatusUploader::UploadStatus,
+                                    base::Unretained(this)));
+  task_runner_->PostDelayedTask(FROM_HERE, upload_callback_.callback(), delay);
+}
+
+void StatusUploader::UploadStatus() {
+  // If we already have an upload request active, don't bother starting another
+  // one.
+  if (request_job_)
+    return;
+
+  enterprise_management::DeviceStatusReportRequest device_status;
+  bool have_device_status = collector_->GetDeviceStatus(&device_status);
+  enterprise_management::SessionStatusReportRequest session_status;
+  bool have_session_status = collector_->GetDeviceSessionStatus(
+      &session_status);
+  if (!have_device_status && !have_session_status) {
+    // Don't have any status to upload - just set our timer for next time.
+    last_upload_ = base::Time::NowFromSystemTime();
+    ScheduleNextStatusUpload();
+    return;
+  }
+
+  client_->UploadDeviceStatus(
+      have_device_status ? &device_status : nullptr,
+      have_session_status ? &session_status : nullptr,
+      base::Bind(&StatusUploader::OnUploadCompleted,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void StatusUploader::OnUploadCompleted(bool success) {
+  // Set the last upload time, regardless of whether the upload was successful
+  // or not (we don't change the time of the next upload based on whether this
+  // upload succeeded or not - if a status upload fails, we just skip it and
+  // wait until it's time to try again.
+  last_upload_ = base::Time::NowFromSystemTime();
+
+  // If the upload was successful, tell the collector so it can clear its cache
+  // of pending items.
+  if (success)
+    collector_->OnSubmittedSuccessfully();
+
+  ScheduleNextStatusUpload();
+}
+
+}  // namespace policy

chrome/browser/chromeos/policy/status_uploader.h

+// Copyright (c) 2015 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_CHROMEOS_POLICY_STATUS_UPLOADER_H_
+#define CHROME_BROWSER_CHROMEOS_POLICY_STATUS_UPLOADER_H_
+
+#include "base/cancelable_callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/prefs/pref_member.h"
+#include "base/time/time.h"
+#include "components/policy/core/common/cloud/cloud_policy_constants.h"
+#include "policy/proto/device_management_backend.pb.h"
+
+namespace base {
+class PrefService;
+class SequencedTaskRunner;
+}
+
+namespace policy {
+
+class CloudPolicyClient;
+class DeviceManagementRequestJob;
+class DeviceStatusCollector;
+
+// Class responsible for periodically uploading device status from the
+// passed DeviceStatusCollector.
+class StatusUploader {
+ public:
+  // Refresh constants.
+  static const int64 kDefaultUploadDelayMs;
+
+  // Constructor. |client| must be registered and must stay
+  // valid and registered through the lifetime of this StatusUploader
+  // object.
+  StatusUploader(
+      PrefService* local_state,
+      CloudPolicyClient* client,
+      scoped_ptr<DeviceStatusCollector> collector,
+      const scoped_refptr<base::SequencedTaskRunner>& task_runner);
+
+  ~StatusUploader();
+
+  // Returns the time of the last successful upload, or Time(0) if no upload
+  // has ever happened.
+  base::Time last_upload() const { return last_upload_; }
+
+ private:
+  // Callback invoked periodically to upload the device status from the
+  // DeviceStatusCollector.
+  void UploadStatus();
+
+  // Invoked once a status upload has completed.
+  void OnUploadCompleted(bool success);
+
+  // Helper method that figures out when the next status upload should
+  // be scheduled.
+  void ScheduleNextStatusUpload();
+
+  // CloudPolicyClient used to issue requests to the server.
+  CloudPolicyClient* client_;
+
+  // The job associated with any ongoing requests to the cloud. We currently
+  // only support a single active request at a time.
+  scoped_ptr<DeviceManagementRequestJob> request_job_;
+
+  // DeviceStatusCollector that provides status for uploading.
+  scoped_ptr<DeviceStatusCollector> collector_;
+
+  // TaskRunner used for scheduling upload tasks.
+  const scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
+  // Pref item that specifies what our upload delay is currently.
+  scoped_ptr<IntegerPrefMember> upload_delay_;
+
+  // The time the last upload was performed.
+  base::Time last_upload_;
+
+  // Callback invoked via a delay to upload device status.
+  base::CancelableClosure upload_callback_;
+
+  // Note: This should remain the last member so it'll be destroyed and
+  // invalidate the weak pointers before any other members are destroyed.
+  base::WeakPtrFactory<StatusUploader> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(StatusUploader);
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_CHROMEOS_POLICY_STATUS_UPLOADER_H_

chrome/browser/chromeos/policy/status_uploader_unittest.cc

+// Copyright (c) 2015 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 "base/prefs/pref_registry_simple.h"
+#include "base/prefs/testing_pref_service.h"
+#include "base/test/test_simple_task_runner.h"
+#include "chrome/browser/chromeos/policy/device_status_collector.h"
+#include "chrome/browser/chromeos/policy/status_uploader.h"
+#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/device_settings_service.h"
+#include "chrome/common/pref_names.h"
+#include "components/policy/core/common/cloud/cloud_policy_client.h"
+#include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
+#include "components/policy/core/common/cloud/mock_device_management_service.h"
+#include "content/public/test/test_utils.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::_;
+using ::testing::Return;
+using ::testing::SaveArg;
+using ::testing::WithArgs;
+
+namespace em = enterprise_management;
+
+namespace {
+
+class MockDeviceStatusCollector : public policy::DeviceStatusCollector {
+ public:
+  explicit MockDeviceStatusCollector(PrefService* local_state)
+      : DeviceStatusCollector(
+            local_state,
+            nullptr,
+            policy::DeviceStatusCollector::LocationUpdateRequester(),
+            policy::DeviceStatusCollector::VolumeInfoFetcher()) {
+  }
+
+  MOCK_METHOD1(GetDeviceStatus, bool(em::DeviceStatusReportRequest*));
+  MOCK_METHOD1(GetDeviceSessionStatus, bool(em::SessionStatusReportRequest*));
+};
+
+}  // namespace
+
+namespace policy {
+class StatusUploaderTest : public testing::Test {
+ public:
+  StatusUploaderTest()
+      : task_runner_(new base::TestSimpleTaskRunner()) {
+    DeviceStatusCollector::RegisterPrefs(prefs_.registry());
+    prefs_.registry()->RegisterIntegerPref(
+        prefs::kDeviceStatusUploadRate,
+        StatusUploader::kDefaultUploadDelayMs);
+  }
+
+  void SetUp() override {
+    client_.SetDMToken("dm_token");
+    collector_.reset(new MockDeviceStatusCollector(&prefs_));
+  }
+
+  void TearDown() override {
+    content::RunAllBlockingPoolTasksUntilIdle();
+  }
+
+  // Given a pending task to upload status, mocks out a server response.
+  void RunPendingUploadTaskAndCheckNext(const StatusUploader& uploader) {
+    EXPECT_FALSE(task_runner_->GetPendingTasks().empty());
+    CloudPolicyClient::StatusCallback callback;
+    EXPECT_CALL(client_, UploadDeviceStatus(_, _, _))
+        .WillOnce(SaveArg<2>(&callback));
+    task_runner_->RunPendingTasks();
+    testing::Mock::VerifyAndClearExpectations(&device_management_service_);
+    // Make sure no status upload is queued up yet (since an upload is in
+    // progress).
+    EXPECT_TRUE(task_runner_->GetPendingTasks().empty());
+
+    // Now invoke the response.
+    callback.Run(true);
+
+    // Now that the previous request was satisfied, a task to do the next
+    // upload should be queued.
+    EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+
+    // The next refresh would have been scheduled when we invoked the callback.
+    // So the next task should be scheduled sometime between |last_upload| +
+    // kDefaultUploadDelayMs and |now| + kDefaultUploadDelayMs.
+    base::Time now = base::Time::NowFromSystemTime();
+    base::Time next_task = now + task_runner_->NextPendingTaskDelay();
+
+    base::TimeDelta expected_delay = base::TimeDelta::FromMilliseconds(
+        StatusUploader::kDefaultUploadDelayMs);
+    EXPECT_LE(next_task, now + expected_delay);
+    EXPECT_GE(next_task, uploader.last_upload() + expected_delay);
+  }
+
+  base::MessageLoop loop_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;
+  chromeos::ScopedTestCrosSettings test_cros_settings_;
+  scoped_ptr<MockDeviceStatusCollector> collector_;
+  MockCloudPolicyClient client_;
+  MockDeviceManagementService device_management_service_;
+  TestingPrefServiceSimple prefs_;
+};
+
+TEST_F(StatusUploaderTest, BasicTest) {
+  EXPECT_TRUE(task_runner_->GetPendingTasks().empty());
+  StatusUploader uploader(
+      &prefs_, &client_, collector_.Pass(), task_runner_);
+  EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+  // On startup, first update should happen immediately.
+  EXPECT_EQ(base::TimeDelta(), task_runner_->NextPendingTaskDelay());
+}
+
+TEST_F(StatusUploaderTest, ResetTimerAfterStatusCollection) {
+  // Keep a pointer to the mock collector because the scoped_ptr gets cleared
+  // when it is passed to CreateStatusUploader() below.
+  MockDeviceStatusCollector* mock_collector = collector_.get();
+  StatusUploader uploader(
+      &prefs_, &client_, collector_.Pass(), task_runner_);
+  EXPECT_CALL(*mock_collector, GetDeviceStatus(_)).WillRepeatedly(Return(true));
+  EXPECT_CALL(*mock_collector, GetDeviceSessionStatus(_)).WillRepeatedly(
+      Return(true));
+  RunPendingUploadTaskAndCheckNext(uploader);
+
+  // Handle this response also, and ensure new task is queued.
+  RunPendingUploadTaskAndCheckNext(uploader);
+
+  // Now that the previous request was satisfied, a task to do the next
+  // upload should be queued again.
+  EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+}
+
+TEST_F(StatusUploaderTest, ResetTimerAfterFailedStatusCollection) {
+  // Keep a pointer to the mock collector because the scoped_ptr gets cleared
+  // when it is passed to CreateStatusUploader() below.
+  MockDeviceStatusCollector* mock_collector = collector_.get();
+  StatusUploader uploader(
+      &prefs_, &client_, collector_.Pass(), task_runner_);
+  EXPECT_CALL(*mock_collector, GetDeviceStatus(_)).WillOnce(Return(false));
+  EXPECT_CALL(*mock_collector, GetDeviceSessionStatus(_)).WillOnce(
+      Return(false));
+  task_runner_->RunPendingTasks();
+
+  // Make sure the next status upload is queued up.
+  EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+}
+
+}  // namespace policy

chrome/browser/policy/test/policy_testserver.py

@@ -311,11 +311,20 @@ class PolicyRequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
     elif request_type == 'device_state_retrieval':
       response = self.ProcessDeviceStateRetrievalRequest(
           rmsg.device_state_retrieval_request)
+    elif request_type == 'status_upload':
+      response = self.ProcessStatusUploadRequest(
+          rmsg.device_status_report_request, rmsg.session_status_report_request)
     else:
       return (400, 'Invalid request parameter')
 
-    self.DumpMessage('Response', response[1])
-    return (response[0], response[1].SerializeToString())
+    if isinstance(response[1], basestring):
+      body = response[1]
+    elif isinstance(response[1], google.protobuf.message.Message):
+      self.DumpMessage('Response', response[1])
+      body = response[1].SerializeToString()
+    else:
+      body = ''
+    return (response[0], body)
 
   def CreatePolicyForExternalPolicyData(self, policy_key):
     """Returns an ExternalPolicyData protobuf for policy_key.
@@ -556,6 +565,24 @@ class PolicyRequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
         device_state_retrieval_response)
     return (200, response)
 
+  def ProcessStatusUploadRequest(self, device_status, session_status):
+    """Handles a device/session status upload request.
+
+    Returns:
+      A tuple of HTTP status code and response data to send to the client.
+    """
+    # Empty responses indicate a successful upload.
+    device_status_report_response = dm.DeviceStatusReportResponse()
+    session_status_report_response = dm.SessionStatusReportResponse()
+
+    response = dm.DeviceManagementResponse()
+    response.device_status_report_response.CopyFrom(
+        device_status_report_response)
+    response.session_status_report_response.CopyFrom(
+        session_status_report_response)
+
+    return (200, response)
+
   def SetProtobufMessageField(self, group_message, field, field_value):
     """Sets a field in a protobuf message.
 

chrome/chrome_browser_chromeos.gypi

@@ -815,6 +815,8 @@
         'browser/chromeos/policy/server_backed_device_state.h',
         'browser/chromeos/policy/server_backed_state_keys_broker.cc',
         'browser/chromeos/policy/server_backed_state_keys_broker.h',
+        'browser/chromeos/policy/status_uploader.cc',
+        'browser/chromeos/policy/status_uploader.h',
         'browser/chromeos/policy/ticl_device_settings_provider.cc',
         'browser/chromeos/policy/ticl_device_settings_provider.h',
         'browser/chromeos/policy/user_cloud_external_data_manager.cc',

chrome/chrome_tests_unit.gypi

@@ -1265,6 +1265,7 @@
       'browser/chromeos/policy/network_configuration_updater_unittest.cc',
       'browser/chromeos/policy/recommendation_restorer_unittest.cc',
       'browser/chromeos/policy/server_backed_state_keys_broker_unittest.cc',
+      'browser/chromeos/policy/status_uploader_unittest.cc',
       'browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc',
       'browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc',
       'browser/chromeos/power/power_data_collector_unittest.cc',

chrome/common/pref_names.cc

@@ -2055,6 +2055,10 @@ const char kCustomHandlersEnabled[] = "custom_handlers.enabled";
 // by the cloud policy subsystem.
 const char kDevicePolicyRefreshRate[] = "policy.device_refresh_rate";
 
+// Integer that specifies the upload rate for device and session status in
+// milliseconds.
+const char kDeviceStatusUploadRate[] = "policy.status_upload_rate";
+
 // A boolean where true means that the browser has previously attempted to
 // enable autoupdate and failed, so the next out-of-date browser start should
 // not prompt the user to enable autoupdate, it should offer to reinstall Chrome

chrome/common/pref_names.h

@@ -718,6 +718,7 @@ extern const char kHardwareAccelerationModeEnabled[];
 extern const char kHardwareAccelerationModePrevious[];
 
 extern const char kDevicePolicyRefreshRate[];
+extern const char kDeviceStatusUploadRate[];
 
 extern const char kFactoryResetRequested[];
 extern const char kDebuggingFeaturesRequested[];

components/policy/core/common/cloud/cloud_policy_client.cc

@@ -45,8 +45,6 @@ CloudPolicyClient::Observer::~Observer() {}
 void CloudPolicyClient::Observer::OnRobotAuthCodesFetched(
     CloudPolicyClient* client) {}
 
-CloudPolicyClient::StatusProvider::~StatusProvider() {}
-
 CloudPolicyClient::CloudPolicyClient(
     const std::string& machine_id,
     const std::string& machine_model,
@@ -186,18 +184,6 @@ void CloudPolicyClient::FetchPolicy() {
     }
   }
 
-  // Add status data.
-  if (status_provider_) {
-    if (!status_provider_->GetDeviceStatus(
-            request->mutable_device_status_report_request())) {
-      request->clear_device_status_report_request();
-    }
-    if (!status_provider_->GetSessionStatus(
-            request->mutable_session_status_report_request())) {
-      request->clear_session_status_report_request();
-    }
-  }
-
   // Add device state keys.
   if (!state_keys_to_upload_.empty()) {
     em::DeviceStateKeyUpdateRequest* key_update_request =
@@ -276,6 +262,36 @@ void CloudPolicyClient::UploadCertificate(
   request_job_->Start(job_callback);
 }
 
+void CloudPolicyClient::UploadDeviceStatus(
+    const em::DeviceStatusReportRequest* device_status,
+    const em::SessionStatusReportRequest* session_status,
+    const CloudPolicyClient::StatusCallback& callback) {
+  CHECK(is_registered());
+  // Should pass in at least one type of status.
+  DCHECK(device_status || session_status);
+  request_job_.reset(
+      service_->CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_STATUS,
+                          GetRequestContext()));
+  request_job_->SetDMToken(dm_token_);
+  request_job_->SetClientID(client_id_);
+
+  em::DeviceManagementRequest* request = request_job_->GetRequest();
+  if (device_status)
+    *request->mutable_device_status_report_request() = *device_status;
+  if (session_status)
+    *request->mutable_session_status_report_request() = *session_status;
+
+  DeviceManagementRequestJob::Callback job_callback = base::Bind(
+      &CloudPolicyClient::OnStatusUploadCompleted,
+      base::Unretained(this),
+      callback);
+
+  // TODO(atwilson): Change CloudPolicyClient to support multiple requests in
+  // parallel, so status upload requests don't get cancelled by things like
+  // policy fetches (http://crbug.com/452563).
+  request_job_->Start(job_callback);
+}
+
 void CloudPolicyClient::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
@@ -284,14 +300,6 @@ void CloudPolicyClient::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-void CloudPolicyClient::SetStatusProvider(scoped_ptr<StatusProvider> provider) {
-  status_provider_ = provider.Pass();
-}
-
-bool CloudPolicyClient::HasStatusProviderForTest() {
-  return status_provider_;
-}
-
 void CloudPolicyClient::AddPolicyTypeToFetch(
     const std::string& policy_type,
     const std::string& settings_entity_id) {
@@ -423,8 +431,6 @@ void CloudPolicyClient::OnPolicyFetchCompleted(
       }
       responses_[key] = new em::PolicyFetchResponse(response);
     }
-    if (status_provider_)
-      status_provider_->OnSubmittedSuccessfully();
     state_keys_to_upload_.clear();
     NotifyPolicyFetched();
   } else {
@@ -470,6 +476,18 @@ void CloudPolicyClient::OnCertificateUploadCompleted(
   callback.Run(true);
 }
 
+void CloudPolicyClient::OnStatusUploadCompleted(
+    const CloudPolicyClient::StatusCallback& callback,
+    DeviceManagementStatus status,
+    int net_error,
+    const enterprise_management::DeviceManagementResponse& response) {
+  status_ = status;
+  if (status != DM_STATUS_SUCCESS)
+    NotifyClientError();
+
+  callback.Run(status == DM_STATUS_SUCCESS);
+}
+
 void CloudPolicyClient::NotifyPolicyFetched() {
   FOR_EACH_OBSERVER(Observer, observers_, OnPolicyFetched(this));
 }

components/policy/core/common/cloud/cloud_policy_client.h

@@ -70,24 +70,6 @@ class POLICY_EXPORT CloudPolicyClient {
     virtual void OnClientError(CloudPolicyClient* client) = 0;
   };
 
-  // Delegate interface for supplying status information to upload to the server
-  // as part of the policy fetch request.
-  class POLICY_EXPORT StatusProvider {
-   public:
-    virtual ~StatusProvider();
-
-    // Retrieves status information to send with the next policy fetch.
-    // Implementations must return true if status information was filled in.
-    virtual bool GetDeviceStatus(
-        enterprise_management::DeviceStatusReportRequest* status) = 0;
-    virtual bool GetSessionStatus(
-        enterprise_management::SessionStatusReportRequest* status) = 0;
-
-    // Called after the status information has successfully been submitted to
-    // the server.
-    virtual void OnSubmittedSuccessfully() = 0;
-  };
-
   // |provider| and |service| are weak pointers and it's the caller's
   // responsibility to keep them valid for the lifetime of CloudPolicyClient.
   // |verification_key_hash| contains an identifier telling the DMServer which
@@ -146,19 +128,21 @@ class POLICY_EXPORT CloudPolicyClient {
   virtual void UploadCertificate(const std::string& certificate_data,
                                  const StatusCallback& callback);
 
+  // Uploads device/session status to the server. As above, the client must be
+  // in a registered state. If non-null, |device_status| and |session_status|
+  // will be included in the upload status request. The |callback| will be
+  // called when the operation completes.
+  virtual void UploadDeviceStatus(
+      const enterprise_management::DeviceStatusReportRequest* device_status,
+      const enterprise_management::SessionStatusReportRequest* session_status,
+      const StatusCallback& callback);
+
   // Adds an observer to be called back upon policy and state changes.
   void AddObserver(Observer* observer);
 
   // Removes the specified observer.
   void RemoveObserver(Observer* observer);
 
-  // Sets the status provider for this client, or NULL if no provider.
-  // ClouldPolicyClient takes ownership of the passed |provider|.
-  void SetStatusProvider(scoped_ptr<StatusProvider> provider);
-
-  // Returns true if this object has a StatusProvider configured.
-  bool HasStatusProviderForTest();
-
   void set_submit_machine_id(bool submit_machine_id) {
     submit_machine_id_ = submit_machine_id;
   }
@@ -270,6 +254,13 @@ class POLICY_EXPORT CloudPolicyClient {
       int net_error,
       const enterprise_management::DeviceManagementResponse& response);
 
+  // Callback for status upload requests.
+  void OnStatusUploadCompleted(
+      const StatusCallback& callback,
+      DeviceManagementStatus status,
+      int net_error,
+      const enterprise_management::DeviceManagementResponse& response);
+
   // Observer notification helpers.
   void NotifyPolicyFetched();
   void NotifyRegistrationStateChanged();
@@ -304,9 +295,6 @@ class POLICY_EXPORT CloudPolicyClient {
   DeviceManagementService* service_;
   scoped_ptr<DeviceManagementRequestJob> request_job_;
 
-  // Status upload data is produced by |status_provider_|.
-  scoped_ptr<StatusProvider> status_provider_;
-
   // The policy responses returned by the last policy fetch operation.
   ResponseMap responses_;
   DeviceManagementStatus status_;

components/policy/core/common/cloud/cloud_policy_constants.cc

@@ -37,6 +37,7 @@ const char kValueRequestApiAuthorization[] = "api_authorization";
 const char kValueRequestUnregister[] = "unregister";
 const char kValueRequestUploadCertificate[] = "cert_upload";
 const char kValueRequestDeviceStateRetrieval[] = "device_state_retrieval";
+const char kValueRequestUploadStatus[] = "status_upload";
 const char kValueUserAffiliationManaged[] = "managed";
 const char kValueUserAffiliationNone[] = "none";
 

components/policy/core/common/cloud/cloud_policy_constants.h

@@ -38,6 +38,7 @@ POLICY_EXPORT extern const char kValueRequestApiAuthorization[];
 POLICY_EXPORT extern const char kValueRequestUnregister[];
 POLICY_EXPORT extern const char kValueRequestUploadCertificate[];
 POLICY_EXPORT extern const char kValueRequestDeviceStateRetrieval[];
+POLICY_EXPORT extern const char kValueRequestUploadStatus[];
 POLICY_EXPORT extern const char kValueUserAffiliationManaged[];
 POLICY_EXPORT extern const char kValueUserAffiliationNone[];
 

components/policy/core/common/cloud/device_management_service.cc

@@ -125,6 +125,8 @@ const char* JobTypeToRequestType(DeviceManagementRequestJob::JobType type) {
       return dm_protocol::kValueRequestUploadCertificate;
     case DeviceManagementRequestJob::TYPE_DEVICE_STATE_RETRIEVAL:
       return dm_protocol::kValueRequestDeviceStateRetrieval;
+    case DeviceManagementRequestJob::TYPE_UPLOAD_STATUS:
+      return dm_protocol::kValueRequestUploadStatus;
   }
   NOTREACHED() << "Invalid job type " << type;
   return "";

components/policy/core/common/cloud/device_management_service.h

@@ -45,6 +45,7 @@ class POLICY_EXPORT DeviceManagementRequestJob {
     TYPE_UNREGISTRATION,
     TYPE_UPLOAD_CERTIFICATE,
     TYPE_DEVICE_STATE_RETRIEVAL,
+    TYPE_UPLOAD_STATUS,
   };
 
   typedef base::Callback<

components/policy/core/common/cloud/mock_cloud_policy_client.h

@@ -30,6 +30,10 @@ class MockCloudPolicyClient : public CloudPolicyClient {
   MOCK_METHOD0(Unregister, void(void));
   MOCK_METHOD2(UploadCertificate,
                void(const std::string&, const StatusCallback&));
+  MOCK_METHOD3(UploadDeviceStatus,
+               void(const enterprise_management::DeviceStatusReportRequest*,
+                    const enterprise_management::SessionStatusReportRequest*,
+                    const StatusCallback&));
 
   // Sets the DMToken.
   void SetDMToken(const std::string& token);