[Startup Tracing] Hook up TraceConfig and remove CategoryFilter & TraceOptions

[Startup Tracing] Hook up TraceConfig and remove CategoryFilter & TraceOptions

This CL hooks up TraceConfig and removes CategoryFilter and TraceOptions. This is step 3 of the following.

1- Move current CategoryFilter and TraceOptions to new file.
2- Create TraceConfig but don't use it anywhere
3- Hookup TraceConfig and remove CategoryFilter and TraceOptions.

Background:
Implementing TraceConfig is the first stage for startup tracing.

Startup tracing design doc:
https://docs.google.com/document/d/1yRCXhrQ-0rsfUgNHt9T4YdnmJYrXKN6aK56Ozk3kPVc/edit?usp=sharing

TraceConfig class design doc:
https://docs.google.com/document/d/1zwc6FdeYyrC7m9fC4jJqbkqpB0il9NV-MWH7lEnL0cc/edit?usp=sharing

trace-config file format doc:
https://docs.google.com/document/d/1auY0Buk-R2tGCzpHJzFgyTfngXTAK0mlxJc6J4M_CK4/edit?usp=sharing

BUG=317481, 482098
TBR=phajdan.jr@chromium.org,nduca@chromium.org,michaelbai@chromium.org,tedchoc@chromium.org,pfeldman@chromium.org,perkj@chromium.org,ccameron@chromium.org,cbentzel@chromium.org,jam@chromium.org

Committed: https://crrev.com/d601ddc554ca0e22fe7b91b93395a421e329ef83
Cr-Commit-Position: refs/heads/master@{#332467}

[Zhen Wang, 2015-06-01 21:22:56]

zhenw@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Zhen Wang, 2015-06-01 21:22:57]

ptal

[dsinclair, 2015-06-02 13:56:55]

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:67: TraceConfig(category, ""),
TraceLog::RECORDING_MODE);
Can we add a default value of "" to the trace config for trace options so we
don't have to pass , "" everywhere?

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_conf...
File base/trace_event/trace_config.cc (right):

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_conf...
base/trace_event/trace_config.cc:60: case ECHO_TO_CONSOLE:
Put this one below RECORD_AS_MUCH_AS_POSSIBLE so we keep the record entries
together.

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_conf...
base/trace_event/trace_config.cc:113: return record_mode_;
These can all be implemented in the header.

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_android.cc (right):

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_android.cc:100: TraceConfig trace_config;
Should we set kDefaultCategoryFilterString here just to make it explicit?

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_unittest.cc (right):

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_unittest.cc:917:
TraceLog::GetInstance()->SetEnabled(TraceConfig("*", ""),
What about defining a TraceLog::kRecordAll = "*" or something so we can use the
constant instead of "*"? (This could also be a future patch as we used "*"
previously as well)

[Zhen Wang, 2015-06-02 16:24:42]

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:67: TraceConfig(category, ""),
TraceLog::RECORDING_MODE);
On 2015/06/02 13:56:54, dsinclair wrote:
> Can we add a default value of "" to the trace config for trace options so we
> don't have to pass , "" everywhere?

TraceConfig has the following two constructors

1. TraceConfig(const std::string&, const std::string&);
This is for category filter string and trace options string

2. explicit TraceConfig(const std::string&);
This is for trace config string (JSON format)

Using the default value "" for trace options will not work.


One way to solve this is to change

explicit TraceConfig(const std::string&);

to

explicit TraceConfig(scoped_ptr<base::Value> value);

where |value| is the parsed values from the JSON formatted config string. This
requires parsing the config string outside of TraceConfig class.

How do you think?

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_conf...
File base/trace_event/trace_config.cc (right):

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_conf...
base/trace_event/trace_config.cc:60: case ECHO_TO_CONSOLE:
On 2015/06/02 13:56:55, dsinclair wrote:
> Put this one below RECORD_AS_MUCH_AS_POSSIBLE so we keep the record entries
> together.

Done.

I also updated the order in the header file and other places.

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_conf...
base/trace_event/trace_config.cc:113: return record_mode_;
On 2015/06/02 13:56:55, dsinclair wrote:
> These can all be implemented in the header.

Done.

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_android.cc (right):

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_android.cc:100: TraceConfig trace_config;
On 2015/06/02 13:56:55, dsinclair wrote:
> Should we set kDefaultCategoryFilterString here just to make it explicit?

We do not use kDefaultCategoryFilterString any more because we now use
TraceConfig::InitializeDefault() to initialize the default values.

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_unittest.cc (right):

https://codereview.chromium.org/1165673002/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_unittest.cc:917:
TraceLog::GetInstance()->SetEnabled(TraceConfig("*", ""),
On 2015/06/02 13:56:55, dsinclair wrote:
> What about defining a TraceLog::kRecordAll = "*" or something so we can use
the
> constant instead of "*"? (This could also be a future patch as we used "*"
> previously as well)

Done.

[dsinclair, 2015-06-02 16:30:40]

On 2015/06/02 at 16:24:42, zhenw wrote:
>
https://codereview.chromium.org/1165673002/diff/1/base/trace_event/memory_dum...
> File base/trace_event/memory_dump_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/1165673002/diff/1/base/trace_event/memory_dum...
> base/trace_event/memory_dump_manager_unittest.cc:67: TraceConfig(category,
""), TraceLog::RECORDING_MODE);
> On 2015/06/02 13:56:54, dsinclair wrote:
> > Can we add a default value of "" to the trace config for trace options so we
> > don't have to pass , "" everywhere?
> 
> TraceConfig has the following two constructors
> 
> 1. TraceConfig(const std::string&, const std::string&);
> This is for category filter string and trace options string
> 
> 2. explicit TraceConfig(const std::string&);
> This is for trace config string (JSON format)
> 
> Using the default value "" for trace options will not work.
> 
> 
> One way to solve this is to change
> 
> explicit TraceConfig(const std::string&);
> 
> to
> 
> explicit TraceConfig(scoped_ptr<base::Value> value);
> 
> where |value| is the parsed values from the JSON formatted config string. This
requires parsing the config string outside of TraceConfig class.
> 
> How do you think?

Probably not worth it, let's leave it as is. lgtm.

[Zhen Wang, 2015-06-02 17:19:06]

Since I have updated many files, I need to ask the corresponding owners to
review. In general, should I ask each owner to review their own part or should I
ask one person who owns all the files (e.g., jam@) to review?

[dsinclair, 2015-06-02 17:26:26]

On 2015/06/02 at 17:19:06, zhenw wrote:
> Since I have updated many files, I need to ask the corresponding owners to
review. In general, should I ask each owner to review their own part or should I
ask one person who owns all the files (e.g., jam@) to review?

I'm not sure, whichever you think is best.

[Zhen Wang, 2015-06-02 21:31:50]

The CQ bit was checked by zhenw@chromium.org

[Zhen Wang, 2015-06-02 21:31:50]

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/1165673002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-02 21:38:11]

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

[commit-bot: I haz the power, 2015-06-02 21:46:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-02 21:47:26]

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