Factor a scheduler object out of MetricsService.

chrome/browser/metrics/metrics_reporting_scheduler.cc

+// Copyright (c) 2011 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/metrics/metrics_reporting_scheduler.h"
+
+#include "base/compiler_specific.h"
+#include "chrome/browser/metrics/metrics_service.h"
+
+using base::Time;
+using base::TimeDelta;
+
+// The delay, in seconds, after startup before sending the first log message.
+static const int kInitialUploadIntervalSeconds = 60;
+
+// The delay, in seconds, between uploading when there are queued logs from
+// previous sessions to send.
+static const int kUnsentLogsIntervalSeconds = 15;
+
+// Standard interval between log uploads, in seconds.
+static const int kStandardUploadIntervalSeconds = 30 * 60;  // Thirty minutes.
+
+// When uploading metrics to the server fails, we progressively wait longer and
+// longer before sending the next log. This backoff process helps reduce load
+// on a server that is having issues.
+// The following is the multiplier we use to expand that inter-log duration.
+static const double kBackoffMultiplier = 1.1;
+
+// The maximum backoff multiplier.
+static const int kMaxBackoffMultiplier = 10;
+
+
+MetricsReportingScheduler::MetricsReportingScheduler(
+    MetricsService* metrics_service)
+    : metrics_service_(metrics_service),
+      ALLOW_THIS_IN_INITIALIZER_LIST(upload_timer_factory_(this)),
+      upload_interval_(TimeDelta::FromSeconds(kInitialUploadIntervalSeconds)),
+      running_(false),
+      timer_pending_(false),
+      callback_pending_(false) {
+}
+
+MetricsReportingScheduler::~MetricsReportingScheduler() {}

[jar (doing other things), 2011/04/16 02:03:13]

Why bother with a destructor?  ...or at best, list it in the header explicitly
(so that reader doesn't have to wonder what it does).

[stuartmorgan, 2011/04/18 17:03:00]

My understanding of
http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
is that this is the preferred style in our code (in fact, this class may reach
the score threshold where doing anything else would be a hard error under
clang).

+
+void MetricsReportingScheduler::Start() {
+  running_ = true;
+  ScheduleNextCallback();
+}
+
+void MetricsReportingScheduler::Stop() {
+  running_ = false;
+}
+
+void MetricsReportingScheduler::UploadFinished(bool server_is_healthy,
+                                               bool more_logs_remaining) {
+  DCHECK(callback_pending_);
+  callback_pending_ = false;
+  // If the server is having issues, back off. Otherwise, reset to default
+  // (unless there are more logs to send, in which case the next upload should
+  // happen sooner).
+  if (!server_is_healthy) {
+    BackOffUploadInterval();
+  } else if (more_logs_remaining) {
+    upload_interval_ = TimeDelta::FromSeconds(kUnsentLogsIntervalSeconds);
+  } else {
+    upload_interval_ = TimeDelta::FromSeconds(kStandardUploadIntervalSeconds);
+  }
+
+  if (running_)
+    ScheduleNextCallback();
+}
+
+void MetricsReportingScheduler::UploadCancelled() {
+  DCHECK(callback_pending_);
+  callback_pending_ = false;
+  if (running_)
+    ScheduleNextCallback();
+}
+
+void MetricsReportingScheduler::TriggerUpload() {
+  timer_pending_ = false;
+  callback_pending_ = true;
+  metrics_service_->StartScheduledUpload();
+}
+
+void MetricsReportingScheduler::ScheduleNextCallback() {
+  DCHECK(running_);
+  if (timer_pending_ || callback_pending_)
+    return;
+
+  // If the metrics service isn't recording and reporting, there's no point in
+  // starting an upload timer.
+  if (!metrics_service_->reporting_active() ||
+      !metrics_service_->recording_active())

[jar (doing other things), 2011/04/16 02:03:13]

I thought you reduced these two items into a single state variable in a prior
CL.  Am I confused?

In addition, you could leave this state checking to metrics_service_, and then
you'd have exactly one method you'd ever call in that class (re:
StartScheduledUpload()).

[stuartmorgan, 2011/04/18 17:03:00]

The prior CL eliminate a variable that was essentially a second copy of
reporting_active. MetricsService still tracks recording and reporting separately
(mostly for unit tests, it seems).
Done.

+    return;
+
+  timer_pending_ = true;
+
+  MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      upload_timer_factory_.NewRunnableMethod(
+          &MetricsReportingScheduler::TriggerUpload),
+      upload_interval_.InMilliseconds());
+}
+
+void MetricsReportingScheduler::BackOffUploadInterval() {
+  DCHECK(kBackoffMultiplier > 1.0);
+  upload_interval_ = TimeDelta::FromMicroseconds(
+      static_cast<int64>(kBackoffMultiplier *
+                         upload_interval_.InMicroseconds()));
+
+  TimeDelta max_interval = kMaxBackoffMultiplier *
+      TimeDelta::FromSeconds(kStandardUploadIntervalSeconds);
+  if (upload_interval_ > max_interval) {

[jar (doing other things), 2011/04/16 02:03:13]

You may as well be defensive and also check:
|| upload_interval_ < 0

[stuartmorgan, 2011/04/18 17:03:00]

Done.

+    upload_interval_ = max_interval;
+  }
+}