Bug 195709 – Web Inspector: Timelines - Import / Export Timeline Recordings

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Timelines - Import / Export Timeline Recordings

[Joseph Pecoraro]

Created attachment 364587 [details]
[PATCH] Proposed Fix

Blocked on HAR Import.

[Devin Rousso]

Comment on attachment 364587 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/timeline/timeline-recording.html:18
> +            InspectorTest.log("-- Loaded");

Using "--" makes it more confusing as to the boundaries of each test case, since the harness automatically prints the test case name with "--" at the beginning of the line.  I'd prefer it if you just used "-", or didn't have any prefix at all.

> LayoutTests/inspector/timeline/timeline-recording.html:36
> +            await InspectorTest.reloadPage();
> +            await InspectorTest.awaitEvent(FrontendTestHarness.Event.TestPageDidLoad);

Cases like this are the only reason I don't like using `await`, as I'd prefer to have the listener added before calling the function that may fire the event.  I've found that a way "around" the issue is to use `Promise.all`.

```
    await Promise.all([
        InspectorTest.awaitEvent(FrontendTestHarness.Eevent.TestPageDidLoad),
        InspectorTest.reloadPage(),
    ]);
```

> LayoutTests/inspector/timeline/timeline-recording.html:64
> +            let filterKeys = new Set(["startTime", "endTime", "time", "records", "displayName"]);

Style: `const`.

> LayoutTests/inspector/timeline/timeline-recording.html:79
> +            // NOTE: This runs the toJSON handlers on the timeline records and more, which is important

What do you mean by "and more"?

> LayoutTests/inspector/timeline/timeline-recording.html:80
> +            // because importing expects the serialized form of the objects, not raw objects.

I'd say "not model objects" rather than "raw".

> LayoutTests/inspector/timeline/timeline-recording.html:84
> +            let recording = WI.TimelineRecording.import(identifier, jsonData, "name");

Could we have a better (e.g. more unique) name?  How about "TEST_NAME", or even just "TEST"?

> Source/WebInspectorUI/ChangeLog:8
> +        Timeline export saves TimelineRecording and TimelineOverview state.

Grammar.  This sentence is a fragment.

> Source/WebInspectorUI/ChangeLog:9
> +        The TimelineRecording includes all kinds of model objects such as

model objects, such as

> Source/WebInspectorUI/ChangeLog:10
> +        records, markers, memory pressure events. It also includes raw

markers, memory pressure events, etc.

> Source/WebInspectorUI/ChangeLog:11
> +        protocol data such as script profiler samples. TimelineOverview

protocol data, such as

> Source/WebInspectorUI/ChangeLog:12
> +        includes some of the view state to restore such as the selected

to restore, such as

> Source/WebInspectorUI/ChangeLog:16
> +        the records, markers, various events, and re-initializing more state.

records, markers, and other events, as well as re-initializing

> Source/WebInspectorUI/ChangeLog:17
> +        To finally display the imported recording the content view will

imported recording, the content

> Source/WebInspectorUI/ChangeLog:46
> +        Save data at the TimelineRecording level which can be used for export.

s/which/that

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:277
> +    importRecording(jsonString, filename)

Similar functions to this in other managers have used `processJSON` as a name.  I'd like to keep things consistent between managers.  Also, they accept an object as a parameter, so it makes it easier to write `WI.FileUtilities.importJSON((result) => WI.timelineManager.processJSON(result));`.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:279
> +        let json;

Please initialize this to some value (e.g. `null`).

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:283
> +            WI.TimelineManager.synthesizeImportError(e);

We should return so that accessing `json` doesn't throw another exception below.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:301
> +            oldRecording.unloaded(importing);

I'm not sure this is the right move.  I don't think we should unload the existing recording, as an imported recording shouldn't change the ability for an existing recording to be able to start/stop additional recording (not to mention this may assert if the `oldRecording` was empty).  I'd expect that importing a recording would just "hide" any existing recording by selecting the newly imported one so that the previously visible one is no longer visible.  Additionally, the start/stop/clear buttons should be disabled, as those operations don't make sense on something effectively constant.

