Trace category groups and category filter.

Support for tracing category groups.
Added support for trace category groups, and changes to simplify the filtering logic.

Stylistic changes found in:
https://codereview.chromium.org/12150004

BUG=168284
TEST=

[nduca, 2013-01-09 09:43:33]

a great first start. figure out how to make it clean, start clarifying naming,
add docs in the _impl.h or _impl.cc too to explain tag versus category
distinction. You're also going to have to edit the callers of Enable() to say
tags instead of categories --- e.g. tracing_controller. Git grep for TraceLog in
the toplevel directory, or temporarily rename Enable to EnableX so that you can
find all the places that need updating.

+caseq who is working on trace_event as well. Andrey, there's some context on
the associated BUG= for you to puruse. Wdyt?

https://codereview.chromium.org/11823016/diff/1/base/debug/trace_event_impl.cc
File base/debug/trace_event_impl.cc (left):

https://codereview.chromium.org/11823016/diff/1/base/debug/trace_event_impl.c...
base/debug/trace_event_impl.cc:396: static void EnableMatchingCategory(int
category_index,
I'd like to update the naming convention throughout this file to be more obvious
what a given std::string is. FOr exmaple you have patterns passed in here --- is
that patterns on tags, or patterns on categories?

https://codereview.chromium.org/11823016/diff/1/base/debug/trace_event_impl.c...
base/debug/trace_event_impl.cc:404: if (is_match)
cant you make this just a change to MatchPattern? This code is insanely hard to
read. Figure out some way to make this read cleanly!

Maybe MatchPattern should be more like IsTagPatternAMatchForCategoryString()?

[rterrazas, 2013-01-15 08:08:31]

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

https://codereview.chromium.org/11823016/diff/4001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:396: static bool
DoesCategoryContainMatchingTag(const char* tagged_category,
Implemented the support for multi tag and single tag categories, per your
suggestion, found this much cleaner.

Trimming tokens to allow whitespace between tags, seems better than having
people waste time by debugging errors caused by not supporting this.

https://codereview.chromium.org/11823016/diff/4001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:499: const std::vector<std::string>&
excluded_tag_patterns) {
Crossed the 80 char barrier, seemed moving the parameters down seemed a bit too
ugly. 

What do you think?

[rterrazas, 2013-01-15 08:11:48]

https://codereview.chromium.org/11823016/diff/4001/content/browser/devtools/d...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/11823016/diff/4001/content/browser/devtools/d...
content/browser/devtools/devtools_tracing_handler.cc:64: if (params &&
params->HasKey("categories"))
Didn't rename key because I couldn't find where it's set using git grep, maybe
it's in other repo?. Should I even worry about that key?.

[rterrazas, 2013-01-18 01:22:05]

https://codereview.chromium.org/11823016/diff/11001/content/browser/devtools/...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/11823016/diff/11001/content/browser/devtools/...
content/browser/devtools/devtools_tracing_handler.cc:64: if (params &&
params->HasKey(kCategoriesParam))
I believe the value of kCategoriesParam comes from trace-event-viewer correct?,
do you think we'd need to change it to "tag_patterns" for consistency?, thinking
yes, but I'd rather ask.

[rterrazas, 2013-01-18 01:25:35]

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

https://codereview.chromium.org/11823016/diff/4001/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:499: const std::vector<std::string>&
excluded_tag_patterns) {
On 2013/01/15 08:08:31, rterrazas wrote:
> Crossed the 80 char barrier, seemed moving the parameters down seemed a bit
too
> ugly. 
> 
> What do you think?

Done.

[rterrazas, 2013-01-18 17:44:18]

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

https://codereview.chromium.org/11823016/diff/11001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:404: TrimWhitespaceASCII(tag_tokens.token(),
TRIM_ALL, &trimmed_tag);
I'm thinking that maybe checking if we have leading or trailing whitespace
before trimming the string, just to save us a bit of time if not.

[rterrazas, 2013-01-21 06:59:53]

https://codereview.chromium.org/11823016/diff/11001/content/browser/devtools/...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/11823016/diff/11001/content/browser/devtools/...
content/browser/devtools/devtools_tracing_handler.cc:64: if (params &&
params->HasKey(kCategoriesParam))
Duh, I was wrong, it seems like this stuff was introduced here:
https://chromiumcodereview.appspot.com/11548032

However, I can't find a call to SetString for kCategoriesParam nor "categories"
which leads me to believe that maybe that isn't implemented yet?, does this
sound right?.

[nduca, 2013-01-28 08:05:20]

Can you tweak it so that categories have no spaces?

foo,bar is good

foo, bar  is bad

[nduca, 2013-01-28 08:15:28]

https://codereview.chromium.org/11823016/diff/11001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/11823016/diff/11001/base/debug/trace_event.h#...
base/debug/trace_event.h:22: // A category can be made up of several tags, for
example:
The first argument to traces is a comma-separated list of categories, for
example:

Sorry to change my mind on this since the last review, but now that I see this
patch, its starting to look clearer. I think we should try to go through and
where we say Category, instead say CategoryGroup. Category just means "tag", and
we avoid the word CategoryGroup to indicate cat1,cat2 type things.

https://codereview.chromium.org/11823016/diff/11001/base/debug/trace_event.h#...
base/debug/trace_event.h:24: // TRACE_EVENT_INSTANT0("MY_SUBSYSTEM_1,
MY_SUB_SYSTEM_2", "ImportantEvent")
Lets give a concerete example here so its less abstract. So you could give an
example like "MouseMove" which is of interest to both Views and Rendering.

It is often the case that one trace may belong in multiple categories at the
same time. The first argument to the trace can be a comma-separated list of
categories, forming a category group, like so:
<example, "Input,Views" maybe.>

Once instrumente in this fashion, the trace event can be enabled or disabled by
either asking for Input, Views, or both.

https://codereview.chromium.org/11823016/diff/11001/base/debug/trace_event.h#...
base/debug/trace_event.h:123: //     TRACE_EVENT1("category_tag1", "name",
i think its safe to say category once we make the previously mentioned changes

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

https://codereview.chromium.org/11823016/diff/11001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:396: static bool
DoesCategoryContainMatchingTag(const char* tagged_category,
So this would be DoesCategoryGroupContainCategory

[nduca, 2013-01-28 08:22:28]

Hmm maybe also we should split the patch into the renaming portion and the
functional changes. E.g. most of the renames outside of base are just stylistic.

A great way to do this is to download this raw patchset to your computer, say
~/foo.patch. like so:
  curl -O https://codereview.chromium.org/download/issue11823016_11001.diff
  mv issue11823016_11001.diff ~/foo.patch
^you get the url for this from "download raw patchset" above.

Now is a good time to do bulk renamings. E.g. if I told you to rename Foo to
Bar, then open foo.patch in your editor, and do the replace in the patch file
itself.

Now its time to split the patch. First, make a new git branch that is clean:

   git checkout -t -b renamings origin/master


Now, apply the patch
   patch -p1 < ~/foo.patch

Now checkout the base/ changes:
   git checkout base/

Now you've got the rest of the changes for your other directory.  apt-get
install git gui if you're on linux, or if you're on osx, download git-osx and
use /usr/local/git/bin/git gui. This allows you to right-click on lines your
branch and individually stage them. Once all the edits you want are staged:

    git checkout . # will undo all the stuf you didn't want int eh branch.

Now git commit -m "Renaming part of my patch" and upload it.

Then, repeat the same process for your base edits.

This sounds like a pain, but once you do it once, its actually super easy and
its a really awesome skill to have, and you might even find a better way to do
it than what I suggest. Before I had a good workflow I get a request to split my
patch up, or bulk rename, I"d have a nervous breakdown because of the effort it
would require. This stil takes time, but significantly less.

LMK if you get stuck along that process.

[rterrazas, 2013-01-28 17:50:28]

On 2013/01/28 08:05:20, nduca wrote:
> Can you tweak it so that categories have no spaces?
> 
> foo,bar is good
> 
> foo, bar  is bad

Do you think we should do a DCHECK(!string has leading or trailing space)?

[nduca, 2013-01-28 18:35:35]

Sgtm

[rterrazas, 2013-02-01 05:08:15]

Just added the very basic core functionality in this changelist (sans tests, I
kept the tests in the other change-list, as I altered them according to the
renaming for easier lading).

[rterrazas, 2013-02-20 02:40:06]

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

https://codereview.chromium.org/11823016/diff/31001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:539: bool
CategoryFilter::IsCategoryGroupEnabled(const char* category_group_name)
This does a lot of the work that was done through
EnableMatching...(..,CATEGORY_ENABLED, 0)/EnableMatching...(..,0,
CATEGORY_ENABLED).

I almost did things that way, but we gained no more readability IMHO, but this
is simpler to read IMHO, anyhow...Lmkwyt.

[rterrazas, 2013-02-21 04:08:05]

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

https://codereview.chromium.org/11823016/diff/31001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:465: const char* category) {
So...I was thinking, I'd like to change this to something like
IsCategoryPatternContained(container, category_pattern) and then consume this
method from IsCategoryEnabled(...). 

wdyt?

[nduca, 2013-02-22 19:54:21]

Oh dear, now I'm confused. I left a bunch of comments on
https://codereview.chromium.org/12150004/ for your feedback. Is that the wrong
one to be reviewing?

[nduca, 2013-02-22 19:55:19]

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

https://codereview.chromium.org/11823016/diff/31001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:465: const char* category) {
This sounds like a metho don the CategoryFilter object, no?
DoesCategoryGroupContainCategory(const char* group) or something or other.

[rterrazas, 2013-02-22 20:58:57]

On 2013/02/22 19:54:21, nduca wrote:
> Oh dear, now I'm confused. I left a bunch of comments on
> https://codereview.chromium.org/12150004/ for your feedback. Is that the wrong
> one to be reviewing?

I believe you requested the split to ease your review, however I don't mind
actually, this is the core functionality patch, but I can address all comments
in the other one, and that's easier to land that way I think :)

[nduca, 2013-03-13 06:02:01]

Should we close this one and do it all as one in the other patch?

[nduca, 2013-03-13 06:02:25]

(to close, you press the little x in the top left corner of the review page to
the left of issue ####)

[rterrazas, 2013-03-13 06:03:11]

On 2013/03/13 06:02:01, nduca wrote:
> Should we close this one and do it all as one in the other patch?

Yeah, I think so, the other is a super set of this one, has been reviewed
extensively. Closing.