Dump touchpad event logs for touch log source

chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc

Index: chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc
diff --git a/chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc b/chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc
index 078b692ee0a62ff3bcf2c38ddabecc91883906a4..6466d7865dfc7c5e2c4f7f236d4a9cfba95c1a0e 100644
--- a/chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc
+++ b/chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc
@@ -9,6 +9,8 @@
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ -28,18 +30,111 @@ const char kHUDLogDataKey[] = "hud_log";
 // We continue to go with it in order to be compatible with the existing touch
 // log processing toolchain.
 const char kDeviceStatusLogDataKey[] = "hack-33025-touchpad";
+const char kTouchpadEventLogDataKey[] = "hack-33025-touchpad_activity";
 
-// Callback for handing the outcome of GetTouchDeviceStatus(). Appends the
-// collected log to the SystemLogsResponse map.
+// Directory for temp touch event logs.
+const char kTouchEventLogDir[] = "/home/chronos/user/log";

[achuithb, 2015/02/12 22:42:04]

Shouldn't this be kTouchpadEventLogDir?

[Shecky Lin, 2015/02/13 05:12:22]

This will be used for all touchpad/mice/touchscreen event logs so I think the
name is fine. I'll add touchscreen event log codes in following patches.

+
+// Prefixes of touch event logs.
+const char kTouchpadGestureLogPrefix[] = "touchpad_activity_";
+const char kTouchpadEvdevLogPrefix[] = "cmt_input_events_";
+
+// Binary paths.
+const char kShellCommand[] = "/bin/sh";
+const char kTarCommand[] = "/bin/tar";
+const char kUuencodeCommand[] = "/usr/bin/uuencode";
+
+const int kMaxDeviceTouchEventLogs = 7;
+
+void CleanupEventLog(scoped_ptr<std::vector<base::FilePath>> log_paths) {

[achuithb, 2015/02/12 22:42:04]

Function comment please

[Shecky Lin, 2015/02/13 05:12:22]

Done.

+  for (size_t i = 0; i < log_paths->size(); ++i)
+    base::DeleteFile((*log_paths)[i], false);
+}
+
+std::string GetEventLogListOfOnePrefix(

[achuithb, 2015/02/12 22:42:04]

Please add a function comment explaining the inputs and output of this function

[Shecky Lin, 2015/02/13 05:12:22]

Done.

+    const std::vector<base::FilePath>* log_paths,

[achuithb, 2015/02/12 22:42:04]

Shouldn't this be const std::vector<base::FilePath>& log_paths

[Shecky Lin, 2015/02/13 05:12:22]

Done.

+    const std::string& prefix) {
+  int collected = 0;
+  std::string log_list;
+  for (size_t i = 0; i < log_paths->size(); ++i) {
+    std::string basename = (*log_paths)[i].BaseName().value();

[achuithb, 2015/02/12 22:42:04]

const std::string

[Shecky Lin, 2015/02/13 05:12:22]

Done.

+    if (StartsWithASCII(basename, prefix, true)) {
+      log_list.append(" " + (*log_paths)[i].value());
+
+      // Limit the max number of collected logs to shorten the log collection
+      // process.
+      ++collected;
+      if (collected >= kMaxDeviceTouchEventLogs)

[achuithb, 2015/02/12 22:42:04]

Feel like kMaxDeviceTouchEventLogs should be an input param to this fn.

You can do
if (++collected >= kMaxDeviceTouchEventLogs)
  break;

[Shecky Lin, 2015/02/13 05:12:22]

Done.

+        break;
+    }
+  }
+
+  return log_list;
+}
+
+// Pack the collected event logs in a way that is compatible with the log
+// analysis tools.
+void PackEventLog(system_logs::SystemLogsResponse* response,
+                  scoped_ptr<std::vector<base::FilePath>> log_paths) {
+  // Combine logs with a command line call that tars them up and uuencode the
+  // result in one string. This is to be compatible with the X11 behavior.
+  std::vector<std::pair<std::string, base::CommandLine>> commands;

[achuithb, 2015/02/12 22:42:04]

You should have a DCHECK to ensure this is running on the browser pool thread.

[Shecky Lin, 2015/02/13 05:12:22]

This function is supposed to be run on the blocking pool so it seems like we
shouldn't check it. It is posted by OnEventLogCollected().

[achuithb, 2015/02/17 21:21:06]

Something like this?
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

[Shecky Lin, 2015/02/24 08:28:40]

Didn't know there is such a thing. Thanks!

+  base::CommandLine command = base::CommandLine(base::FilePath(kShellCommand));

[achuithb, 2015/02/12 22:42:04]

Might be more readable as:
base::CommandLine command(base::FilePath(kShellCommand));

[Shecky Lin, 2015/02/13 05:12:22]

Unfortunately, it would result in compile error because the compiler will run
into trouble on the constructors (not sure about the exact reason). If you check
it, there are some examples in the codebase which use the same trick to avoid
the problem.

+  command.AppendArg("-c");

[achuithb, 2015/02/12 22:42:04]

Why not add this to kShellCommand?
const char kShellCommand[] = "/bin/sh -c";

[Shecky Lin, 2015/02/13 05:12:22]

I won't be able to pass it to base::FilePath in the way. It also make the
argument stand out to readers.

+
+  // Make a list that contains touchpad (and mouse) event logs only.
+  std::string touchpad_log_list =

[achuithb, 2015/02/12 22:42:04]

const

[Shecky Lin, 2015/02/13 05:12:22]

Done.

+      GetEventLogListOfOnePrefix(log_paths.get(), kTouchpadGestureLogPrefix) +
+      GetEventLogListOfOnePrefix(log_paths.get(), kTouchpadEvdevLogPrefix);
+  command.AppendArg(std::string(kTarCommand) + " cf -" + touchpad_log_list +

[achuithb, 2015/02/12 22:42:05]

I'd propose
const char kTarCommand[] = "/bin/tar cf -";

[Shecky Lin, 2015/02/13 05:12:22]

Done.

+                    " 2>/dev/null | " + kUuencodeCommand +
+                    " -m touchpad_activity_log.tar");
+  commands.push_back(std::make_pair(kTouchpadEventLogDataKey, command));
+
+  // For now only touchpad (and mouse) logs are actually collected.
+  for (size_t i = 0; i < commands.size(); ++i) {
+    std::string output;
+    base::GetAppOutput(commands[i].second, &output);
+    (*response)[commands[i].first] = output;
+  }
+
+  // Cleanup these temporary log files.
+  BrowserThread::PostBlockingPoolTask(
+      FROM_HERE, base::Bind(CleanupEventLog, base::Passed(&log_paths)));
+}
+
+// Callback for handing the outcome of GetTouchEventLog().
+//
+// This is the end of the whole touch log collection process.
+void OnEventLogCollected(scoped_ptr<system_logs::SystemLogsResponse> response,
+                         const system_logs::SysLogsSourceCallback& callback,
+                         scoped_ptr<std::vector<base::FilePath>> log_paths) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  base::Closure pack_closure =