> Source/WebInspectorUI/UserInterface/Models/HeapAllocationsTimelineRecord.js:46
> +        // NOTE: This just goes through and generates a new heap snapshot,
> +        // it is not perfect but does what we want. It asynchronously creates
> +        // a heap snapshot at the right time, and insert it into the active
> +        // recording, which on an import should be the newly imported recording.

Why not just make `fromJSON` async, and have the entire import process become `async`?  The Audit import export uses that method and works great :)

> Source/WebInspectorUI/UserInterface/Models/ResourceTimelineRecord.js:50
> +            entry: WI.HARBuilder.entry(this._resource, ""),

This is a perfect example of where a `const` parameter or a object-bag would be awesome.  I had to go look at HARBuilder to understand what the `""` means :(

Please add a `const content = "";` so that anyone else like me doesn't have to go searching through code.

> Source/WebInspectorUI/UserInterface/Models/ScriptTimelineRecord.js:-199
> -    GarbageCollected: "script-timeline-record-garbage-collected",

This is used as a CSS class name, so you'll have to adjust TimelineRecordBar.css for this as well.

> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:77
> -    requestContentFromBackend(callback)
> +    requestContentFromBackend()

While I'm not against this change, is there a reason for it being in this patch?  Just a drive-by, or something more?

> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:67
> +            console.error("Unknown TimelineRecord.Type: " + json.type);

I'd also recommend logging `json` as another argument so that if inspector2 is open, we can immediately see information about it without having to try to reproduce it.

> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:74
> +        throw WI.NotImplementedError.subclassMustOverride();

Given that every subclass exports `this.type`, perhaps we can do a multilevel approach to this, where the subclasses call `super.toJSON()` and then add their own keys/values to the returned object.

```
    toJSON()
    {
        console.assert(this.constructor !== WI.TimelineRecord && this instanceof WI.TimelineRecord);

        return {
            type: this.type,
        };
    }
```

And then a subclass could call it.

```
    toJSON()
    {
        let json = super.toJSON();
        json.x = 1;
        json.y = 2;
        return json;
    }
```

> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:90
> +        recording.initializeCallingContextTrees(sampleStackTraces, sampleDurations);

Should this happen after we add all of the records, seeing as the ScriptProfiler sends this data after the recording has stopped (e.g. "mimic" behavior)?

> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:108
> +        // Add markers once we've transitioned the active recording.

Is there an event we can add a `singleFireEventListener` to instead, like `WI.TimelineManager.Event.RecordingImported`?

> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:136
> +            instrumentTypes: this._instruments.map((x) => x.timelineRecordType),

NIT: I'd prefer you use a more descriptive variable name, seeing as the value is known to be of a specific type/shape.

```
    instrumentTypes: this._instruments.map((instrument) => instrument.timelineRecordType),
```

> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:429
> +        this._exportDataSampleStackTraces = stackTraces;
> +        this._exportDataSampleDurations = sampleDurations;

Shouldn't this be an additive process (e.g. `Array.prototype.concat`)?  If we start/stop a single recording multiple times, we'll end up calling this each time, which will override it each time.

> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:434
> +        let topDownCallingContextTree = this._topDownCallingContextTree;
> +        let bottomUpCallingContextTree = this._bottomUpCallingContextTree;
> +        let topFunctionsTopDownCallingContextTree = this._topFunctionsTopDownCallingContextTree;
> +        let topFunctionsBottomUpCallingContextTree = this._topFunctionsBottomUpCallingContextTree;

What's the point of saving these to local variables?

> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:452
> +        if (this._capturing)
> +            return false;
> +
> +        if (isNaN(this._startTime))
> +            return false;
> +
> +        return true;

Should we inline all of this?

```
    return !this._capturing && !isNaN(this._startTime);
```

I find that that is much easier to wrap my head around at a glance.

> Source/WebInspectorUI/UserInterface/Views/MediaTimelineView.js:200
> +        this._processRecord(mediaTimelineRecord);

This should be `record`, not `mediaTimelineRecord` (unless you also meant to change `let mediaTimelineRecord = event.data.record`).

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:56
> +        if (this._memoryTimeline.records.length) {

Why is this check necessary?

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:252
>  

For the sake of uniformity, should we add a `console.assert(memoryTimelineRecord instanceof WI.MemoryTimelineRecord);`?

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineOverviewGraph.js:122
> +        let compareByStartTime = (a, b) => a.startTime - b.startTime;

Why was this changed into an arrow function?  Keeping it as a non-arrow function should be less work as it doesn't have to capture `this`.

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:1046
> +            if (overviewData.selectionDuration === Number.MAX_VALUE)

How well does this translate to JSON?  How about instead of trying to convey a concept of "everything", we only select everything if it's `-1`?  That way, we have a concrete "identifier" as to that particular state.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:70
> +        this._exportButtonNavigationItem.visibilityPriority = WI.NavigationItem.VisibilityPriority.High;

I'd argue that this isn't that important, as it's not "important" to being able to use the Timelines tab.  It's more of a "portability" feature, less so a functionality one.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:71
> +        this._exportButtonNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, () => { this._exportTimelineRecording(); });

I prefer putting this on a new line, so it isn't confused with an implicit return arrow function.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:187
> +        navigationItems.push(this._exportButtonNavigationItem);
>          navigationItems.push(this._clearTimelineNavigationItem);

We should have a divider between the import/export and clear.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:209
> +        if (this._recording.canExport())
> +            return {customSaveHandler: () => { this._exportTimelineRecording(); }};

We should do this if `currentContentView.saveData` is `null` below.  If what we're currently looking at is able to save something, that should take precedence over the entire timeline .

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:554
> +        let {startTime, endTime} = this._recording;
> +        this._updateTimes(startTime, endTime, endTime);

I'd inline this in the constructor, unless it's likely to be called more in the future.  Nothing about the logic is particularly "unique" to an imported recording, other than the circumstances in which it's called.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:602
> +        let recordingData = this._recording.exportData();
> +        let overviewData = this._timelineOverview.exportData();

You could avoid these locals by using `json` from the getgo.

```
    let json = {
        recording: this._recording.exportData(),
        overview: this._timelineOverview.exportData(),
    };
    if (!json.recording || !json.overview) {
        InspectorFrontendHost.beep();
        return;
    }
```

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:618
> +        let filename = frameName ? `${frameName}-recording` : WI.UIString("Timeline Recording");

We should use `this._recording.displayName` instead of `WI.UIString("Timeline Recording")`, as we may be exporting the 2nd or 3rd (or nth) recording, not always the 1st or "only" one.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:619
> +        let url = "web-inspector:///" + encodeURI(filename) + ".json";

I'd inline this.

[Joseph Pecoraro]

Comment on attachment 364587 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/timeline/timeline-recording.html:79
>> +            // NOTE: This runs the toJSON handlers on the timeline records and more, which is important
> 
> What do you mean by "and more"?

Anything in the `exportData` object with custom toJSON (records, markers, and other model objects).

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:301
>> +            oldRecording.unloaded(importing);
> 
> I'm not sure this is the right move.  I don't think we should unload the existing recording, as an imported recording shouldn't change the ability for an existing recording to be able to start/stop additional recording (not to mention this may assert if the `oldRecording` was empty).  I'd expect that importing a recording would just "hide" any existing recording by selecting the newly imported one so that the previously visible one is no longer visible.  Additionally, the start/stop/clear buttons should be disabled, as those operations don't make sense on something effectively constant.

We currently don't have the ability to start an older recording. That seems rather low priority.

Would you expect to be able to:

    1. Start and stop a recording.
    2. Start and stop a new recording.
    3. Go back to and start Recording from (1)?

We currently don't allow that, but it is not really any different from this case.

• The `importing` parameter here avoids the assertion if oldRecording was empty.
• I think this patch does disable the clear / export buttons appropriately. It does not disable the start button, since clicking that will start a new recording and that is the behavior I would expect. I don't think developers will easily pick up on the fact that they can toggle between recordings and would rather click the start button to start a new recording. All of these features were implemented in the related bug for `readonly` recordings. <https://bugs.webkit.org/show_bug.cgi?id=195594>

>> Source/WebInspectorUI/UserInterface/Models/HeapAllocationsTimelineRecord.js:46
>> +        // recording, which on an import should be the newly imported recording.
> 
> Why not just make `fromJSON` async, and have the entire import process become `async`?  The Audit import export uses that method and works great :)

