Bug 151372 - Web Inspector: Initial support for variable timelines
Summary: Web Inspector: Initial support for variable timelines
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-11-17 17:02 PST by Joseph Pecoraro
Modified: 2015-12-01 13:54 PST (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (48.73 KB, patch)
2015-11-17 17:13 PST, Joseph Pecoraro
bburg: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-11-17 17:02:55 PST
* SUMMARY
Initial support for variable timelines.

Currently we assume a fixed set of timelines. This adds basic "Instrument" support so that a recording has a set of instruments not a set of timelines. There is currently a 1-to-1 mapping of Instruments to a Timeline, and that may or may not continue to be the case.

This starts by exposing the Instruments a Recording has, instead of its Timelines.
Comment 1 Radar WebKit Bug Importer 2015-11-17 17:03:59 PST
<rdar://problem/23586257>
Comment 2 Joseph Pecoraro 2015-11-17 17:13:16 PST
Created attachment 265718 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2015-11-18 12:17:28 PST
Comment on attachment 265718 [details]
[PATCH] Proposed Fix

This needs to update Test.html to include the new Models.
Comment 4 BJ Burg 2015-11-18 13:56:31 PST
Comment on attachment 265718 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=265718&action=review

A few things:

 - this patch and the followup make the view classes tied to Instrument instances. But, when we load a timeline recording from disk, I don't think we should create real Instruments and attach them to a readonly recording. A readonly recording will just have a bunch of deserialized Timelines and 

 - Perhaps we can have TimelineRecording's clients look at the InstrumentType and then ask for the relevant timeline(s) using that as the key.

 - To avoid always creating all timelines, even if nothing will ever dump data into it, requesting creation of Timelines should be delegated to the individual recordings or TimelineRecording deserializer.

An instrument should initiate a needed Timeline when it's added to the recording (if a default instrument, like network / scripts) or is started/stopped (if a dynamic instrument, like memory profiling).

So, we would have:

let t = new WebInspector.TimelineRecording(identifier, name);
let defaultInstruments = WebInspector.TimelineManager.defaultInstruments();
for (let instrument of defaultInstruments)
    t.addInstrument(instrument)

Then,

 - Inside TimelineRecording.{addInstrument,removeInstrument}, the recording notifies the instrument with Instrument.{wasAddedTo,willRemoveFrom}Timeline(this).
 - Inside the instrument's wasAddedToTimeline(), it requests necessary timeline instances using TimelineRecording.getOrCreateTimelineOfType(timelineType).
 - TimelineRecording looks up that timeline, creates a new one if necessary, adds change listeners, returns it to the Instrument.

> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:42
> +            WebInspector.TimelineRecord.Type.Script,

I think we only want the timeline to exist if there's any data being added to it. This will always create a rendering frames timeline. See general comment for a sketch of how we might do it differently.

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:514
> +        let instrument = instrumentOrEvent instanceof WebInspector.Instrument ? instrumentOrEvent : instrumentOrEvent.data.instrument;

I prefer the old style here, less horizontal scanning to find what the possible assignments are.

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:517
> +        let timeline = this._recording.timelineForInstrument(instrument);

maybe this should be timelineForInstrumentType. See general comment.

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:76
> +        if (WebInspector.FPSInstrument.supported()) {

Nice!
Comment 5 Joseph Pecoraro 2015-11-18 14:55:15 PST
(In reply to comment #4)
> Comment on attachment 265718 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265718&action=review
> 
> A few things:
> 
>  - this patch and the followup make the view classes tied to Instrument
> instances. But, when we load a timeline recording from disk, I don't think
> we should create real Instruments and attach them to a readonly recording. A
> readonly recording will just have a bunch of deserialized Timelines and 

If we load a recording from disk I would want it to match what the UI looked like when it was exported. That means knowing the instruments and their order. This will likely mean giving instruments an identifier type for export.


>  - Perhaps we can have TimelineRecording's clients look at the
> InstrumentType and then ask for the relevant timeline(s) using that as the
> key.
>
>  - To avoid always creating all timelines, even if nothing will ever dump
> data into it, requesting creation of Timelines should be delegated to the
> individual recordings or TimelineRecording deserializer.
> 
> An instrument should initiate a needed Timeline when it's added to the
> recording (if a default instrument, like network / scripts) or is
> started/stopped (if a dynamic instrument, like memory profiling).
> 
> So, we would have:
> 
> let t = new WebInspector.TimelineRecording(identifier, name);
> let defaultInstruments = WebInspector.TimelineManager.defaultInstruments();
> for (let instrument of defaultInstruments)
>     t.addInstrument(instrument)
> 
> Then,
> 
>  - Inside TimelineRecording.{addInstrument,removeInstrument}, the recording
> notifies the instrument with
> Instrument.{wasAddedTo,willRemoveFrom}Timeline(this).
>  - Inside the instrument's wasAddedToTimeline(), it requests necessary
> timeline instances using
> TimelineRecording.getOrCreateTimelineOfType(timelineType).
>  - TimelineRecording looks up that timeline, creates a new one if necessary,
> adds change listeners, returns it to the Instrument.

Seems like a lot of work for little pay off. Having a few extra unused timelines in the Recording seems fine to me. Is there a particular case you are thinking of where this approach would improve things?

> > Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:42
> > +            WebInspector.TimelineRecord.Type.Script,
> 
> I think we only want the timeline to exist if there's any data being added
> to it. This will always create a rendering frames timeline. See general
> comment for a sketch of how we might do it differently.
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:514
> > +        let instrument = instrumentOrEvent instanceof WebInspector.Instrument ? instrumentOrEvent : instrumentOrEvent.data.instrument;
> 
> I prefer the old style here, less horizontal scanning to find what the
> possible assignments are.

Bah. We should just not have functions called with multiple types like this.
Comment 6 BJ Burg 2015-11-18 16:48:12 PST
(In reply to comment #4)
> Comment on attachment 265718 [details]
>
>  - To avoid always creating all timelines, even if nothing will ever dump
> data into it, requesting creation of Timelines should be delegated to the
> individual recordings or TimelineRecording deserializer.
> 
> An instrument should initiate a needed Timeline when it's added to the
> recording (if a default instrument, like network / scripts) or is
> started/stopped (if a dynamic instrument, like memory profiling).

After discussion, I don't think this is necessary for any features in the short term. Having empty timelines always around is okay. We can revisit this when it's needed.
Comment 7 BJ Burg 2015-11-18 16:50:21 PST
Comment on attachment 265718 [details]
[PATCH] Proposed Fix

r=me, let's keep going.
Comment 8 Joseph Pecoraro 2015-12-01 13:54:48 PST
<http://trac.webkit.org/changeset/192906>