[achuithb, 2015/02/12 22:42:05]

Personally I don't think these temporaries pack_closure and callback_closure add
any value. I think inlining them saves space and is more readable.

[spang, 2015/02/13 02:15:13]

Unfortunately in-lining both closures would be wrong, because the compiler can
put the call to release() before get(). Passed() has the same pitfall.

[Shecky Lin, 2015/02/13 05:12:22]

Yes, I do this for the specific reason.

[achuithb, 2015/02/17 21:21:06]

Perhaps we should add a comment here then in case somebody tries to inline this
in the future and runs afoul of the indeterminism noted.

[Shecky Lin, 2015/02/24 08:28:40]

Done.

+      base::Bind(&PackEventLog, base::Unretained(response.get()),
+                 base::Passed(&log_paths));
+  base::Closure callback_closure =
+      base::Bind(callback, base::Owned(response.release()));
+  BrowserThread::PostBlockingPoolTaskAndReply(FROM_HERE, pack_closure,
+                                              callback_closure);
+}
+
+// Callback for handing the outcome of GetTouchDeviceStatus().
+//
+// Appends the collected log to the SystemLogsResponse map. Also goes on to
+// collect touch event logs.
 void OnStatusLogCollected(scoped_ptr<system_logs::SystemLogsResponse> response,
                           const system_logs::SysLogsSourceCallback& callback,
                           scoped_ptr<std::string> log) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   (*response)[kDeviceStatusLogDataKey] = *log;
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(callback, base::Owned(response.release())));
+  // Collect touch event logs.
+  const base::FilePath kBaseLogPath(kTouchEventLogDir);
+  ui::OzonePlatform::GetInstance()->GetInputController()->GetTouchEventLog(
+      kBaseLogPath,
+      base::Bind(&OnEventLogCollected, base::Passed(&response), callback));
 }
 
 // Collect touch HUD debug logs. This needs to be done on the UI thread.