Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
331303002:
Add Tracing.getCategories to protocol.json. (Closed)
|
Created:
6 years, 6 months ago by chrishenry Modified:
6 years, 6 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionAdd Tracing.getCategories to protocol.json.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176439
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address code review comment. #
Total comments: 2
Patch Set 3 : Address review comment. #Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:4136: "handlers": ["browser", "frontend"] I'm not sure about this handlers field. Do you know what this is for? What would be the right value here? I just copied it from Tracing.end above.
https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:4134: { "name": "categories", "type": "array", "description": "A list of supported tracing categories." } You probably want items: { "type": "string" } here. https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:4136: "handlers": ["browser", "frontend"] On 2014/06/17 17:23:50, chrishenry wrote: > I'm not sure about this handlers field. Do you know what this is for? What would > be the right value here? I just copied it from Tracing.end above. This one is correct (this tells the code generator in what components the code needs to be generated; here, the command will be sent from front-end to the browser and will not be handled in the renderer, so you just want these two).
https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:4134: { "name": "categories", "type": "array", "description": "A list of supported tracing categories." } On 2014/06/17 18:38:23, caseq wrote: > You probably want items: { "type": "string" } here. Curious, why should this be string instead of an array? The categories are really a list of strings. I can of course send them as comma-separated string. Here's the implementation: https://codereview.chromium.org/339743004/ This isn't working yet, so far I'm getting undefined from ChromeShell. s: https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:4136: "handlers": ["browser", "frontend"] On 2014/06/17 18:38:23, caseq wrote: > On 2014/06/17 17:23:50, chrishenry wrote: > > I'm not sure about this handlers field. Do you know what this is for? What > would > > be the right value here? I just copied it from Tracing.end above. > > This one is correct (this tells the code generator in what components the code > needs to be generated; here, the command will be sent from front-end to the > browser and will not be handled in the renderer, so you just want these two). Thank you! I'm curious why Tracing.start needs to be handled in the renderer, but Tracing.end doesn't. What does it mean by handled by the renderer?
A few more questions if you don't mind (I'm very new to this): 1) Is there a way to test the new code locally without building ChromeShell for android? 2) What happen if we connect to a browser instance that doesn't support Tracing.getCategories? How can I ensure that it doesn't explode (there is a good default we can use for older browsers)?
On 2014/06/17 18:53:41, chrishenry wrote: > https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.json > File Source/devtools/protocol.json (right): > > https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.jso... > Source/devtools/protocol.json:4134: { "name": "categories", "type": "array", > "description": "A list of supported tracing categories." } > On 2014/06/17 18:38:23, caseq wrote: > > You probably want items: { "type": "string" } here. > > Curious, why should this be string instead of an array? The categories are > really a list of strings. I can of course send them as comma-separated string. "items": { "type": "string" } goes in addition to "type": "array" and specifies the type of array items, so it won't be / shouldn't be a string, it will still be an array of strings. Have a look at other arrays there: https://code.google.com/p/chromium/codesearch#search/&q=file:protocol.json%20... > > I'm curious why Tracing.start needs to be handled in the renderer, but > Tracing.end doesn't. What does it mean by handled by the renderer? There's a bit of magic there -- we used to handle Tracing.start in the browser only, in the way quite similar to Tracing.end. Then we realized that for using trace events to build timeline in DevTools we'd like to have some token that identifies the inspected page and we want it to be emitted as soon as tracing is started. So in addition to handling the command in the browser, we're also getting it in the renderer (provided your target is renderer, not browser) and use it to return the session id and emit a couple of timeline-specific trace events. > 1) Is there a way to test the new code locally without building ChromeShell for android? Well, there's nothing android-specific in the files you touch, so you should be able to test that in anything content-based, i.e. either content_shell or chromium for any platform. > 2) What happen if we connect to a browser instance that doesn't support > Tracing.getCategories? How can I ensure that it doesn't explode (there is a good > default we can use for older browsers)? You're supposed to get a protocol error, MethodNotFound, though I have to admit I failed to find a test for that right now (perhaps Pavel can help here).
On 2014/06/17 19:26:27, caseq wrote: > On 2014/06/17 18:53:41, chrishenry wrote: > > https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.json > > File Source/devtools/protocol.json (right): > > > > > https://codereview.chromium.org/331303002/diff/1/Source/devtools/protocol.jso... > > Source/devtools/protocol.json:4134: { "name": "categories", "type": "array", > > "description": "A list of supported tracing categories." } > > On 2014/06/17 18:38:23, caseq wrote: > > > You probably want items: { "type": "string" } here. > > > > Curious, why should this be string instead of an array? The categories are > > really a list of strings. I can of course send them as comma-separated string. > > "items": { "type": "string" } goes in addition to "type": "array" and specifies > the type of array items, so it won't be / shouldn't be a string, it will still > be an array of strings. Have a look at other arrays there: > https://code.google.com/p/chromium/codesearch#search/&q=file:protocol.json%20... Done, thanks for catching this! > > > > > I'm curious why Tracing.start needs to be handled in the renderer, but > > Tracing.end doesn't. What does it mean by handled by the renderer? > > There's a bit of magic there -- we used to handle Tracing.start in the browser > only, in the way quite similar to Tracing.end. Then we realized that for using > trace events to build timeline in DevTools we'd like to have some token that > identifies the inspected page and we want it to be emitted as soon as tracing is > started. So in addition to handling the command in the browser, we're also > getting it in the renderer (provided your target is renderer, not browser) and > use it to return the session id and emit a couple of timeline-specific trace > events. > > > 1) Is there a way to test the new code locally without building ChromeShell > for > android? > > Well, there's nothing android-specific in the files you touch, so you should be > able to test that in anything content-based, i.e. either content_shell or > chromium for any platform. I guess my question was about how to test this at all. I don't know how to invoke the inspector API and couldn't find any reference to it. I did manage to test this with chrome://inspect + chrome://tracing, but would love to know whether I can invoke Tracing.getCategories on the current Chrome instance. > > > 2) What happen if we connect to a browser instance that doesn't support > > Tracing.getCategories? How can I ensure that it doesn't explode (there is a > good > > default we can use for older browsers)? > > You're supposed to get a protocol error, MethodNotFound, though I have to admit > I failed to find a test for that right now (perhaps Pavel can help here). Yes, I do get this, yay!
lgtm https://codereview.chromium.org/331303002/diff/20001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/331303002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:4135: "name": "categories", nit: we use line-per parameter code style here.
Thanks for the review! https://codereview.chromium.org/331303002/diff/20001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/331303002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:4135: "name": "categories", On 2014/06/18 03:35:00, pfeldman wrote: > nit: we use line-per parameter code style here. Done.
The CQ bit was checked by chrishenry@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrishenry@google.com/331303002/40001
Message was sent while issue was closed.
Change committed as 176439 |
