Refactor trace_event_impl's SetEnabled to use TraceOptions. Propagate this through the whole stack.

Refactor tracing to pass around base::debug::TraceOptions to reduce spaghetti

Previously, the options for tracing were passed around with different ad hoc
systems. Strings in some places, base::debug::TraceOptions enum in others,
and a content::TraceOptions in yet another. There were two separate ad-hoc
string formats. Similar messes were present with category filters: sometimes
we passed strings, sometimes the CategoryFilter.

This patch though enormous looking simply consolidates all this ad-hockery
into base::debug::TraceOptions. It may look like the call sites have gotten
more verbose,
but the end result of this is a consistent understanding of TraceOptions.

There is one exception to this consolidation: devtools has to maintain its own
mapping of string->TraceOptions encoding. This is because DevTools makes
guarantees about backward compatibility: if base changes its mind about the
string form of a TraceOption, devtools needs to keep supporting the old form.

BUG=396081
TBR=cevans@chromium.org

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

[dsinclair, 2014-07-28 18:52:37]

This feels like it's adding a bunch of duplication and confusing code for
minimal gain.

The storage of trace options is more complicated (with the struct and a separate
internal int) along with the api's getting more complicated with this split
between things that are and aren't trace-options.

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
File base/debug/trace_event_android.cc (right):

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_android.cc:102:
base::debug::TraceOptions::RECORD_CONTINUOUSLY, false));
I'm not a fan of the use of false here. Looking at the callsite I have no idea
what false means.

TraceOptions options(TraceOptions::RECORD_CONTINUOUSLY);
options.enableSysTrace(true);

Or, if we made enableSysTrace return this we could chain it.

TraceOptions(RECORD_CONTINOUSLY).enableSysTrace(true) in the call above.

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_impl.cc:1453: TraceLog::InternalTraceOptions
TraceLog::GetInternalOptionsFromTraceOptions(
The spit between trace options and internal trace options seems like it's going
to cause bugs in the future.

When can we use one, and when the other? What happens if we grab the
non-internal one at some point, does things get out of sync?

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_impl.h:706: static const InternalTraceOptions NONE;
Not a big fan of this duplication. We now have to remember to define each of
these things in two different spots. Once here and once in TraceOptions above.

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
File base/debug/trace_event_impl_constants.cc (right):

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_impl_constants.cc:14: const char*
TraceOptions::kRecordUntilFull = "record-until-full";
Do these get used outside of trace_event_impl? Can we just declare them inside
that cc in an anonymous namespace?

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_impl_constants.cc:19: const
TraceLog::InternalTraceOptions TraceLog::NONE = 0;
I guess these can't just be assigned in the enum anymore because they don't all
exist in the enum?

https://codereview.chromium.org/425593002/diff/260001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/425593002/diff/260001/content/app/content_mai...
content/app/content_main_runner.cc:635:
base::debug::TraceOptions("record-until-full"));
This should be using the constant.

https://codereview.chromium.org/425593002/diff/260001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.h (right):

https://codereview.chromium.org/425593002/diff/260001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.h:35: bool enable_systrace,
I find this bool makes the call sites a lot harder to read. There are a lot of
'false's scattered around now that used to be ENABLE_SYSTRACE.

It also strikes me as odd as enabling sys-trace is a tracing option. I'm not
sure how we define what is a tracing option and what isn't?

https://codereview.chromium.org/425593002/diff/260001/content/browser/tracing...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/425593002/diff/260001/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:168:
static_cast<base::debug::TraceOptions>(tracing_options),
Why does this need a cast?

https://codereview.chromium.org/425593002/diff/260001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/425593002/diff/260001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:52:
RECORD_UNTIL_FULL("record-until-full"),
Are we required to duplicate the options into there along with the names?

[nednguyen, 2014-07-28 20:27:22]

I like the chaining suggestion. I will fix that and other API issues after we
agree that this patch can be submitted.

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_impl.cc:1453: TraceLog::InternalTraceOptions
TraceLog::GetInternalOptionsFromTraceOptions(
On 2014/07/28 18:52:36, dsinclair wrote:
> The spit between trace options and internal trace options seems like it's
going
> to cause bugs in the future.
> 
> When can we use one, and when the other? What happens if we grab the
> non-internal one at some point, does things get out of sync?

The InternalTraceOptions is only used by the trace_event_impl's implementation
as they are private. So all callers of TraceLog must use the TraceOptions. 

There maybe a better way to convey this, maybe a private namespace? I am not
sure, please give some advices.

https://codereview.chromium.org/425593002/diff/260001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/425593002/diff/260001/content/app/content_mai...
content/app/content_main_runner.cc:635:
base::debug::TraceOptions("record-until-full"));
On 2014/07/28 18:52:36, dsinclair wrote:
> This should be using the constant.

Done.

https://codereview.chromium.org/425593002/diff/260001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.h (right):