That would be fine, but I still would not want to wait for HeapSnapshot processing before showing all the other data. I did consider an Import splash screen, but so often the data loaded quickly that I was not concerned.

>> Source/WebInspectorUI/UserInterface/Models/ScriptTimelineRecord.js:-199
>> -    GarbageCollected: "script-timeline-record-garbage-collected",
> 
> This is used as a CSS class name, so you'll have to adjust TimelineRecordBar.css for this as well.

Arg this was surely a merge issue as I did that one before. Thanks for catching.

>> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:77
>> +    requestContentFromBackend()
> 
> While I'm not against this change, is there a reason for it being in this patch?  Just a drive-by, or something more?

Drive-by. I provided a new implementation of a `requestContentFromBackend()` subclass and this one signature does not match all the rest.

>> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:74
>> +        throw WI.NotImplementedError.subclassMustOverride();
> 
> Given that every subclass exports `this.type`, perhaps we can do a multilevel approach to this, where the subclasses call `super.toJSON()` and then add their own keys/values to the returned object.
> 
> ```
>     toJSON()
>     {
>         console.assert(this.constructor !== WI.TimelineRecord && this instanceof WI.TimelineRecord);
> 
>         return {
>             type: this.type,
>         };
>     }
> ```
> 
> And then a subclass could call it.
> 
> ```
>     toJSON()
>     {
>         let json = super.toJSON();
>         json.x = 1;
>         json.y = 2;
>         return json;
>     }
> ```

