Bug 127184 – Web Inspector: Implement basic versions of the TimelineOverview graphs

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Timothy Hatcher]

We can implement basic record row versions of these graphs now. Doing the versions detailed in the mock-up will require more data from the backend (frame boundaries and script profiles).

[Radar WebKit Bug Importer]

<rdar://problem/15848431>

[Timothy Hatcher]

Created attachment 221486 [details]
Patch

[Timothy Hatcher]

Created attachment 221487 [details]
Screenshot

[Joseph Pecoraro]

Comment on attachment 221486 [details]
Patch

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

Looks good! r=me

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.css:37
> +.timeline-overview-graph.layout > .timeline-record-bar > .segment {
> +    box-shadow: white 0 0px 0px 1px;
> +}
> +
> +.timeline-overview-graph.layout:nth-child(even) > .timeline-record-bar > .segment {
> +    box-shadow: rgb(247, 247, 247) 0 0px 0px 1px;
> +}

Is this what produces the "empty stroke" preceding segments?

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.js:61
> +        for (var record of this._layoutTimeline.records) {
> +            var timelineRecordBar = this._timelineRecordBarMap.get(record);
> +            if (!timelineRecordBar) {

This could be a performance issue when there are a lot of records:

    for each record (simple loop)
        find record in map (lookup)
            ...

Ideally we could just iterate all of the values in the map (assuming it has a guaranteed order)

    for each record in map.values (simple loop)
        ...

Apparently though we may also be creating record bars if they didn't exist. If we can just keep track of the records without bars, we can eliminate that step and just iterate timelineRecordBarMap.values(). I'd be much happier with that.

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:81
> +            for (var record of rowRecords) {
> +                var timelineRecordBar = this._timelineRecordBarMap.get(record);
> +                if (!timelineRecordBar) {

Ditto

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:112
> +                rowRecords.splice(insertionIndexForObjectInListSortedByFunction(resourceTimelineRecord, rowRecords, compareByStartTime), 0, resourceTimelineRecord);

You could create a helper for this:

    function insertObjectIntoSortedArray(object, array, compareFunction) {
        var index = insertionIndexForObjectInListSortedByFunction(object, array, compareFunction);
        array.splice(index, 0, object);
    }

and usage here would read much better:

    insertObjectIntoSortedArray(resourceTimelineRecord, rowRecords, compareByStartTime);

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:123
> +            this._timelineRecordGridRows[this._nextDumpRow++].push(resourceTimelineRecord);

Should this insert be sorted? Or this is specifically not a sorted insert so that the bar for this record will draw in a particular way (on top of earlier items).

> Source/WebInspectorUI/UserInterface/ScriptTimelineOverviewGraph.js:60
> +        for (var record of this._scriptTimeline.records) {
> +            var timelineRecordBar = this._timelineRecordBarMap.get(record);

Ditto

> Source/WebInspectorUI/UserInterface/TimelineOverviewGraph.js:87
> +        this._endTime = x || 0;

Nit: "|| 5" like the default? 0..0 might be weird but might also make an issue easier to diagnose.

[Timothy Hatcher]

Created attachment 221522 [details]
Patch

[Timothy Hatcher]

Comment on attachment 221486 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.css:37
>> +}
> 
> Is this what produces the "empty stroke" preceding segments?

Yep!

>> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.js:61
>> +            if (!timelineRecordBar) {
> 
> This could be a performance issue when there are a lot of records:
> 
>     for each record (simple loop)
>         find record in map (lookup)
>             ...
> 
> Ideally we could just iterate all of the values in the map (assuming it has a guaranteed order)
> 
>     for each record in map.values (simple loop)
>         ...
> 
> Apparently though we may also be creating record bars if they didn't exist. If we can just keep track of the records without bars, we can eliminate that step and just iterate timelineRecordBarMap.values(). I'd be much happier with that.

I rewrote this patch for performance. The new patch does not use TimelineRecordBar, or the map. The new patch loops over records per row.

>> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:81
>> +                if (!timelineRecordBar) {
> 
> Ditto

Good point. I'll rework these too, not just Network.

>> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:112
>> +                rowRecords.splice(insertionIndexForObjectInListSortedByFunction(resourceTimelineRecord, rowRecords, compareByStartTime), 0, resourceTimelineRecord);
> 
> You could create a helper for this:
> 
>     function insertObjectIntoSortedArray(object, array, compareFunction) {
>         var index = insertionIndexForObjectInListSortedByFunction(object, array, compareFunction);
>         array.splice(index, 0, object);
>     }
> 
> and usage here would read much better:
> 
>     insertObjectIntoSortedArray(resourceTimelineRecord, rowRecords, compareByStartTime);

Good idea.

>> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:123
>> +            this._timelineRecordGridRows[this._nextDumpRow++].push(resourceTimelineRecord);
> 
> Should this insert be sorted? Or this is specifically not a sorted insert so that the bar for this record will draw in a particular way (on top of earlier items).

It should be sorted too. Will fix.

>> Source/WebInspectorUI/UserInterface/TimelineOverviewGraph.js:87
>> +        this._endTime = x || 0;
> 
> Nit: "|| 5" like the default? 0..0 might be weird but might also make an issue easier to diagnose.

Yeah it would be an error to do this, and 0 is likely best.

[Timothy Hatcher]

Created attachment 221523 [details]
Patch

[Timothy Hatcher]

Created attachment 221524 [details]
Screenshot

[Timothy Hatcher]

Comment on attachment 221523 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.js:59
> +    updateLayout: function()

This code is repeated in ScriptTimelineOverviewGraph (and a more complex version is in NetworkTimelineOverviewGraph). I would normally try to share code this size, however this is just temp code. The final implementations will be different.

[Timothy Hatcher]

Created attachment 221594 [details]
Patch

[Timothy Hatcher]

Sorry for the patch churn. This version moved some of the bar combining logic into TimelineRecordBar and uses it for TimelineDataGridNode.

[Joseph Pecoraro]

Comment on attachment 221594 [details]
Patch

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

r=me!

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.js:91
> +        for (var record of this._layoutTimeline.records) {
> +            // If this bar is completely before the bounds of the graph, skip this record.
> +            if (record.endTime < startTime)
> +                continue;
> +
> +            // If this record is completely after the current time or end time, break out now.
> +            // Records are sorted, so all records after this will be beyond the current or end time too.
> +            if (record.startTime > currentTime || record.startTime > endTime)
> +                break;

Nice! What we can do in the future, if needed because this will change, would be:

  1. Binary search (based on startTime) for the first record to start showing.
  2. Walk through relevant records until this break

Currently for a long recording with the overview graph only showing the last second it looks like this will still walk through all records.

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:98
> +            var barElement = barElementCache.shift();
> +            if (!barElement) {
> +                barElement = document.createElement("div");
> +                barElement.classList.add(WebInspector.NetworkTimelineOverviewGraph.BarStyleClassName);
> +            }

Awesome!

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:134
> +            var rowElement = rowRecords.__rowElement;
> +            if (!rowElement) {
> +                rowElement = rowRecords.__rowElement = document.createElement("div");
> +                rowElement.className = WebInspector.NetworkTimelineOverviewGraph.GraphRowStyleClassName;
> +                this.element.appendChild(rowElement);
> +            }
> +
> +            if (!rowRecords.length)
> +                continue;

Nit: I wonder if this may be worth doing in reset(). Then we won't have to do do this if check 6 times every updateLayout. Or at least move it past the rowRecords.length check, so you will lazily create rows as needed.

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:148
> +                // If this bar is completly before the bounds of the graph, skip this record.

Typo: completly => completely

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:152
> +                // If this record is completly after the current time or end time, break out now.

Typo: completly => completely

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:222
> +        function insertObjectIntoSortedArray(value, array, compareFunction)
> +        {
> +           var index = insertionIndexForObjectInListSortedByFunction(value, array, compareFunction);
> +           array.splice(index, 0, value);
> +        }

Nit: BinarySearch.js

> Source/WebInspectorUI/UserInterface/ScriptTimelineOverviewGraph.js:86
> +        for (var record of this._scriptTimeline.records) {
> +            // If this bar is completely before the bounds of the graph, skip this record.
> +            if (record.endTime < startTime)
> +                continue;

Ditto RE binary search.

> Source/WebInspectorUI/UserInterface/TimelineDataGridNode.js:262
> +            // Combining multiple record bars is not supported with records that have inactive time.
> +            // ResourceTimelineRecord is the only one right, and it is always a single record handled above.
> +            console.assert(!record.usesActiveStartTime);
> +
> +            if (isNaN(record.startTime))
> +                continue;
> +
> +            // If this bar is completely before the bounds of the graph, skip this record.
> +            if (record.endTime < startTime)
> +                continue;

Ditto. Although here it may be useful because I don't suspect this code will change (it is not part of the overview graphs).

> Source/WebInspectorUI/UserInterface/TimelineRecordBar.js:110
> +        // Inactive time is only supported with one record.
> +        if (this._records.length === 1 && this._records[0].usesActiveStartTime) {
> +            this._inactiveBarElement = document.createElement("div");

If you went from a TimelineRecordBar with 1 record with inactive to 1 record with inactive you could leak the old _inactiveBarElement?

You could just add an: if (!this._inactiveBarElement) inside this section and reuse the existing element.

[Timothy Hatcher]

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