https://codereview.chromium.org/425593002/diff/260001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.h:35: bool enable_systrace,
On 2014/07/28 18:52:36, dsinclair wrote:
> I find this bool makes the call sites a lot harder to read. There are a lot of
> 'false's scattered around now that used to be ENABLE_SYSTRACE.
> 
> It also strikes me as odd as enabling sys-trace is a tracing option. I'm not
> sure how we define what is a tracing option and what isn't?

I think the distinction here is tracing option is option of chrome browser
tracing system, which belongs to each browser process. Things like systrace
belong to the OS, hence are in a different category.

Although this make the call sites harder to read, it does clear thing a little
bit.

https://codereview.chromium.org/425593002/diff/260001/content/browser/tracing...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/425593002/diff/260001/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:168:
static_cast<base::debug::TraceOptions>(tracing_options),
On 2014/07/28 18:52:36, dsinclair wrote:
> Why does this need a cast?

No it doesn't. I did a s/TracingController.../..TraceOptions/g and forgot to fix
this one.

The fact that we don't need to cast this is the signal that this approach is
more sane than using the integer bit, I think.

https://codereview.chromium.org/425593002/diff/260001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/425593002/diff/260001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:52:
RECORD_UNTIL_FULL("record-until-full"),
On 2014/07/28 18:52:36, dsinclair wrote:
> Are we required to duplicate the options into there along with the names?

This will call the nativeStartTracing(..) method which takes options as string,
so I need to duplicate the options string here. Not sure how to allow this java
class to use the string options declared in trace_event_impl.h.

[nduca, 2014-07-28 20:39:32]

sorry dan, lemme review this a bit until i like it and then we can run it by
you. I do think there's a win to be had here but its not quite coded up yet s.t.
the win is clear.

https://codereview.chromium.org/425593002/diff/280001/content/public/browser/...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/425593002/diff/280001/content/public/browser/...
content/public/browser/tracing_controller.h:65: bool enable_systrace,
please move enable_systrace into TraceOptions, update the patch and ping me
again :)

[nduca, 2014-07-29 00:15:01]

some thoughts looks really solid

