Dump touchpad event logs for touch log source

Dump touchpad event logs for touch log source

The CL dumps event logs for devices that run with the CrOS
gesture library. It is designed to be backward-compatible
with the X11 behaviour.

Contributed by sheckylin@chromium.org

BUG=450159
TEST=samus ChromeOS build

Committed: https://crrev.com/e1fd503701a7499e6e634ad13d114e7de07936bf
Cr-Commit-Position: refs/heads/master@{#318182}

[Shecky Lin, 2015-01-31 02:22:00]

sheckylin@chromium.org changed reviewers:
+ achuith@chromium.org, adlr@chromium.org, alexst@chromium.org,
spang@chromium.org

[Shecky Lin, 2015-01-31 02:37:45]

Hey guys, please take a look. Thanks!

[spang, 2015-02-02 21:43:17]

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:55: std::string
log_dir(kTouchEventLogDir);
Don't need this, you can concatenate a const char* directly.

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:63:
kUuencodeCommand + " -m touchpad_activity_log.tar");
Can't this use log_paths instead of doing the $(ls ...) command substitution?

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
File ui/events/ozone/evdev/input_device_factory_evdev.cc (right):

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:50: const char
kTouchpadEvdevLogPath[] = "/var/log/xorg/cmt_input_events.dat";
Are you planning to make a ChromeOS change to create /var/log/xorg & make it
writable by chronos? Otherwise I don't think you'll be able to write into this
directory.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:193:
command.AppendArg("+%Y%m%d-%H%M%S");
Does it make sense for this to be in local time with no UTC offset? You cannot
even tell what time it actually represents.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:195:
base::GetAppOutput(command, &output);
Can't you use just "strftime" or similar? Should not be necessary to fork the
browser just to format a date.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:220: void
DumpTouchEventLog(GesturePropertyProvider* provider,
Can you move this function somewhere inside libgestures_glue (probably a new
file)? It will save on #ifdefs and be a bit more organized.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:232:
scoped_ptr<std::vector<std::string>> log_paths_to_be_compressed(
I don't think you need a new vector here.

You should be able to do

  std::vector<base::FilePaths>* log_paths_to_be_compressed = log_paths.get();
  base::WorkerPool::PostTaskAndReply(FROM_HERE,
    base::Bind(&CompressDumpedLog, log_paths_to_be_compressed),
    base::Bind(reply, base::Passed(&log_paths));

below. This is OK because the reply closure outlives the task closure.

[Shecky Lin, 2015-02-03 08:20:14]

Thanks for the kind suggestion! I haven't got time to form the next patch but
please take a look of my replies.

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:55: std::string
log_dir(kTouchEventLogDir);
On 2015/02/02 21:43:16, spang wrote:
> Don't need this, you can concatenate a const char* directly.

It doesn't look like so. I tried it before and got a compile error. Just wrote a
small c++ file to verify that it really doesn't work.

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:63:
kUuencodeCommand + " -m touchpad_activity_log.tar");
On 2015/02/02 21:43:16, spang wrote:
> Can't this use log_paths instead of doing the $(ls ...) command substitution?

I will have to use two separate vectors in the callback to separate touchpad
logs and touchscreen logs because they will need to go to different files. Does
that sound like a better plan?

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
File ui/events/ozone/evdev/input_device_factory_evdev.cc (right):

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:50: const char
kTouchpadEvdevLogPath[] = "/var/log/xorg/cmt_input_events.dat";
On 2015/02/02 21:43:16, spang wrote:
> Are you planning to make a ChromeOS change to create /var/log/xorg & make it
> writable by chronos? Otherwise I don't think you'll be able to write into this
> directory.

Yeah, sorry, this is probably the problem I am running into. Thanks.

Do you know where I can add it back though? We were using these directories for
touch event logs in the old X11 world.

Or it might be better to just write to a new directory that we have access to.
What directories do we currently have permission to dump log files on Chrome?
Would it work if I write to "/var/log/gesture/"?

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:193:
command.AppendArg("+%Y%m%d-%H%M%S");
On 2015/02/02 21:43:16, spang wrote:
> Does it make sense for this to be in local time with no UTC offset? You cannot
> even tell what time it actually represents.

I was just trying to do exactly the same thing in the inputcontrol script to
prevent any possible regression. I think the dates are just used to dedup the
file name. Their values are not actually used.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:195:
base::GetAppOutput(command, &output);
On 2015/02/02 21:43:16, spang wrote:
> Can't you use just "strftime" or similar? Should not be necessary to fork the
> browser just to format a date.

Good idea. Will do. Thanks.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:220: void
DumpTouchEventLog(GesturePropertyProvider* provider,
On 2015/02/02 21:43:17, spang wrote:
> Can you move this function somewhere inside libgestures_glue (probably a new
> file)? It will save on #ifdefs and be a bit more organized.

Yeah, make sense, will do.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:232:
scoped_ptr<std::vector<std::string>> log_paths_to_be_compressed(
On 2015/02/02 21:43:17, spang wrote:
> I don't think you need a new vector here.
> 
> You should be able to do
> 
>   std::vector<base::FilePaths>* log_paths_to_be_compressed = log_paths.get();
>   base::WorkerPool::PostTaskAndReply(FROM_HERE,
>     base::Bind(&CompressDumpedLog, log_paths_to_be_compressed),
>     base::Bind(reply, base::Passed(&log_paths));
> 
> below. This is OK because the reply closure outlives the task closure.

No, this is not for recording the pointer. As said in the comment below, I do
this because we only compress touchpad and mouse logs and not the touchscreen
logs. So I need 2 separate vectors for each of them.

We don't have touchscreen logs for now but I will add it at the next patch. So
we are going to have two vectors anyway.

[Shecky Lin, 2015-02-03 11:29:22]

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
File ui/events/ozone/evdev/input_device_factory_evdev.cc (right):

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:50: const char
kTouchpadEvdevLogPath[] = "/var/log/xorg/cmt_input_events.dat";
On 2015/02/03 08:20:14, Shecky Lin wrote:
> On 2015/02/02 21:43:16, spang wrote:
> > Are you planning to make a ChromeOS change to create /var/log/xorg & make it
> > writable by chronos? Otherwise I don't think you'll be able to write into
this
> > directory.
> 
> Yeah, sorry, this is probably the problem I am running into. Thanks.
> 
> Do you know where I can add it back though? We were using these directories
for
> touch event logs in the old X11 world.
> 
> Or it might be better to just write to a new directory that we have access to.
> What directories do we currently have permission to dump log files on Chrome?
> Would it work if I write to "/var/log/gesture/"?

Just got a second thought on this. Could we instead dump it to a temporary file?
Looks like temporary file is an even better candidate for the purpose.

[spang, 2015-02-03 16:31:20]

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:55: std::string
log_dir(kTouchEventLogDir);
On 2015/02/03 08:20:14, Shecky Lin wrote:
> On 2015/02/02 21:43:16, spang wrote:
> > Don't need this, you can concatenate a const char* directly.
> 
> It doesn't look like so. I tried it before and got a compile error. Just wrote
a
> small c++ file to verify that it really doesn't work.

Ah, right, you're building a new string rather than appending onto a
std::string. One of the arguments to + must be a std::string to get the
overload.

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:63:
kUuencodeCommand + " -m touchpad_activity_log.tar");
On 2015/02/03 08:20:14, Shecky Lin wrote:
> On 2015/02/02 21:43:16, spang wrote:
> > Can't this use log_paths instead of doing the $(ls ...) command
substitution?
> 
> I will have to use two separate vectors in the callback to separate touchpad
> logs and touchscreen logs because they will need to go to different files.
Does
> that sound like a better plan?

Sure, I think we should really minimize use of shell scripting in here.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
File ui/events/ozone/evdev/input_device_factory_evdev.cc (right):

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:50: const char
kTouchpadEvdevLogPath[] = "/var/log/xorg/cmt_input_events.dat";
On 2015/02/03 11:29:22, Shecky Lin wrote:
> On 2015/02/03 08:20:14, Shecky Lin wrote:
> > On 2015/02/02 21:43:16, spang wrote:
> > > Are you planning to make a ChromeOS change to create /var/log/xorg & make
it
> > > writable by chronos? Otherwise I don't think you'll be able to write into
> this
> > > directory.
> > 
> > Yeah, sorry, this is probably the problem I am running into. Thanks.
> > 
> > Do you know where I can add it back though? We were using these directories
> for
> > touch event logs in the old X11 world.
> > 
> > Or it might be better to just write to a new directory that we have access
to.
> > What directories do we currently have permission to dump log files on
Chrome?
> > Would it work if I write to "/var/log/gesture/"?
> 
> Just got a second thought on this. Could we instead dump it to a temporary
file?
> Looks like temporary file is an even better candidate for the purpose.

Yep, /tmp would work. If you want to use /var/log that's probably OK too, it
would just need to be created with proper permissions during startup.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:193:
command.AppendArg("+%Y%m%d-%H%M%S");
On 2015/02/03 08:20:14, Shecky Lin wrote:
> On 2015/02/02 21:43:16, spang wrote:
> > Does it make sense for this to be in local time with no UTC offset? You
cannot
> > even tell what time it actually represents.
> 
> I was just trying to do exactly the same thing in the inputcontrol script to
> prevent any possible regression. I think the dates are just used to dedup the
> file name. Their values are not actually used.

Ok, "we don't actually care about the exact time" is a totally reasonable
answer.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:232:
scoped_ptr<std::vector<std::string>> log_paths_to_be_compressed(
On 2015/02/03 08:20:14, Shecky Lin wrote:
> On 2015/02/02 21:43:17, spang wrote:
> > I don't think you need a new vector here.
> > 
> > You should be able to do
> > 
> >   std::vector<base::FilePaths>* log_paths_to_be_compressed =
log_paths.get();
> >   base::WorkerPool::PostTaskAndReply(FROM_HERE,
> >     base::Bind(&CompressDumpedLog, log_paths_to_be_compressed),
> >     base::Bind(reply, base::Passed(&log_paths));
> > 
> > below. This is OK because the reply closure outlives the task closure.
> 
> No, this is not for recording the pointer. As said in the comment below, I do
> this because we only compress touchpad and mouse logs and not the touchscreen
> logs. So I need 2 separate vectors for each of them.
> 
> We don't have touchscreen logs for now but I will add it at the next patch. So
> we are going to have two vectors anyway.

Ah, ok.

[Shecky Lin, 2015-02-10 08:21:22]

spang: I've addressed your previous comments. Could you take another look?
Thanks!

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:63:
kUuencodeCommand + " -m touchpad_activity_log.tar");
On 2015/02/03 16:31:20, spang wrote:
> On 2015/02/03 08:20:14, Shecky Lin wrote:
> > On 2015/02/02 21:43:16, spang wrote:
> > > Can't this use log_paths instead of doing the $(ls ...) command
> substitution?
> > 
> > I will have to use two separate vectors in the callback to separate touchpad
> > logs and touchscreen logs because they will need to go to different files.
> Does
> > that sound like a better plan?
> 
> Sure, I think we should really minimize use of shell scripting in here.

Done.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
File ui/events/ozone/evdev/input_device_factory_evdev.cc (right):

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:50: const char
kTouchpadEvdevLogPath[] = "/var/log/xorg/cmt_input_events.dat";
On 2015/02/03 16:31:20, spang wrote:
> On 2015/02/03 11:29:22, Shecky Lin wrote:
> > On 2015/02/03 08:20:14, Shecky Lin wrote:
> > > On 2015/02/02 21:43:16, spang wrote:
> > > > Are you planning to make a ChromeOS change to create /var/log/xorg &
make
> it
> > > > writable by chronos? Otherwise I don't think you'll be able to write
into
> > this
> > > > directory.
> > > 
> > > Yeah, sorry, this is probably the problem I am running into. Thanks.
> > > 
> > > Do you know where I can add it back though? We were using these
directories
> > for
> > > touch event logs in the old X11 world.
> > > 
> > > Or it might be better to just write to a new directory that we have access
> to.
> > > What directories do we currently have permission to dump log files on
> Chrome?
> > > Would it work if I write to "/var/log/gesture/"?
> > 
> > Just got a second thought on this. Could we instead dump it to a temporary
> file?
> > Looks like temporary file is an even better candidate for the purpose.
> 
> Yep, /tmp would work. If you want to use /var/log that's probably OK too, it
> would just need to be created with proper permissions during startup.

I decided to use /home/chronos/user/log/ directly. Thanks.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:195:
base::GetAppOutput(command, &output);
On 2015/02/03 08:20:14, Shecky Lin wrote:
> On 2015/02/02 21:43:16, spang wrote:
> > Can't you use just "strftime" or similar? Should not be necessary to fork
the
> > browser just to format a date.
> 
> Good idea. Will do. Thanks.

Done.

https://codereview.chromium.org/893753002/diff/20001/ui/events/ozone/evdev/in...
ui/events/ozone/evdev/input_device_factory_evdev.cc:220: void
DumpTouchEventLog(GesturePropertyProvider* provider,
On 2015/02/03 08:20:14, Shecky Lin wrote:
> On 2015/02/02 21:43:17, spang wrote:
> > Can you move this function somewhere inside libgestures_glue (probably a new
> > file)? It will save on #ifdefs and be a bit more organized.
> 
> Yeah, make sense, will do.

Done.

[Shecky Lin, 2015-02-11 08:36:16]

Pinging spang@/alexst@. Thanks!

[spang, 2015-02-11 19:31:46]

oops, thanks for ping

Just a couple more concerns.

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc (right):

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:91: timeinfo =
localtime(&rawtime);
Use the reentrant variant (localtime_r). The non-reentrant variant is
thread-unsafe.

struct tm timeinfo;
if (!localtime_r(&rawtime, &timeinfo)) {
  PLOG(ERROR) << "localtime_r";
  return "";
}

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:101: if
(!std::isalpha(ret[i], loc))
I don't think we should use std::isalpha or in general do anything
locale-dependent here. We don't even use std::locale for localization in
chromium.

Use IsAsciiAlpha().

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:112: return
out_dir.MaybeAsASCII() + "/" + prefix + now + "." +
What's with the MaybeAsASCII()? This says to create the string

"/" + prefix + now + "." + base::IntToString(id)+  "." + ...

if it's not ASCII.

I think you just want value().

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:113:
std::to_string(id) + "." +
C++11 library functions are not allowed. Use base::IntToString.

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:130:
DCHECK(property);
These DCHECKs are redundant. If it's null the next line will crash whether or
not dchecks are enabled.

[Shecky Lin, 2015-02-12 06:51:54]

spang@: please take another look. Thanks!

achuith@ Seems like the patch is now good enough for you to  check the
touch_log_source part. Could you review it? Thank you!

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc (right):

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:91: timeinfo =
localtime(&rawtime);
On 2015/02/11 19:31:46, spang wrote:
> Use the reentrant variant (localtime_r). The non-reentrant variant is
> thread-unsafe.
> 
> struct tm timeinfo;
> if (!localtime_r(&rawtime, &timeinfo)) {
>   PLOG(ERROR) << "localtime_r";
>   return "";
> }

Done. Thanks!

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:101: if
(!std::isalpha(ret[i], loc))
On 2015/02/11 19:31:46, spang wrote:
> I don't think we should use std::isalpha or in general do anything
> locale-dependent here. We don't even use std::locale for localization in
> chromium.
> 
> Use IsAsciiAlpha().

Done. Thanks!

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:112: return
out_dir.MaybeAsASCII() + "/" + prefix + now + "." +
On 2015/02/11 19:31:46, spang wrote:
> What's with the MaybeAsASCII()? This says to create the string
> 
> "/" + prefix + now + "." + base::IntToString(id)+  "." + ...
> 
> if it's not ASCII.
> 
> I think you just want value().

Yeah, my bad. Thanks!

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:113:
std::to_string(id) + "." +
On 2015/02/11 19:31:46, spang wrote:
> C++11 library functions are not allowed. Use base::IntToString.

Done.

https://codereview.chromium.org/893753002/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:130:
DCHECK(property);
On 2015/02/11 19:31:46, spang wrote:
> These DCHECKs are redundant. If it's null the next line will crash whether or
> not dchecks are enabled.

Done.

[spang, 2015-02-12 18:10:38]

lgtm

https://codereview.chromium.org/893753002/diff/60001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc (right):

https://codereview.chromium.org/893753002/diff/60001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:95: strftime(buffer,
kTouchLogTimestampMaxSize, "%Y%m%d-%H%M%S", &timeinfo);
It looks like buffer may not be NULL terminated if strftime() returns 0. Doesn't
seem like it should be possible here but to be extra safe, I would do:

if (!strftime(..))
  return "";

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

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:36: const char
kTouchEventLogDir[] = "/home/chronos/user/log";
Shouldn't this be kTouchpadEventLogDir?

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:49: void
CleanupEventLog(scoped_ptr<std::vector<base::FilePath>> log_paths) {
Function comment please

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:54: std::string
GetEventLogListOfOnePrefix(
Please add a function comment explaining the inputs and output of this function

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:55: const
std::vector<base::FilePath>* log_paths,
Shouldn't this be const std::vector<base::FilePath>& log_paths

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:60: std::string
basename = (*log_paths)[i].BaseName().value();
const std::string

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:67: if (collected
>= kMaxDeviceTouchEventLogs)
Feel like kMaxDeviceTouchEventLogs should be an input param to this fn.

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

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:81:
std::vector<std::pair<std::string, base::CommandLine>> commands;
You should have a DCHECK to ensure this is running on the browser pool thread.

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:82:
base::CommandLine command = base::CommandLine(base::FilePath(kShellCommand));
Might be more readable as:
base::CommandLine command(base::FilePath(kShellCommand));

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:83:
command.AppendArg("-c");
Why not add this to kShellCommand?
const char kShellCommand[] = "/bin/sh -c";

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:86: std::string
touchpad_log_list =
const

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:89:
command.AppendArg(std::string(kTarCommand) + " cf -" + touchpad_log_list +
I'd propose
const char kTarCommand[] = "/bin/tar cf -";

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:114: base::Closure
pack_closure =
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]

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:114: base::Closure
pack_closure =
On 2015/02/12 22:42:05, achuithb wrote:
> 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.

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:11:42]

New patchsets have been uploaded after l-g-t-m from spang@chromium.org

[Shecky Lin, 2015-02-13 05:12:23]

achuith@: please take another look. Thanks!

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:36: const char
kTouchEventLogDir[] = "/home/chronos/user/log";
On 2015/02/12 22:42:04, achuithb wrote:
> Shouldn't this be kTouchpadEventLogDir?

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.

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:49: void
CleanupEventLog(scoped_ptr<std::vector<base::FilePath>> log_paths) {
On 2015/02/12 22:42:04, achuithb wrote:
> Function comment please

Done.

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:54: std::string
GetEventLogListOfOnePrefix(
On 2015/02/12 22:42:04, achuithb wrote:
> Please add a function comment explaining the inputs and output of this
function

Done.

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:55: const
std::vector<base::FilePath>* log_paths,
On 2015/02/12 22:42:04, achuithb wrote:
> Shouldn't this be const std::vector<base::FilePath>& log_paths

Done.

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:60: std::string
basename = (*log_paths)[i].BaseName().value();
On 2015/02/12 22:42:04, achuithb wrote:
> const std::string

Done.

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:67: if (collected
>= kMaxDeviceTouchEventLogs)
On 2015/02/12 22:42:04, achuithb wrote:
> Feel like kMaxDeviceTouchEventLogs should be an input param to this fn.
> 
> You can do
> if (++collected >= kMaxDeviceTouchEventLogs)
>   break;

Done.

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:81:
std::vector<std::pair<std::string, base::CommandLine>> commands;
On 2015/02/12 22:42:04, achuithb wrote:
> You should have a DCHECK to ensure this is running on the browser pool thread.

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().

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:82:
base::CommandLine command = base::CommandLine(base::FilePath(kShellCommand));
On 2015/02/12 22:42:04, achuithb wrote:
> Might be more readable as:
> base::CommandLine command(base::FilePath(kShellCommand));

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.

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:83:
command.AppendArg("-c");
On 2015/02/12 22:42:04, achuithb wrote:
> Why not add this to kShellCommand?
> const char kShellCommand[] = "/bin/sh -c";

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

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:86: std::string
touchpad_log_list =
On 2015/02/12 22:42:04, achuithb wrote:
> const

Done.

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:89:
command.AppendArg(std::string(kTarCommand) + " cf -" + touchpad_log_list +
On 2015/02/12 22:42:05, achuithb wrote:
> I'd propose
> const char kTarCommand[] = "/bin/tar cf -";

Done.

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:114: base::Closure
pack_closure =
On 2015/02/13 02:15:13, spang wrote:
> On 2015/02/12 22:42:05, achuithb wrote:
> > 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.
> 
> Unfortunately in-lining both closures would be wrong, because the compiler can
> put the call to release() before get(). Passed() has the same pitfall.

Yes, I do this for the specific reason.

https://codereview.chromium.org/893753002/diff/60001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc (right):

https://codereview.chromium.org/893753002/diff/60001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_feedback.cc:95: strftime(buffer,
kTouchLogTimestampMaxSize, "%Y%m%d-%H%M%S", &timeinfo);
On 2015/02/12 18:10:38, spang wrote:
> It looks like buffer may not be NULL terminated if strftime() returns 0.
Doesn't
> seem like it should be possible here but to be extra safe, I would do:
> 
> if (!strftime(..))
>   return "";

Done.

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

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:81:
std::vector<std::pair<std::string, base::CommandLine>> commands;
On 2015/02/13 05:12:22, Shecky Lin wrote:
> On 2015/02/12 22:42:04, achuithb wrote:
> > You should have a DCHECK to ensure this is running on the browser pool
thread.
> 
> 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().

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

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:114: base::Closure
pack_closure =
On 2015/02/13 05:12:22, Shecky Lin wrote:
> On 2015/02/13 02:15:13, spang wrote:
> > On 2015/02/12 22:42:05, achuithb wrote:
> > > 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.
> > 
> > Unfortunately in-lining both closures would be wrong, because the compiler
can
> > put the call to release() before get(). Passed() has the same pitfall.
> 
> Yes, I do this for the specific reason.

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]

achuith@: Sorry for the delay. I've addressed your comments. Could you take
another look of it? Thanks!

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:81:
std::vector<std::pair<std::string, base::CommandLine>> commands;
On 2015/02/17 21:21:06, achuithb wrote:
> On 2015/02/13 05:12:22, Shecky Lin wrote:
> > On 2015/02/12 22:42:04, achuithb wrote:
> > > You should have a DCHECK to ensure this is running on the browser pool
> thread.
> > 
> > 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().
> 
> Something like this?
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

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

https://codereview.chromium.org/893753002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:114: base::Closure
pack_closure =
On 2015/02/17 21:21:06, achuithb wrote:
> On 2015/02/13 05:12:22, Shecky Lin wrote:
> > On 2015/02/13 02:15:13, spang wrote:
> > > On 2015/02/12 22:42:05, achuithb wrote:
> > > > 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.
> > > 
> > > Unfortunately in-lining both closures would be wrong, because the compiler
> can
> > > put the call to release() before get(). Passed() has the same pitfall.
> > 
> > Yes, I do this for the specific reason.
> 
> 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.

Done.

[achuithb, 2015-02-24 19:32:07]

https://codereview.chromium.org/893753002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:85:
CHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
Let's just make this a DCHECK similar to other threading asserts.

https://codereview.chromium.org/893753002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:125: //
get()/Passed().
Not sure if this is clear enough. How about:
// We cannot eliminate these temporaries and inline these closures because the
compiler may call release() before get().

https://codereview.chromium.org/893753002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:126: base::Closure
pack_closure =
Make this const

https://codereview.chromium.org/893753002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:129: base::Closure
callback_closure =
Make this const

[Shecky Lin, 2015-02-25 06:21:46]

achuith@: Could you take another look? Thanks!

https://codereview.chromium.org/893753002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right):

https://codereview.chromium.org/893753002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:85:
CHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2015/02/24 19:32:07, achuithb wrote:
> Let's just make this a DCHECK similar to other threading asserts.

Yep, sorry for the typo.

https://codereview.chromium.org/893753002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:125: //
get()/Passed().
On 2015/02/24 19:32:07, achuithb wrote:
> Not sure if this is clear enough. How about:
> // We cannot eliminate these temporaries and inline these closures because the
> compiler may call release() before get().

Done.

https://codereview.chromium.org/893753002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:126: base::Closure
pack_closure =
On 2015/02/24 19:32:07, achuithb wrote:
> Make this const

Done.

https://codereview.chromium.org/893753002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:129: base::Closure
callback_closure =
On 2015/02/24 19:32:07, achuithb wrote:
> Make this const

Done.

[achuithb, 2015-02-25 20:35:51]

lgtm.

THanks for your patience

[Shecky Lin, 2015-02-26 01:52:14]

The CQ bit was checked by sheckylin@chromium.org

[Shecky Lin, 2015-02-26 01:52:14]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org
Link to the patchset: https://codereview.chromium.org/893753002/#ps140001
(title: "tot rebase")

[commit-bot: I haz the power, 2015-02-26 01:53:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/893753002/140001

[commit-bot: I haz the power, 2015-02-26 02:27:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-26 02:27:36]

Failed to commit the patch.

[commit-bot: I haz the power, 2015-02-26 02:27:52]

The CQ bit was unchecked by commit-bot@chromium.org

[Shecky Lin, 2015-02-26 04:06:35]

The CQ bit was checked by sheckylin@chromium.org

[commit-bot: I haz the power, 2015-02-26 04:08:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/893753002/140001

[commit-bot: I haz the power, 2015-02-26 04:09:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-26 04:10:19]

Patchset 8 (id:??) landed as
https://crrev.com/e1fd503701a7499e6e634ad13d114e7de07936bf
Cr-Commit-Position: refs/heads/master@{#318182}