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 doing a single system 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;
+
+  // A delegate interface used by SystemLogUploadJob to read the system logs
+  // from the disk and create an upload job.
+  class Delegate {
+   public:
+    typedef base::Callback<void(const SystemLogs* system_logs)>
+        LogUploadCallback;
+
+    virtual ~Delegate() {}
+
+    // Loads system logs and invokes |upload_callback|.
+    virtual void LoadSystemLogs(const LogUploadCallback& upload_callback) = 0;

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

One question - why do we have a Delegate class? Why not just pass in SystemLogs
and an UploadJob to Run()? It seems unnecessarily complicated to call back to a
delegate. Just leave it to the caller to know the upload URL for creating the
upload job and to fetch the logs before calling Run(logs, upload_job)?

[Polina Bondarenko, 2015/07/23 14:27:31]

Removed SystemLogUploadJob at all. This Delegate is moved to SystemLogUploader.

+
+    // Creates a new fully configured instance of an UploadJob. This method
+    // may be called any number of times.

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

Either document under what situation it may be called multiple times, or else
document that it should only be called once (when Run() is called).

[Polina Bondarenko, 2015/07/23 14:27:31]

After changing structure this delegate (SystemLogDelegate) is created only once
on SystemLogUploader creation. And re-creates UploadJobs on every system log
upload.

+    virtual scoped_ptr<UploadJob> CreateUploadJob(const GURL&,

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

nit: this is fine, but I still usually put names with the variables.

[Polina Bondarenko, 2015/07/23 14:27:31]

Done.

+                                                  UploadJob::Delegate*) = 0;

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

It's vaguely confusing that you pass an UploadJob::Delegate to an
UploadJob::Delegate::Delegate. It's fine, though - just amusing that we are
defining a Delegate class in a class that is itself an implementation of a
Delegate interface.

Wonder if this would be clearer if you passed in a SystemLogUploadJob here
instead of an UploadJob::Delegate, if you know you are only passing in
SystemLogUploadJobs.

Maybe the confusion is that you've named the parent class SystemLogUploadJob,
when it's not actually an UploadJob, it's an UploadJob::Delegate. So I'd suggest
calling this SystemLogUploadJobDelegate, that way when you say things like "Run
is called only once for every created upload job", I know whether you are
talking about SystemLogUploadJobDelegate, or one of the UploadJobs returned by
CreateUploadJob().

[Polina Bondarenko, 2015/07/23 14:27:31]

Now it is moved to SystemLogUploader and SystemLogUploader is delegate of
UploadJob. It is only for handling UploadJob::Delegate callbacks to receive
responses from server.

+  };
+
+  SystemLogUploadJob(scoped_ptr<Delegate> syslog_delegate,
+                     const base::Closure& succeeded_callback,
+                     const base::Closure& failed_callback);
+
+  ~SystemLogUploadJob() override;
+
+  // Starts the system log upload, could be called only once for every created

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

Nit: should only be called once on any SystemLogUploadJob.

[Polina Bondarenko, 2015/07/23 14:27:31]

Fixed comment.

+  // upload job.
+  void Run();
+
+ private:
+  // UploadJob::Delegate:
+  void OnSuccess() override;

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

Think these should be public to match their definition in your derived class, or
at most protected?

[Polina Bondarenko, 2015/07/23 14:27:31]

Moved to the public section.

+  void OnFailure(UploadJob::ErrorCode error_code) override;
+
+  // Callback invoked 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 callback that will be called when the system logs were successfully
+  // uploaded. It destroys the job.

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

note instead that the callback *may* destroy this object.

[Polina Bondarenko, 2015/07/23 14:27:31]

Removed this class and callbacks at all.

+  base::Closure succeeded_callback_;
+
+  // The callback that will be called when system log upload failed. It destroys

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

Ditto - note that invoking this callback can free the object

[Polina Bondarenko, 2015/07/23 14:27:31]

Removed this class and callbacks at all.

+  // this job.
+  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_