tracing: split trace event filter classes out of TraceLog

tracing: split trace event filter classes out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and isolating dependencies of the tracing codebase.
This CL is a pure refactor with no intended behavioral changes.
It moves all the trace filters classes outside of the TraceLog.
This is required to make the next CLs which will change the lifetime
management of filters easier to review.

BUG=659689
Committed: https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3
Cr-Commit-Position: refs/heads/master@{#437721}

[Primiano Tucci (use gerrit), 2016-12-06 09:56:30]

The CQ bit was checked by primiano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 09:56:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Primiano Tucci (use gerrit), 2016-12-06 10:12:38]

Description was changed from

==========
tracing: split trace event filter classes out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and simplifying dependencies of the tracing codebase.
This CL is a pure refactor with no intended behavioral changes.
It moves all the trace filters classes outside of the TraceLog.
This is required to make the next CLs which will change the lifetime
management of filters easier to review.

BUG=659689
==========

to

==========
tracing: split trace event filter classes out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and isolating dependencies of the tracing codebase.
This CL is a pure refactor with no intended behavioral changes.
It moves all the trace filters classes outside of the TraceLog.
This is required to make the next CLs which will change the lifetime
management of filters easier to review.

BUG=659689
==========

[Primiano Tucci (use gerrit), 2016-12-06 10:12:38]

primiano@chromium.org changed reviewers:
+ kraynov@chromium.org, oysteine@chromium.org

[Primiano Tucci (use gerrit), 2016-12-06 11:02:43]

The CQ bit was checked by primiano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 11:02:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 12:15:40]

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

[commit-bot: I haz the power, 2016-12-06 12:15:40]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-12-06 13:57:31]

Patchset #1 (id:1) has been deleted

[Primiano Tucci (use gerrit), 2016-12-06 14:02:31]

I AM BACK!
This is a "Tick"*: quite some diff lines but should be pretty straightforward to
review as it doesn't introduce any change (at least not intentionally)  other
than making some checks stricter.

Kraynov: can you do a pass as well? So you get familiar with this code.

The "Tock"* will come soon next :)

*https://en.wikipedia.org/wiki/Tick-Tock_model

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_unittest.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_unittest.cc:2975: TestEventFilter::clear_counts();
this is purely for test isolation. I think I notice a while ago that if the
other test fails it will subtly cause a mysterious failure here because we don't
get to execute the clear at the end of the other.
So clearing also upfront makes the test easier to understand in this situation.

[kraynov, 2016-12-06 18:24:23]

