Refactored device status uploading out of policy fetch infrastructure.

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

 // Copyright (c) 2012 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 COMPONENTS_POLICY_CORE_COMMON_CLOUD_CLOUD_POLICY_CLIENT_H_
 #define COMPONENTS_POLICY_CORE_COMMON_CLOUD_CLOUD_POLICY_CLIENT_H_
 
 #include <map>
 #include <set>
 #include <string>
@@ skipping 52 matching lines @@
 
     // Called when a request for device robot OAuth2 authorization tokens
     // returns successfully. Only occurs during enrollment. Optional
     // (default implementation is a noop).
     virtual void OnRobotAuthCodesFetched(CloudPolicyClient* client);
 
     // Indicates there's been an error in a previously-issued request.
     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
   // verification key to use.
   CloudPolicyClient(
       const std::string& machine_id,
       const std::string& machine_model,
       const std::string& verification_key_hash,
       UserAffiliation user_affiliation,
       DeviceManagementService* service,
@@ skipping 38 matching lines @@
   // Sends an unregistration request to the server.
   virtual void Unregister();
 
   // Upload a device certificate to the server.  Like FetchPolicy, this method
   // requires that the client is in a registered state.  |certificate_data| must
   // hold the X.509 certificate data to be sent to the server.  The |callback|
   // will be called when the operation completes.
   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;
   }
 
   void set_last_policy_timestamp(const base::Time& timestamp) {
     last_policy_timestamp_ = timestamp;
   }
 
   void set_public_key_version(int public_key_version) {
     public_key_version_ = public_key_version;
@@ skipping 125 matching lines @@
   int64 invalidation_version_;
   std::string invalidation_payload_;
 
   // The invalidation version used for the most recent fetch operation.
   int64 fetched_invalidation_version_;
 
   // Used for issuing requests to the cloud.
   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_;
 
   ObserverList<Observer, true> observers_;
   scoped_refptr<net::URLRequestContextGetter> request_context_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CloudPolicyClient);
 };
 
 }  // namespace policy
 
 #endif  // COMPONENTS_POLICY_CORE_COMMON_CLOUD_CLOUD_POLICY_CLIENT_H_