Added system log uploader.

chrome/browser/chromeos/policy/system_log_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_SYSTEM_LOG_UPLOADER_H_
+#define CHROME_BROWSER_CHROMEOS_POLICY_SYSTEM_LOG_UPLOADER_H_
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/ref_counted_memory.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
+#include "chrome/browser/chromeos/policy/system_log_upload_job.h"
+
+namespace base {
+class SequencedTaskRunner;
+}
+
+namespace policy {
+
+// Class responsible for periodically uploading system logs.
+class SystemLogUploader {
+ public:
+  // Constructor.
+  SystemLogUploader(const scoped_refptr<base::SequencedTaskRunner>& task_runner,
+                    scoped_ptr<SystemLogUploadJob> upload_job);

[Andrew T Wilson (Slow), 2015/06/29 15:06:09]

I don't like having a persistent upload_job that you start/stop over and over -
much cleaner to just have a class that you call Run() then delete when done.
Better to either have this class create its own upload_job, or if tests need to
use mock upload jobs, then inject a factory.

[Polina Bondarenko, 2015/07/02 15:28:04]

Done.

+
+  // Destructor.

[Andrew T Wilson (Slow), 2015/06/29 15:06:09]

Remove this comment (and the one above) - no need to comment that these are
constructors/destructors since it's clear that's what they are. Only comment on
constructors if you are adding information/documentation on their use.

[Polina Bondarenko, 2015/07/02 15:28:04]

Done.

+  ~SystemLogUploader();
+
+  // 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:
+  // Handles success and failure results of upload.
+  void OnSuccess();
+  void OnFailure();
+
+  // Start upload system logs, run upload job.
+  void StartLogUpload();
+
+  // Helper method that figures out when the next system log upload should
+  // be scheduled.
+  void ScheduleNextSystemLogUpload(base::TimeDelta frequency);
+
+  // Indicates whether the last log upload was successful or not the first
+  // failed. Because if log upload is failed, only one retry log upload should

[Andrew T Wilson (Slow), 2015/06/29 15:06:09]

This comment is a bit unclear. Maybe this flag should be called "retrying_" or
something, and it's only set to |true| if we are doing a retry upload after an
error?

[Polina Bondarenko, 2015/07/02 15:28:04]

Done.

+  // be done.
+  bool last_success_;
+
+  // How long to wait between system log uploads.
+  base::TimeDelta upload_frequency_;
+
+  // The time the last upload was performed.

[Andrew T Wilson (Slow), 2015/06/29 15:06:09]

nit: "last successful upload"? Or just last upload attempt?

If it's just the last upload attempt, then perhaps name this
last_uplaod_attempt_ ?

[Polina Bondarenko, 2015/07/02 15:28:04]

Done. Renamed to last_upload_attempt_.

+  base::Time last_upload_;
+
+  // TaskRunner used for scheduling upload tasks.
+  const scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
+  // System log upload job.

[Andrew T Wilson (Slow), 2015/06/29 15:06:09]

Delete this comment or make it useful. Maybe talk about when it is set?

[Polina Bondarenko, 2015/07/02 15:28:04]

Done.

+  scoped_ptr<SystemLogUploadJob> upload_job_;
+
+  // 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<SystemLogUploader> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SystemLogUploader);
+};
+
+}  // namespace policy
+#endif  // CHROME_BROWSER_CHROMEOS_POLICY_SYSTEM_LOG_UPLOADER_H_