Bug 160061 – Web Inspector: Waterfall view should be visible in Network tab and Network Timeline

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Currently no waterfall view exists in the Network tab, and in the Timelines tab it's only shown in the Overview timeline view. We should include a waterfall column in both grids.

[Matt Baker]

<rdar://problem/24063884>

[Johan K. Jensen]

Created attachment 284291 [details]
WIP Patch

[Joseph Pecoraro]

Could you add a screenshot?

[Johan K. Jensen]

Created attachment 284379 [details]
Screenshot of WIP waterfall

Yeah, of course.

[Matt Baker]

Comment on attachment 284291 [details]
WIP Patch

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

This is looking really nice. I applied the patch locally and everything is very smooth!

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:87
> +        columns.graph.width = "14%";

This should be 15%.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:108
> +

Nit: remove blank line

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:111
> +        this._lastUpdateTimestamp = NaN;

Nit: add a blank line

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:150
>      }

The view is being updated all the time, even when no resources are loading. You could add this._loadingResourceCount, which would be incremented when records are added and decremented when the associated resources finish or fail (see ResourceDataGridNode). Updating should start when the counter goes from zero to one, and stop when it goes from one to zero.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:177
> +            rDataGridNode.refresh();

rDataGridNode should just be dataGridNode.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:270
> +        this._startUpdatingCurrentTime();

I think the comments are unnecessary, the intention is pretty clear. This could be rewritten as:

if (!WebInspector.visible) {
    if (this._updating)
        this._stopUpdatingCurrentTime();
    return;
}

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.css:63
> +}

It would be nice not to duplicate these styles in three places. TimelineDataGrid gets its own class name, so the common styles could be moved from OverviewTimelineView.css to TimelineDataGrid.css, and removed from the other style sheets.

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:95
> +        columns.graph.width = "14%";

The sum of the column widths is > 100%.

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:122
> +

Trivial getters and setters can be one line.

[Matt Baker]

Created attachment 284535 [details]
[Demo] resource load count

[Johan K. Jensen]

Created attachment 284627 [details]
WIP Patch 2

[Joseph Pecoraro]

Comment on attachment 284627 [details]
WIP Patch 2

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

After applying this I saw an issue drawing the Timeline Ruler after switching tabs.

Steps to Reproduce:
1. Inspect this page
2. Show Network Tab
3. Reload => Populates with data
4. Switch tabs
5. Show Network Tab
  => Timeline Ruler for graph column may be missing labels or drawing labels in the wrong place

The reset issue is also important to fix:

Steps to Reproduce:
1. Inspect this page
2. Show Network Tab
3. Reload => Populates with data
4. Click the reset button in the top right of the network tab
  => Timeline Ruler shows old values, should be cleared

I think with these issue fixed, this should be suitable for landing.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:86
> +        columns.graph.title = WebInspector.UIString("Timeline");

This is not visible. Perhaps it can be dropped?

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:110
>          this._pendingRecords = [];
> +        this._loadingResourceCount = 0;
> +        this._updating = false;
> +        this._lastUpdateTimestamp = NaN;

Nit: _updating is a rather generic name. Maybe we should start specific, like "_updatingCurrentTime". 

All of these should probably be reset in reset(). reset should be doing:

    - clear these instance variables which no longer make sense
    - reset / clear the timeline ruler so it clears its display which will no longer be accurate
    - stop updating current time because we forced loadingResourceCount to 0 (I see now that you did this part)

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:178
> +        for (let dataGridNode of this._dataGrid.children)
> +            dataGridNode.refresh();

It may be possible to reproduce the amount of work here by doing something akin to "refreshGraph" on the existing nodes. That should avoid rebuilding a lot of DOM nodes in the table that do not need to be refreshed.

Likewise, putting this before _processPendingRecords means you don't double update all the new nodes.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:203
> +            if (!isFinite(this._timelineRuler.startTime) || this._timelineRuler.startTime > resourceTimelineRecord.resource.firstTimestamp)
> +                this._timelineRuler.startTime = resourceTimelineRecord.resource.firstTimestamp;
> +            if (!isFinite(this._timelineRuler.endTime) || this._timelineRuler.endTime < resourceTimelineRecord.resource.lastTimestamp)
> +                this._timelineRuler.endTime = resourceTimelineRecord.resource.lastTimestamp;

Nit: We generally prefer isNaN() to isFinite.

This is confusing to me. Why is this resetting the timeline ruler's start/end time every iteration of the loop? This looks like it tries to do something if a resource's first/last timestamp is before/after the timeline ruler, but that change will immediately get overwritten by the next timeline record. Something is fishy here.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:218
> +        const resourceWasAddedSymbol = Symbol("resource-was-added");

This seems kind of wasteful to me. It creates a new Symbol for every single timeline record we process. But we really only need one or a __name.

You could create a WebInspector.NetworkGridContentView.ResourceLoadingSymbol = Symbol("resource-loading") and use that instead of a new local each time.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:227
> +            event.target[resourceWasAddedSymbol] = false;

You use this boolean inconsistently. Sometimes false means it was processed, sometimes true means it was processed.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:291
> +        var startTime = this.startTime;
> +        var currentTime = this.currentTime;
> +        var endTime = this.endTime;
> +        var timespanSinceLastUpdate = (timestamp - this._lastUpdateTimestamp) / 1000 || 0;

Style: `let` instead of `var`.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:300
> +        if (!this._updating && (currentTime >= endTime || isNaN(endTime))) {

What does it mean for _update to be called with _updating false?

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:332
> +        this._updating = false;

Nit: Maybe also add an early return after the assert like the other case.
Nit: Add a cancelAnimationFrame(this._updateCallback)

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:219
> +        for (let dataGridNode of this._resourceDataGridNodeMap.values())
> +            dataGridNode.refresh();

Same comment here about refreshGraph and the order of operations.

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.css:55
> +.data-grid.timeline td.graph-column > div {

This seems super generic. Does the div have a class name? If not it is fine to keep, this just seems likely to cause an accident later.

[Matt Baker]

Comment on attachment 284627 [details]
WIP Patch 2

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

>> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:86
>> +        columns.graph.title = WebInspector.UIString("Timeline");
> 
> This is not visible. Perhaps it can be dropped?

It's shown in the show/hide column context menu.

[Johan K. Jensen]

Created attachment 284748 [details]
Patch 3

[Joseph Pecoraro]

Comment on attachment 284748 [details]
Patch 3

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

This patch looks much better! I understand the tab switching issue is being addressed separately by bug 160311.

r=me

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:172
> +        this._scheduledCurrentTimeUpdateIdentifier = undefined;

Nit: This is handled by the stopUpdatingCurrentTime above.

[Johan K. Jensen]

Created attachment 284816 [details]
Patch 4

[WebKit Commit Bot]

Comment on attachment 284816 [details]
Patch 4

Clearing flags on attachment: 284816

Committed r203843: <http://trac.webkit.org/changeset/203843>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.