Refactoring the SysInfoProvider.

chrome/browser/chromeos/system_logs/commandline_log_source.cc

Index: chrome/browser/chromeos/system_logs/commandline_log_source.cc
diff --git a/chrome/browser/chromeos/system_logs/commandline_log_source.cc b/chrome/browser/chromeos/system_logs/commandline_log_source.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e430528d5973c093a13a8bf5c64fe1e5a5bffad3
--- /dev/null
+++ b/chrome/browser/chromeos/system_logs/commandline_log_source.cc
@@ -0,0 +1,71 @@
+// 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/commandline_log_source.h"
+
+#include <string>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/command_line.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/process_util.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace chromeos {
+
+void CommandLineLogSource::Fetch(const SysLogsFetcherCallback& request) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  SystemLogsResponse* response = new SystemLogsResponse;

[satorux1, 2012/08/13 18:29:03]

who deletes |response|? will this be deleted by SysLogsFetcherCallback?


If so, you should probably make the callback to take
scoped_ptr<SystemLogsResponse> to make it clear that you transfer the ownership
of the object to the callback.

[tudalex(Chromium), 2012/08/14 03:24:00]

Yes, it is deleted there.
Used base::Owned() to do that.

+  BrowserThread::PostBlockingPoolTaskAndReply(FROM_HERE,
+                                              base::Bind(
+                                                &CommandLineLogSource::Execute,

[satorux1, 2012/08/13 18:29:03]

indentation is wrong. four spaces, instead of two. I'd do

BrowserThread::PostBlockingPoolTaskAndReply(
    FROM_HERE,
    base::Bind(&CommandLineLogSource::Execute, response),
    base::Bind(request, response));

[tudalex(Chromium), 2012/08/14 03:24:00]

Thanks, I was wondering what the best approach would be. Done.

+                                                response),
+                                              base::Bind(request, response));

[satorux1, 2012/08/13 18:29:03]

if you want to delete |response| after the callback is run, you can do:

  base::Owned(response)

[tudalex(Chromium), 2012/08/14 03:24:00]

Used base::Owned().

+}
+
+void CommandLineLogSource::Execute(SystemLogsResponse* response) {

[satorux1, 2012/08/13 18:29:03]

Could you move this to anonymous namespace in the .cc file?

Seems to me that this function doesn't have to belong to CommandLineLogSource
class. Less thing to have in .h file is preferable.

[tudalex(Chromium), 2012/08/14 03:24:00]

I had it originally in an anonymous namespace in patch set 10. rkc@ suggested
that I move it in the class cause it's related to what the class does. Should I
move it back to an anonymous namespace? 
For now I moved it into an anonymous space and changed it's name to
ExecuteCommandLines.

[satorux1, 2012/08/14 05:51:52]

It's probably a matter of taste, but I personally like to have less things in .h
file. See also http://en.wikipedia.org/wiki/Don't_repeat_yourself

+  // TODO(tudalex): Move program calling in a array or something similar to make
+  // it more easier to modify and understand.
+  std::vector<std::pair<std::string, CommandLine> > commands;
+
+  // Needed to initialize an empty command, but the constructor was private
+  CommandLine command(FilePath("/usr/bin/cras_test_client"));
+  command.AppendArg("--dump_server_info");
+  commands.push_back(std::make_pair("cras", command));
+
+  command = CommandLine((FilePath("set")));
+  commands.push_back(std::make_pair("env", command));
+
+  command = CommandLine(FilePath("/usr/bin/setxkbmap"));
+  command.AppendArg("-print");
+  command.AppendArg("-query");
+  commands.push_back(std::make_pair("setxkbmap", command));
+
+  command = CommandLine(FilePath("/usr/bin/xrandr"));
+  command.AppendArg("--verbose");
+  commands.push_back(std::make_pair("xrandr", command));
+
+  command = CommandLine(FilePath("/opt/google/touchpad/tpcontrol"));
+  commands.push_back(std::make_pair("hack-33025-touchpad", command));
+
+  command = CommandLine(FilePath("/opt/google/touchpad/generate_userfeedback"));
+  commands.push_back(std::make_pair("hack-33025-touchpad_activity", command));
+
+
+  std::vector< std::pair<std::string, CommandLine> >::iterator it;
+  for (it = commands.begin(); it != commands.end(); ++it) {
+    std::string output;
+    base::GetAppOutput(it->second, &output);
+    (*response)[it->first] = output;
+  }
+}
+
+}  // namespace chromeos