Refactoring the SysInfoProvider.

chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc

+// Copyright (c) 2012 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/chromeos/system_logs/lsbrelease_log_source.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/string_util.h"
+#include "base/string_split.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace {
+
+const char kInvalidLogEntry[] = "<invalid characters in log entry>";
+const char kEmptyLogEntry[] = "<no value>";
+
+typedef std::pair<std::string, std::string> KV_Pair;

[satorux1, 2012/08/15 22:01:09]

no abbreviation. KeyValuePair

[tudalex(Chromium), 2012/08/16 01:08:57]

Done.

+
+}  // namespace
+
+namespace chromeos {
+
+void LsbReleaseLogSource::Fetch(const SysLogsFetcherCallback& callback) {
+  SystemLogsResponse* response = new SystemLogsResponse;
+  BrowserThread::PostBlockingPoolTaskAndReply(
+      FROM_HERE,
+      base::Bind(&ReadLSBRelease, response),
+      base::Bind(callback,
+                 base::Owned(response)));
+}
+
+void LsbReleaseLogSource::ReadLSBRelease(SystemLogsResponse* response) {
+  DCHECK(response);
+
+  const FilePath lsb_release_file("/etc/lsb-release");
+  std::string lsb_data;
+  bool read_success = file_util::ReadFileToString(lsb_release_file,
+                                                      &lsb_data);

[satorux1, 2012/08/15 22:01:09]

indentation.

[tudalex(Chromium), 2012/08/16 01:08:57]

Done.

+  // if we were using an internal temp file, the user does not need the
+  // logs to stay past the ReadFile call - delete the file
+  if (!read_success) {
+    LOG(ERROR) << "Can't access /etc/lsb-release file.";
+    return;
+  }
+  ParseLSBRelease(lsb_data, response);
+}
+
+void LsbReleaseLogSource::ParseLSBRelease(const std::string lsb_data,
+                                          SystemLogsResponse* response) {
+  std::vector<KV_Pair> keys;

[satorux1, 2012/08/15 22:01:09]

|keys| sounds wrong because it's pairs. maybe |pairs|?

[tudalex(Chromium), 2012/08/16 01:08:57]

Done.

+  base::SplitStringIntoKeyValuePairs(lsb_data, '=', '\n', &keys);
+  for (std::vector<KV_Pair>::iterator it = keys.begin();
+       it != keys.end();

[satorux1, 2012/08/15 22:01:09]

btw, it's a matter of taste, but the following is more succinct.

for (size_t i = 0; i < keys.size(); ++i) {

[tudalex(Chromium), 2012/08/16 01:08:57]

Done.

+       ++it) {
+    std::string key, value;
+    TrimWhitespaceASCII(it->first, TRIM_ALL, &key);
+    TrimWhitespaceASCII(it->second, TRIM_ALL, &value);
+    if (!key.empty()) {
+      if (IsStringUTF8(it->second)) {

[satorux1, 2012/08/15 22:01:09]

it->second -> value ??

shouldn't we also check the key?

[tudalex(Chromium), 2012/08/16 01:08:57]

Done.

+        if (value.empty())
+          (*response)[key] = kEmptyLogEntry;
+        else
+          (*response)[key] = value;
+      } else {
+        LOG(WARNING) << "Invalid characters in system log entry: " << it->first;

[satorux1, 2012/08/15 22:01:09]

value?

[tudalex(Chromium), 2012/08/16 01:08:57]

I think the key is better than the value, because it is less prone to errors,
assuming the key is hardcoded somewhere, so you then know where to look or what
produced the error.

[satorux1, 2012/08/16 10:50:57]

sounds good

+        (*response)[key] = kInvalidLogEntry;
+      }
+    }
+  }
+}
+
+}  // namespace chromeos
+