Added system log uploader.

chrome/browser/chromeos/policy/system_log_upload_job.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_UPLOAD_JOB_H_
+#define CHROME_BROWSER_CHROMEOS_POLICY_SYSTEM_LOG_UPLOAD_JOB_H_
+
+#include "base/cancelable_callback.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/chromeos/policy/upload_job.h"
+
+namespace policy {
+
+// Class responsible for periodically uploading system logs.

[Andrew T Wilson (Slow), 2015/07/03 15:40:47]

Is this true? Is this class responsible for doing a single log upload, or for
doing multiple ones periodically?

[Polina Bondarenko, 2015/07/08 10:07:23]

Fixed comment to single log upload.

+class SystemLogUploadJob : public UploadJob::Delegate {
+ public:
+  // Structure that stores the system log files as pairs: (file name, loaded
+  // from the disk binary file data).
+  typedef std::vector<std::pair<std::string, std::string>> SystemLogs;
+
+  typedef base::Callback<void(const SystemLogs* system_logs)> LogUploadCallback;

[Andrew T Wilson (Slow), 2015/07/03 15:40:48]

Pass read-only objects as const SystemLogs& instead of as a ptr.

Also, move this into Delegate.

[Polina Bondarenko, 2015/07/08 10:07:23]

Yes, having const& instead of const*  is more convenient, but I need this ptr to
be owned and destroyed by LogUploadCallback.

Moved to Delegate.

[Andrew T Wilson (Slow), 2015/07/17 15:43:56]

OK, understood. You are passing a pointer and binding it to the callback in a
way that will cause the pointer to be freed when the callback is destroyed.
Makes sense - perhaps pass it as a scoped_ptr to make it explicit?

+
+  class Delegate {

[Andrew T Wilson (Slow), 2015/07/03 15:40:48]

Useful to explain why you are declaring this Delegate class. Is it because you
want to override these for tests?

[Polina Bondarenko, 2015/07/08 10:07:23]

Yes, it is needed for tests and separate some functionality from the system log
upload job. Added a comment.

+   public:
+    virtual ~Delegate() {}
+
+    // Loads system logs and invokes |upload_callback|.
+    virtual void LoadSystemLogs(const LogUploadCallback& upload_callback) = 0;
+
+    // Creates a new fully configured instance of an UploadJob. This method
+    // may be called any number of times.
+    virtual scoped_ptr<UploadJob> CreateUploadJob(const GURL&,
+                                                  UploadJob::Delegate*) = 0;
+  };
+
+  explicit SystemLogUploadJob(scoped_ptr<Delegate> syslog_delegate);
+
+  ~SystemLogUploadJob() override;
+
+  // Starts the system log upload.
+  virtual void Run(const base::Closure& succeeded_callback,

[Andrew T Wilson (Slow), 2015/07/03 15:40:48]

Why is this virtual?

[Polina Bondarenko, 2015/07/08 10:07:23]

Yes, it is overridden in the tests. 
Removed 'virtual'.

+                   const base::Closure& failed_callback);
+
+ private:
+  // UploadJob::Delegate:
+  void OnSuccess() override;
+  void OnFailure(UploadJob::ErrorCode error_code) override;
+
+  // Callback invoked periodically to start loading system logs.
+  void StartLoadSystemLogs();
+
+  // Callback invoked when system logs are successfully loaded,
+  // starts the system log upload.
+  void StartUploadSystemLogs(const SystemLogs* system_logs);
+
+  // The URL to which the POST request should be directed.
+  GURL upload_url_;

[Andrew T Wilson (Slow), 2015/07/03 15:40:47]

why are we caching this instead of creating it every time we do an upload?

[Polina Bondarenko, 2015/07/08 10:07:23]

Fixed, don't need anymore.

+
+  // The callback that will be called when the system logs were successfully
+  // uploaded.
+  base::Closure succeeded_callback_;
+
+  // The callback that will be called when system log upload failed.
+  base::Closure failed_callback_;
+
+  // The Delegate is used to load system logs and create UploadJobs.
+  scoped_ptr<Delegate> syslog_delegate_;
+
+  // The upload job instance that will upload the log files.
+  scoped_ptr<UploadJob> 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<SystemLogUploadJob> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SystemLogUploadJob);
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_CHROMEOS_POLICY_SYSTEM_LOG_UPLOAD_JOB_H_