Bug 61098 – Web Inspector: Provide inspector extension API to access timeline data

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Michael Schneider]

We would like to access the timeline (instrumentation) data from an Chrome devtools extension.

Currently, this data is only accessible to the inspector itself. Therefore, we propose to add a webInspector.timeline API, where inspector extensions can subscribe to the timeline record events.

[Michael Schneider]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Michael Schneider]

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.

[Andrey Kosyakov]

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.

[Andrey Kosyakov]

(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.

[Pavel Feldman]

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.

[Andrey Kosyakov]

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) {

===

[Michael Schneider]

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.

[Andrey Kosyakov]

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: ===

[Andrey Kosyakov]

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.

[Andrey Kosyakov]

(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).

[Andrey Kosyakov]

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.

[Michael Schneider]

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.

[Michael Schneider]

(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.

[Andrey Kosyakov]

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.

[Pavel Feldman]

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.

[Michael Schneider]

(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?

[Andrey Kosyakov]

(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.

[Andrey Kosyakov]

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

[Michael Schneider]

(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!