Summary: | Web Inspector: Provide inspector extension API to access timeline data | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Schneider <michschn> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, caseq, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | 61529 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Michael Schneider
2011-05-18 17:00:18 PDT
Created attachment 94008 [details]
Proposed Patch
This proposed patch adds an webInspector.timeline API. It contains three event sinks:
- onStarted: Listeners are notified when the backend starts collecting timeline data.
- onStopped: Listeners are notified when the backend stops collecting timeline data.
- onEventRecorded: Listeners are notified when a timeline record is collected. The record is passed as first argument to the listener.
The start/stop is implicitly controlled if onEventRecorded listeners are present.
For it to work, a TimelineManager was introduced to broadcast the events received via InspectorBackend.registerDomainDispatcher("Timeline", ...). The TimelinePanel now registers at the TimelineManager to receive events.
Comment on attachment 94008 [details] Proposed Patch Attachment 94008 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8711532 New failing tests: inspector/timeline/timeline-network-resource.html Created attachment 94013 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 94008 [details]
Proposed Patch
Looks like I overlooked this test failure and submitted the patch prematurely.
After looking at the failure, my initial analysis is as follows:
- The TimelinePanel was active all the time before, now it ignores the events when the it is not activated.
- The TimelineTests just activate the instrumentation data collection using TimelineAgent.start().
- The WebInspector.TimelinePanel.FormattedRecord constructor has a side effect, which adds the now missing url property to the record.
- This function is not called anymore, as the TimelinePanel is in a disabled state.
- The failing test explicitly checks for that side effect.
IMO, this side effect must either be keept local to the TimelinePanel or be applied to all listeners, regardless if the the TimelinePanel is activated or not.
- we could copy the record, then modify the test to enable the TimelinePanel.
- we could add the URL property in the TimelineManager already, so that every listener receives the modified record
What do you think? I do not know what the best solution is here.
Comment on attachment 94008 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94008&action=review > Source/WebCore/inspector/front-end/ExtensionAPI.js:385 > + this.onStopped = new EventSink("timeline-stopped"); Why do we need onStarted/onStopped? Looks like start/stop will only be triggered upon client actions (i.e. subscription, unsubscription), so we don't seem to need another notification that timeline has actually started/stopped. > Source/WebCore/inspector/front-end/TimelinePanel.js:281 > + this.toggleTimelineButton.toggled ^= true; this.toggleTimelineButton.toggled = !this.toggleTimelineButton.toggled; We don't use bit-wise ^ on booleans normally. This works, but will change type from Boolean to Number, which looks a bit strange. (In reply to comment #4) > (From update of attachment 94008 [details]) > Looks like I overlooked this test failure and submitted the patch prematurely. > > After looking at the failure, my initial analysis is as follows: > - The TimelinePanel was active all the time before, now it ignores the events when the it is not activated. > - The TimelineTests just activate the instrumentation data collection using TimelineAgent.start(). > - The WebInspector.TimelinePanel.FormattedRecord constructor has a side effect, which adds the now missing url property to the record. > - This function is not called anymore, as the TimelinePanel is in a disabled state. > - The failing test explicitly checks for that side effect. > > IMO, this side effect must either be keept local to the TimelinePanel or be applied to all listeners, regardless if the the TimelinePanel is activated or not. > - we could copy the record, then modify the test to enable the TimelinePanel. > - we could add the URL property in the TimelineManager already, so that every listener receives the modified record > > What do you think? I do not know what the best solution is here. This is definitely weird on part of TimelinePanel. We discussed this with loislo, and it looks like this belongs to the higher level (i.e. panel), so let's start copying the event record in the panel. Comment on attachment 94008 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94008&action=review > Source/WebCore/inspector/front-end/TimelineManager.js:85 > + this._manager.dispatchEventToListeners(WebInspector.TimelineManager.EventTypes.TimelineEventRecorded, record); We should fix TimelinePanel to make it not mutate records before we land this. Comment on attachment 94008 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94008&action=review > Source/WebCore/inspector/front-end/TimelineManager.js:48 > + if (this._enablementCount == 1) We normally use === instead of == where possible. > Source/WebCore/inspector/front-end/TimelineManager.js:54 > + if (this._enablementCount == 0) { We normally put this as !this._enablementCount (or at least use ===) > Source/WebCore/inspector/front-end/TimelineManager.js:59 > + if (this._enablementCount == 0) ditto. > Source/WebCore/inspector/front-end/TimelinePanel.js:303 > if (record.type == WebInspector.TimelineAgent.RecordType.ResourceSendRequest) { === Created attachment 94123 [details]
Proposed patch
- Fixed the failing test by copying the record, enabling the timeline panel from the test and sniffing for the records on the timeline panel.
- Incorporated review feedback.
- removed onStarted and onStopped
There are open questions:
- What is a good way to copy the record, is there a utility? I use JSON.parse(JSON.stringify(record)) currently
- When adding a listener, the actual start of the instrumentation could be delayed. Do we have to add a callback to EventSink.addListener which is called as soon as the subscription is successful (seems strange, in this case I would rather have a explicit start/stop again)? Or can we neglect that? If the async requests wont be executed out-of-order, this is probably not an issue.
Comment on attachment 94123 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=94123&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:281 > + this.toggleTimelineButton.toggled != this.toggleTimelineButton.toggled; Does this work? != is a boolean operator, not an assignment. > Source/WebCore/inspector/front-end/TimelinePanel.js:299 > + if (this.toggleTimelineButton.toggled) { Nit: WebKit style implies that we skip block parenthesis {} in case there's just one statement under the conditional/loop operator. > LayoutTests/inspector/extensions/extensions-events.html:109 > + if (record.type == "Layout") { Nit: === Comment on attachment 94123 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=94123&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:300 > + this._addRecordToTimeline(JSON.parse(JSON.stringify(event.data))); Can we move record clonging down to the place we actually modify data, i.e. to FormattedRecord, and only do conditionally for certain record types that we modify? A bit of a hack, perhaps, but given timeline has to process large amount of events, this may save us from regressing performance. (In reply to comment #9) > There are open questions: > - What is a good way to copy the record, is there a utility? I use JSON.parse(JSON.stringify(record)) currently Either this way, or create a new object that has __proto__ set to the original object, e.g. record.data = { __proto__: record.data } I have no idea which one is actually faster (using prototypes may have some performance impact as well). As long as we only do this for certain record types, especially those that a rare (e.g. network), the performance impact may be negligible. > - When adding a listener, the actual start of the instrumentation could be delayed. Do we have to add a callback to EventSink.addListener which is called as soon as the subscription is successful (seems strange, in this case I would rather have a explicit start/stop again)? Or can we neglect that? I'd rather neglect this, unless you, as the API client, happen to have a specific use case for this. > If the async requests wont be executed out-of-order, this is probably not an issue. We do expect that sequential calls from the same client are received by the server in the same sequence, and the callbacks on the client are invoked in the same order that server invokes them. This seems to be guaranteed by the transport we use (MessagePort has to use a single event queue, as per DOM messaging spec). Comment on attachment 94123 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=94123&action=review >> Source/WebCore/inspector/front-end/TimelinePanel.js:300 >> + this._addRecordToTimeline(JSON.parse(JSON.stringify(event.data))); > > Can we move record clonging down to the place we actually modify data, i.e. to FormattedRecord, and only do conditionally for certain record types that we modify? A bit of a hack, perhaps, but given timeline has to process large amount of events, this may save us from regressing performance. Please disregard this, Pavel kindly pointed out that we poison original record in too many places, so perhaps we should clone everything. I'll have a look at moving TimelinePanel's attributes to FromattedRecord, though. Created attachment 94216 [details] Proposed Patch Incorporated the review from comment #10. Sorry for the oversights. I tested the timeline together with an extension that also listens to the timeline. (In reply to comment #12) > > I'd rather neglect this, unless you, as the API client, happen to have a specific use case for this. > OK, as the requests are processed in the same order, this works fine for us. Comment on attachment 94216 [details]
Proposed Patch
LGTM. Pavel/Yury, could you please do a formal review?
Michael: please set r?, not r+ on future patches -- r+ is for reviewers only to indicate the patch has been successfully reviewed.
Comment on attachment 94216 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94216&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:300 > + this._addRecordToTimeline(JSON.parse(JSON.stringify(event.data))); Extensions should not regress devtools performance for no good reason. Andrei has a nasty workaround for it if you want this patch to land sooner. I can r+ it provided that Andrei patches record's proto and lands this change by hand. (In reply to comment #17) > Extensions should not regress devtools performance for no good reason. Andrei has a nasty workaround for it if you want this patch to land sooner. I can r+ it provided that Andrei patches record's proto and lands this change by hand. What are my next steps I'm supposed to do to land this patch? (In reply to comment #18) > > What are my next steps I'm supposed to do to land this patch? None -- I'll take care of landing this, either today or tomorrow morning MSK. Manually committed a variation of patch as 87399: http://trac.webkit.org/changeset/87399 - removed cloning of timeline event (no longer necessary, we don't modify event in TimelinePanel) - changed timeline tests to use event listener instead of sniffers (In reply to comment #20) > Manually committed a variation of patch as 87399: http://trac.webkit.org/changeset/87399 > > - removed cloning of timeline event (no longer necessary, we don't modify event in TimelinePanel) > - changed timeline tests to use event listener instead of sniffers Cool! Andrey, thank you so much for helping to land this patch so quickly! |