Refactored device status uploading out of policy fetch infrastructure.

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

Index: components/policy/core/common/cloud/cloud_policy_client.h
diff --git a/components/policy/core/common/cloud/cloud_policy_client.h b/components/policy/core/common/cloud/cloud_policy_client.h
index fbc215def972becff2ada99a32ad7e550c8500a6..df8512906396c5430e16eb07edcdc7c211a975d2 100644
--- a/components/policy/core/common/cloud/cloud_policy_client.h
+++ b/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,20 @@ class POLICY_EXPORT CloudPolicyClient {
   virtual void UploadCertificate(const std::string& certificate_data,
                                  const StatusCallback& callback);
 
+  // Creates a job to upload 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.
+  virtual scoped_ptr<DeviceManagementRequestJob> CreateUploadStatusJob(
+      const enterprise_management::DeviceStatusReportRequest* device_status,
+      const enterprise_management::SessionStatusReportRequest* session_status);

[Mattias Nissler (ping if slow), 2015/01/23 13:46:35]

Is there a good reason to return the request job instead of just taking a
callback that will be invoked once the operation completes? The latter would be
consistent with UploadCertificate() above at least and hides the existence of
request job objects (which can be considered an implementation detail).

[Andrew T Wilson (Slow), 2015/01/23 19:16:19]

Because I want to be able to cancel the request job if StatusUploader goes away.
Fundamentally, the request should have the lifetime of the StatusUploader
object, not the CloudPolicyClient.

Agreed that it would be nice to hide the type of request as an implementation
detail, but building the level of abstraction that would still let
StatusUploader control the lifetime of the request is way more work than this
abstraction is worth. And I don't think it's a particularly leaky abstraction to
expose the fact that DeviceManagementService is handling the network connection.

If you prefer, I could move the StartJob() call into CloudPolicyClient, though -
let me know, or if you have an alternative implementation.

[Mattias Nissler (ping if slow), 2015/01/26 10:37:02]

What's the benefit of cancelling the job, instead of just using a WeakPtr in the
callback? The request probably hits the net quickly, so cancelling while it's
pending on the server doesn't seem to give you any benefit? In that light, a
callback bound to a WeakPtr seems the more commonly used solution.

[Andrew T Wilson (Slow), 2015/01/28 13:19:12]

Done.

+

[Mattias Nissler (ping if slow), 2015/01/23 13:46:35]

remove extra blank line

[Andrew T Wilson (Slow), 2015/01/23 19:16:19]

Done.

+
   // 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;
   }
@@ -304,9 +287,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_;