Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1115023003:
[Startup Tracing] The TraceConfig class [STALE] (Closed)
|
Created:
5 years, 7 months ago by Zhen Wang Modified:
5 years, 6 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, fmeawad, shatch, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Startup Tracing] The TraceConfig class
This is the first step to implement startup tracing. This CL implements the TraceConfig class, which combines CategoryFilter and TraceOptions. It preserves all the old signatures. The implementation of CategoryFilter is now based on TraceConfig.
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
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : implement CategoryFilter with TraceConfig #Patch Set 5 : #Patch Set 6 : #
Total comments: 61
Patch Set 7 : review fix #
Total comments: 6
Patch Set 8 : review fix #Patch Set 9 : add test for GetSyntheticDelayValues #
Total comments: 15
Patch Set 10 : rebase #Patch Set 11 : review fix and add a separate trace_config file #Patch Set 12 : rebase #
Total comments: 2
Messages
Total messages: 31 (6 generated)
zhenw@chromium.org changed reviewers: + nduca@chromium.org, pasko@chromium.org
ptal. By the way, this CL does not use SyntheticDelayRule mentioned in the TraceConfig design doc because existing implementation depends on the old string format, e.g. "DELAY(test.Delay1;16)". And it seems more than 3 fields are supported (though only the rightmost mode will be used). See: https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t... Shall I update the CL to use the format mentioned in the TraceConfig design doc or keep the old string format?
ping
this lgtm but can you have dsinclair or wangxianzhu review it exhaustively? i hope a next step is to remove category filter and the options enum, right? having duplicative code is a bummer...
sullivan@chromium.org changed reviewers: + nednguyen@google.com, sullivan@chromium.org
Ned, can you also review?
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org - nednguyen@google.com, sullivan@chromium.org
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org - nednguyen@google.com, sullivan@chromium.org
https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2738: void TraceConfig::Initialize(const std::string& config_string) { Can you move this up to just below the constructor? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:348: friend class TraceConfig; Can we just merge this straight into TraceConfig without the indirection step? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:428: typedef std::vector<std::string> StringList; Is this worth typedef'ing? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:431: // Allows all categories through, except if they end in the suffix 'Debug' or Is this true? We don't want to allow disable-by-default- through by default .... https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:433: static const char kDefaultTraceConfigString[]; Does this need to be in the public header? Can it be moved to an anonymous namespace inside the cc file? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:444: // |config_string| is a JSON formated string, containing both category filters Can you add a similar comment to the above here: Create TraceConfig object from JSON encoded trace config string. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:446: TraceConfig(const std::string& config_string); nit: explicit https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:448: TraceConfig(const TraceConfig& tc); nit: explicit https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:452: TraceConfig& operator=(const TraceConfig& rhs); Do we use copy and assign for this? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:458: void ToCategoryFilter(CategoryFilter& cf) const; Are these ToXXX methods only for tests? I don't see them being called anywhere. I'd prefer if we could get rid of them as it's making the public interfact of this class a lot bigger. Or, get rid of the bulk of them and just have toString https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:476: // Returns true if at least one category in the list is enabled by this How does this interact with disabled categories? If I do "foo,-bar" and TRACE("foo,bar") will it be considered enabled? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:481: void Merge(const TraceConfig& config); Will this go away when CategoryFilter is removed? Or will this have to live on? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:483: void Clear(); Is this needed? Or just for tests? If for tests, delete it and just create a new object. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:507: bool enable_systrace_; bool enable_sampling_ : 1; bool enable_systrace_ : 1; https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:513: }; Should this have a DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl_constants.cc (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl_constants.cc:10: const char TraceConfig::kDefaultTraceConfigString[] = I'd prefer not to have the default values encoded into a JSON string. We should just have an InitializeDefault which does it and not pretend it's a string.
zhenw@chromium.org changed reviewers: + nednguyen@chromium.org
+nednguyen@ for review Yes. The next step is to remove category filter and trace options, and preserve backward compatibility. I will start to work on the comments from dsinclair@.
On 2015/05/08 17:59:31, Zhen Wang wrote:
> +nednguyen@ for review
>
> Yes. The next step is to remove category filter and trace options, and
preserve
> backward compatibility.
>
> I will start to work on the comments from dsinclair@.
Chat with Zhen offline, in the future, people will add/modify the TraceConfig's
fields. That raises the question of how do we maintain backward compatibility at
that point?
My proposal is we have an extra field of version or "branch-number" in the
TraceConfig. so it looks s.t like:
{
[
{ "version": 124124,
...
},
{ "version": 12132,
...
},
{ "version": 1200,
...
},
]
The chrome binary will look at the highest version specified and use the config
of that version. This allows the client (telemetry) to have a sane strategy to
deal with multiple browser binary, and allows chrome backend side to make
informed decisions about maintaining backward compatibility.
There are still some open question about how would chrome indicates to the
client that it cannot support any specified version of the trace config.
On 2015/05/08 at 18:27:49, nednguyen wrote:
> On 2015/05/08 17:59:31, Zhen Wang wrote:
> > +nednguyen@ for review
> >
> > Yes. The next step is to remove category filter and trace options, and
preserve
> > backward compatibility.
> >
> > I will start to work on the comments from dsinclair@.
>
> Chat with Zhen offline, in the future, people will add/modify the
TraceConfig's fields. That raises the question of how do we maintain backward
compatibility at that point?
>
> My proposal is we have an extra field of version or "branch-number" in the
TraceConfig. so it looks s.t like:
>
> {
> [
> { "version": 124124,
> ...
> },
> { "version": 12132,
> ...
> },
> { "version": 1200,
> ...
> },
>
> ]
>
> The chrome binary will look at the highest version specified and use the
config of that version. This allows the client (telemetry) to have a sane
strategy to deal with multiple browser binary, and allows chrome backend side to
make informed decisions about maintaining backward compatibility.
>
> There are still some open question about how would chrome indicates to the
client that it cannot support any specified version of the trace config.
I don't think we need anything that complicated. As long as the logic to read
the struct is smart enough it shouldn't matter. We won't be removing fields that
often (I don't think we've ever removed a field from CategoryFilter). Things
will get added, so the parsing just needs to ignore anything that it doesn't
know about.
On 2015/05/08 18:30:04, (OOO until May 19th) dsinclair wrote:
> On 2015/05/08 at 18:27:49, nednguyen wrote:
> > On 2015/05/08 17:59:31, Zhen Wang wrote:
> > > +nednguyen@ for review
> > >
> > > Yes. The next step is to remove category filter and trace options, and
> preserve
> > > backward compatibility.
> > >
> > > I will start to work on the comments from dsinclair@.
> >
> > Chat with Zhen offline, in the future, people will add/modify the
> TraceConfig's fields. That raises the question of how do we maintain backward
> compatibility at that point?
> >
> > My proposal is we have an extra field of version or "branch-number" in the
> TraceConfig. so it looks s.t like:
> >
> > {
> > [
> > { "version": 124124,
> > ...
> > },
> > { "version": 12132,
> > ...
> > },
> > { "version": 1200,
> > ...
> > },
> >
> > ]
> >
> > The chrome binary will look at the highest version specified and use the
> config of that version. This allows the client (telemetry) to have a sane
> strategy to deal with multiple browser binary, and allows chrome backend side
to
> make informed decisions about maintaining backward compatibility.
> >
> > There are still some open question about how would chrome indicates to the
> client that it cannot support any specified version of the trace config.
>
>
> I don't think we need anything that complicated. As long as the logic to read
> the struct is smart enough it shouldn't matter. We won't be removing fields
that
> often (I don't think we've ever removed a field from CategoryFilter). Things
> will get added, so the parsing just needs to ignore anything that it doesn't
> know about.
If that's our stance on backward compatibility, we should at least have some
documentation about making changes to TraceConfig format.
On 2015/05/08 at 18:47:00, nednguyen wrote: > If that's our stance on backward compatibility, we should at least have some documentation about making changes to TraceConfig format. We haven't had any issues with the trace-event format so far without adding in any special versioning. We can add special documentation, but it's really up to the reviewers to make sure it doesn't get busted.
nednguyen@google.com changed reviewers: + nednguyen@google.com
https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2518: StringTokenizer tokens(category_filter_string, ","); nits: you want to be consistent about using single quotes vs double quotes. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2525: // Synthetic delays are of the form 'DELAY(delay;option;option;...)'. What about cases like ' Delay(delay; option; option;...) '? Should we remove whitespaces first before processing category_filter_string & trace_options_string? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2558: base::SplitString(trace_options_string, ',', &split); while use StringTokenizer above but use SplitString here? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:363: scoped_ptr<TraceConfig> config_; Why not just make this a value instead of scoped_ptr? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:464: void ToDict(base::DictionaryValue& dict) const; Do we actually need this API? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3139: config = TraceConfig("", "foo-bar-baz"); I would move these invalid trace option cases to a separate test. Few other cases to test are: valid category, invalid trace option invalid category, valid trace option invalid category, invalid trace option https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3207: TEST(TraceConfigTest, TraceConfig) { This test should also split into multiple ones, each focuses on testing a single aspect of TraceConfig. TEST(TraceConfigTest, ConstructDefaultTraceConfig) { } TEST(TraceConfigTest, TraceConfigFromValidString) { } TEST(TraceConfigTest, TraceConfigFromInvalidString) { } TEST(TraceConfigTest, MergingTraceConfigs) { } https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3259: EXPECT_FALSE(tc.IsCategoryGroupEnabled("CategoryTest,Category2")); what about synthetic delay values?
CL updated according to the review comments. ptal. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2518: StringTokenizer tokens(category_filter_string, ","); On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > nits: you want to be consistent about using single quotes vs double quotes. Use SplitString now. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2525: // Synthetic delays are of the form 'DELAY(delay;option;option;...)'. On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > What about cases like ' Delay(delay; option; option;...) '? Should we remove > whitespaces first before processing category_filter_string & > trace_options_string? Use SplitString now to get leading-and-trailing-space-trimmed version. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2558: base::SplitString(trace_options_string, ',', &split); On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > while use StringTokenizer above but use SplitString here? Updated above with SplitString, so that leading and trailing white spaces are trimmed. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2738: void TraceConfig::Initialize(const std::string& config_string) { On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > Can you move this up to just below the constructor? Done. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:348: friend class TraceConfig; On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > Can we just merge this straight into TraceConfig without the indirection step? Many places are using CategoryFilter. Do you mean just update all the implementations with CategoryFilter by TraceConfig directly in this CL? I planed to do them in 2 steps. Doing them in one step leads to a much longer CL. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:363: scoped_ptr<TraceConfig> config_; On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > Why not just make this a value instead of scoped_ptr? There will be extra copies in the constructors, right? Or that does not matter much? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:428: typedef std::vector<std::string> StringList; On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > Is this worth typedef'ing? I was following the style used in CategoryFilter, so that it is consistent. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:431: // Allows all categories through, except if they end in the suffix 'Debug' or On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > Is this true? We don't want to allow disable-by-default- through by default .... This is converted from kDefaultCategoryFilterString. See above in CategoryFilter. Now kDefaultTraceConfigString is removed as we will use InitializeDefault(). https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:433: static const char kDefaultTraceConfigString[]; On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > Does this need to be in the public header? Can it be moved to an anonymous > namespace inside the cc file? This is removed now as we will use InitializeDefault(). https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:444: // |config_string| is a JSON formated string, containing both category filters On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > Can you add a similar comment to the above here: > > Create TraceConfig object from JSON encoded trace config string. Done. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:446: TraceConfig(const std::string& config_string); On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > nit: explicit Done. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:448: TraceConfig(const TraceConfig& tc); On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > nit: explicit Done. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:452: TraceConfig& operator=(const TraceConfig& rhs); On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > Do we use copy and assign for this? Not now. But it may be used later when updating the implementations with CategoryFilter to TraceConfig. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:458: void ToCategoryFilter(CategoryFilter& cf) const; On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > Are these ToXXX methods only for tests? I don't see them being called anywhere. > I'd prefer if we could get rid of them as it's making the public interfact of > this class a lot bigger. > > Or, get rid of the bulk of them and just have toString Removed now. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:464: void ToDict(base::DictionaryValue& dict) const; On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > Do we actually need this API? I think this can be used by slow report. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:476: // Returns true if at least one category in the list is enabled by this On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > How does this interact with disabled categories? If I do "foo,-bar" and > TRACE("foo,bar") will it be considered enabled? Yes, it will be considered enabled. This comes from the original implementation in CategoryFilter. See https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t... And I add one more check for this in TraceConfigTest.TraceConfigFromValidString: EXPECT_TRUE(tc.IsCategoryGroupEnabled("included,excluded")); https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:481: void Merge(const TraceConfig& config); On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > Will this go away when CategoryFilter is removed? Or will this have to live on? This will live on. It is used by TraceLog. See https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t... https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:483: void Clear(); On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > Is this needed? Or just for tests? If for tests, delete it and just create a new > object. Clear() will be used by TraceLog. See https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t... https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:507: bool enable_systrace_; On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > bool enable_sampling_ : 1; > bool enable_systrace_ : 1; Those two members will be used in TraceConfig::Initialize: dict->GetBoolean(kEnableSamplingParam, &enable_sampling_) dict->GetBoolean(kEnableSystraceParam, &enable_systrace_) Specify them as bit field will make them not addressable. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:513: }; On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > Should this have a DISALLOW_COPY_AND_ASSIGN? Existing CategoryFilter has copy constructor and operator= function defined. So TraceConfig will also need them. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl_constants.cc (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl_constants.cc:10: const char TraceConfig::kDefaultTraceConfigString[] = On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > I'd prefer not to have the default values encoded into a JSON string. We should > just have an InitializeDefault which does it and not pretend it's a string. Done. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3139: config = TraceConfig("", "foo-bar-baz"); On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > I would move these invalid trace option cases to a separate test. Few other > cases to test are: > valid category, invalid trace option > invalid category, valid trace option > invalid category, invalid trace option Added TraceConfigFromInvalidLegacyString. There is no actual invalid category. Category name can be specified as arbitrary string. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3207: TEST(TraceConfigTest, TraceConfig) { On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > This test should also split into multiple ones, each focuses on testing a single > aspect of TraceConfig. > > TEST(TraceConfigTest, ConstructDefaultTraceConfig) { > } > > TEST(TraceConfigTest, TraceConfigFromValidString) { > } > > TEST(TraceConfigTest, TraceConfigFromInvalidString) { > } > > TEST(TraceConfigTest, MergingTraceConfigs) { > } > Done. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3259: EXPECT_FALSE(tc.IsCategoryGroupEnabled("CategoryTest,Category2")); On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > what about synthetic delay values? Synthetic delay values are not considered in IsCategoryGroupEnabled
https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:363: scoped_ptr<TraceConfig> config_; On 2015/05/13 00:01:11, Zhen Wang wrote: > On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > > Why not just make this a value instead of scoped_ptr? > > There will be extra copies in the constructors, right? Or that does not matter > much? Chatted offline. It doesn't matter much & the compiler probably will do the right thing for you. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3259: EXPECT_FALSE(tc.IsCategoryGroupEnabled("CategoryTest,Category2")); On 2015/05/13 00:01:11, Zhen Wang wrote: > On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > > what about synthetic delay values? > > Synthetic delay values are not considered in IsCategoryGroupEnabled I mean we should also add test for synthetic delay. https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2541: } else if (category.compare(0, strlen(TRACE_DISABLED_BY_DEFAULT("")), nits: '' instead of ""? https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3090: TEST(TraceConfigTest, TraceConfigFromValidLegacyString) { "...LegacyStrings"? https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3196: TEST(TraceConfigTest, TraceConfigFromInvalidLegacyString) { "...LegacyStrings"?
https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:363: scoped_ptr<TraceConfig> config_; On 2015/05/13 18:16:31, nednguyen (slow review) wrote: > On 2015/05/13 00:01:11, Zhen Wang wrote: > > On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > > > Why not just make this a value instead of scoped_ptr? > > > > There will be extra copies in the constructors, right? Or that does not matter > > much? > > Chatted offline. It doesn't matter much & the compiler probably will do the > right thing for you. Done. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3259: EXPECT_FALSE(tc.IsCategoryGroupEnabled("CategoryTest,Category2")); On 2015/05/13 18:16:31, nednguyen (slow review) wrote: > On 2015/05/13 00:01:11, Zhen Wang wrote: > > On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > > > what about synthetic delay values? > > > > Synthetic delay values are not considered in IsCategoryGroupEnabled > > I mean we should also add test for synthetic delay. This test includes "\"synthetic_delays\":[\"test.Delay1;16\"]". I also add a invalid test for synthetic delays in TraceConfigFromInvalidString. https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2541: } else if (category.compare(0, strlen(TRACE_DISABLED_BY_DEFAULT("")), On 2015/05/13 18:16:32, nednguyen (slow review) wrote: > nits: '' instead of ""? I think the macro requires a string to do string concatenation. https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3090: TEST(TraceConfigTest, TraceConfigFromValidLegacyString) { On 2015/05/13 18:16:32, nednguyen (slow review) wrote: > "...LegacyStrings"? Done. https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3196: TEST(TraceConfigTest, TraceConfigFromInvalidLegacyString) { On 2015/05/13 18:16:32, nednguyen (slow review) wrote: > "...LegacyStrings"? Done.
On 2015/05/13 20:59:17, Zhen Wang wrote: > https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... > File base/trace_event/trace_event_impl.h (right): > > https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... > base/trace_event/trace_event_impl.h:363: scoped_ptr<TraceConfig> config_; > On 2015/05/13 18:16:31, nednguyen (slow review) wrote: > > On 2015/05/13 00:01:11, Zhen Wang wrote: > > > On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > > > > Why not just make this a value instead of scoped_ptr? > > > > > > There will be extra copies in the constructors, right? Or that does not > matter > > > much? > > > > Chatted offline. It doesn't matter much & the compiler probably will do the > > right thing for you. > > Done. > > https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... > File base/trace_event/trace_event_unittest.cc (right): > > https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... > base/trace_event/trace_event_unittest.cc:3259: > EXPECT_FALSE(tc.IsCategoryGroupEnabled("CategoryTest,Category2")); > On 2015/05/13 18:16:31, nednguyen (slow review) wrote: > > On 2015/05/13 00:01:11, Zhen Wang wrote: > > > On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > > > > what about synthetic delay values? > > > > > > Synthetic delay values are not considered in IsCategoryGroupEnabled > > > > I mean we should also add test for synthetic delay. > > This test includes "\"synthetic_delays\":[\"test.Delay1;16\"]". I also add a > invalid test for synthetic delays in TraceConfigFromInvalidString. > > https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... > File base/trace_event/trace_event_impl.cc (right): > > https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... > base/trace_event/trace_event_impl.cc:2541: } else if (category.compare(0, > strlen(TRACE_DISABLED_BY_DEFAULT("")), > On 2015/05/13 18:16:32, nednguyen (slow review) wrote: > > nits: '' instead of ""? > > I think the macro requires a string to do string concatenation. > > https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... > File base/trace_event/trace_event_unittest.cc (right): > > https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... > base/trace_event/trace_event_unittest.cc:3090: TEST(TraceConfigTest, > TraceConfigFromValidLegacyString) { > On 2015/05/13 18:16:32, nednguyen (slow review) wrote: > > "...LegacyStrings"? > > Done. > > https://codereview.chromium.org/1115023003/diff/120001/base/trace_event/trace... > base/trace_event/trace_event_unittest.cc:3196: TEST(TraceConfigTest, > TraceConfigFromInvalidLegacyString) { > On 2015/05/13 18:16:32, nednguyen (slow review) wrote: > > "...LegacyStrings"? > > Done. Sorry for not making it clear, my concern is that we don't seem to have any coverage for GetSyntheticDelayValues() method.
On 2015/05/13 21:16:20, nednguyen (slow review) wrote: > Sorry for not making it clear, my concern is that we don't seem to have any > coverage for GetSyntheticDelayValues() method. Test for GetSyntheticDelayValues is now added.
lgtm on my part. Please wait for Dan's stamp.
Egor and I have been discussing the security issues related to the JSON reader and we are suggested by the security team to use SafeJsonParser because the JSON string passed to TraceConfig could have arbitrary content. SafeJsonParser is currently in chrome/browser and is being moved to components/. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf... https://codereview.chromium.org/1140053003/ Shall we change the constructor of TraceConfig to accept base::Value instead of std::string? In this way, we can do the JSON parsing in DevTools or right after the trace config file is read. Or should we move TraceConfig class to somewhere in chrome/browser?
On 2015/05/19 15:53:43, Zhen Wang wrote: > Egor and I have been discussing the security issues related to the JSON reader > and we are suggested by the security team to use SafeJsonParser because the JSON > string passed to TraceConfig could have arbitrary content. > > SafeJsonParser is currently in chrome/browser and is being moved to components/. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf... > https://codereview.chromium.org/1140053003/ > > Shall we change the constructor of TraceConfig to accept base::Value instead of > std::string? In this way, we can do the JSON parsing in DevTools or right after > the trace config file is read. Or should we move TraceConfig class to somewhere > in chrome/browser? Is it possible to move the SafeJsonParser to base/?
On 2015/05/19 22:02:07, nednguyen wrote: > On 2015/05/19 15:53:43, Zhen Wang wrote: > > Egor and I have been discussing the security issues related to the JSON reader > > and we are suggested by the security team to use SafeJsonParser because the > JSON > > string passed to TraceConfig could have arbitrary content. > > > > SafeJsonParser is currently in chrome/browser and is being moved to > components/. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf... > > https://codereview.chromium.org/1140053003/ > > > > Shall we change the constructor of TraceConfig to accept base::Value instead > of > > std::string? In this way, we can do the JSON parsing in DevTools or right > after > > the trace config file is read. Or should we move TraceConfig class to > somewhere > > in chrome/browser? > > Is it possible to move the SafeJsonParser to base/? Not without a whole lot of hoops; it depends on content/ to offload work to the IO thread and sends IPCs to a specific child process (base doesn't know about multiprocess stuff AFAIK).
nduca@ thoughs on the safeJSON stuff? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:452: TraceConfig& operator=(const TraceConfig& rhs); On 2015/05/13 00:01:11, Zhen Wang wrote: > On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > > Do we use copy and assign for this? > > Not now. But it may be used later when updating the implementations with > CategoryFilter to TraceConfig. In that case, let's add the DISALLOW for now and we can remove it later if we determine we need it. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:464: void ToDict(base::DictionaryValue& dict) const; On 2015/05/13 00:01:11, Zhen Wang wrote: > On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > > Do we actually need this API? > > I think this can be used by slow report. Can we determine if it is or isn't? I'd prefer to leave it out of it isn't needed. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:507: bool enable_systrace_; On 2015/05/13 00:01:11, Zhen Wang wrote: > On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > > bool enable_sampling_ : 1; > > bool enable_systrace_ : 1; > > Those two members will be used in TraceConfig::Initialize: > > dict->GetBoolean(kEnableSamplingParam, &enable_sampling_) > dict->GetBoolean(kEnableSystraceParam, &enable_systrace_) > > Specify them as bit field will make them not addressable. Sure, but we can also easily change the initialize call to write into a temp boolean and then assign the value. We're using more space then needed here, although it isn't a lot, but why not save the extra byte? https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:513: }; On 2015/05/13 00:01:10, Zhen Wang wrote: > On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > > Should this have a DISALLOW_COPY_AND_ASSIGN? > > Existing CategoryFilter has copy constructor and operator= function defined. So > TraceConfig will also need them. If it isn't needed let, let's put in the DISALLOW. We can remove it in the future if needed, but it's possible CategoryFilter just never had one. https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2496: //////////////////////////////////////////////////////////////////////////////// Remove this comment block. https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2610: scoped_ptr<base::Value> value(base::JSONReader::Read(config_string)); Do we need to convert this before landing? https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2710: std::string TraceConfig::ToTraceOptionsString() const { If these are just for testing, remove them and use the ToString method. https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2883: bool TraceConfig::IsEmptyOrContainsLeadingOrTrailingWhitespace( Is there no method in base that does this already? https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.h:339: class BASE_EXPORT TraceConfig { This file is getting pretty big, lets split these classes out to their own files. https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.h:402: // 'Test'. I still think this comment is wrong. It doesn't allow all categories through, it allows all non-disabled-by-default categories through, right?
https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:452: TraceConfig& operator=(const TraceConfig& rhs); On 2015/05/21 13:42:07, dsinclair wrote: > On 2015/05/13 00:01:11, Zhen Wang wrote: > > On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > > > Do we use copy and assign for this? > > > > Not now. But it may be used later when updating the implementations with > > CategoryFilter to TraceConfig. > > In that case, let's add the DISALLOW for now and we can remove it later if we > determine we need it. COPY and ASSIGN are needed to implement CategoryFilter for now. I can remove them when cleaning all CategoryFilter implementations in the next CL. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:464: void ToDict(base::DictionaryValue& dict) const; On 2015/05/21 13:42:07, dsinclair wrote: > On 2015/05/13 00:01:11, Zhen Wang wrote: > > On 2015/05/11 17:59:28, nednguyen (slow review) wrote: > > > Do we actually need this API? > > > > I think this can be used by slow report. > > Can we determine if it is or isn't? I'd prefer to leave it out of it isn't > needed. Fadi has confirmed that this is not needed in the 1st stage of Deep Report. Since it is used by ToString(). I moved it to private. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:507: bool enable_systrace_; On 2015/05/21 13:42:07, dsinclair wrote: > On 2015/05/13 00:01:11, Zhen Wang wrote: > > On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > > > bool enable_sampling_ : 1; > > > bool enable_systrace_ : 1; > > > > Those two members will be used in TraceConfig::Initialize: > > > > dict->GetBoolean(kEnableSamplingParam, &enable_sampling_) > > dict->GetBoolean(kEnableSystraceParam, &enable_systrace_) > > > > Specify them as bit field will make them not addressable. > > Sure, but we can also easily change the initialize call to write into a temp > boolean and then assign the value. > > We're using more space then needed here, although it isn't a lot, but why not > save the extra byte? Done. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:513: }; On 2015/05/21 13:42:07, dsinclair wrote: > On 2015/05/13 00:01:10, Zhen Wang wrote: > > On 2015/05/08 17:44:50, (OOO until May 19th) dsinclair wrote: > > > Should this have a DISALLOW_COPY_AND_ASSIGN? > > > > Existing CategoryFilter has copy constructor and operator= function defined. > So > > TraceConfig will also need them. > > > If it isn't needed let, let's put in the DISALLOW. We can remove it in the > future if needed, but it's possible CategoryFilter just never had one. COPY and ASSIGN are needed to implement CategoryFilter for now. I can remove them when cleaning all CategoryFilter implementations in the next CL. https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2496: //////////////////////////////////////////////////////////////////////////////// On 2015/05/21 13:42:08, dsinclair wrote: > Remove this comment block. Done. https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2610: scoped_ptr<base::Value> value(base::JSONReader::Read(config_string)); On 2015/05/21 13:42:07, dsinclair wrote: > Do we need to convert this before landing? I don't understand. There is no convert, right? https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2710: std::string TraceConfig::ToTraceOptionsString() const { On 2015/05/21 13:42:07, dsinclair wrote: > If these are just for testing, remove them and use the ToString method. This is the way to convert between the legacy format and the new format. And tracing_ui may use it. See https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr... On UI, we still want to have category filter separated from the options, right? https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2883: bool TraceConfig::IsEmptyOrContainsLeadingOrTrailingWhitespace( On 2015/05/21 13:42:08, dsinclair wrote: > Is there no method in base that does this already? I don't think so. This was a function from CategoryFilter. Now TraceConfig implements it and CategoryFilter calls it. https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.h:339: class BASE_EXPORT TraceConfig { On 2015/05/21 13:42:08, dsinclair wrote: > This file is getting pretty big, lets split these classes out to their own > files. I move TraceConfig, CategoryFilter and TraceOptions to trace_config.h now. In the next CL, the implementation of CategoryFilter and TraceOptions will be removed. All the references to them will be updated to TraceConfig. https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.h:402: // 'Test'. On 2015/05/21 13:42:08, dsinclair wrote: > I still think this comment is wrong. It doesn't allow all categories through, it > allows all non-disabled-by-default categories through, right? I think the comment is correct. See TraceConfigTest.ConstructDefaultTraceConfig in trace_config_unittest.cc
This is getting difficult to review, can we split this up into several CLs. 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. Something along those lines? It's difficult to tell what's new code and what's moved code at this point. https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.h:452: TraceConfig& operator=(const TraceConfig& rhs); On 2015/05/22 23:38:04, Zhen Wang wrote: > On 2015/05/21 13:42:07, dsinclair wrote: > > On 2015/05/13 00:01:11, Zhen Wang wrote: > > > On 2015/05/08 17:44:49, (OOO until May 19th) dsinclair wrote: > > > > Do we use copy and assign for this? > > > > > > Not now. But it may be used later when updating the implementations with > > > CategoryFilter to TraceConfig. > > > > In that case, let's add the DISALLOW for now and we can remove it later if we > > determine we need it. > > COPY and ASSIGN are needed to implement CategoryFilter for now. I can remove > them when cleaning all CategoryFilter implementations in the next CL. I'm confused, above you said they aren't used? So, are they currently used or not? https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2610: scoped_ptr<base::Value> value(base::JSONReader::Read(config_string)); On 2015/05/22 23:38:04, Zhen Wang wrote: > On 2015/05/21 13:42:07, dsinclair wrote: > > Do we need to convert this before landing? > > I don't understand. There is no convert, right? Sorry, thought this was related to to the SafeJSONReader thread you forwarded to me. https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2710: std::string TraceConfig::ToTraceOptionsString() const { On 2015/05/22 23:38:04, Zhen Wang wrote: > On 2015/05/21 13:42:07, dsinclair wrote: > > If these are just for testing, remove them and use the ToString method. > > This is the way to convert between the legacy format and the new format. And > tracing_ui may use it. See > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr... > > On UI, we still want to have category filter separated from the options, right? They UI should be separate yes, but I'm not sure why that would require a ToTraceOptions method? If this isn't used, let's remove it. https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1115023003/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.h:402: // 'Test'. On 2015/05/22 23:38:04, Zhen Wang wrote: > On 2015/05/21 13:42:08, dsinclair wrote: > > I still think this comment is wrong. It doesn't allow all categories through, > it > > allows all non-disabled-by-default categories through, right? > > I think the comment is correct. See TraceConfigTest.ConstructDefaultTraceConfig > in trace_config_unittest.cc If the comment is correct, then the code is wrong and has to be fixed. We should not be enabling disabled-by-default- categories by default. https://codereview.chromium.org/1115023003/diff/220001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1115023003/diff/220001/base/trace_event/trace... base/trace_event/trace_config.h:35: struct BASE_EXPORT TraceOptions { Why a struct and not a class? https://codereview.chromium.org/1115023003/diff/220001/base/trace_event/trace... base/trace_event/trace_config.h:75: bool enable_sampling; Can you bit pack these please.
On 2015/05/25 13:36:02, dsinclair wrote: > This is getting difficult to review, can we split this up into several CLs. > > 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. > > Something along those lines? It's difficult to tell what's new code and what's > moved code at this point. Sure. I created a separate CL to split this. Please see https://codereview.chromium.org/1154133003/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