https://codereview.chromium.org/425593002/diff/340002/base/debug/trace_event_...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/425593002/diff/340002/base/debug/trace_event_...
base/debug/trace_event_impl.h:370: enum RecordMode {
you could hoist this to be parallel to trace options so callsites suck less. And
call it TraceRecordMode. So you'd have

  base::debug::TraceOptions(base::debug::RECORD_UNTIL_FULL)

https://codereview.chromium.org/425593002/diff/340002/base/debug/trace_event_...
base/debug/trace_event_impl.h:392: TraceOptions& EnableSampling(bool option) {
you've exposed the fields so you don't need these Enable methods.

https://codereview.chromium.org/425593002/diff/340002/base/debug/trace_event_...
base/debug/trace_event_impl.h:402: explicit TraceOptions(StringPiece options);
std::string

https://codereview.chromium.org/425593002/diff/340002/content/browser/tracing...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/425593002/diff/340002/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:77: bool use_system_tracing;
remove these

https://codereview.chromium.org/425593002/diff/340002/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:83: options_ok &=
options->GetBoolean("useSystemTracing", &use_system_tracing);
this needs updating.


if (GetString("traceOptions", &tmp))
  *tracing_options = TraceOptions(tmp);
else {
  ... read the booleans only in this case, and put them into tracing options
directly
}

https://codereview.chromium.org/425593002/diff/340002/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/425593002/diff/340002/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:44:
/**
nooo, just pass the TraceOptions-as-a-string around in java. Dont ever have a
enum version.

[nednguyen, 2014-07-29 19:33:21]

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
File base/debug/trace_event_android.cc (right):

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_android.cc:102:
base::debug::TraceOptions::RECORD_CONTINUOUSLY, false));
On 2014/07/28 18:52:36, dsinclair wrote:
> I'm not a fan of the use of false here. Looking at the callsite I have no idea
> what false means.
> 
> TraceOptions options(TraceOptions::RECORD_CONTINUOUSLY);
> options.enableSysTrace(true);
> 
> Or, if we made enableSysTrace return this we could chain it.
> 
> TraceOptions(RECORD_CONTINOUSLY).enableSysTrace(true) in the call above.

Done.

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
File base/debug/trace_event_impl_constants.cc (right):

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_impl_constants.cc:14: const char*
TraceOptions::kRecordUntilFull = "record-until-full";
On 2014/07/28 18:52:36, dsinclair wrote:
> Do these get used outside of trace_event_impl? Can we just declare them inside
> that cc in an anonymous namespace?

Done.

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_impl_constants.cc:19: const
TraceLog::InternalTraceOptions TraceLog::NONE = 0;
On 2014/07/28 18:52:36, dsinclair wrote:
> I guess these can't just be assigned in the enum anymore because they don't
all
> exist in the enum?

I don't find the need for these to be enum if it's not public API and we just
typecast the internal option back & forth with int anyway.

https://codereview.chromium.org/425593002/diff/260001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.h (right):

https://codereview.chromium.org/425593002/diff/260001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.h:35: bool enable_systrace,
On 2014/07/28 20:27:22, nednguyen wrote:
> On 2014/07/28 18:52:36, dsinclair wrote:
> > I find this bool makes the call sites a lot harder to read. There are a lot
of
> > 'false's scattered around now that used to be ENABLE_SYSTRACE.
> > 
> > It also strikes me as odd as enabling sys-trace is a tracing option. I'm not
> > sure how we define what is a tracing option and what isn't?
> 
> I think the distinction here is tracing option is option of chrome browser
> tracing system, which belongs to each browser process. Things like systrace
> belong to the OS, hence are in a different category.
> 
> Although this make the call sites harder to read, it does clear thing a little
> bit.

Added enable_systrace to TraceOptions.

https://codereview.chromium.org/425593002/diff/340002/base/debug/trace_event_...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/425593002/diff/340002/base/debug/trace_event_...
base/debug/trace_event_impl.h:370: enum RecordMode {
On 2014/07/29 00:15:00, nduca wrote:
> you could hoist this to be parallel to trace options so callsites suck less.
And
> call it TraceRecordMode. So you'd have
> 
>   base::debug::TraceOptions(base::debug::RECORD_UNTIL_FULL)

Done.

https://codereview.chromium.org/425593002/diff/340002/base/debug/trace_event_...
base/debug/trace_event_impl.h:392: TraceOptions& EnableSampling(bool option) {
On 2014/07/29 00:15:01, nduca wrote:
> you've exposed the fields so you don't need these Enable methods.

This allow inlining TraceOptions(...).EnableSampling(true), so it's less painful
for the call sites.

https://codereview.chromium.org/425593002/diff/340002/base/debug/trace_event_...
base/debug/trace_event_impl.h:402: explicit TraceOptions(StringPiece options);
On 2014/07/29 00:15:00, nduca wrote:
> std::string

Done.

https://codereview.chromium.org/425593002/diff/340002/content/browser/tracing...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/425593002/diff/340002/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:77: bool use_system_tracing;
On 2014/07/29 00:15:01, nduca wrote:
> remove these

Done.

https://codereview.chromium.org/425593002/diff/340002/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:83: options_ok &=
options->GetBoolean("useSystemTracing", &use_system_tracing);
On 2014/07/29 00:15:01, nduca wrote:
> this needs updating.
> 
> 
> if (GetString("traceOptions", &tmp))
>   *tracing_options = TraceOptions(tmp);
> else {
>   ... read the booleans only in this case, and put them into tracing options
> directly
> }

Modified to read the booleans directly. I will modify the traceOptions string
options here and in javascript code in the next patch. This patch is quite big
already.

https://codereview.chromium.org/425593002/diff/340002/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/425593002/diff/340002/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:44:
/**
On 2014/07/29 00:15:01, nduca wrote:
> nooo, just pass the TraceOptions-as-a-string around in java. Dont ever have a
> enum version.

Done. Hard code these string below. Will fix the intent tracing tool to use
TraceOption-as-a-string in subsequent patch.

[Xianzhu, 2014-07-29 21:59:05]

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_impl.h:391: : record_mode(record_mode),
enable_sampling(enable_sampling) {}
Indentation of the above lines seems incorrect.

https://codereview.chromium.org/425593002/diff/510001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/425593002/diff/510001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:180:
* @param traceOptions Record until the user ends the trace. The trace buffer is
fixed
Please update the description.

[Xianzhu, 2014-07-29 22:00:27]

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/425593002/diff/260001/base/debug/trace_event_...
base/debug/trace_event_impl.h:391: : record_mode(record_mode),
enable_sampling(enable_sampling) {}
On 2014/07/29 21:59:05, Xianzhu wrote:
> Indentation of the above lines seems incorrect.

Sorry, this is an old comment. Please ignore it.

[nednguyen, 2014-07-29 22:33:59]

https://codereview.chromium.org/425593002/diff/510001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/425593002/diff/510001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:180:
* @param traceOptions Record until the user ends the trace. The trace buffer is
fixed
On 2014/07/29 21:59:05, Xianzhu wrote:
> Please update the description.

Done.

[Xianzhu, 2014-07-29 22:35:29]

Android-related changes lgtm.

[nduca, 2014-07-29 22:40:13]

lgtm

https://codereview.chromium.org/425593002/diff/510001/base/debug/trace_event_...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/425593002/diff/510001/base/debug/trace_event_...
base/debug/trace_event_impl.h:392: TraceOptions& EnableSampling(bool option) {
please make these go away, they dont add value.

https://codereview.chromium.org/425593002/diff/510001/content/browser/tracing...
File content/browser/tracing/trace_message_filter.h (right):

https://codereview.chromium.org/425593002/diff/510001/content/browser/tracing...
content/browser/tracing/trace_message_filter.h:28: base::debug::TraceOptions
options);
const base::debug::TraceOptions& options

[piman, 2014-07-30 01:08:17]

OWNER LGTM for content/ once reviewers are happy, but nit: can you wrap the
issue description? (I think 72 characters is the agreed-upon limit)

[dsinclair, 2014-07-30 14:25:27]

This is looking a lot better, thanks a lot for the changes.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl.cc:1198: trace_options_(kInternalRecordUntilFull),
RECORD_UNTIL_FULL is the default value, shouldn't need to provide it to the
constructor.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl.cc:1471: options.enable_sampling ?
kInternalEnableSampling : kInternalNone;
What about systrace?

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl.cc:1492: ret.enable_sampling = (option &
kInternalEnableSampling) != 0;
What about systrace?

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl.h:398: // recording mode is specified, recording
mode is RECORD_UNTIL_FULL.
nit: recording modes are specified, the recording mode is RECORD_UNTIL_FULL

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl.h:723: 
Why no kInternalEnableSystrace here?

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
File base/debug/trace_event_impl_constants.cc (right):

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl_constants.cc:13: // Constant used by TraceLog's
internal implementation of trace_option.
nit: Constants

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl_constants.cc:20: << 2;
This wrapping is nasty. What about something like:

const TraceLog::InternalTraceOptions
    TraceLog::kInternalNone = 0;
const TraceLog::InternalTraceOptions
    TraceLog::kInternalRecordUntilFull = 1 << 0;
const TraceLog::InternalTraceOptions
    TraceLog::kInternalRecordContinuously = 1 << 1;
const TraceLog::InternalTraceOptions
    TraceLog::kInternalEnableSampling = 1 << 2;
const TraceLog::InternalTraceOptions
    TraceLog::kInternalEchoToConsole = 1 << 3;

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl_constants.cc:22: << 3;
Systrace?

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:87: TraceOptions(RECORD_UNTIL_FULL));
RECORD_UNTIL_FULL is default, no need to pass. (and below)

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:2501: TraceOptions("record-continuously"));
Why not use RECORD_CONTINUOUSLY?

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:2561: TraceOptions("record-continuously"));
as above.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:2589: TraceOptions("record-continuously"));
as above

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:2749:
trace_log->GetInternalOptionsFromTraceOptions(options));
Do we need to check the other bits to make sure they're all zero?

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:2760: 
options.enable_systrace ?

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:3002: 
systrace is checked below, why not on this one?

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:3027: }
options =
TraceOptions("record-continuously,record-until-full,trace-to-console");
EXPECT_EQ(ECHO_TO_CONSOLE, options.record_mode);
EXPECT_FALSE(options.enable_systrace);
EXPECT_FALSE(options.enable_sampling);

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:3027: }
EXPECT_THROWS (I think that's what it's called? and I think you can do this with
a NOT_REACHED?)

EXPECT_THROWS( TraceOptions("foo-bar-baz"); )

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:3028: 
options = TraceOptions("");

I assume this should set RECORD_UNTIL_FULL and that's all?

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:3037: for (int i = 0; i < 3; ++i) {
question: Is there some way we can make sure that a) this 3 doesn't become less
then modes length and b) that modes[] actually includes all of the modes?

https://codereview.chromium.org/425593002/diff/570001/base/test/trace_event_a...
File base/test/trace_event_analyzer_unittest.cc (right):

https://codereview.chromium.org/425593002/diff/570001/base/test/trace_event_a...
base/test/trace_event_analyzer_unittest.cc:51:
base::debug::TraceOptions(base::debug::RECORD_UNTIL_FULL));
RECORD_UNTIL_FULL is default, no need to pass.

https://codereview.chromium.org/425593002/diff/570001/chrome/browser/lifetime...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/425593002/diff/570001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime.cc:186:
base::debug::RECORD_UNTIL_FULL));
RECORD_UNTIL_FULL is default, no need to pass.

https://codereview.chromium.org/425593002/diff/570001/chrome/test/base/tracin...
File chrome/test/base/tracing.cc (right):

https://codereview.chromium.org/425593002/diff/570001/chrome/test/base/tracin...
chrome/test/base/tracing.cc:36: base::debug::CategoryFilter(category_patterns),
Why did this change? Seems unrelated to the rest of the CL.

https://codereview.chromium.org/425593002/diff/570001/chrome/test/base/tracin...
chrome/test/base/tracing.cc:55: base::debug::CategoryFilter(category_patterns),
dito.

https://codereview.chromium.org/425593002/diff/570001/components/feedback/tra...
File components/feedback/tracing_manager.cc (right):

https://codereview.chromium.org/425593002/diff/570001/components/feedback/tra...
components/feedback/tracing_manager.cc:94: base::debug::CategoryFilter(""),
and this one.

https://codereview.chromium.org/425593002/diff/570001/content/app/android/lib...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:87:
base::debug::RECORD_UNTIL_FULL));
nit: Does this fit on the line above?

https://codereview.chromium.org/425593002/diff/570001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:87:
base::debug::RECORD_UNTIL_FULL));
RECORD_UNTIL_FULL is default, no need to pass.

https://codereview.chromium.org/425593002/diff/570001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/app/content_mai...
content/app/content_main_runner.cc:637: base::debug::RECORD_UNTIL_FULL));
RECORD_UNTIL_FULL is default, no need to pass.

https://codereview.chromium.org/425593002/diff/570001/content/browser/android...
File content/browser/android/tracing_controller_android.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/android...
content/browser/android/tracing_controller_android.cc:45:
base::debug::CategoryFilter(categories),
Seems unreleated.

https://codereview.chromium.org/425593002/diff/570001/content/browser/devtool...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/devtool...
content/browser/devtools/devtools_tracing_handler.cc:133: ret.enable_sampling =
true;
Should we add systrace here as well?

https://codereview.chromium.org/425593002/diff/570001/content/browser/devtool...
content/browser/devtools/devtools_tracing_handler.cc:181:
base::debug::CategoryFilter(categories),
unrelated?

https://codereview.chromium.org/425593002/diff/570001/content/browser/devtool...
content/browser/devtools/devtools_tracing_handler.cc:188:
base::debug::CategoryFilter(categories),
Unrelated?

https://codereview.chromium.org/425593002/diff/570001/content/browser/media/w...
File content/browser/media/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:86:
base::debug::TraceOptions trace_options(base::debug::RECORD_UNTIL_FULL);
We don't actually need the base::debug::RECORD_UNTIL_FULL here do we since it's
the default option?

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
File content/browser/tracing/trace_message_filter.h (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/trace_message_filter.h:27: void SendBeginTracing(const
base::debug::CategoryFilter& category_filter_str,
Let's leave this as strings for now and do a followup CL that converts to
CategoryFilter. Seems separate from the current CL.

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
File content/browser/tracing/tracing_controller_browsertest.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_controller_browsertest.cc:175: TraceOptions
trace_options(RECORD_UNTIL_FULL);
RECORD_UNTIL_FULL is default, no need to pass.

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_controller_browsertest.cc:311: TraceOptions
trace_options(RECORD_UNTIL_FULL);
RECORD_UNTIL_FULL is default, no need to pass.

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.cc (left):

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.cc:393: trace_options |=
TraceLog::ENABLE_SAMPLING;
So, previously, this was _only_ passing ENABLED_SAMPLING to the methods below
otherwise it passed 0.  Now, it's passing all of the currently set trace
options.

Does this change of behaviour have any effect?

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.h (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.h:33: const
base::debug::CategoryFilter& category_filter,
Leave for a separate CL.

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:103: base::debug::RECORD_CONTINUOUSLY;
This looks like it fits above, other wise add { } since the body is multiple
lines.

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:105: tracing_options->record_mode =
base::debug::RECORD_UNTIL_FULL;
RECORD_UNTIL_FULL is the default, so we shouldn't need to reset it.

https://codereview.chromium.org/425593002/diff/570001/content/common/gpu/clie...
File content/common/gpu/client/gl_helper_unittest.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/common/gpu/clie...
content/common/gpu/client/gl_helper_unittest.cc:81:
base::debug::RECORD_UNTIL_FULL));
RECORD_UNTIL_FULL is default, so no need to pass here.

https://codereview.chromium.org/425593002/diff/570001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/425593002/diff/570001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:150:
String traceOptions) {
Fits above?

https://codereview.chromium.org/425593002/diff/570001/content/renderer/devtoo...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/renderer/devtoo...
content/renderer/devtools/devtools_agent.cc:177:
TraceOptions(base::debug::RECORD_UNTIL_FULL));
RECORD_UNTIL_FULL is the default value, don't need to pass.

[nednguyen, 2014-07-30 16:51:16]

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl.cc:1198: trace_options_(kInternalRecordUntilFull),
On 2014/07/30 14:25:24, dsinclair wrote:
> RECORD_UNTIL_FULL is the default value, shouldn't need to provide it to the
> constructor.

It's the default value of TraceOptions, not the internal trace_options_ which is
just int type.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl.cc:1471: options.enable_sampling ?
kInternalEnableSampling : kInternalNone;
On 2014/07/30 14:25:24, dsinclair wrote:
> What about systrace?

Systrace is not handled here.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl.cc:1492: ret.enable_sampling = (option &
kInternalEnableSampling) != 0;
On 2014/07/30 14:25:24, dsinclair wrote:
> What about systrace?

ditto

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl.h:398: // recording mode is specified, recording
mode is RECORD_UNTIL_FULL.
On 2014/07/30 14:25:24, dsinclair wrote:
> nit: recording modes are specified, the recording mode is RECORD_UNTIL_FULL

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl.h:723: 
On 2014/07/30 14:25:24, dsinclair wrote:
> Why no kInternalEnableSystrace here?
Add comment that explains it.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
File base/debug/trace_event_impl_constants.cc (right):

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl_constants.cc:13: // Constant used by TraceLog's
internal implementation of trace_option.
On 2014/07/30 14:25:24, dsinclair wrote:
> nit: Constants

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl_constants.cc:20: << 2;
On 2014/07/30 14:25:24, dsinclair wrote:
> This wrapping is nasty. What about something like:
> 
> const TraceLog::InternalTraceOptions
>     TraceLog::kInternalNone = 0;
> const TraceLog::InternalTraceOptions
>     TraceLog::kInternalRecordUntilFull = 1 << 0;
> const TraceLog::InternalTraceOptions
>     TraceLog::kInternalRecordContinuously = 1 << 1;
> const TraceLog::InternalTraceOptions
>     TraceLog::kInternalEnableSampling = 1 << 2;
> const TraceLog::InternalTraceOptions
>     TraceLog::kInternalEchoToConsole = 1 << 3;

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_impl_constants.cc:22: << 3;
On 2014/07/30 14:25:25, dsinclair wrote:
> Systrace?

No systrace as above.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:87: TraceOptions(RECORD_UNTIL_FULL));
On 2014/07/30 14:25:25, dsinclair wrote:
> RECORD_UNTIL_FULL is default, no need to pass. (and below)

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:2501: TraceOptions("record-continuously"));
On 2014/07/30 14:25:25, dsinclair wrote:
> Why not use RECORD_CONTINUOUSLY?

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:2561: TraceOptions("record-continuously"));
On 2014/07/30 14:25:25, dsinclair wrote:
> as above.

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:2589: TraceOptions("record-continuously"));
On 2014/07/30 14:25:25, dsinclair wrote:
> as above

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:2749:
trace_log->GetInternalOptionsFromTraceOptions(options));
On 2014/07/30 14:25:25, dsinclair wrote:
> Do we need to check the other bits to make sure they're all zero?

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:2760: 
On 2014/07/30 14:25:25, dsinclair wrote:
> options.enable_systrace ?

Done. It's not handled by trace_event_impl though, so I will just test that it
doesn't change the internal trace_option.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:3002: 
On 2014/07/30 14:25:25, dsinclair wrote:
> systrace is checked below, why not on this one?

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:3027: }
On 2014/07/30 14:25:25, dsinclair wrote:
> options =
> TraceOptions("record-continuously,record-until-full,trace-to-console");
> EXPECT_EQ(ECHO_TO_CONSOLE, options.record_mode);
> EXPECT_FALSE(options.enable_systrace);
> EXPECT_FALSE(options.enable_sampling);

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:3027: }
On 2014/07/30 14:25:25, dsinclair wrote:
> options =
> TraceOptions("record-continuously,record-until-full,trace-to-console");
> EXPECT_EQ(ECHO_TO_CONSOLE, options.record_mode);
> EXPECT_FALSE(options.enable_systrace);
> EXPECT_FALSE(options.enable_sampling);

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:3028: 
On 2014/07/30 14:25:25, dsinclair wrote:
> options = TraceOptions("");
> 
> I assume this should set RECORD_UNTIL_FULL and that's all?

Done.

https://codereview.chromium.org/425593002/diff/570001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:3037: for (int i = 0; i < 3; ++i) {
On 2014/07/30 14:25:25, dsinclair wrote:
> question: Is there some way we can make sure that a) this 3 doesn't become
less
> then modes length and b) that modes[] actually includes all of the modes?

ENUM are essentially just const int, so I can do:

for (int i = MIN, i == MAX; ++i) {
  TraceOptions option (static_cast<TraceRecordMode>(i));
  ....
}

Not sure whether this worths the hassle though.

https://codereview.chromium.org/425593002/diff/570001/base/test/trace_event_a...
File base/test/trace_event_analyzer_unittest.cc (right):

https://codereview.chromium.org/425593002/diff/570001/base/test/trace_event_a...
base/test/trace_event_analyzer_unittest.cc:51:
base::debug::TraceOptions(base::debug::RECORD_UNTIL_FULL));
On 2014/07/30 14:25:25, dsinclair wrote:
> RECORD_UNTIL_FULL is default, no need to pass.

Done.

https://codereview.chromium.org/425593002/diff/570001/chrome/browser/lifetime...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/425593002/diff/570001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime.cc:186:
base::debug::RECORD_UNTIL_FULL));
On 2014/07/30 14:25:25, dsinclair wrote:
> RECORD_UNTIL_FULL is default, no need to pass.

Done.

https://codereview.chromium.org/425593002/diff/570001/chrome/test/base/tracin...
File chrome/test/base/tracing.cc (right):

https://codereview.chromium.org/425593002/diff/570001/chrome/test/base/tracin...
chrome/test/base/tracing.cc:36: base::debug::CategoryFilter(category_patterns),
On 2014/07/30 14:25:25, dsinclair wrote:
> Why did this change? Seems unrelated to the rest of the CL.

I also refactor TracingController to use CategoryFilter instead of raw string.
"Similar messes were present with category filters: sometimes we passed strings,
sometimes the CategoryFilter."

https://codereview.chromium.org/425593002/diff/570001/chrome/test/base/tracin...
chrome/test/base/tracing.cc:55: base::debug::CategoryFilter(category_patterns),
On 2014/07/30 14:25:25, dsinclair wrote:
> dito.

Acknowledged.

https://codereview.chromium.org/425593002/diff/570001/components/feedback/tra...
File components/feedback/tracing_manager.cc (right):

https://codereview.chromium.org/425593002/diff/570001/components/feedback/tra...
components/feedback/tracing_manager.cc:94: base::debug::CategoryFilter(""),
On 2014/07/30 14:25:25, dsinclair wrote:
> and this one.

Acknowledged.

https://codereview.chromium.org/425593002/diff/570001/content/app/android/lib...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:87:
base::debug::RECORD_UNTIL_FULL));
On 2014/07/30 14:25:26, dsinclair wrote:
> RECORD_UNTIL_FULL is default, no need to pass.

Done.

https://codereview.chromium.org/425593002/diff/570001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:87:
base::debug::RECORD_UNTIL_FULL));
On 2014/07/30 14:25:26, dsinclair wrote:
> RECORD_UNTIL_FULL is default, no need to pass.

Done.

https://codereview.chromium.org/425593002/diff/570001/content/browser/android...
File content/browser/android/tracing_controller_android.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/android...
content/browser/android/tracing_controller_android.cc:45:
base::debug::CategoryFilter(categories),
On 2014/07/30 14:25:26, dsinclair wrote:
> Seems unreleated.
ditto.

https://codereview.chromium.org/425593002/diff/570001/content/browser/devtool...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/devtool...
content/browser/devtools/devtools_tracing_handler.cc:133: ret.enable_sampling =
true;
On 2014/07/30 14:25:26, dsinclair wrote:
> Should we add systrace here as well?

I would leave this to another patch since it's a devtool API extension.

https://codereview.chromium.org/425593002/diff/570001/content/browser/devtool...
content/browser/devtools/devtools_tracing_handler.cc:181:
base::debug::CategoryFilter(categories),
On 2014/07/30 14:25:26, dsinclair wrote:
> unrelated?

ditto

https://codereview.chromium.org/425593002/diff/570001/content/browser/devtool...
content/browser/devtools/devtools_tracing_handler.cc:188:
base::debug::CategoryFilter(categories),
On 2014/07/30 14:25:26, dsinclair wrote:
> Unrelated?

ditto

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
File content/browser/tracing/tracing_controller_browsertest.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_controller_browsertest.cc:175: TraceOptions
trace_options(RECORD_UNTIL_FULL);
On 2014/07/30 14:25:26, dsinclair wrote:
> RECORD_UNTIL_FULL is default, no need to pass.

Done.

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_controller_browsertest.cc:311: TraceOptions
trace_options(RECORD_UNTIL_FULL);
On 2014/07/30 14:25:26, dsinclair wrote:
> RECORD_UNTIL_FULL is default, no need to pass.

Done.

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.cc (left):

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.cc:393: trace_options |=
TraceLog::ENABLE_SAMPLING;
On 2014/07/30 14:25:26, dsinclair wrote:
> So, previously, this was _only_ passing ENABLED_SAMPLING to the methods below
> otherwise it passed 0.  Now, it's passing all of the currently set trace
> options.
> 
> Does this change of behaviour have any effect?

Good catch. This behavior seems strange though, since your current "options_" is
different from what you actually use. 

+haraken: can you explain whether this is intentional?

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:103: base::debug::RECORD_CONTINUOUSLY;
On 2014/07/30 14:25:26, dsinclair wrote:
> This looks like it fits above, other wise add { } since the body is multiple
> lines.

Done.

https://codereview.chromium.org/425593002/diff/570001/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:105: tracing_options->record_mode =
base::debug::RECORD_UNTIL_FULL;
On 2014/07/30 14:25:26, dsinclair wrote:
> RECORD_UNTIL_FULL is the default, so we shouldn't need to reset it.

This relies on the assumption that *tracing_options was constructed with the
default though. I will leave this as is to make it clearer.

https://codereview.chromium.org/425593002/diff/570001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/425593002/diff/570001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:150:
String traceOptions) {
On 2014/07/30 14:25:26, dsinclair wrote:
> Fits above?

Done.

https://codereview.chromium.org/425593002/diff/570001/content/renderer/devtoo...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/425593002/diff/570001/content/renderer/devtoo...
content/renderer/devtools/devtools_agent.cc:177:
TraceOptions(base::debug::RECORD_UNTIL_FULL));
On 2014/07/30 14:25:26, dsinclair wrote:
> RECORD_UNTIL_FULL is the default value, don't need to pass.

Done.

[dsinclair, 2014-07-30 17:23:39]

lgtm once you've confirmed the changed behaviour is acceptable.

[nednguyen, 2014-07-30 17:41:37]

+haraken: can you help reviewing changes in tracing_controller_impl? Thanks.

[haraken, 2014-07-30 18:10:51]

On 2014/07/30 17:41:37, nednguyen wrote:
> +haraken: can you help reviewing changes in tracing_controller_impl? Thanks.

Changes to trace_controller_impl and trace_event_impl LGTM.

[nednguyen, 2014-07-30 18:15:49]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-07-30 18:16:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nednguyen@google.com/425593002/590001

[commit-bot: I haz the power, 2014-07-30 21:02:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-30 21:08:44]

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

[commit-bot: I haz the power, 2014-07-30 21:08:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nednguyen, 2014-07-31 19:16:15]

@willchan: I am looking for lg to base/ and components/
@jochen: I am looking for lg to chrome/

Thanks!

[willchan no longer on Chromium, 2014-07-31 19:20:22]

I'm not in components/OWNERS.


On Thu, Jul 31, 2014 at 12:16 PM, <nednguyen@google.com> wrote:

> @willchan: I am looking for lg to base/ and components/
> @jochen: I am looking for lg to chrome/
>
> Thanks!
>
> https://codereview.chromium.org/425593002/
>

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

[willchan no longer on Chromium, 2014-07-31 19:26:44]

base/LGTM

https://codereview.chromium.org/425593002/diff/590001/base/debug/trace_event_...
File base/debug/trace_event_android.cc (right):

https://codereview.chromium.org/425593002/diff/590001/base/debug/trace_event_...
base/debug/trace_event_android.cc:101: base::debug::TraceOptions(
Nit: I'd ditch the base::debug::. You're already in that namespace.

https://codereview.chromium.org/425593002/diff/590001/base/debug/trace_event_...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/425593002/diff/590001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:86: base::debug::TraceLog::RECORDING_MODE,
Nit: ditch base::debug here.

[jochen (gone - plz use gerrit), 2014-08-01 09:56:17]

lgtm with nits.

you'll need a security review for the ipc changes

[nednguyen, 2014-08-04 02:07:14]

https://codereview.chromium.org/425593002/diff/590001/base/debug/trace_event_...
File base/debug/trace_event_android.cc (right):

https://codereview.chromium.org/425593002/diff/590001/base/debug/trace_event_...
base/debug/trace_event_android.cc:101: base::debug::TraceOptions(
On 2014/07/31 19:26:43, willchan wrote:
> Nit: I'd ditch the base::debug::. You're already in that namespace.

Done.

https://codereview.chromium.org/425593002/diff/590001/base/debug/trace_event_...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/425593002/diff/590001/base/debug/trace_event_...
base/debug/trace_event_unittest.cc:86: base::debug::TraceLog::RECORDING_MODE,
On 2014/07/31 19:26:43, willchan wrote:
> Nit: ditch base::debug here.

Done.

[nednguyen, 2014-08-04 02:19:33]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-08-04 02:20:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nednguyen@google.com/425593002/630001

[nednguyen, 2014-08-04 05:00:30]

The CQ bit was unchecked by nednguyen@google.com

[nednguyen, 2014-08-04 05:01:20]

+cevans for change to tracing_messages.h

PTAL

[jochen (gone - plz use gerrit), 2014-08-04 06:54:19]

sorry, I failed to publish my drafts...

https://codereview.chromium.org/425593002/diff/590001/components/feedback/tra...
File components/feedback/tracing_manager.cc (right):

https://codereview.chromium.org/425593002/diff/590001/components/feedback/tra...
components/feedback/tracing_manager.cc:94: base::debug::CategoryFilter(""),
should be std::string() instead of ""

https://codereview.chromium.org/425593002/diff/590001/content/browser/tracing...
File content/browser/tracing/tracing_controller_browsertest.cc (right):

https://codereview.chromium.org/425593002/diff/590001/content/browser/tracing...
content/browser/tracing/tracing_controller_browsertest.cc:130:
CategoryFilter(""), TraceOptions(), callback);
std::string()

https://codereview.chromium.org/425593002/diff/590001/content/browser/tracing...
content/browser/tracing/tracing_controller_browsertest.cc:284:
CategoryFilter(""),
std::string()

[nednguyen, 2014-08-04 13:58:36]

On 2014/08/04 06:54:19, jochen wrote:
> sorry, I failed to publish my drafts...
> 
>
https://codereview.chromium.org/425593002/diff/590001/components/feedback/tra...
> File components/feedback/tracing_manager.cc (right):
> 
>
https://codereview.chromium.org/425593002/diff/590001/components/feedback/tra...
> components/feedback/tracing_manager.cc:94: base::debug::CategoryFilter(""),
> should be std::string() instead of ""
> 
>
https://codereview.chromium.org/425593002/diff/590001/content/browser/tracing...
> File content/browser/tracing/tracing_controller_browsertest.cc (right):
> 
>
https://codereview.chromium.org/425593002/diff/590001/content/browser/tracing...
> content/browser/tracing/tracing_controller_browsertest.cc:130:
> CategoryFilter(""), TraceOptions(), callback);
> std::string()
> 
>
https://codereview.chromium.org/425593002/diff/590001/content/browser/tracing...
> content/browser/tracing/tracing_controller_browsertest.cc:284:
> CategoryFilter(""),
> std::string()

I will just make a default constructor that initialize CategoryFilter with
kDefaultCategoryFilterString instead.

[nednguyen, 2014-08-04 14:20:12]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-08-04 14:20:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nednguyen@google.com/425593002/650001

[commit-bot: I haz the power, 2014-08-04 16:31:02]

Change committed as 287348