Android: Fix default filename for kTraceStartup file

Android: Fix default filename for kTraceStartup file

Putting chrometrace.log in the current working directory is not
useful on Android. Generate a filepath on the sdcard instead the
same way we do it for intent triggered tracing.

R=piman@chromium.org, wangxianzhu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263153

[no sievers, 2014-04-09 21:34:21]

ptal

[Xianzhu, 2014-04-09 21:50:17]

I think it will cause adb_profile_chrome.py to be incompatible with older
versions of Chrome, so we should either keep the original protocol between
Chrome and adb_profile_chrome.py, or let adb_profile_chrome.py support two
protocols. I'd prefer the former.

Other parts look good.

[no sievers, 2014-04-09 22:34:51]

On 2014/04/09 21:50:17, Xianzhu wrote:
> I think it will cause adb_profile_chrome.py to be incompatible with older
> versions of Chrome, so we should either keep the original protocol between
> Chrome and adb_profile_chrome.py, or let adb_profile_chrome.py support two
> protocols. I'd prefer the former.
> 
> Other parts look good.

See updated version which I tested with stable and build.

[Xianzhu, 2014-04-09 23:00:23]

lgtm

[no sievers, 2014-04-09 23:04:15]

On 2014/04/09 23:00:23, Xianzhu wrote:
> lgtm

+piman for browser_main_loop.cc

[piman, 2014-04-09 23:07:15]

why not simply pass --trace--startup-file ?

[no sievers, 2014-04-09 23:13:46]

On 2014/04/09 23:07:15, piman wrote:
> why not simply pass --trace--startup-file ?

--trace-startup-file can still be used but it takes slightly more effort to come
up with a working path on Android :)

Mostly I wanted to prepare it so that I can adapt the script to support startup
tracing, which I think can easily be done now.

[piman, 2014-04-09 23:49:54]

On Wed, Apr 9, 2014 at 4:13 PM, <sievers@chromium.org> wrote:

> On 2014/04/09 23:07:15, piman wrote:
>
>> why not simply pass --trace--startup-file ?
>>
>
> --trace-startup-file can still be used but it takes slightly more effort
> to come
> up with a working path on Android :)
>
> Mostly I wanted to prepare it so that I can adapt the script to support
> startup
> tracing, which I think can easily be done now.
>

LGTM if you want, but it feels like generating a path and then parsing it
from the logs in the python script is super convoluted.


>
> https://codereview.chromium.org/232053002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-04-09 23:54:28]

lgtm

(lgtm from the tool)

[no sievers, 2014-04-10 00:11:56]

On 2014/04/09 23:49:54, piman wrote:
> On Wed, Apr 9, 2014 at 4:13 PM, <mailto:sievers@chromium.org> wrote:
> 
> > On 2014/04/09 23:07:15, piman wrote:
> >
> >> why not simply pass --trace--startup-file ?
> >>
> >
> > --trace-startup-file can still be used but it takes slightly more effort
> > to come
> > up with a working path on Android :)
> >
> > Mostly I wanted to prepare it so that I can adapt the script to support
> > startup
> > tracing, which I think can easily be done now.
> >
> 
> LGTM if you want, but it feels like generating a path and then parsing it
> from the logs in the python script is super convoluted.
> 

I'll land this for now just so that the functionality is slightly more easily
accessible (I think trying to create chrometrace.log in PWD always fails).

But how we could refactor both the startup and runtime start/stop tracing
through intent is by bundling the tracing filename with the intent, so that it
can be passed in by the script.

If I then add an option to launch Chrome through an intent that allows
specifying trace-at-startup as an extra (+filename), I won't need to mess with
the commandline at all (which is annoying because it requires a rooted device).


> 
> >
> > https://codereview.chromium.org/232053002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[no sievers, 2014-04-10 00:32:59]

On 2014/04/10 00:11:56, sievers wrote:
> On 2014/04/09 23:49:54, piman wrote:
> > On Wed, Apr 9, 2014 at 4:13 PM, <mailto:sievers@chromium.org> wrote:
> > 
> > > On 2014/04/09 23:07:15, piman wrote:
> > >
> > >> why not simply pass --trace--startup-file ?
> > >>
> > >
> > > --trace-startup-file can still be used but it takes slightly more effort
> > > to come
> > > up with a working path on Android :)
> > >
> > > Mostly I wanted to prepare it so that I can adapt the script to support
> > > startup
> > > tracing, which I think can easily be done now.
> > >
> > 
> > LGTM if you want, but it feels like generating a path and then parsing it
> > from the logs in the python script is super convoluted.
> > 
> 
> I'll land this for now just so that the functionality is slightly more easily
> accessible (I think trying to create chrometrace.log in PWD always fails).
> 
> But how we could refactor both the startup and runtime start/stop tracing
> through intent is by bundling the tracing filename with the intent, so that it
> can be passed in by the script.
> 
> If I then add an option to launch Chrome through an intent that allows
> specifying trace-at-startup as an extra (+filename), I won't need to mess with
> the commandline at all (which is annoying because it requires a rooted
device).
> 

actually, the script has to parse the log no matter what to know when tracing
finished and the file is ready to be grabbed.
> 
> > 
> > >
> > > https://codereview.chromium.org/232053002/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[no sievers, 2014-04-11 01:28:57]

Committed patchset #7 manually as r263153 (tree was closed).

[piman, 2014-04-11 07:45:19]

https://codereview.chromium.org/232053002/diff/110001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/232053002/diff/110001/content/browser/browser...
content/browser/browser_main_loop.cc:258: LOG(INFO) << "Completed startup
tracing to " << trace_file.value();
Hmm, I'm getting presubmit issues in a patch that touches this file because of
this line.
Did this land ignoring presubmit checks?

See
http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...

[no sievers, 2014-04-11 16:02:35]

On 2014/04/11 07:45:19, piman wrote:
>
https://codereview.chromium.org/232053002/diff/110001/content/browser/browser...
> File content/browser/browser_main_loop.cc (right):
> 
>
https://codereview.chromium.org/232053002/diff/110001/content/browser/browser...
> content/browser/browser_main_loop.cc:258: LOG(INFO) << "Completed startup
> tracing to " << trace_file.value();
> Hmm, I'm getting presubmit issues in a patch that touches this file because of
> this line.
> Did this land ignoring presubmit checks?
> 
> See
>
http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...

My bad. Although the check is silly in this case because i certainly doesn't
spam the log.
I'll try and change it to VLOG() for now.