Category group support/Renamings.

Category group support/Renamings.

Related review:
https://codereview.chromium.org/11823016/

BUG=168284
TEST=TraceEventTestFixture.Categories, TraceEventTestFixture.CategoryFilter
R=nduca

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

[nduca, 2013-02-21 06:02:36]

This is looking really good! Some minor tweaks and I think we're good to go,
actually. Dan, can you look at this?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_and...
File base/debug/trace_event_android.cc (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_and...
base/debug/trace_event_android.cc:53: std::string out =
StringPrintf("B|%d|%s-%s", getpid(), category_group,
style nit - you could wrap this at the opening ( and then indent 4 to keep all
this on one line. chrome style is annoyingly vague on when to do one vs the
other, but ~eh~

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:58: const char*
g_category_groups[TRACE_EVENT_MAX_CATEGORIES] = {
MAX_CATEGORY_GROUPS

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:393: g_category_group_enabled);
check your indentation throughout, as noted before. 4 in or matching previous
line's (. Chrome style http://dev.chromium.org/developers/coding-style

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:404: void
TraceLog::EnableIncludedCategoryGroup(int category_index,
any reason we're passing the filter when this is a member function?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:502: // Pass 1: Tokenize list of categories,
delimited by ','.
There's only one pass....?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:543: void CategoryFilter::ToString(std::string*
filter_string) const {
not sure that this makes sense... or works in all cases? you'd end up with a
trailing , all the time? Why not just build a vector of strings. Then append all
the excudeds with - prefixes, then the includes, then use StringUtils join to
actually create the joined string?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:549: const {
indentation

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:571: excluded_.insert(excluded_.end(),
indentation is funky

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:580: void CategoryFilter::Clear() {
what use case necessitated this?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:587: // Actually enable/disable the categories.
categyr groups

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:593: if (category_filter_.HasIncludedCategories()
&&
Can this just be part of merge? E.g. make merge do its best, and internally do
all this logic?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:181: // Example:
SetEnabled("-excluded_category1,-excluded_category2");
can you add an example of how to disable everything but 1 thing? E.g. -*,foo
 would catch ONLY the foo category, right?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:199: bool HasIncludedCategories() const;
Not sure I get what this function does by its name....  I think the word
"included" is a bit too implementation specific.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:201: // Clears both included/excluded pattern
lists.
So then what does it default to? kDefaultCategoryFilterString, or ""? We should
be very very explicit.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:239: // meaningful if |IsEnabled()| is true.
Dont like that this returns a non-const reference. It seems very dangerous to
allow this to be mutated on the fly?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:369: const CategoryFilter& category_filter);
indent is funky here. Indent to 4 lines when you wrap, if indenting to the
column after the open ( doesn't fit.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_int...
File base/debug/trace_event_internal.h (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_int...
base/debug/trace_event_internal.h:28: // We can enable/disable tracing of
ImportantEvent by enabling/disabling either
ImportantEvent -> OnMouseOver?

[dsinclair, 2013-02-22 21:13:44]

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:421: DCHECK(!strchr(category_group, '"')) <<
Can we do this before we lock?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:422: "Category group names may not contain double
quote";
contain a double

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:422: "Category group names may not contain double
quote";
Why can't it contain a quote? Can it contain a , or blank?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:516: if (is_included)
Instead of storing is_included, why not just do the excluded_.push_back above
and add an else included_.push_back?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:539: StringAppendF(out, "-%s,", ci->c_str());
StringAppendF(out, "%s%s,", (included ? "" : "-"), ci->c_str())?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:169: public:
nit: public should be indented 1 instead of 2.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:181: // Example:
SetEnabled("-excluded_category1,-excluded_category2");
Should these be s/SetEnabled/CategoryFilter/?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:189: void ToString(std::string* filter_string)
const;
Why write to an input parameter instead of just returning?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:204: private:
private should be indented 1.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:211: };
Does this need a DISALLOW_COPY_AND_ASSIGN(CategoryFilter)?

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:292: const unsigned char* category_group_enabled);
nit: This should be indented 4 from static on the line above.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:299: const char* category_group,
nit: indenting.

https://codereview.chromium.org/12150004/diff/7001/base/test/trace_event_anal...
File base/test/trace_event_analyzer_unittest.cc (right):

https://codereview.chromium.org/12150004/diff/7001/base/test/trace_event_anal...
base/test/trace_event_analyzer_unittest.cc:43:
base::debug::CategoryFilter("*"));
nit: indenting.

https://codereview.chromium.org/12150004/diff/7001/chrome/test/base/tracing.cc
File chrome/test/base/tracing.cc (right):

https://codereview.chromium.org/12150004/diff/7001/chrome/test/base/tracing.c...
chrome/test/base/tracing.cc:140: category_patterns);
nit: indenting.

https://codereview.chromium.org/12150004/diff/7001/components/tracing/child_t...
File components/tracing/child_trace_message_filter.cc (right):

https://codereview.chromium.org/12150004/diff/7001/components/tracing/child_t...
components/tracing/child_trace_message_filter.cc:49: std::string
category_filter_str,
nit: indenting.

https://codereview.chromium.org/12150004/diff/7001/content/app/android/librar...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/12150004/diff/7001/content/app/android/librar...
content/app/android/library_loader_hooks.cc:44: base::debug::CategoryFilter
category_filter(command_line->
I'd wrap from just after the ( and indent 4 from base below.

https://codereview.chromium.org/12150004/diff/7001/content/app/content_main_r...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/12150004/diff/7001/content/app/content_main_r...
content/app/content_main_runner.cc:639:
GetSwitchValueASCII(switches::kTraceStartup));
nit: wrap from (.

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
File content/browser/tracing/trace_controller_impl.cc (right):

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
content/browser/tracing/trace_controller_impl.cc:110: TraceSubscriber*
subscriber) {
nit: indenting.

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
content/browser/tracing/trace_controller_impl.cc:124: const std::string&
category_patterns) {
nit: indenting.

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
content/browser/tracing/trace_controller_impl.cc:277: new
base::RefCountedString();
nit: indenting.

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
content/browser/tracing/trace_controller_impl.cc:296:
known_category_groups.end());
nit: indenting.

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
File content/browser/tracing/trace_message_filter.cc (right):

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
content/browser/tracing/trace_message_filter.cc:58: const std::string&
category_filter_str) {
nit: indenting.

https://codereview.chromium.org/12150004/diff/7001/ppapi/shared_impl/ppb_trac...
File ppapi/shared_impl/ppb_trace_event_impl.cc (right):

https://codereview.chromium.org/12150004/diff/7001/ppapi/shared_impl/ppb_trac...
ppapi/shared_impl/ppb_trace_event_impl.cc:27:
GetCategoryGroupEnabled(category_name)));
Not sure what the indenting should be here? Maybe check the styleguide?

[rterrazas, 2013-02-25 05:55:02]

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_and...
File base/debug/trace_event_android.cc (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_and...
base/debug/trace_event_android.cc:53: std::string out =
StringPrintf("B|%d|%s-%s", getpid(), category_group,
On 2013/02/21 06:02:36, nduca wrote:
> style nit - you could wrap this at the opening ( and then indent 4 to keep all
> this on one line. chrome style is annoyingly vague on when to do one vs the
> other, but ~eh~

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:58: const char*
g_category_groups[TRACE_EVENT_MAX_CATEGORIES] = {
On 2013/02/21 06:02:36, nduca wrote:
> MAX_CATEGORY_GROUPS

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:393: g_category_group_enabled);
On 2013/02/21 06:02:36, nduca wrote:
> check your indentation throughout, as noted before. 4 in or matching previous
> line's (. Chrome style http://dev.chromium.org/developers/coding-style

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:393: g_category_group_enabled);
On 2013/02/21 06:02:36, nduca wrote:
> check your indentation throughout, as noted before. 4 in or matching previous
> line's (. Chrome style http://dev.chromium.org/developers/coding-style

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:404: void
TraceLog::EnableIncludedCategoryGroup(int category_index,
On 2013/02/21 06:02:36, nduca wrote:
> any reason we're passing the filter when this is a member function?

It's static function, at the time I liked it better that way, but it makes sense
to make it a member function.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:421: DCHECK(!strchr(category_group, '"')) <<
On 2013/02/22 21:13:44, dsinclair wrote:
> Can we do this before we lock?

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:422: "Category group names may not contain double
quote";
On 2013/02/22 21:13:44, dsinclair wrote:
> Why can't it contain a quote? Can it contain a , or blank?

The double quote restriction must be related to the JSON string that's written
for TEV, I'm pretty sure it would mess up parsing of that string.

As for commas, yeah, that's the category delimiter in a category group, and I
don't see why we wouldn't allow an empty string, however, that wouldn't be
meaningful, I think.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:502: // Pass 1: Tokenize list of categories,
delimited by ','.
On 2013/02/21 06:02:36, nduca wrote:
> There's only one pass....?

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:516: if (is_included)
Like.

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:539: StringAppendF(out, "-%s,", ci->c_str());
On 2013/02/22 21:13:44, dsinclair wrote:
> StringAppendF(out, "%s%s,", (included ? "" : "-"), ci->c_str())?

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:543: void CategoryFilter::ToString(std::string*
filter_string) const {
In Initialize, I ignored empty category patterns (and still do, I think it's
safer) that would result from this.

I've fixed the problem of having a trailing comma at the cost of a tiny bit more
complexity.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:549: const {
On 2013/02/21 06:02:36, nduca wrote:
> indentation

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:571: excluded_.insert(excluded_.end(),
On 2013/02/21 06:02:36, nduca wrote:
> indentation is funky

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:580: void CategoryFilter::Clear() {
On 2013/02/21 06:02:36, nduca wrote:
> what use case necessitated this?

SetEnabled() // If either old or new included categories are empty, allow all
events.

Did this in order to keep equivalent behavior to the previous approach.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:593: if (category_filter_.HasIncludedCategories()
&&
We could do something like MergeIfNeeded and ClearIfNeeded, but I actually
prefer the dumb merge/clear, and keep the conditional here just to be explicit
of when this happens.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:169: public:
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: public should be indented 1 instead of 2.

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:181: // Example:
SetEnabled("-excluded_category1,-excluded_category2");
On 2013/02/22 21:13:44, dsinclair wrote:
> Should these be s/SetEnabled/CategoryFilter/?

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:181: // Example:
SetEnabled("-excluded_category1,-excluded_category2");
On 2013/02/21 06:02:36, nduca wrote:
> can you add an example of how to disable everything but 1 thing? E.g. -*,foo
>  would catch ONLY the foo category, right?

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:189: void ToString(std::string* filter_string)
const;
On 2013/02/22 21:13:44, dsinclair wrote:
> Why write to an input parameter instead of just returning?

Did that to be consistent with the approach used when building the JSON string
that's read by TEV.

I believe we use that approach for that piece of the code to avoid copying that
string, which can get really large, however in this case, I believe what you're
suggesting looks prettier and the string that we'll be writing is smallish.

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:199: bool HasIncludedCategories() const;
On 2013/02/21 06:02:36, nduca wrote:
> Not sure I get what this function does by its name....  I think the word
> "included" is a bit too implementation specific.

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:201: // Clears both included/excluded pattern
lists.
On 2013/02/21 06:02:36, nduca wrote:
> So then what does it default to? kDefaultCategoryFilterString, or ""? We
should
> be very very explicit.

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:204: private:
On 2013/02/22 21:13:44, dsinclair wrote:
> private should be indented 1.

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:211: };
On 2013/02/22 21:13:44, dsinclair wrote:
> Does this need a DISALLOW_COPY_AND_ASSIGN(CategoryFilter)?

Nope, we can't do that as we do assign to copy the object through stackframes.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:239: // meaningful if |IsEnabled()| is true.
On 2013/02/21 06:02:36, nduca wrote:
> Dont like that this returns a non-const reference. It seems very dangerous to
> allow this to be mutated on the fly?

Right, that wasn't by design, that was a mistake I made. Thanks for pointing it
out!.
Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:292: const unsigned char* category_group_enabled);
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: This should be indented 4 from static on the line above.

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:299: const char* category_group,
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: indenting.

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:369: const CategoryFilter& category_filter);
On 2013/02/21 06:02:36, nduca wrote:
> indent is funky here. Indent to 4 lines when you wrap, if indenting to the
> column after the open ( doesn't fit.

Done.

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_int...
File base/debug/trace_event_internal.h (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_int...
base/debug/trace_event_internal.h:28: // We can enable/disable tracing of
ImportantEvent by enabling/disabling either
On 2013/02/21 06:02:36, nduca wrote:
> ImportantEvent -> OnMouseOver?

Done.

https://codereview.chromium.org/12150004/diff/7001/chrome/test/base/tracing.cc
File chrome/test/base/tracing.cc (right):

https://codereview.chromium.org/12150004/diff/7001/chrome/test/base/tracing.c...
chrome/test/base/tracing.cc:140: category_patterns);
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: indenting.

Done.

https://codereview.chromium.org/12150004/diff/7001/components/tracing/child_t...
File components/tracing/child_trace_message_filter.cc (right):

https://codereview.chromium.org/12150004/diff/7001/components/tracing/child_t...
components/tracing/child_trace_message_filter.cc:49: std::string
category_filter_str,
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: indenting.

Done.

https://codereview.chromium.org/12150004/diff/7001/content/app/android/librar...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/12150004/diff/7001/content/app/android/librar...
content/app/android/library_loader_hooks.cc:44: base::debug::CategoryFilter
category_filter(command_line->
On 2013/02/22 21:13:44, dsinclair wrote:
> I'd wrap from just after the ( and indent 4 from base below.

Done.

https://codereview.chromium.org/12150004/diff/7001/content/app/content_main_r...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/12150004/diff/7001/content/app/content_main_r...
content/app/content_main_runner.cc:639:
GetSwitchValueASCII(switches::kTraceStartup));
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: wrap from (.

Done.

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
File content/browser/tracing/trace_controller_impl.cc (right):

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
content/browser/tracing/trace_controller_impl.cc:110: TraceSubscriber*
subscriber) {
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: indenting.

Done.

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
content/browser/tracing/trace_controller_impl.cc:124: const std::string&
category_patterns) {
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: indenting.

Done.

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
content/browser/tracing/trace_controller_impl.cc:124: const std::string&
category_patterns) {
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: indenting.

Done.

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
content/browser/tracing/trace_controller_impl.cc:296:
known_category_groups.end());
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: indenting.

Done.

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
File content/browser/tracing/trace_message_filter.cc (right):

https://codereview.chromium.org/12150004/diff/7001/content/browser/tracing/tr...
content/browser/tracing/trace_message_filter.cc:58: const std::string&
category_filter_str) {
On 2013/02/22 21:13:44, dsinclair wrote:
> nit: indenting.

Done.

https://codereview.chromium.org/12150004/diff/7001/ppapi/shared_impl/ppb_trac...
File ppapi/shared_impl/ppb_trace_event_impl.cc (right):

https://codereview.chromium.org/12150004/diff/7001/ppapi/shared_impl/ppb_trac...
ppapi/shared_impl/ppb_trace_event_impl.cc:27:
GetCategoryGroupEnabled(category_name)));
On 2013/02/22 21:13:44, dsinclair wrote:
> Not sure what the indenting should be here? Maybe check the styleguide?

Done.

[rterrazas, 2013-02-25 05:57:04]

Also, while uploading, the presubmits complained about some files w/o owners,
and suggested darin@chromium.org.

[rterrazas, 2013-02-25 06:02:44]

And also, hope you don't mind, I rearranged a few things in trace_event_impl,
because applying patches the way things were arranged was a nightmare.

[rterrazas, 2013-02-25 06:05:44]

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12150004/diff/7001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:211: };
However, I did overload = and provided an explicit copy constructor, as per the
guidelines.

[rterrazas, 2013-03-04 16:55:46]

https://codereview.chromium.org/12150004/diff/22001/base/debug/trace_event_im...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12150004/diff/22001/base/debug/trace_event_im...
base/debug/trace_event_impl.h:139: // Example: CategoryFilter("*,-webkit");
would enable everything but webkit.
^Self picked nit:
Change to->
Example: CategoryFilter("-webkit"); would enable everything but webkit.

[nduca, 2013-03-12 19:49:39]

How're we doing on this? @dsinclair, can you do a review on this?

[dsinclair, 2013-03-12 21:27:24]

Looking pretty good, mostly nits.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:577: g_category_group_enabled +
MAX_CATEGORY_GROUPS)) <<
Indenting, should be in 4 more.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:926: category_name.c_str());
nit: indenting.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1017: category_group_token.at(0) != ' '&&
nit: Space before &&

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1047: if(category.empty())
nit: space before (

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1050: if (category.at(0) == '-') {
I'm not seeing on a quick check, does StringTokenizer strip the string for us?
If I pass in "Foo, Bar, -Baz" will this correctly pickup the - on baz or will it
match the ' ' that is left by the tokenizer?

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1076: if (token_cnt++ > 0 ||
prepend_comma_for_first_excluded)
Can we move the token_cnt++ out to after the if statement, makes it clearer
what's being checked.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1118: bool CategoryFilter::HasAllowedPatterns()
const {
Would HasIncludedPatterns be a better name then Allowed? Reading the header file
I assumed this would check if you have included_ or excluded_ patterns.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.h:141: CategoryFilter(const std::string&
filter_string);
explicit CategoryFilter(const std::string& filter_string);

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.h:143: CategoryFilter(const CategoryFilter& cf)
The chromium style is to avoid overloading functions unless you can tell what
will happen at the call site:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

Not sure if these violate or not, nduca?

Should we remove the no args constructor and just have call sites pass in
kDefaultCategoryFilterString so it's obvious?

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.h:165: // disabled by this filter.
disabled by this category filter.

On first reading I thought you'd be passing a filter into the method.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_un...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_un...
base/debug/trace_event_unittest.cc:865: //Category groups containing more than
one category.
nit: space after //

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_un...
base/debug/trace_event_unittest.cc:1637: 
Nit: extra blank line.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_un...
base/debug/trace_event_unittest.cc:1657: CategoryFilter
cf("included,-excluded,inc_pattern*,-exc_pattern*");
Can you add some spacing in here to make sure the match works.

("   blah");
("  ,  foo,    -blah   ")

That kind of thing.

https://codereview.chromium.org/12150004/diff/50001/components/tracing/child_...
File components/tracing/child_trace_message_filter.cc (right):

https://codereview.chromium.org/12150004/diff/50001/components/tracing/child_...
components/tracing/child_trace_message_filter.cc:48: void
ChildTraceMessageFilter::OnBeginTracing(std::string category_filter_str,
const std::string&

https://codereview.chromium.org/12150004/diff/50001/components/tracing/child_...
File components/tracing/child_trace_message_filter.h (right):

https://codereview.chromium.org/12150004/diff/50001/components/tracing/child_...
components/tracing/child_trace_message_filter.h:31: void
OnBeginTracing(std::string category_filter_str,
const std::string& category_filter_str?

https://codereview.chromium.org/12150004/diff/50001/content/browser/tracing/t...
File content/browser/tracing/trace_controller_impl.cc (right):

https://codereview.chromium.org/12150004/diff/50001/content/browser/tracing/t...
content/browser/tracing/trace_controller_impl.cc:253:
filter->SendBeginTracing(cf_str, trace_options_);
Do you need to store cf_str or can you do
filter->SendBeginTracing(category_filter.ToString(), trace_options_)

[nduca, 2013-03-13 01:46:32]

Wrt the default constructor, ~shrug~ i'm fine with anything.

[rterrazas, 2013-03-20 08:48:49]

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:577: g_category_group_enabled +
MAX_CATEGORY_GROUPS)) <<
On 2013/03/12 21:27:24, dsinclair wrote:
> Indenting, should be in 4 more.

Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:926: category_name.c_str());
On 2013/03/12 21:27:24, dsinclair wrote:
> nit: indenting.

Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1017: category_group_token.at(0) != ' '&&
On 2013/03/12 21:27:24, dsinclair wrote:
> nit: Space before &&

Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1047: if(category.empty())
On 2013/03/12 21:27:24, dsinclair wrote:
> nit: space before (

Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1050: if (category.at(0) == '-') {
On 2013/03/12 21:27:24, dsinclair wrote:
> I'm not seeing on a quick check, does StringTokenizer strip the string for us?
> If I pass in "Foo, Bar, -Baz" will this correctly pickup the - on baz or will
it
> match the ' ' that is left by the tokenizer?

It does not, we'd need to strip the string of whitespace, I had that actually,
but Nat suggested that we remove that trimming and force that we don't have
leading/trailing whitespace (which is why I added the DCHECK in
DoesCategoryGroupContainCategory).

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1076: if (token_cnt++ > 0 ||
prepend_comma_for_first_excluded)
On 2013/03/12 21:27:24, dsinclair wrote:
> Can we move the token_cnt++ out to after the if statement, makes it clearer
> what's being checked.

Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1118: bool CategoryFilter::HasAllowedPatterns()
const {
Yeah, I think had named it originally to HasIncludedPatterns(), trying to be
very explicit about what was being checked. Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.h:141: CategoryFilter(const std::string&
filter_string);
On 2013/03/12 21:27:24, dsinclair wrote:
> explicit CategoryFilter(const std::string& filter_string);

Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.h:143: CategoryFilter(const CategoryFilter& cf)
On 2013/03/12 21:27:24, dsinclair wrote:
> The chromium style is to avoid overloading functions unless you can tell what
> will happen at the call site:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...
> 
> Not sure if these violate or not, nduca?
> 
> Should we remove the no args constructor and just have call sites pass in
> kDefaultCategoryFilterString so it's obvious?

Like, reads better that way. Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_im...
base/debug/trace_event_impl.h:165: // disabled by this filter.
On 2013/03/12 21:27:24, dsinclair wrote:
> disabled by this category filter.
> 
> On first reading I thought you'd be passing a filter into the method.

Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_un...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_un...
base/debug/trace_event_unittest.cc:865: //Category groups containing more than
one category.
On 2013/03/12 21:27:24, dsinclair wrote:
> nit: space after //

Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_un...
base/debug/trace_event_unittest.cc:1637: 
On 2013/03/12 21:27:24, dsinclair wrote:
> Nit: extra blank line.

Done.

https://codereview.chromium.org/12150004/diff/50001/base/debug/trace_event_un...
base/debug/trace_event_unittest.cc:1657: CategoryFilter
cf("included,-excluded,inc_pattern*,-exc_pattern*");
Actually, I had whitespace trimming in another changelist, but Nat asked me to
forbid that (for reference: https://codereview.chromium.org/11823016/#msg9).

I believe his concern was doing whitespace trimming in this code path. I do
agree that we needed tests for that, so I exposed a function
IsEmptyOrContainsLeadingOrTrailingWhitespace() and DCHECK against the negation
of that.

https://codereview.chromium.org/12150004/diff/50001/components/tracing/child_...
File components/tracing/child_trace_message_filter.cc (right):

https://codereview.chromium.org/12150004/diff/50001/components/tracing/child_...
components/tracing/child_trace_message_filter.cc:48: void
ChildTraceMessageFilter::OnBeginTracing(std::string category_filter_str,
On 2013/03/12 21:27:24, dsinclair wrote:
> const std::string&

Done.

https://codereview.chromium.org/12150004/diff/50001/components/tracing/child_...
File components/tracing/child_trace_message_filter.h (right):

https://codereview.chromium.org/12150004/diff/50001/components/tracing/child_...
components/tracing/child_trace_message_filter.h:31: void
OnBeginTracing(std::string category_filter_str,
On 2013/03/12 21:27:24, dsinclair wrote:
> const std::string& category_filter_str?

Done.

https://codereview.chromium.org/12150004/diff/50001/content/browser/tracing/t...
File content/browser/tracing/trace_controller_impl.cc (right):

https://codereview.chromium.org/12150004/diff/50001/content/browser/tracing/t...
content/browser/tracing/trace_controller_impl.cc:253:
filter->SendBeginTracing(cf_str, trace_options_);
Nope, left it there by mistake. Done.

[dsinclair, 2013-03-21 18:18:52]

LGTM with one minor nit.

https://codereview.chromium.org/12150004/diff/96004/chrome/test/pyautolib/pya...
File chrome/test/pyautolib/pyautolib.i (right):

https://codereview.chromium.org/12150004/diff/96004/chrome/test/pyautolib/pya...
chrome/test/pyautolib/pyautolib.i:133: %feature("docstring", "Begins tracing
with the given category_patterns"
Need a space after patterns.

[rterrazas, 2013-03-21 18:29:43]

Thanks for the review! :) Should I wait for Nat or someone else's approval or
can I rebase, fix the nit and commit?.

[dennis_jeffrey, 2013-03-21 18:33:45]

On 2013/03/21 18:29:43, rterrazas wrote:
> Thanks for the review! :) Should I wait for Nat or someone else's approval or
> can I rebase, fix the nit and commit?.

LGTM for chrome/test/pyautolib/pyautolib.i (after you address the earlier nit),
in case you need OWNERS approval for that.

[dsinclair, 2013-03-21 18:34:27]

On 2013/03/21 18:29:43, rterrazas wrote:
> Thanks for the review! :) Should I wait for Nat or someone else's approval or
> can I rebase, fix the nit and commit?.

I don't think you'll be able to commit at this point. You're going to need
OWNERS permission for a bunch of those directories. I believe it gives some
suggestions when you upload the cl, or, for each directory look for an OWNERS
file, if not there look in the parent directory, pick someone from each and add
them as a reviewer.

When you add them as a reviewer put a note in the message box for which
directory you're looking for them to review. (You can do this in one message as
a@ please look at foo; b@ please look at bar.)

[nduca, 2013-03-21 19:07:50]

LGTM on my part, good job. I have added OWNERS for review.

[nduca, 2013-03-21 19:12:24]

Actually, +jar for base OWNERS review.

When @jar is done, lets add these folks for review of the textual edits
elsewhere: @jam, @brettw, @jamesr

[rterrazas, 2013-03-22 03:58:42]

Fixed nit.

jar@ can you please review for base?.

Thanks!.

https://codereview.chromium.org/12150004/diff/96004/chrome/test/pyautolib/pya...
File chrome/test/pyautolib/pyautolib.i (right):

https://codereview.chromium.org/12150004/diff/96004/chrome/test/pyautolib/pya...
chrome/test/pyautolib/pyautolib.i:133: %feature("docstring", "Begins tracing
with the given category_patterns"
On 2013/03/21 18:18:53, dsinclair wrote:
> Need a space after patterns.

Done.

[nduca, 2013-03-30 22:52:31]

@jar, ping

[jar (doing other things), 2013-03-31 15:14:07]

base in patch set 9 LGTM with tiny nits below

https://codereview.chromium.org/12150004/diff/85011/base/debug/trace_event_im...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12150004/diff/85011/base/debug/trace_event_im...
base/debug/trace_event_impl.h:228: const std::string str);
nit: should this be a const ref?

https://codereview.chromium.org/12150004/diff/85011/base/debug/trace_event_im...
base/debug/trace_event_impl.h:236: };
nit: can we have a DISALLOW_COPY_AND_ASSIGN, or is this used in stl containers?

https://codereview.chromium.org/12150004/diff/85011/base/debug/trace_event_un...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/12150004/diff/85011/base/debug/trace_event_un...
base/debug/trace_event_unittest.cc:1664: category_filter_str.c_str());
nit: undent 1 char

[rterrazas, 2013-04-15 01:41:11]

https://codereview.chromium.org/12150004/diff/85011/base/debug/trace_event_im...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12150004/diff/85011/base/debug/trace_event_im...
base/debug/trace_event_impl.h:228: const std::string str);
On 2013/03/31 15:14:08, jar wrote:
> nit: should this be a const ref?

Done.

https://codereview.chromium.org/12150004/diff/85011/base/debug/trace_event_im...
base/debug/trace_event_impl.h:236: };
On 2013/03/31 15:14:08, jar wrote:
> nit: can we have a DISALLOW_COPY_AND_ASSIGN, or is this used in stl
containers?

Nope, it's created/copied in other classes.

https://codereview.chromium.org/12150004/diff/85011/base/debug/trace_event_un...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/12150004/diff/85011/base/debug/trace_event_un...
base/debug/trace_event_unittest.cc:1664: category_filter_str.c_str());
On 2013/03/31 15:14:08, jar wrote:
> nit: undent 1 char

Done.

[dsinclair, 2013-04-16 19:28:48]

brettw, can you please take a look for chrome/, content/, ppapi/, and webkit/
OWNERS.

apatrick, can you please take a look for gpu/ OWNERS.

[brettw, 2013-04-17 05:46:53]

lgtm

[apatrick_chromium, 2013-04-18 20:05:01]

gpu/command_buffer/service/gpu_tracer.cc LGTM

[commit-bot: I haz the power, 2013-04-18 20:43:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rubentopo@gmail.com/12150004/154002

[rterrazas, 2013-04-18 20:43:30]

Thank you all for the reviews! :)

[commit-bot: I haz the power, 2013-04-18 20:43:38]

Presubmit check for 12150004-154002 failed and returned exit status 1.

INFO:root:Found 35 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/base/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ppapi/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    components/tracing/tracing_messages.h

Presubmit checks took 3.8s to calculate.

[nduca, 2013-04-18 20:46:27]

You'll need a security team review on that file.

[dsinclair, 2013-04-18 20:48:52]

cdn, can you please take a look for components/tracing/tracing_messages.h
OWNERS.

Thanks,
dan

[Cris Neckar, 2013-04-18 20:56:06]

ipc security review lgtm

[commit-bot: I haz the power, 2013-04-18 21:01:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rubentopo@gmail.com/12150004/154002

[commit-bot: I haz the power, 2013-04-18 21:02:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rubentopo@gmail.com/12150004/154002

[commit-bot: I haz the power, 2013-04-18 21:13:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
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-04-19 00:46:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rubentopo@gmail.com/12150004/199001

[commit-bot: I haz the power, 2013-04-19 08:49:07]

Change committed as 195109