Enable trace with multiple categories with any one category ON

Enable trace with multiple categories with any one category ON

As per documentation   https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/trace_event.h&l=27
for any trace event which specifies category group, the trace event needs to be
recorded when either of category present in the group is enabled.

Current implementation short circuits the check for enabled category group
and returns false as soon as any category is found which is not enabled.
Rather, short circuit for returning true when any enabled category is found.
Otherwise, should check for each category present in the group until any
one enabled category is found. Return false only if no enabled category
is found.

BUG=463385
TESTS=Updated TraceEventTestFixture tests in base_unittests

Committed: https://crrev.com/2c3eb845ad50b3f7fd8f4b675c21e04cc393d9b0
Cr-Commit-Position: refs/heads/master@{#319417}

[r.kasibhatla, 2015-03-03 08:15:30]

r.kasibhatla@samsung.com changed reviewers:
+ danakj@chromium.org, dsinclair@chromium.org, nduca@chromium.org

[r.kasibhatla, 2015-03-03 08:15:48]

PTAL!!!

[dsinclair, 2015-03-03 14:54:05]

https://codereview.chromium.org/971673004/diff/60001/base/trace_event/trace_e...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/971673004/diff/60001/base/trace_event/trace_e...
base/trace_event/trace_event_impl.cc:2493: bool had_category_group_enabled =
true;
This flag is backwards to what we're checking and makes the logic hard to
follow. Can you rename it to had_category_group_disabled and update the logic
below? This check is running through the explicitly disabled categories.

Actually, can we call it 'all_category_groups_disabled' I think that would be
the clearest. Then as soon as something doesn't match in the loops below, you
can set it to true.

You then don't need the final if (!had_category_group_enabled) you can put it
right into the return as:

return !all_category_groups_disabled || (included_.empty() &&
had_enabled_by_default)

https://codereview.chromium.org/971673004/diff/60001/base/trace_event/trace_e...
base/trace_event/trace_event_impl.cc:2499: // Current token of
category_group_name is present in excluded_ list.
Nit: remove space between excluded_ and list.

https://codereview.chromium.org/971673004/diff/60001/base/trace_event/trace_e...
base/trace_event/trace_event_impl.cc:2505: } else {
Since the if () has a break, you don't need else here. We can only get here if
the if didn't execute.

[r.kasibhatla, 2015-03-04 05:06:16]

On 2015/03/03 at 14:54:05, dsinclair wrote:
>
https://codereview.chromium.org/971673004/diff/60001/base/trace_event/trace_e...
> File base/trace_event/trace_event_impl.cc (right):
> 
>
https://codereview.chromium.org/971673004/diff/60001/base/trace_event/trace_e...
> base/trace_event/trace_event_impl.cc:2493: bool had_category_group_enabled =
true;
> This flag is backwards to what we're checking and makes the logic hard to
follow. Can you rename it to had_category_group_disabled and update the logic
below? This check is running through the explicitly disabled categories.
> 
> Actually, can we call it 'all_category_groups_disabled' I think that would be
the clearest. Then as soon as something doesn't match in the loops below, you
can set it to true.
In my first patch set, I had it exactly like this but changed it in last patch
set. Not sure why. :p. Will update accordingly.

> 
> You then don't need the final if (!had_category_group_enabled) you can put it
right into the return as:
> 
> return !all_category_groups_disabled || (included_.empty() &&
had_enabled_by_default)
Done. But it will be !all_category_groups_disabled && (included_.empty() &&
had_enabled_by_default).
If all_category_groups_disabled is true => all of the categories **are present**
in excluded list. So return immediately without checking the value of
had_enabled_by_default.
If all_category_groups_disabled is false => some or all categories **are not
present** in excluded list. So, check if any of them had been enabled by
default.
> 
>
https://codereview.chromium.org/971673004/diff/60001/base/trace_event/trace_e...
> base/trace_event/trace_event_impl.cc:2499: // Current token of
category_group_name is present in excluded_ list.
> Nit: remove space between excluded_ and list.
Done.

> 
>
https://codereview.chromium.org/971673004/diff/60001/base/trace_event/trace_e...
> base/trace_event/trace_event_impl.cc:2505: } else {
> Since the if () has a break, you don't need else here. We can only get here if
the if didn't execute.
Done.

[r.kasibhatla, 2015-03-06 03:41:45]

gentle ping!!!

[dsinclair, 2015-03-06 04:04:45]

lgtm with nit.

https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
base/trace_event/trace_event_impl.cc:2503: all_category_groups_disabled = true;
This isn't really all_, it's one category group disabled isn't it?

https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
base/trace_event/trace_event_impl.cc:2521: (included_.empty() &&
had_enabled_by_default);
nit: Don't need the brackets.

[r.kasibhatla, 2015-03-06 04:10:34]

On 2015/03/06 at 04:04:45, dsinclair wrote:
> lgtm with nit.
> 
>
https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
> File base/trace_event/trace_event_impl.cc (right):
> 
>
https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
> base/trace_event/trace_event_impl.cc:2503: all_category_groups_disabled =
true;
> This isn't really all_, it's one category group disabled isn't it?
How about naming it simply as 'category_group_disabled'?

> 
>
https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
> base/trace_event/trace_event_impl.cc:2521: (included_.empty() &&
had_enabled_by_default);
> nit: Don't need the brackets.

[dsinclair, 2015-03-06 04:13:37]

On 2015/03/06 at 04:10:34, r.kasibhatla wrote:
> On 2015/03/06 at 04:04:45, dsinclair wrote:
> > lgtm with nit.
> > 
> >
https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
> > File base/trace_event/trace_event_impl.cc (right):
> > 
> >
https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
> > base/trace_event/trace_event_impl.cc:2503: all_category_groups_disabled =
true;
> > This isn't really all_, it's one category group disabled isn't it?
> How about naming it simply as 'category_group_disabled'?
> 

sounds good.

> > 
> >
https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
> > base/trace_event/trace_event_impl.cc:2521: (included_.empty() &&
had_enabled_by_default);
> > nit: Don't need the brackets.

[r.kasibhatla, 2015-03-06 04:17:54]

On 2015/03/06 at 04:13:37, dsinclair wrote:
> On 2015/03/06 at 04:10:34, r.kasibhatla wrote:
> > On 2015/03/06 at 04:04:45, dsinclair wrote:
> > > lgtm with nit.
> > > 
> > >
https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
> > > File base/trace_event/trace_event_impl.cc (right):
> > > 
> > >
https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
> > > base/trace_event/trace_event_impl.cc:2503: all_category_groups_disabled =
true;
> > > This isn't really all_, it's one category group disabled isn't it?
> > How about naming it simply as 'category_group_disabled'?
> > 
> 
> sounds good.
> 
> > > 
> > >
https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
> > > base/trace_event/trace_event_impl.cc:2521: (included_.empty() &&
had_enabled_by_default);
> > > nit: Don't need the brackets.

Done. Thanks for the review. :)

[r.kasibhatla, 2015-03-06 04:19:32]

https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
base/trace_event/trace_event_impl.cc:2503: all_category_groups_disabled = true;
On 2015/03/06 at 04:04:45, dsinclair wrote:
> This isn't really all_, it's one category group disabled isn't it?

Done.

https://codereview.chromium.org/971673004/diff/80001/base/trace_event/trace_e...
base/trace_event/trace_event_impl.cc:2521: (included_.empty() &&
had_enabled_by_default);
On 2015/03/06 at 04:04:45, dsinclair wrote:
> nit: Don't need the brackets.

Done.

[r.kasibhatla, 2015-03-06 04:20:07]

The CQ bit was checked by r.kasibhatla@samsung.com

[r.kasibhatla, 2015-03-06 04:20:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/971673004/#ps100001
(title: "Patch for landing!")

[commit-bot: I haz the power, 2015-03-06 04:20:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/971673004/100001

[commit-bot: I haz the power, 2015-03-06 06:17:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-06 06:17:55]

Patchset 6 (id:??) landed as
https://crrev.com/2c3eb845ad50b3f7fd8f4b675c21e04cc393d9b0
Cr-Commit-Position: refs/heads/master@{#319417}