https://codereview.chromium.org/2549103003/diff/20001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2549103003/diff/20001/base/BUILD.gn#newcode985
base/BUILD.gn:985: "trace_event/trace_event_filter_test_utils.h",
Why you build that with non-test code too?

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/event_...
File base/trace_event/event_name_filter.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/event_...
base/trace_event/event_name_filter.cc:17: :
event_names_whitelist_(std::move(event_names_whitelist)) {}
is it clang formatted?

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_event_filter.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_event_filter.cc:34:
trace_event.category_group_enabled())
hard readable

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_event_filter.cc:54:
AllocationContextTracker::GetInstanceForCurrentThread()
Should you check that it's TRACE_EVENT_PHASE_COMPLETE? Like you mentioned at
line 41?

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_filter.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_filter.cc:13: void TraceEventFilter::EndEvent(const
char* category_name,
Why not pure virtual?

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_filter.h (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_filter.h:37: // dropped on the floor.
Maybe 'discarded'? For me, 'dropped on the floor' sounds like we're forced to
discard silently and we don't like it.

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_filter_test_utils.h (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_filter_test_utils.h:15: class BASE_EXPORT
TestEventFilter : public TraceEventFilter {
TestEventFilter says "some TraceEventFilter with trivial implementation for
testing purposes". But in fact it's interceptor which counts number of
invocations. Let's call it e.g. EventCounterFilter.

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_filter_test_utils.h:44: static size_t
end_event_hit_count_;
Yes, I understand that this class is written for testing purposes, but I don't
like when non-static functions change static fields. Is there any strong reason
to make it static?

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_unittest.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_unittest.cc:2975: TestEventFilter::clear_counts();
On 2016/12/06 14:02:31, Primiano Tucci wrote:
> this is purely for test isolation. I think I notice a while ago that if the
> other test fails it will subtly cause a mysterious failure here because we
don't
> get to execute the clear at the end of the other.
> So clearing also upfront makes the test easier to understand in this
situation.

I would prefer not making stateful design.

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_log.cc:171: for (int index = 0; filter_bitmap != 0;
filter_bitmap >>= 1, index++) {
It became more readable, but could be improved, something like:
int index = 0;
for (uint32_t mask = 1; mask <= filter_bitmap; mask <<= 1, index++) {
  auto filter = g_category_group_filters.Get()[index];
  if ((filter_bitmap & mask) && filter)
    filter_fn(filter.get());
}

https://codereview.chromium.org/2549103003/diff/20001/tools/gn/bootstrap/boot...
File tools/gn/bootstrap/bootstrap.py (right):

https://codereview.chromium.org/2549103003/diff/20001/tools/gn/bootstrap/boot...
tools/gn/bootstrap/bootstrap.py:508:
'base/trace_event/trace_event_filter_test_utils.cc',
Prefer not to include test code into main build.

[Primiano Tucci (use gerrit), 2016-12-07 11:09:06]

Patchset #2 (id:40001) has been deleted

[Primiano Tucci (use gerrit), 2016-12-07 11:09:20]

The CQ bit was checked by primiano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 11:09:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Primiano Tucci (use gerrit), 2016-12-07 11:17:25]

The CQ bit was checked by primiano@chromium.org to run a CQ dry run

[Primiano Tucci (use gerrit), 2016-12-07 11:17:36]

Patchset #2 (id:60001) has been deleted

[Primiano Tucci (use gerrit), 2016-12-07 11:17:47]

https://codereview.chromium.org/2549103003/diff/20001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2549103003/diff/20001/base/BUILD.gn#newcode985
base/BUILD.gn:985: "trace_event/trace_event_filter_test_utils.h",
On 2016/12/06 18:24:23, kraynov wrote:
> Why you build that with non-test code too?

Fixed thanks!

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/event_...
File base/trace_event/event_name_filter.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/event_...
base/trace_event/event_name_filter.cc:17: :
event_names_whitelist_(std::move(event_names_whitelist)) {}
On 2016/12/06 18:24:23, kraynov wrote:
> is it clang formatted?

yes.

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_event_filter.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_event_filter.cc:34:
trace_event.category_group_enabled())
On 2016/12/06 18:24:23, kraynov wrote:
> hard readable

unfortunately this is what our style imposes and what git cl format does.
Feel free to suggest a better way around it.

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_event_filter.cc:54:
AllocationContextTracker::GetInstanceForCurrentThread()
On 2016/12/06 18:24:23, kraynov wrote:
> Should you check that it's TRACE_EVENT_PHASE_COMPLETE? Like you mentioned at
> line 41?

No, in an "EndEvent" there cannot be any BEGIN phases. Also I concretely don't
have any event object on which to test the PHASE_COMPLETE here.

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_filter.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_filter.cc:13: void TraceEventFilter::EndEvent(const
char* category_name,
On 2016/12/06 18:24:23, kraynov wrote:
> Why not pure virtual?

to not require classes that implement the filter interface to mandatorily define
this.
This method is only for snoop-mode to "see" the end of duration events (concrete
case: heap profiler) but is not required to implement this in order to implement
a filter interface.

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_filter.h (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_filter.h:37: // dropped on the floor.
On 2016/12/06 18:24:23, kraynov wrote:
> Maybe 'discarded'? For me, 'dropped on the floor' sounds like we're forced to
> discard silently and we don't like it.

Done.

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_filter_test_utils.h (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_filter_test_utils.h:15: class BASE_EXPORT
TestEventFilter : public TraceEventFilter {
On 2016/12/06 18:24:23, kraynov wrote:
> TestEventFilter says "some TraceEventFilter with trivial implementation for
> testing purposes". 
?? Where?

> But in fact it's interceptor which counts number of
> invocations. Let's call it e.g. EventCounterFilter.
This is a testing-only class and should have Test in the name.
However see the new patchset. I split out a HitsCOunter class

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_filter_test_utils.h:44: static size_t
end_event_hit_count_;
On 2016/12/06 18:24:23, kraynov wrote:
> Yes, I understand that this class is written for testing purposes, but I don't
> like when non-static functions change static fields. Is there any strong
reason
> to make it static?

The problem is that we cannot manage the lifetime of this class in the test
fixture. This is created and owned by the tracelog.
Having said this, I did split a bit the design, separating a HitsCounter struct.
SO now the test fixture owns the struct

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_unittest.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_unittest.cc:2975: TestEventFilter::clear_counts();
On 2016/12/06 18:24:23, kraynov wrote:
> On 2016/12/06 14:02:31, Primiano Tucci wrote:
> > this is purely for test isolation. I think I notice a while ago that if the
> > other test fails it will subtly cause a mysterious failure here because we
> don't
> > get to execute the clear at the end of the other.
> > So clearing also upfront makes the test easier to understand in this
> situation.
> 
> I would prefer not making stateful design.

Fixed by splitting out the HitCounter class

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_log.cc:171: for (int index = 0; filter_bitmap != 0;
filter_bitmap >>= 1, index++) {
On 2016/12/06 18:24:23, kraynov wrote:
> It became more readable, but could be improved, something like:
> int index = 0;
> for (uint32_t mask = 1; mask <= filter_bitmap; mask <<= 1, index++) {
>   auto filter = g_category_group_filters.Get()[index];
>   if ((filter_bitmap & mask) && filter)
>     filter_fn(filter.get());
> }

not worth it, I'm going to change ths in the next CL. See
https://codereview.chromium.org/2557743002/

https://codereview.chromium.org/2549103003/diff/20001/tools/gn/bootstrap/boot...
File tools/gn/bootstrap/bootstrap.py (right):

https://codereview.chromium.org/2549103003/diff/20001/tools/gn/bootstrap/boot...
tools/gn/bootstrap/bootstrap.py:508:
'base/trace_event/trace_event_filter_test_utils.cc',
On 2016/12/06 18:24:23, kraynov wrote:
> Prefer not to include test code into main build.

fixed

[commit-bot: I haz the power, 2016-12-07 11:17:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kraynov, 2016-12-07 11:30:50]

lgtm thanks!

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_filter_test_utils.h (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_filter_test_utils.h:15: class BASE_EXPORT
TestEventFilter : public TraceEventFilter {
On 2016/12/07 11:17:47, Primiano Tucci wrote:
> On 2016/12/06 18:24:23, kraynov wrote:
> > TestEventFilter says "some TraceEventFilter with trivial implementation for
> > testing purposes". 
> ?? Where?
> 
> > But in fact it's interceptor which counts number of
> > invocations. Let's call it e.g. EventCounterFilter.
> This is a testing-only class and should have Test in the name.
> However see the new patchset. I split out a HitsCOunter class
> 

Okay. In patch set 2 it looks fine, thanks. But it was in the main codebase in
patch set 1, so I've complained about naming.

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_filter_test_utils.h:44: static size_t
end_event_hit_count_;
On 2016/12/07 11:17:47, Primiano Tucci wrote:
> On 2016/12/06 18:24:23, kraynov wrote:
> > Yes, I understand that this class is written for testing purposes, but I
don't
> > like when non-static functions change static fields. Is there any strong
> reason
> > to make it static?
> 
> The problem is that we cannot manage the lifetime of this class in the test
> fixture. This is created and owned by the tracelog.
> Having said this, I did split a bit the design, separating a HitsCounter
struct.
> SO now the test fixture owns the struct

Acknowledged.

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/2549103003/diff/20001/base/trace_event/trace_...
base/trace_event/trace_log.cc:171: for (int index = 0; filter_bitmap != 0;
filter_bitmap >>= 1, index++) {
On 2016/12/07 11:17:47, Primiano Tucci wrote:
> On 2016/12/06 18:24:23, kraynov wrote:
> > It became more readable, but could be improved, something like:
> > int index = 0;
> > for (uint32_t mask = 1; mask <= filter_bitmap; mask <<= 1, index++) {
> >   auto filter = g_category_group_filters.Get()[index];
> >   if ((filter_bitmap & mask) && filter)
> >     filter_fn(filter.get());
> > }
> 
> not worth it, I'm going to change ths in the next CL. See
> https://codereview.chromium.org/2557743002/

Acknowledged.

[commit-bot: I haz the power, 2016-12-07 13:52:16]

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

[commit-bot: I haz the power, 2016-12-07 13:52:19]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[oystein (OOO til 10th of July), 2016-12-08 20:40:20]

lgtm with a few nits/suggestions, and a couple of comments I wrote and then
deleted after looking through the "tock" CL :P.

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/event_...
File base/trace_event/event_name_filter.h (right):

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/event_...
base/trace_event/event_name_filter.h:24: // and use a bloom filter trie.
However, today this is used quite rarely to
nit: s/quite/too/

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_event_filter.cc (right):

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/heap_p...
base/trace_event/heap_profiler_event_filter.cc:24: if
(AllocationContextTracker::capture_mode() !=
This class name is adding a lot of verbosity; would it be clearer if we had
something like "using act = AllocationContextTracker;"? Can't figure out if the
style guide has an opinion on this or not.

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_event_filter.h (right):

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/heap_p...
base/trace_event/heap_profiler_event_filter.h:18: // thread-local
AllocationContextTracker instance as then are seen.
nit: s/then/they/

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
File base/trace_event/trace_event_filter_test_utils.h (right):

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
base/trace_event/trace_event_filter_test_utils.h:5: #ifndef
BASE_TRACE_EVENT_TRACE_EVENT_FILTER_TEST_UTILS_H_
If it's worth splitting this out of the main unit test class, maybe it'd be
better to just take all of the filter events and merge it it all into one
trace_event_filter_test.h/.cc pair (and include the whitelist test there)?

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
base/trace_event/trace_log.cc:169: uint32_t filter_bitmap =
cat->enabled_filters();
nit: Not convinced auto and the abbreviation makes this any more readable than
the additional line in the previous version :).

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
base/trace_event/trace_log.cc:512: const std::string predicate_name =
filter_config.predicate_name();
reference

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
base/trace_event/trace_log.cc:516: if
(filter_config.filter_args()->GetList("event_name_whitelist", &list)) {
Hrm. I get why you're doing this, but maybe it'd be slightly less smelly to move
this (the config parsing) to within TraceConfig instead? e.g.
TraceConfig::ConfigureFilter(EventNameFilter&) or something.

[Primiano Tucci (use gerrit), 2016-12-09 11:49:57]

Patchset #3 (id:100001) has been deleted

[Primiano Tucci (use gerrit), 2016-12-09 11:50:10]

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/event_...
File base/trace_event/event_name_filter.h (right):

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/event_...
base/trace_event/event_name_filter.h:24: // and use a bloom filter trie.
However, today this is used quite rarely to
On 2016/12/08 20:40:20, oystein wrote:
> nit: s/quite/too/

Done.

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_event_filter.cc (right):

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/heap_p...
base/trace_event/heap_profiler_event_filter.cc:24: if
(AllocationContextTracker::capture_mode() !=
On 2016/12/08 20:40:20, oystein wrote:
> This class name is adding a lot of verbosity; would it be clearer if we had
> something like "using act = AllocationContextTracker;"? Can't figure out if
the
> style guide has an opinion on this or not.

I looked on the codebase and didn't find too much precedent *.
However I think I solved this by adding two inline getters on the top, so this
entire TU should now be more readable.


*
https://cs.chromium.org/search/?q=%5Eusing%5C+%5Cw%7B1,8%7D%5C+%5C%3D%5C+%5Cw...

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_event_filter.h (right):

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/heap_p...
base/trace_event/heap_profiler_event_filter.h:18: // thread-local
AllocationContextTracker instance as then are seen.
On 2016/12/08 20:40:20, oystein wrote:
> nit: s/then/they/

Done.

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
File base/trace_event/trace_event_filter_test_utils.h (right):

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
base/trace_event/trace_event_filter_test_utils.h:5: #ifndef
BASE_TRACE_EVENT_TRACE_EVENT_FILTER_TEST_UTILS_H_
On 2016/12/08 20:40:20, oystein wrote:
> If it's worth splitting this out of the main unit test class, maybe it'd be
> better to just take all of the filter events and merge it it all into one
> trace_event_filter_test.h/.cc pair (and include the whitelist test there)?

So, the reason why I split this class out is because in the next CL
(https://codereview.chromium.org/2557743002/) I'm reusing this in another
unittest (event_filter_registry_unittest.cc).

I took a look to the existing trace_event_unittest.cc, but there is too much
shared harness (EndTraceAndFlush(), trace read-back logic etc) which would be a
pity to duplicate.

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
base/trace_event/trace_log.cc:169: uint32_t filter_bitmap =
cat->enabled_filters();
On 2016/12/08 20:40:20, oystein wrote:
> nit: Not convinced auto and the abbreviation makes this any more readable than
> the additional line in the previous version :).

alright undid the auto+cat

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
base/trace_event/trace_log.cc:512: const std::string predicate_name =
filter_config.predicate_name();
On 2016/12/08 20:40:20, oystein wrote:
> reference

Oops. well spotted, thanks.

https://codereview.chromium.org/2549103003/diff/80001/base/trace_event/trace_...
base/trace_event/trace_log.cc:516: if
(filter_config.filter_args()->GetList("event_name_whitelist", &list)) {
On 2016/12/08 20:40:20, oystein wrote:
> Hrm. I get why you're doing this, but maybe it'd be slightly less smelly to
move
> this (the config parsing) to within TraceConfig instead? e.g.
> TraceConfig::ConfigureFilter(EventNameFilter&) or something.

I don't like too much coupling all that into TraceConfig. The main problem with
that is that we'd end up in a state where TraceLog knows about *most* of the
strings of the filter configs, but TraceConfig knows about
"event_name_whitelist", which would be inconsistent.
However I agree that all this code here hurts readability.

I added a more generic bool GetArgAsSet(const char* key,
std::unordered_set<std::string>*) method to TraceConfig::EventFilterConfig.
This still allows me to move all the code there as you proposed, but I can keep
the "event_name_whitelist" in the tracelog and still get rid of all this code
here.

[Primiano Tucci (use gerrit), 2016-12-09 11:51:18]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-12-09 11:51:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, kraynov@chromium.org
Link to the patchset: https://codereview.chromium.org/2549103003/#ps120001
(title: "oysteine review, add EventFilterConfig::GetArgAsSet")

[commit-bot: I haz the power, 2016-12-09 11:51:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 11:58:38]

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

[commit-bot: I haz the power, 2016-12-09 11:58:39]

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

[Primiano Tucci (use gerrit), 2016-12-09 14:17:20]

primiano@chromium.org changed reviewers:
+ thakis@chromium.org

[Primiano Tucci (use gerrit), 2016-12-09 14:17:21]

+thakis for changes to base/BUILD.gn

[Nico, 2016-12-09 14:23:13]

buld.gn lgtm

(Even remembered to update gn's bootstrap script, nice :-) )

[Primiano Tucci (use gerrit), 2016-12-09 14:36:38]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2016-12-09 14:36:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Primiano Tucci (use gerrit), 2016-12-09 16:17:46]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-12-09 16:17:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, thakis@chromium.org, kraynov@chromium.org
Link to the patchset: https://codereview.chromium.org/2549103003/#ps140001
(title: "Add BASE_EXPORT")

[commit-bot: I haz the power, 2016-12-09 16:17:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 20:37:53]

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

[commit-bot: I haz the power, 2016-12-09 20:37:54]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Primiano Tucci (use gerrit), 2016-12-09 22:38:36]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2016-12-09 22:39:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 02:04:19]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481323116911220,
"parent_rev": "209fd085f790bc324cee8a7ad61596e92ad597fb", "commit_rev":
"eac91ecda483b6f07b3bb2a46e824466efabace9"}

[commit-bot: I haz the power, 2016-12-10 02:04:42]

Description was changed from

==========
tracing: split trace event filter classes out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and isolating dependencies of the tracing codebase.
This CL is a pure refactor with no intended behavioral changes.
It moves all the trace filters classes outside of the TraceLog.
This is required to make the next CLs which will change the lifetime
management of filters easier to review.

BUG=659689
==========

to

==========
tracing: split trace event filter classes out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and isolating dependencies of the tracing codebase.
This CL is a pure refactor with no intended behavioral changes.
It moves all the trace filters classes outside of the TraceLog.
This is required to make the next CLs which will change the lifetime
management of filters easier to review.

BUG=659689

Review-Url: https://codereview.chromium.org/2549103003
==========

[commit-bot: I haz the power, 2016-12-10 02:04:44]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-12-12 15:05:15]

Description was changed from

==========
tracing: split trace event filter classes out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and isolating dependencies of the tracing codebase.
This CL is a pure refactor with no intended behavioral changes.
It moves all the trace filters classes outside of the TraceLog.
This is required to make the next CLs which will change the lifetime
management of filters easier to review.

BUG=659689

Review-Url: https://codereview.chromium.org/2549103003
==========

to

==========
tracing: split trace event filter classes out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and isolating dependencies of the tracing codebase.
This CL is a pure refactor with no intended behavioral changes.
It moves all the trace filters classes outside of the TraceLog.
This is required to make the next CLs which will change the lifetime
management of filters easier to review.

BUG=659689

Committed: https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3
Cr-Commit-Position: refs/heads/master@{#437721}
==========

[commit-bot: I haz the power, 2016-12-12 15:05:16]

Patchset 4 (id:??) landed as
https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3
Cr-Commit-Position: refs/heads/master@{#437721}