Enable Tracing on content shell for Android

Enable Tracing on content shell for Android


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

[kangyuan.shu, 2012-12-26 04:43:40]

hi experts,

The cl is to enable collecting trace event on content shell for Android. I
thought it’s a very useful tool. Especially on Android, we can’t build (clank)
chromium for Android now.

[Ted C, 2012-12-26 16:18:28]

Cool stuff, but converting to the stdio subscriber is the main thing I think
needs to be addressed as it should really cut down on the duplicate effort you
had to go through to make this work.

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/java...
File
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java
(right):

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:35:
private static final String ACTION_START_TRACE =
"com.contentshell.action.PROFILE_START";
This should probably be:

org.chromium.content_shell.action.PROFILE_START (don't know if the action is
required at all)

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:154:
if (action.equals(ACTION_START_TRACE) && (extra.length() > 0)) {
When comparing against constants, I prefer to use the non-NPE-able format of:

ACTION_START_TRACE.equals(action)

Also, I would move the extraction of the file into this conditional as you
should probably print an error if people are passing this intent without a file
parameter.

Also, I would use TextUtils.isEmpty(extra) instead of explicitly checking the
length.

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:166:

no need for this extra line

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/java...
File
content/shell/android/java/src/org/chromium/content_shell/TraceController.java
(right):

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/TraceController.java:9:
@JNINamespace("content")
class level javadoc needed

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/TraceController.java:13:
* Begin to record trace event.
Begin recording trace events.

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/TraceController.java:15:
* @param path Where the trace data will save when stop tracing.
Specifies where the trace data will be saved when tracing is stopped.

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/TraceController.java:22:
* Stop recording trace event, and dump data to file
event[s], and dump [the] data to the file.

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
File content/shell/android/tracing_shell.cc (right):

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
content/shell/android/tracing_shell.cc:5: #include "tracing_shell.h"
should be the full path

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
content/shell/android/tracing_shell.cc:19:
base::LazyInstance<TracingIntentHandler> g_traceIntentHandler =
no camel caps in the name, so this should be g_trace_intent_handler

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
content/shell/android/tracing_shell.cc:28: : in_tracing_(false) {
indent to 4

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
content/shell/android/tracing_shell.cc:36: path_to_save_,
align with the (

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
File content/shell/android/tracing_shell.h (right):

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
content/shell/android/tracing_shell.h:17: bool RegisterTracingHandler(JNIEnv*
env);
this should be RegisterTracingIntentHandler

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
content/shell/android/tracing_shell.h:19: class TracingIntentHandler
the file name and ifdef should match this class name.

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
content/shell/android/tracing_shell.h:20: :  public content::TraceSubscriber {
I think it would be better to just extend from TraceSubscriberStdio to avoid
having to handle the formatting yourself.  That would require this to move to
content/browser/android and content/public/android/java, but that would allow us
to share this implementation with TestShell as well (and hopefully Chrome on
Android too).

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
content/shell/android/tracing_shell.h:25: // TraceSubscriber
Add "implementation" to the end so it's clearer.

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
content/shell/android/tracing_shell.h:35: void SetInTracing(bool tracing) {
in_tracing_ = tracing; }
if you are inlining functions in the header, it should be named in hacker style.

set_in_tracing(bool tracing) {...}

http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

https://codereview.chromium.org/11647013/diff/1009/content/shell/android/trac...
content/shell/android/tracing_shell.h:37: struct JsonString {
This should probably be a class as well since it's not just a simple data
object.

[kangyuan.shu, 2013-01-04 10:22:08]

I am just back from vacation.
Thanks for kindly review. I modified the code according to the comments. And
submit again. The latest cl goes to https://codereview.chromium.org/11761034.

Should be my mistake on git, I created a new issue instead of add a new patch
set. I am so sorry...

[kangyuan.shu, 2013-01-04 10:29:53]

New patch set is added.
Please have a review. Thanks!

[Ted C, 2013-01-04 17:40:48]

I like this approach a lot more.  A few nits/renames and I think it will be good
to go.

https://codereview.chromium.org/11647013/diff/9001/content/browser/android/br...
File content/browser/android/browser_jni_registrar.cc (right):

https://codereview.chromium.org/11647013/diff/9001/content/browser/android/br...
content/browser/android/browser_jni_registrar.cc:50: { "TraceController",
content::RegisterTracingIntentHandler },
TraceController -> TracingIntentHandler (w/ rename of java file as I mention
later).

https://codereview.chromium.org/11647013/diff/9001/content/browser/android/tr...
File content/browser/android/tracing_intent_handler.h (right):

https://codereview.chromium.org/11647013/diff/9001/content/browser/android/tr...
content/browser/android/tracing_intent_handler.h:20: : public
TraceSubscriberStdio {
this looks like it might fit on the previous line.

https://codereview.chromium.org/11647013/diff/9001/content/browser/android/tr...
content/browser/android/tracing_intent_handler.h:23: virtual
~TracingIntentHandler() {}
we rarely inline destructors, so you might want to just define this in the cc

https://codereview.chromium.org/11647013/diff/9001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/TraceController.java
(right):

https://codereview.chromium.org/11647013/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/TraceController.java:9:
@JNINamespace("content")
Convention is to put the annotations below the javadoc right above the
declaration.

https://codereview.chromium.org/11647013/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/TraceController.java:13:
public class TraceController {
I would rename this to TracingIntentHandler as well to match the native side of
things.

https://codereview.chromium.org/11647013/diff/9001/content/shell/android/java...
File
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java
(right):

https://codereview.chromium.org/11647013/diff/9001/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:36:
private static final String ACTION_START_TRACE =
"org.chromium.content_shell.action.PROFILE_START";
>100 chars for this and the following line

https://codereview.chromium.org/11647013/diff/9001/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:156:
if (extra.isEmpty())
sadly the conventions across languages are different, but in java braces are
always required unless the conditional and statement fit on one line.

i.e. this is fine.

if (x > 0) printSomething();

this is not:

if (x > 0)
    printSomething();

this is fine to:

if (x > 0) {
    printSomething();
}

Style guide for android:
http://source.android.com/source/code-style.html#use-standard-brace-style

With the if else format, you're going to need to use braces probably as we like
it to be consistent across the condition checks.

https://codereview.chromium.org/11647013/diff/9001/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:157:
Log.e("ContentShell", "Can not start tracing without specifing saving
location");
> 100 chars.

Although, you should use TAG from above instead of "ContentShell" and that might
get you under the limit.

https://codereview.chromium.org/11647013/diff/9001/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:160:
Log.i("ContentShell", "start tracing");
Use TAG here too and in the else if below.

[kangyuan.shu, 2013-01-05 07:07:00]

Fix coding style and rename java class to align with native.

[Ted C, 2013-01-08 01:47:19]

lgtm

[commit-bot: I haz the power, 2013-01-08 02:14:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kangyuan.shu@intel.com/11647013/15001

[commit-bot: I haz the power, 2013-01-08 02:14:47]

Presubmit check for 11647013-15001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/content_jni.gypi
    content

Presubmit checks took 2.4s to calculate.

[jam, 2013-01-08 16:31:00]

gypi lgtm

[commit-bot: I haz the power, 2013-01-09 02:13:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kangyuan.shu@intel.com/11647013/15001

[commit-bot: I haz the power, 2013-01-09 02:41:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-09 02:57:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kangyuan.shu@intel.com/11647013/33002

[commit-bot: I haz the power, 2013-01-09 07:42:08]

Change committed as 175730