Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1765153002:
Update DevTools Tracing.Start to accept trace config as a parameter (Closed)
|
Created:
4 years, 9 months ago by Zhen Wang Modified:
4 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, caseq+blink_chromium.org, vmpstr+watch_chromium.org, wfh+watch_chromium.org, blink-reviews, jam, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, mkwst+moarreviews-renderer_chromium.org, dglazkov+blink, darin-cc_chromium.org, devtools-reviews_chromium.org, tracing+reviews_chromium.org, apavlov+blink_chromium.org, sergeyv+blink_chromium.org, blink-reviews-api_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org, rnephew (Reviews Here), fmeawad, petrcermak, lpy Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate DevTools Tracing.Start to accept trace config as
a parameter
This CL updates Tracing.Start to accept trace config as
a parameter when starting tracing. It is backward compatible
with the old way.
Design doc: https://goo.gl/GxQ23k
BUG=579358
Committed: https://crrev.com/c0e3792f832fc454324dbe35d01079b124b072cd
Cr-Commit-Position: refs/heads/master@{#381783}
Patch Set 1 #
Total comments: 16
Patch Set 2 : review fix #Patch Set 3 : add test #
Total comments: 3
Patch Set 4 : fix link error #
Total comments: 14
Patch Set 5 : review fix #Patch Set 6 : #
Total comments: 53
Patch Set 7 : review fix for primiano and petrcermak's comments #
Total comments: 23
Patch Set 8 : review fix #
Total comments: 16
Patch Set 9 : rebase #Patch Set 10 : inline enum #Messages
Total messages: 43 (13 generated)
Description was changed from ========== Update Devtools Tracing.Start to take trace config BUG=579358 ========== to ========== Update Devtools Tracing.Start to take trace config This CL updates Tracing.Start to take trace config when starting tracing. It is backward compatible with the old way. Design doc: https://goo.gl/GxQ23k BUG=579358 ==========
zhenw@chromium.org changed reviewers: + caseq@chromium.org, nednguyen@google.com, primiano@chromium.org
ptal
https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:77: const char& separator) { no need to pass a char by reference :) https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:90: scoped_ptr<base::Value> convertDictKeyStyle(const base::Value& value) { style: s/convert/Convert/ https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:91: const base::DictionaryValue* dict = NULL; nit: nullptr https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:92: const base::ListValue* list = NULL; ditto (and please move down to actual usage) https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:102: } else if (value.GetAsList(&list)) { style: no else after return https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:108: } else { ditto. https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.h:57: static scoped_ptr<base::DictionaryValue> DevToolsToTracingStyle( I think it would be better out of base -- doing this in DevTools or tracing controller would be the best perhaps. https://codereview.chromium.org/1765153002/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/tracing_handler.cc (right): https://codereview.chromium.org/1765153002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/tracing_handler.cc:161: base::JSONWriter::Write(*tracing_style_dict, &trace_config_str); Why do we have to do an extra roundtrip of writing/parsing JSON? Perhaps, have TraceConfig's constructor accept base::DictionaryValue? https://codereview.chromium.org/1765153002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/protocol.json:4832: "description": "" Here and below -- please add descriptions. https://codereview.chromium.org/1765153002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/protocol.json:4870: { "name": "categories", "type": "string", "optional": true, "description": "Category/tag filter" }, deprecated: true? https://codereview.chromium.org/1765153002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/protocol.json:4871: { "name": "options", "type": "string", "optional": true, "description": "Tracing options" }, ditto. https://codereview.chromium.org/1765153002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebDevToolsAgentImpl.h (right): https://codereview.chromium.org/1765153002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebDevToolsAgentImpl.h:120: void enableTracingWithConfigStr(const String& devtoolsStyleConfig) override; Having to do all this plumbing is quite unfortunate. I'll see if I can help here by removing the need for starting tracing from renderer.
Some delta comments on top of caseq's https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:76: std::string convertStringStyle(const std::string& in_str, nit: UpperCase function names: https://google.github.io/styleguide/cppguide.html#Function_Names https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:76: std::string convertStringStyle(const std::string& in_str, This function should not be in base, and once you move it, should have a more explicative name, such as ToCamelCase ? https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1765153002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.h:57: static scoped_ptr<base::DictionaryValue> DevToolsToTracingStyle( On 2016/03/05 02:25:24, caseq wrote: > I think it would be better out of base -- doing this in DevTools or tracing > controller would be the best perhaps. +1. Base should not know anything about devtools. https://codereview.chromium.org/1765153002/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/tracing_handler.cc (right): https://codereview.chromium.org/1765153002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/tracing_handler.cc:161: base::JSONWriter::Write(*tracing_style_dict, &trace_config_str); On 2016/03/05 02:25:24, caseq wrote: > Why do we have to do an extra roundtrip of writing/parsing JSON? Perhaps, have > TraceConfig's constructor accept base::DictionaryValue? I think a good layering would be: - move your dictionary conversion logic (that right now you have in base) here. - I think you want a TraceConfigFromDevtoolsConfig(scoped_ptr<DictValue>, TraceConfig* out_trace_config) method in the anonymous namespace here. Having to deserialize -> reserialize -> reparse JSON feels a bit... not ideal
I uploaded a fix based on caseq and primiano's comments. Please take a look. https://codereview.chromium.org/1765153002/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/tracing_handler_unittest.cc (right): https://codereview.chromium.org/1765153002/diff/40001/content/browser/devtool... content/browser/devtools/protocol/tracing_handler_unittest.cc:66: TracingHandler::GetTraceConfigFromDevtoolsConfig(*devtools_style_dict); Andrey, I get link error here. Do you have any idea what is wrong? I think tracing_handler.cc is built in content_browser and should be accessible from content_unittests, right? ../../content/browser/devtools/protocol/tracing_handler_unittest.cc:66: error: undefined reference to 'content::devtools::tracing::TracingHandler::GetTraceConfigFromDevtoolsConfig(base::DictionaryValue const&)' clang: error: linker command failed with exit code 1 (use -v to see invocation)
https://codereview.chromium.org/1765153002/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/tracing_handler_unittest.cc (right): https://codereview.chromium.org/1765153002/diff/40001/content/browser/devtool... content/browser/devtools/protocol/tracing_handler_unittest.cc:66: TracingHandler::GetTraceConfigFromDevtoolsConfig(*devtools_style_dict); On 2016/03/08 00:41:52, Zhen Wang wrote: > Andrey, I get link error here. Do you have any idea what is wrong? I think > tracing_handler.cc is built in content_browser and should be accessible from > content_unittests, right? > > ../../content/browser/devtools/protocol/tracing_handler_unittest.cc:66: error: > undefined reference to > 'content::devtools::tracing::TracingHandler::GetTraceConfigFromDevtoolsConfig(base::DictionaryValue > const&)' > clang: error: linker command failed with exit code 1 (use -v to see invocation) You need to CONTENT_EXPORT (or whatever is the right macro for the component where you define the GetTraceConfigFromDevtoolsConfig) the GetTraceConfigFromDevtoolsConfig. in component build, content and the unittest are independent different linker units, and the unittest needs to import the GetTraceConfigFromDevtoolsConfig defined in content
https://codereview.chromium.org/1765153002/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/tracing_handler_unittest.cc (right): https://codereview.chromium.org/1765153002/diff/40001/content/browser/devtool... content/browser/devtools/protocol/tracing_handler_unittest.cc:66: TracingHandler::GetTraceConfigFromDevtoolsConfig(*devtools_style_dict); On 2016/03/08 21:30:12, Primiano wrote: > On 2016/03/08 00:41:52, Zhen Wang wrote: > > Andrey, I get link error here. Do you have any idea what is wrong? I think > > tracing_handler.cc is built in content_browser and should be accessible from > > content_unittests, right? > > > > ../../content/browser/devtools/protocol/tracing_handler_unittest.cc:66: error: > > undefined reference to > > > 'content::devtools::tracing::TracingHandler::GetTraceConfigFromDevtoolsConfig(base::DictionaryValue > > const&)' > > clang: error: linker command failed with exit code 1 (use -v to see > invocation) > > You need to CONTENT_EXPORT (or whatever is the right macro for the component > where you define the GetTraceConfigFromDevtoolsConfig) the > GetTraceConfigFromDevtoolsConfig. in component build, content and the unittest > are independent different linker units, and the unittest needs to import the > GetTraceConfigFromDevtoolsConfig defined in content Ah, I see. Thanks! Fixed.
ping :)
lgtm https://codereview.chromium.org/1765153002/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/tracing_handler.cc (right): https://codereview.chromium.org/1765153002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/tracing_handler.cc:38: const char separator) { nit: nuke const from parameter passed by value. https://codereview.chromium.org/1765153002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/tracing_handler.cc:187: "Both trace config and categories/options are specified. Only one way " nit: "Either trace config (preferred) or categories/options should be specified, but not both." https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorTracingAgent.cpp (right): https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorTracingAgent.cpp:56: ASSERT(!config.isJust()); Looks like this should be a check that would return protocol error rather than an assert -- i.e. if (!config.isJust()) { *errorString = "Using trace config on render targets is not yet supported"; return; } https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:4855: { "name": "recordMode", "$ref": "Tracing.RecordMode", "optional": true, "description": "The option that determines how the trace buffer stores data." }, nit: here and below, drop "The option that" from description, e.g. "control how trace buffer stores data". https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:4856: { "name": "enableSampling", "type": "boolean", "optional": true, "description": "The option that enables sampling." }, "description": "Turn on JavaScript stack sampling"
https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:4862: { "name": "memoryDumpConfig", "$ref": "Tracing.MemoryDumpConfig", "optional": true, "description": "The configuration for memory dump. Used only when \"memory-infra\" category is enabled." } "$ref": "MemoryDumpConfig", remove "Tracing." prefix. https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:4876: { "name": "traceConfig", "$ref": "Tracing.TraceConfig", "optional": true, "description": "" } ditto.
https://codereview.chromium.org/1765153002/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/tracing_handler.cc (right): https://codereview.chromium.org/1765153002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/tracing_handler.cc:38: const char separator) { On 2016/03/12 00:13:32, caseq wrote: > nit: nuke const from parameter passed by value. Done. https://codereview.chromium.org/1765153002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/tracing_handler.cc:187: "Both trace config and categories/options are specified. Only one way " On 2016/03/12 00:13:32, caseq wrote: > nit: "Either trace config (preferred) or categories/options should be specified, > but not both." Done. https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorTracingAgent.cpp (right): https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorTracingAgent.cpp:56: ASSERT(!config.isJust()); On 2016/03/12 00:13:32, caseq wrote: > Looks like this should be a check that would return protocol error rather than > an assert -- i.e. > > if (!config.isJust()) { > *errorString = "Using trace config on render targets is not yet supported"; > return; > } Done. https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:4855: { "name": "recordMode", "$ref": "Tracing.RecordMode", "optional": true, "description": "The option that determines how the trace buffer stores data." }, On 2016/03/12 00:13:33, caseq wrote: > nit: here and below, drop "The option that" from description, e.g. "control how > trace buffer stores data". Done. https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:4856: { "name": "enableSampling", "type": "boolean", "optional": true, "description": "The option that enables sampling." }, On 2016/03/12 00:13:33, caseq wrote: > "description": "Turn on JavaScript stack sampling" Done. https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:4862: { "name": "memoryDumpConfig", "$ref": "Tracing.MemoryDumpConfig", "optional": true, "description": "The configuration for memory dump. Used only when \"memory-infra\" category is enabled." } On 2016/03/12 00:33:33, caseq wrote: > "$ref": "MemoryDumpConfig", remove "Tracing." prefix. Done. https://codereview.chromium.org/1765153002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:4876: { "name": "traceConfig", "$ref": "Tracing.TraceConfig", "optional": true, "description": "" } On 2016/03/12 00:33:33, caseq wrote: > ditto. Done.
Primiano, can you take a look at trace_config.[cc|h] again? :)
Many thanks for the work, the layering looks great in this patchset. I have some minor comments here and there. Also: - I think components/tracing/trace_config_file.cc can already benefit of your cl. - Can you cover this new ctor in the trace_config_unittest? thanks! https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:108: TraceConfig::TraceConfig(const base::DictionaryValue& config) { ditto https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:295: void TraceConfig::InitializeFromConfigDict(const base::DictionaryValue& dict) { ditto (base::) https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:353: scoped_ptr<base::DictionaryValue> dict( I know that this is not your fault and you are just moving the code, but I think there is a cleaner way to rewrite this code without cast that is: scoped_ptr<base::Value> value(base::JSONReader::Read(config_string)); const DictionaryValue* dict = nullptr; bool is_dictionary = false; if (value) is_dictionary = value->GetAsDictionary(&dict); if (!is_dictionary) return InitDefault(); DCHECK(dict); InitializeFromConfigDict(*dict); https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:354: static_cast<base::DictionaryValue*>(value.release())); nit: base:: https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:133: // Create TraceConfig object from the trace config dictionary. I would probably reword this comment as: // Functionally identical to the above, but takes a parsed dictionary as input instead of its JSON serialization. https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:134: explicit TraceConfig(const base::DictionaryValue& config); Since you introduced this, I think you can make a 2 line fix to components/tracing/trace_config_file.cc. That one is creating a DictionaryValue and serializing to JSON. THe latter does not need to happen anymore thanks to your CL https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:134: explicit TraceConfig(const base::DictionaryValue& config); nit: no need of base::, you are already in namespace base :) https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:134: explicit TraceConfig(const base::DictionaryValue& config); Can we haz a unittest to cover this casE? I think you want to refactor some of the existing ones, so you can call them either from the string or dictionary. https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:197: void InitializeFromConfigDict(const base::DictionaryValue& dict); nit, ditto: no need of base:: https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... File content/browser/devtools/protocol/tracing_handler.cc (right): https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.cc:39: for (const char& c : in_str) { maybe if you add a out_str.reserve(in_str.size()) this might be slighly faster
primiano@chromium.org changed reviewers: + petrcermak@chromium.org
+petrcermak, can you take a look to this to see if everything looks sane to you?
Looks good overall. Here's just a couple of comments. One patch title and description nit: s/take trace config/accept trace config as a parameter/g Thanks, Petr https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:196: // Initialize from the config dictionary. supernit: s/the/a/ https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:199: // Initialize from the config string. ditto (s/the/a/) https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... File content/browser/devtools/protocol/tracing_handler.cc (right): https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.cc:65: for (const auto& value : *list) { nit: No need for braces https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.cc:186: "Either trace config (preferred) or categories/options should be " nit: I'd say "categories+options" instead of "categories/options". https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.cc:186: "Either trace config (preferred) or categories/options should be " nit: Since this is an exclusive "or", there should be a comma in front of it. https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... File content/browser/devtools/protocol/tracing_handler.h (right): https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.h:80: GetTraceConfigFromDevtoolsConfig( nit: s/Devtools/DevTools/g (everywhere including the patch description) https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorTracingAgent.cpp (right): https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorTracingAgent.cpp:57: "Using trace config on render targets is not yet supported."; super nit: s/yet supported/supported yet/ https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorTracingAgent.cpp:57: "Using trace config on render targets is not yet supported."; Also, shouldn't it be "renderer"? https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4832: "description": "The option that determines how the trace buffer stores data." nit: I do NOT think the descriptions should start with "The". I think it should just be "Option that ..." https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4838: { "name": "mode", "type": "string", "optional": false, "description": "" }, Shouldn't we have an enum for this? How else will the user know what are valid values??? Primiano: WDYT? Also I think you should provide an actual description. https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4839: { "name": "periodicIntervalMs", "type": "integer", "optional": false, "description": "" } I think you should provide an actual description. https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4841: "description": "The configurations that trigger memory dump." ditto drop "The" https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4849: "description": "The configuration for memory dump. Used only when \"memory-infra\" category is enabled." ditto drop "The" https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4859: { "name": "includedCategories", "type": "array", "items": { "type": "string" }, "optional": true, "description": "The category filters that are included." }, ditto drop "The" https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4860: { "name": "excludedCategories", "type": "array", "items": { "type": "string" }, "optional": true, "description": "The category filters that are excluded." }, ditto drop "The" https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4861: { "name": "syntheticDelays", "type": "array", "items": { "type": "string" }, "optional": true, "description": "The configuration to synthesize the delays in tracing." }, ditto drop "The" https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4861: { "name": "syntheticDelays", "type": "array", "items": { "type": "string" }, "optional": true, "description": "The configuration to synthesize the delays in tracing." }, Shouldn't this be an enum?? https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4862: { "name": "memoryDumpConfig", "$ref": "MemoryDumpConfig", "optional": true, "description": "The configuration for memory dump. Used only when \"memory-infra\" category is enabled." } ditto drop "The"
I have uploaded a new patch according to primiano and petrcermak's comments. ptal https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:108: TraceConfig::TraceConfig(const base::DictionaryValue& config) { On 2016/03/14 16:53:02, Primiano wrote: > ditto Done. https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:295: void TraceConfig::InitializeFromConfigDict(const base::DictionaryValue& dict) { On 2016/03/14 16:53:02, Primiano wrote: > ditto (base::) Done. https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:353: scoped_ptr<base::DictionaryValue> dict( On 2016/03/14 16:53:02, Primiano wrote: > I know that this is not your fault and you are just moving the code, but I think > there is a cleaner way to rewrite this code without cast that is: > > scoped_ptr<base::Value> value(base::JSONReader::Read(config_string)); > const DictionaryValue* dict = nullptr; > bool is_dictionary = false; > if (value) > is_dictionary = value->GetAsDictionary(&dict); > if (!is_dictionary) > return InitDefault(); > DCHECK(dict); > InitializeFromConfigDict(*dict); Done. https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:354: static_cast<base::DictionaryValue*>(value.release())); On 2016/03/14 16:53:02, Primiano wrote: > nit: base:: Done. https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:133: // Create TraceConfig object from the trace config dictionary. On 2016/03/14 16:53:02, Primiano wrote: > I would probably reword this comment as: > // Functionally identical to the above, but takes a parsed dictionary as input > instead of its JSON serialization. Done. https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:134: explicit TraceConfig(const base::DictionaryValue& config); Updated trace_config_file.cc and trace_config_unittest.cc https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:196: // Initialize from the config dictionary. On 2016/03/14 18:05:09, petrcermak wrote: > supernit: s/the/a/ Done. https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:197: void InitializeFromConfigDict(const base::DictionaryValue& dict); On 2016/03/14 16:53:02, Primiano wrote: > nit, ditto: no need of base:: Done. https://codereview.chromium.org/1765153002/diff/100001/base/trace_event/trace... base/trace_event/trace_config.h:199: // Initialize from the config string. On 2016/03/14 18:05:09, petrcermak wrote: > ditto (s/the/a/) Done. https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... File content/browser/devtools/protocol/tracing_handler.cc (right): https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.cc:39: for (const char& c : in_str) { On 2016/03/14 16:53:02, Primiano wrote: > maybe if you add a out_str.reserve(in_str.size()) this might be slighly faster Done. https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.cc:65: for (const auto& value : *list) { On 2016/03/14 18:05:09, petrcermak wrote: > nit: No need for braces Done. https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.cc:186: "Either trace config (preferred) or categories/options should be " done https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... File content/browser/devtools/protocol/tracing_handler.h (right): https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.h:80: GetTraceConfigFromDevtoolsConfig( On 2016/03/14 18:05:09, petrcermak wrote: > nit: s/Devtools/DevTools/g (everywhere including the patch description) Done. https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorTracingAgent.cpp (right): https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorTracingAgent.cpp:57: "Using trace config on render targets is not yet supported."; done https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4832: "description": "The option that determines how the trace buffer stores data." On 2016/03/14 18:05:09, petrcermak wrote: > nit: I do NOT think the descriptions should start with "The". I think it should > just be "Option that ..." Done. https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4838: { "name": "mode", "type": "string", "optional": false, "description": "" }, On 2016/03/14 18:05:09, petrcermak wrote: > Shouldn't we have an enum for this? How else will the user know what are valid > values??? > > Primiano: WDYT? > > Also I think you should provide an actual description. Updated this to an enum. https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4839: { "name": "periodicIntervalMs", "type": "integer", "optional": false, "description": "" } On 2016/03/14 18:05:09, petrcermak wrote: > I think you should provide an actual description. How about the new one? https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4841: "description": "The configurations that trigger memory dump." On 2016/03/14 18:05:09, petrcermak wrote: > ditto drop "The" Done. https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4849: "description": "The configuration for memory dump. Used only when \"memory-infra\" category is enabled." On 2016/03/14 18:05:09, petrcermak wrote: > ditto drop "The" Done. https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4859: { "name": "includedCategories", "type": "array", "items": { "type": "string" }, "optional": true, "description": "The category filters that are included." }, On 2016/03/14 18:05:09, petrcermak wrote: > ditto drop "The" Done. https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4860: { "name": "excludedCategories", "type": "array", "items": { "type": "string" }, "optional": true, "description": "The category filters that are excluded." }, On 2016/03/14 18:05:09, petrcermak wrote: > ditto drop "The" Done. https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4861: { "name": "syntheticDelays", "type": "array", "items": { "type": "string" }, "optional": true, "description": "The configuration to synthesize the delays in tracing." }, On 2016/03/14 18:05:09, petrcermak wrote: > Shouldn't this be an enum?? No. I think this is different. Synthetic delays are specified as list of strings, like categories. And the string contains event name, which can be arbitrary. https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t... https://codereview.chromium.org/1765153002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4862: { "name": "memoryDumpConfig", "$ref": "MemoryDumpConfig", "optional": true, "description": "The configuration for memory dump. Used only when \"memory-infra\" category is enabled." } On 2016/03/14 18:05:09, petrcermak wrote: > ditto drop "The" Done.
Description was changed from ========== Update Devtools Tracing.Start to take trace config This CL updates Tracing.Start to take trace config when starting tracing. It is backward compatible with the old way. Design doc: https://goo.gl/GxQ23k BUG=579358 ========== to ========== Update Devtools Tracing.Start to accept trace config as a parameter This CL updates Tracing.Start to accept trace config as a parameter when starting tracing. It is backward compatible with the old way. Design doc: https://goo.gl/GxQ23k BUG=579358 ==========
LGTM with a couple more nits. Please wait for Primiano to do his final pass. Thanks and sorry for being so pedantic :-) Petr https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... File content/browser/devtools/protocol/tracing_handler.h (right): https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.h:80: GetTraceConfigFromDevtoolsConfig( On 2016/03/14 21:41:45, Zhen Wang wrote: > On 2016/03/14 18:05:09, petrcermak wrote: > > nit: s/Devtools/DevTools/g (everywhere including the patch description) > > Done. You haven't updated the patch title and description though :-) https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4838: "description": "Option that determines how to trigger memory dumps." I think that the id should be "MemoryDumpLevelOfDetail" and the description should be "Level of detail of a memory dump." Primiano: What do you think about the naming here? The field in the trace config is indeed called "mode", but it's then stored as a "level_of_detail" (https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...). Do we want to go with my above proposal for id ("MemoryDumpLevelOfDetail"), or should we match the public trace config and keep "MemoryDumpTriggerMode"? What about the description? "Level of detail of a memory dump." or a generic "Memory dump trigger mode."? https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4844: { "name": "mode", "$ref": "MemoryDumpTriggerMode", "optional": false, "description": "Option that determines how to trigger memory dumps." }, s/Option that .../Level of detail of the triggered memory dumps./ https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4845: { "name": "periodicIntervalMs", "type": "integer", "optional": false, "description": "Length of the periodic intervals to trigger memory dumps in milliseconds." } nit: s/intervals to trigger memory dumps/intervals between the triggered memory dumps/ https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4847: "description": "Configurations that trigger memory dump." s/Configurations .../Memory dump trigger configuration./ https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4853: { "name": "triggers", "type": "array", "items": { "$ref": "MemoryDumpTrigger" }, "optional": false, "description": "Configurations that trigger memory dump." } s/Configurations .../Memory dump trigger configurations./ https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4855: "description": "Configuration for memory dump. Used only when \"memory-infra\" category is enabled." nit: s/memory dump/memory dump triggers/ https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4865: { "name": "includedCategories", "type": "array", "items": { "type": "string" }, "optional": true, "description": "Category filters that are included." }, "Included category filters." https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4866: { "name": "excludedCategories", "type": "array", "items": { "type": "string" }, "optional": true, "description": "Category filters that are excluded." }, "Excluded category filters." https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4868: { "name": "memoryDumpConfig", "$ref": "MemoryDumpConfig", "optional": true, "description": "Configuration for memory dump. Used only when \"memory-infra\" category is enabled." } nit: s/memory dump/memory dump triggers/
LGTM with 2 final comments (please look at them) https://codereview.chromium.org/1765153002/diff/120001/base/trace_event/trace... File base/trace_event/trace_config_unittest.cc (right): https://codereview.chromium.org/1765153002/diff/120001/base/trace_event/trace... base/trace_event/trace_config_unittest.cc:298: DCHECK(default_value->GetAsDictionary(&default_dict)); Don't put code with side effects in DHCEKC. I expect this test to fail in release builds if you don't have dcheck_always_on=true (the bot do). You want here: res = ...GetAsDictionary ASSERT_TRUE(res) https://codereview.chromium.org/1765153002/diff/120001/components/tracing/tra... File components/tracing/trace_config_file.cc (right): https://codereview.chromium.org/1765153002/diff/120001/components/tracing/tra... components/tracing/trace_config_file.cc:110: DCHECK(trace_config_dict); If you dereference a ptr there is no point adding a DCHECK, your code will crash on the next line anyways, DCHECK does not add anything useful. It's different if you were storing a pointer and reusing it later.
https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4838: "description": "Option that determines how to trigger memory dumps." On 2016/03/15 12:15:07, petrcermak wrote: > I think that the id should be "MemoryDumpLevelOfDetail" and the description > should be "Level of detail of a memory dump." > > Primiano: What do you think about the naming here? The field in the trace config > is indeed called "mode", but it's then stored as a "level_of_detail" > (https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...). > Do we want to go with my above proposal for id ("MemoryDumpLevelOfDetail"), or > should we match the public trace config and keep "MemoryDumpTriggerMode"? What > about the description? "Level of detail of a memory dump." or a generic "Memory > dump trigger mode."? I think we should reflect the trace_config as much as possible [1] MDTMode honestly looks good to me here. [1] https://chromium.googlesource.com/chromium/src/+/master/components/tracing/do...
Description was changed from ========== Update Devtools Tracing.Start to accept trace config as a parameter This CL updates Tracing.Start to accept trace config as a parameter when starting tracing. It is backward compatible with the old way. Design doc: https://goo.gl/GxQ23k BUG=579358 ========== to ========== Update DevTools Tracing.Start to accept trace config as a parameter This CL updates Tracing.Start to accept trace config as a parameter when starting tracing. It is backward compatible with the old way. Design doc: https://goo.gl/GxQ23k BUG=579358 ==========
https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... File content/browser/devtools/protocol/tracing_handler.h (right): https://codereview.chromium.org/1765153002/diff/100001/content/browser/devtoo... content/browser/devtools/protocol/tracing_handler.h:80: GetTraceConfigFromDevtoolsConfig( On 2016/03/15 12:15:07, petrcermak wrote: > On 2016/03/14 21:41:45, Zhen Wang wrote: > > On 2016/03/14 18:05:09, petrcermak wrote: > > > nit: s/Devtools/DevTools/g (everywhere including the patch description) > > > > Done. > > You haven't updated the patch title and description though :-) Ah, right. Thanks! Fixed. :) https://codereview.chromium.org/1765153002/diff/120001/base/trace_event/trace... File base/trace_event/trace_config_unittest.cc (right): https://codereview.chromium.org/1765153002/diff/120001/base/trace_event/trace... base/trace_event/trace_config_unittest.cc:298: DCHECK(default_value->GetAsDictionary(&default_dict)); On 2016/03/15 12:22:41, Primiano wrote: > Don't put code with side effects in DHCEKC. > I expect this test to fail in release builds if you don't have > dcheck_always_on=true (the bot do). > > You want here: > res = ...GetAsDictionary > ASSERT_TRUE(res) Ah, right. Thanks! done. https://codereview.chromium.org/1765153002/diff/120001/components/tracing/tra... File components/tracing/trace_config_file.cc (right): https://codereview.chromium.org/1765153002/diff/120001/components/tracing/tra... components/tracing/trace_config_file.cc:110: DCHECK(trace_config_dict); On 2016/03/15 12:22:41, Primiano wrote: > If you dereference a ptr there is no point adding a DCHECK, your code will crash > on the next line anyways, DCHECK does not add anything useful. > It's different if you were storing a pointer and reusing it later. I see. removed. https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4838: "description": "Option that determines how to trigger memory dumps." On 2016/03/15 15:36:52, Primiano wrote: > On 2016/03/15 12:15:07, petrcermak wrote: > > I think that the id should be "MemoryDumpLevelOfDetail" and the description > > should be "Level of detail of a memory dump." > > > > Primiano: What do you think about the naming here? The field in the trace > config > > is indeed called "mode", but it's then stored as a "level_of_detail" > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...). > > Do we want to go with my above proposal for id ("MemoryDumpLevelOfDetail"), or > > should we match the public trace config and keep "MemoryDumpTriggerMode"? What > > about the description? "Level of detail of a memory dump." or a generic > "Memory > > dump trigger mode."? > I think we should reflect the trace_config as much as possible [1] > MDTMode honestly looks good to me here. > > [1] > https://chromium.googlesource.com/chromium/src/+/master/components/tracing/do... > Acknowledged. Updated the description. https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4844: { "name": "mode", "$ref": "MemoryDumpTriggerMode", "optional": false, "description": "Option that determines how to trigger memory dumps." }, On 2016/03/15 12:15:08, petrcermak wrote: > s/Option that .../Level of detail of the triggered memory dumps./ Done. https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4845: { "name": "periodicIntervalMs", "type": "integer", "optional": false, "description": "Length of the periodic intervals to trigger memory dumps in milliseconds." } On 2016/03/15 12:15:08, petrcermak wrote: > nit: s/intervals to trigger memory dumps/intervals between the triggered memory > dumps/ Done. https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4847: "description": "Configurations that trigger memory dump." On 2016/03/15 12:15:08, petrcermak wrote: > s/Configurations .../Memory dump trigger configuration./ Done. https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4853: { "name": "triggers", "type": "array", "items": { "$ref": "MemoryDumpTrigger" }, "optional": false, "description": "Configurations that trigger memory dump." } On 2016/03/15 12:15:08, petrcermak wrote: > s/Configurations .../Memory dump trigger configurations./ Done. https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4855: "description": "Configuration for memory dump. Used only when \"memory-infra\" category is enabled." On 2016/03/15 12:15:08, petrcermak wrote: > nit: s/memory dump/memory dump triggers/ Done. https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4865: { "name": "includedCategories", "type": "array", "items": { "type": "string" }, "optional": true, "description": "Category filters that are included." }, On 2016/03/15 12:15:08, petrcermak wrote: > "Included category filters." Done. https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4866: { "name": "excludedCategories", "type": "array", "items": { "type": "string" }, "optional": true, "description": "Category filters that are excluded." }, On 2016/03/15 12:15:08, petrcermak wrote: > "Excluded category filters." Done. https://codereview.chromium.org/1765153002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4868: { "name": "memoryDumpConfig", "$ref": "MemoryDumpConfig", "optional": true, "description": "Configuration for memory dump. Used only when \"memory-infra\" category is enabled." } On 2016/03/15 12:15:08, petrcermak wrote: > nit: s/memory dump/memory dump triggers/ Done.
The CQ bit was checked by zhenw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org, primiano@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1765153002/#ps140001 (title: "review fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1765153002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1765153002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zhenw@chromium.org changed reviewers: + pfeldman@chromium.org, simonhatch@chromium.org
+simonhatch for components/tracing/trace_config_file.cc +pfeldman for devtools
https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4829: "id": "RecordMode", Inline enum, call property "bufferType". https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4835: "id": "MemoryDumpTriggerMode", Inline enum. https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4844: { "name": "mode", "$ref": "MemoryDumpTriggerMode", "optional": false, "description": "Level of detail of the triggered memory dumps." }, remove "optional": false. https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4845: { "name": "periodicIntervalMs", "type": "integer", "optional": false, "description": "Length of the periodic intervals between the triggered memory dumps." } ditto https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4845: { "name": "periodicIntervalMs", "type": "integer", "optional": false, "description": "Length of the periodic intervals between the triggered memory dumps." } Everything is ms, this could be called "interval". https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4853: { "name": "triggers", "type": "array", "items": { "$ref": "MemoryDumpTrigger" }, "optional": false, "description": "Memory dump trigger configuration." } So we have an array of triggers, but they only carry "mode" and "interval". Are you defining an interval for "light" and an interval for "detailed"? I.e. array is up to two items until you get more modes? https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4861: { "name": "recordMode", "$ref": "RecordMode", "optional": true, "description": "Controls how the trace buffer stores data." }, bufferType ? https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4862: { "name": "enableSampling", "type": "boolean", "optional": true, "description": "Turns on JavaScript stack sampling." }, Should we get an array of "features" you'd like enabled to make it more scaleable? agents: ["systrace", "sampling", etc.] so that you could easily add more and conditionally trigger them? i.e. array of enum. https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4865: { "name": "includedCategories", "type": "array", "items": { "type": "string" }, "optional": true, "description": "Included category filters." }, "include" and "exclude" ? https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4867: { "name": "syntheticDelays", "type": "array", "items": { "type": "string" }, "optional": true, "description": "Configuration to synthesize the delays in tracing." }, What does this do? Can it be computed on the client side? https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4868: { "name": "memoryDumpConfig", "$ref": "MemoryDumpConfig", "optional": true, "description": "Configuration for memory dump triggers. Used only when \"memory-infra\" category is enabled." } memory.
components/tracing/trace_config_file.cc lgtm
Hi Pavel, As we discussed offline, the names of the properties come from the trace config format. You can see an example here: https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t... And the names in protocol.json are camel case versions. They will be converted to underscore versions in tracing handler. Please see other inline replies below. Best -Zhen https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4829: "id": "RecordMode", On 2016/03/15 18:03:53, pfeldman wrote: > Inline enum, call property "bufferType". Done. https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4835: "id": "MemoryDumpTriggerMode", On 2016/03/15 18:03:53, pfeldman wrote: > Inline enum. Done. https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4844: { "name": "mode", "$ref": "MemoryDumpTriggerMode", "optional": false, "description": "Level of detail of the triggered memory dumps." }, On 2016/03/15 18:03:53, pfeldman wrote: > remove "optional": false. Done. https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4853: { "name": "triggers", "type": "array", "items": { "$ref": "MemoryDumpTrigger" }, "optional": false, "description": "Memory dump trigger configuration." } On 2016/03/15 18:03:53, pfeldman wrote: > So we have an array of triggers, but they only carry "mode" and "interval". Are > you defining an interval for "light" and an interval for "detailed"? I.e. array > is up to two items until you get more modes? Currently there are only two items by default. I think it could be more in the future. https://codereview.chromium.org/1765153002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4867: { "name": "syntheticDelays", "type": "array", "items": { "type": "string" }, "optional": true, "description": "Configuration to synthesize the delays in tracing." }, On 2016/03/15 18:03:53, pfeldman wrote: > What does this do? Can it be computed on the client side? This simulates additional delays in the browser for the specified trace events.
lgtm
The CQ bit was checked by zhenw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, caseq@chromium.org, simonhatch@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1765153002/#ps180001 (title: "inline enum")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1765153002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1765153002/180001
Message was sent while issue was closed.
Description was changed from ========== Update DevTools Tracing.Start to accept trace config as a parameter This CL updates Tracing.Start to accept trace config as a parameter when starting tracing. It is backward compatible with the old way. Design doc: https://goo.gl/GxQ23k BUG=579358 ========== to ========== Update DevTools Tracing.Start to accept trace config as a parameter This CL updates Tracing.Start to accept trace config as a parameter when starting tracing. It is backward compatible with the old way. Design doc: https://goo.gl/GxQ23k BUG=579358 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Update DevTools Tracing.Start to accept trace config as a parameter This CL updates Tracing.Start to accept trace config as a parameter when starting tracing. It is backward compatible with the old way. Design doc: https://goo.gl/GxQ23k BUG=579358 ========== to ========== Update DevTools Tracing.Start to accept trace config as a parameter This CL updates Tracing.Start to accept trace config as a parameter when starting tracing. It is backward compatible with the old way. Design doc: https://goo.gl/GxQ23k BUG=579358 Committed: https://crrev.com/c0e3792f832fc454324dbe35d01079b124b072cd Cr-Commit-Position: refs/heads/master@{#381783} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c0e3792f832fc454324dbe35d01079b124b072cd Cr-Commit-Position: refs/heads/master@{#381783}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/1814043002/ by fgorski@chromium.org. The reason for reverting is: TraceConfigTest.TraceConfigFromDict fialed https://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/2128... https://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/21289. |
