Refactored device status uploading out of policy fetch infrastructure.

chrome/browser/chromeos/policy/status_uploader.cc

Index: chrome/browser/chromeos/policy/status_uploader.cc
diff --git a/chrome/browser/chromeos/policy/status_uploader.cc b/chrome/browser/chromeos/policy/status_uploader.cc
new file mode 100644
index 0000000000000000000000000000000000000000..61aa419ab5b558a853f9130296d2b72e30531c67
--- /dev/null
+++ b/chrome/browser/chromeos/policy/status_uploader.cc
@@ -0,0 +1,108 @@
+// 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
+  // should be destroyed by the owner if the client is ever unregistered. If we

[Mattias Nissler (ping if slow), 2015/01/28 15:44:06]

nit: "should be destroyed" is ambiguous and might easily be understood to be "by
design", which is not the case. Can you clarify to say "and it is currently safe
to assume that the client doesn't unregister while StatusUploader is alive".

Similarly, can you change "If we want to support unregistered clients," to "If
future changes result in StatusUploader's lifetime colliding with unregistration
events,"

[Andrew T Wilson (Slow), 2015/01/28 16:13:09]

Done.

+  // want to support unregistered clients, 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.

[Mattias Nissler (ping if slow), 2015/01/28 15:44:06]

Tangential comment: If we allow multiple status upload jobs to be active, then
OnSubmittedSuccessfully might discard data in the following case:

1. Call UploadStatus()
2. Call UploadStatus() again, resulting in an upload that contains all data
collected by the first call + some more.
3. The first UploadStatus() succeeds and results in OnSubmittedSuccessfully()
being invoked, the collector discards its accumulated state.
4. The second request actually fails and ever reaches the server. This means the
data present in the second request but not the first is lost.

I think you'll want to consider this in the follow-up change to allow more than
one active request in CloudPolicyClient.

[Andrew T Wilson (Slow), 2015/01/28 16:13:09]

Yeah, I'll probably reset the WeakPtrFactory so only one callback will be
invoked at a time (the latest one). In any case, OnSubmittedSuccessfully() is
known to be leaky, since it clears the current cache, including any entries that
may have been collected between the last call to GetDeviceStatus() and the
success notification, so I'm not that concerned about this in general.

+  if (success)
+    collector_->OnSubmittedSuccessfully();
+
+  ScheduleNextStatusUpload();
+}
+
+}  // namespace policy