I didn't like this for a few reasons.

1. The syntax of the subclasses is not ugly.
2. I want the timeline export to fail immediately when we are have not implemented something that should be required for import/export. Putting a base implementation that doesn't know what to do will defer the issue until import time.

>> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:90
>> +        recording.initializeCallingContextTrees(sampleStackTraces, sampleDurations);
> 
> Should this happen after we add all of the records, seeing as the ScriptProfiler sends this data after the recording has stopped (e.g. "mimic" behavior)?

I want this to happen before we process records to that we can use the _topDownCallingContextTree to create `record.profilePayload` below (line 98).

>> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:108
>> +        // Add markers once we've transitioned the active recording.
> 
> Is there an event we can add a `singleFireEventListener` to instead, like `WI.TimelineManager.Event.RecordingImported`?

That might be a good idea. The ModelObject shouldn't need to know about most Controller / View state, but maybe we can act on something.

>> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:429
>> +        this._exportDataSampleDurations = sampleDurations;
> 
> Shouldn't this be an additive process (e.g. `Array.prototype.concat`)?  If we start/stop a single recording multiple times, we'll end up calling this each time, which will override it each time.

Good catch! This does not handle starting and stopping multiple times. I'll fix.

>> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:434
>> +        let topFunctionsBottomUpCallingContextTree = this._topFunctionsBottomUpCallingContextTree;
> 
> What's the point of saving these to local variables?

Older code this moved from had it, I'll inline.

>> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:452
>> +        return true;
> 
> Should we inline all of this?
> 
> ```
>     return !this._capturing && !isNaN(this._startTime);
> ```
> 
> I find that that is much easier to wrap my head around at a glance.

I find the returns much easier, because each return states a reason why we cannot export.

>> Source/WebInspectorUI/UserInterface/Views/MediaTimelineView.js:200
>> +        this._processRecord(mediaTimelineRecord);
> 
> This should be `record`, not `mediaTimelineRecord` (unless you also meant to change `let mediaTimelineRecord = event.data.record`).

