Added system log uploader.

chrome/browser/chromeos/policy/system_log_upload_job.cc

Index: chrome/browser/chromeos/policy/system_log_upload_job.cc
diff --git a/chrome/browser/chromeos/policy/system_log_upload_job.cc b/chrome/browser/chromeos/policy/system_log_upload_job.cc
new file mode 100644
index 0000000000000000000000000000000000000000..dbbfd296eb4624f9bb6f43ef239bf2a0c4504d97
--- /dev/null
+++ b/chrome/browser/chromeos/policy/system_log_upload_job.cc
@@ -0,0 +1,99 @@
+// 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 "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/location.h"
+#include "base/strings/stringprintf.h"
+#include "base/thread_task_runner_handle.h"
+#include "chrome/browser/chromeos/policy/system_log_upload_job.h"
+#include "net/http/http_request_headers.h"
+
+namespace {
+// String constant defining the upload url.
+const char* kDefaultUploadUrl =
+    "https://m.google.com/devicemanagement/data/api/upload";
+
+// String constant identifying the header field which stores the file type.
+const char* kFileTypeHeaderName = "File-Type";
+
+// String constant signalling that the data segment contains log files.
+const char* const kFileTypeLogFile = "log_file";
+
+// String constant signalling that the segment contains a plain text.
+const char* const kContentTypePlainText = "text/plain";
+
+// Template string constant for populating the name field.
+const char* const kNameFieldTemplate = "file%zu";
+}  // namespace
+
+namespace policy {
+
+SystemLogUploadJob::SystemLogUploadJob(scoped_ptr<Delegate> syslog_delegate)
+    : upload_url_(GURL(kDefaultUploadUrl)),

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

Why make this a data member? Why not just use GURL(kDefaultUploadUrl) below?

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

Removed.

+      syslog_delegate_(syslog_delegate.Pass()),
+      weak_factory_(this) {
+  DCHECK(syslog_delegate_);
+}
+
+SystemLogUploadJob::~SystemLogUploadJob() {
+}
+
+void SystemLogUploadJob::OnSuccess() {

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

Why are we posting a task here instead of just invoking the callback? And why
call base::Bind() on the callback?

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

Removed PostTask and Bind.

+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(succeeded_callback_));
+}
+
+void SystemLogUploadJob::OnFailure(UploadJob::ErrorCode error_code) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                base::Bind(failed_callback_));

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

Should we pass the error code to the callback? Why are we calling base:Bind()?

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

I don't use the error_code in the system log uploader now.
There are 4 errors: server, network, content encoding and authentication. Should
the retry-behavior be different according to the error type?

Removed PostTask and Bind.

+}
+
+void SystemLogUploadJob::StartLoadSystemLogs() {
+  syslog_delegate_->LoadSystemLogs(base::Bind(
+      &SystemLogUploadJob::StartUploadSystemLogs, weak_factory_.GetWeakPtr()));
+}
+
+void SystemLogUploadJob::StartUploadSystemLogs(const SystemLogs* system_logs) {
+  size_t file_number = 1;

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

Can this be an int instead? ints are preferred if they are possible (and they
should be possible here)

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

Done.

+  if (system_logs) {
+    for (const auto& syslog_entry : (*system_logs)) {
+      std::map<std::string, std::string> header_fields;
+      scoped_ptr<std::string> data =
+          make_scoped_ptr(new std::string(syslog_entry.second));
+      header_fields.insert(
+          std::make_pair(kFileTypeHeaderName, kFileTypeLogFile));
+      header_fields.insert(std::make_pair(net::HttpRequestHeaders::kContentType,
+                                          kContentTypePlainText));
+      upload_job_->AddDataSegment(
+          base::StringPrintf(kNameFieldTemplate, file_number),
+          syslog_entry.first, header_fields, data.Pass());
+      ++file_number;
+    }
+  }
+  upload_job_->Start();
+}
+
+void SystemLogUploadJob::Run(const base::Closure& succeeded_callback,
+                             const base::Closure& failed_callback) {
+  // Ensure nobody calls Run() multiple times.

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

Document in header file how this class should be used?

Also, maybe pass callbacks in constructor, not in Run()?

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

Moved callbacks to constructor.
Added comment.

+  DCHECK(!upload_job_);
+
+  succeeded_callback_ = succeeded_callback;
+  failed_callback_ = failed_callback;
+
+  // Immediately fail if the upload url is invalid.
+  if (!upload_url_.is_valid()) {

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

Why would the upload_url_ be invalid, since it's just GURL(kDefaultUploadUrl)?

Should this be a DCHECK instead of handling it at runtime? Since it's a
constant, won't it always be either valid or invalid for a given binary?

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

Fixed.

+    LOG(ERROR) << upload_url_ << " is not a valid URL.";
+    base::Bind(failed_callback_);
+    return;
+  }
+
+  upload_job_ = syslog_delegate_->CreateUploadJob(upload_url_, this);
+  DCHECK(upload_job_);
+
+  StartLoadSystemLogs();
+}
+
+}  // namespace policy