Oops, yes. Thanks.

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:56
>> +        if (this._memoryTimeline.records.length) {
> 
> Why is this check necessary?

This isn't and after removing it 3 times on my laptop I never ended up removing it on my iMac... thanks.

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:252
>>  
> 
> For the sake of uniformity, should we add a `console.assert(memoryTimelineRecord instanceof WI.MemoryTimelineRecord);`?

Heh yeah I was going to add these in as well.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineOverviewGraph.js:122
>> +        let compareByStartTime = (a, b) => a.startTime - b.startTime;
> 
> Why was this changed into an arrow function?  Keeping it as a non-arrow function should be less work as it doesn't have to capture `this`.

An arrow function that does not use the `this` keyword does not capture this in JavaScriptCore. It should not be less work to use an arrow function that is equivalent to a classic function.

>> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:1046
>> +            if (overviewData.selectionDuration === Number.MAX_VALUE)
> 
> How well does this translate to JSON?  How about instead of trying to convey a concept of "everything", we only select everything if it's `-1`?  That way, we have a concrete "identifier" as to that particular state.

Number.MAX_VALUE is the value that TimelineRuler uses when it is selecting everything. It is the concrete identify that TimelineRuler uses.

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:209
>> +            return {customSaveHandler: () => { this._exportTimelineRecording(); }};
> 
> We should do this if `currentContentView.saveData` is `null` below.  If what we're currently looking at is able to save something, that should take precedence over the entire timeline .

Hmm, that makes sense, but think I would rather ⌘S in the Timelines Tab always global import/export. We can eliminate the sub-view path and instead allow Heap Snapshot import/export only by the button.

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:554
>> +        this._updateTimes(startTime, endTime, endTime);
> 
> I'd inline this in the constructor, unless it's likely to be called more in the future.  Nothing about the logic is particularly "unique" to an imported recording, other than the circumstances in which it's called.

I expected to do more here, but since it ended up just being this I'll inline.

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:602
>> +        let overviewData = this._timelineOverview.exportData();
> 
> You could avoid these locals by using `json` from the getgo.
> 
> ```
>     let json = {
>         recording: this._recording.exportData(),
>         overview: this._timelineOverview.exportData(),
>     };
>     if (!json.recording || !json.overview) {
>         InspectorFrontendHost.beep();
>         return;
>     }
> ```

I like that!

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:618
>> +        let filename = frameName ? `${frameName}-recording` : WI.UIString("Timeline Recording");
> 
> We should use `this._recording.displayName` instead of `WI.UIString("Timeline Recording")`, as we may be exporting the 2nd or 3rd (or nth) recording, not always the 1st or "only" one.

I waffled on this.

• The name "Timeline Recording 1" will end up being so common that it will be annoying.
• A timestamp based filename (like images) ends up being super long and ugly on re-import.

I still prefer the domain name title, even if that collides the user can name them and get a better idea what it was about "webkit.org-scrolling.json" and "webkit.org-loading.json".

[Joseph Pecoraro]

> 1. The syntax of the subclasses is not ugly.

Correction: *now* ugly

[Joseph Pecoraro]

<rdar://problem/23188921>

[Joseph Pecoraro]

Created attachment 364697 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

Attachment 364697 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Views/TimelineRecordingImportedView.css:27:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Views/TimelineRecordingImportedView.css:28:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebInspectorUI/UserInterface/Views/TimelineRecordingImportedView.css:29:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 364735 [details]
[PATCH] Proposed Fix

Previous patch forgot the updated test.

[Devin Rousso]

Comment on attachment 364735 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingImportedView.js:38
> +    get visible() { return this._visible; }

Style: this should be on separate lines, since the setter is "complex".

[Joseph Pecoraro]

https://trac.webkit.org/changeset/243024/webkit

[Truitt Savell]

It looks like the new test inspector/timeline/timeline-recording.html

added in https://trac.webkit.org/changeset/243024/webkit

is a flakey failure.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Ftimeline%2Ftimeline-recording.html

Diff:
--- /Volumes/Data/slave/mojave-leaks/build/layout-test-results/inspector/timeline/timeline-recording-expected.txt
+++ /Volumes/Data/slave/mojave-leaks/build/layout-test-results/inspector/timeline/timeline-recording-actual.txt
@@ -46,10 +46,6 @@
     {
       "time": "<filtered>",
       "type": "dom-content-event"
-    },
-    {
-      "time": "<filtered>",
-      "type": "load-event"
     }
   ],
   "memoryPressureEvents": [],

[Joseph Pecoraro]

(In reply to Truitt Savell from comment #11)
> It looks like the new test inspector/timeline/timeline-recording.html
> 
> added in https://trac.webkit.org/changeset/243024/webkit
> 
> is a flakey failure.

I relaxed the test in:
https://trac.webkit.org/r243084