Bug 160062 – Web Inspector: Show resource timing details in Network waterfall

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

[Matt Baker]

Show a breakdown of the different resource loading phases (App Cache, DNS, Secure Connection Start, etc) in the Network waterfall:

1) Split the "inactive" portion of a resource's timeline record bar into colored segments
2) Segments smaller than a certain pixel threshold (WebInspector.TimelineRecordBar.MinimumWidthPixels?) should not be shown
3) Gaps between segments should not be shown
4) An explanation of the different phases and their timestamps should be added to the Resource details sidebar and/or a popover in the Network grids

[Radar WebKit Bug Importer]

<rdar://problem/27482816>

[Johan K. Jensen]

Created attachment 284930 [details]
WIP Patch

[Timothy Hatcher]

Please attach screenshots to patches like this.

[Matt Baker]

Comment on attachment 284930 [details]
WIP Patch

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

> Source/WebInspectorUI/UserInterface/Models/ResourceTiming.js:28
> +    constructor(resource, startTime, domainLookupStart, domainLookupEnd, connectStart, connectEnd, secureConnectionStart, requestStart, responseStart/*, responseEnd*/)

Remove the commented references to "responseEnd" throughout this class. If we plan on using a more accurate end time in the future, file a follow up bug and add a comment (see below).

> Source/WebInspectorUI/UserInterface/Models/ResourceTiming.js:55
> +    get responseEnd() { return this._resource.finishedOrFailedTimestamp; }

Add a FIXME comment in the body of the responseEnd getter referencing the follow up issue if a separate "this._responseEnd" is going to be used in the future. Search for "// FIXME: " in the codebase for examples.

> Source/WebInspectorUI/UserInterface/Models/ResourceTiming.js:62
> +            return offset > 0 ? payload.startTime + offset/1000 : NaN;

Operators and operands should be separated by a single space: "offset / 1000"

> Source/WebInspectorUI/UserInterface/Views/RecordBarTimelineDataGridNode.js:26
> +WebInspector.RecordBarTimelineDataGridNode = class RecordBarTimelineDataGridNode extends WebInspector.TimelineDataGridNode

I think RecordBar is too general here. What about ResourceTimingPopoverDataGridNode?

> Source/WebInspectorUI/UserInterface/Views/RecordBarTimelineDataGridNode.js:66
> +    }

Not necessary.

> Source/WebInspectorUI/UserInterface/Views/RecordBarTimelineDataGridNode.js:71
> +    }

Icons aren't shown in the popover grid, so this can be removed.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:245
> +    _mouseoverRecordBar(event, recordBar) {

Newline before open brace.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:257
> +        popoverContentElement.classList.add("record-bar-timeline");

The class name should indicate that it's for a popover (heap-snapshot-instance-popover-content, heap-snapshot-instance-popover-content). What about "resource-timing-popover-content"?

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:267
> +        dataGrid.createSettings("network-grid-popover");

This is a read only grid, settings aren't needed.

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGridNode.js:245
> +                timelineRecordBar.element.addEventListener("mouseenter", (e) => this._mouseoverRecordBar(e, timelineRecordBar));

Exposing an event handler to be implemented by subclasses is a little unusual, and also fragile. I'd recommend adding protected methods "didAddRecordBar" and "didRemoveRecordBar" to TimelineDataGridNode, which could be implemented by subclasses. In this case, ResourceTimelineDataGridNode could override these methods and add/remove an event listener.

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGridNode.js:384
> +    }

Remove, see above.

[Matt Baker]

Comment on attachment 284930 [details]
WIP Patch

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

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:250
> +        let resource = recordBar.records[0].resource;

Add a console.assert(recordBar.records.length) before accessing the array.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:256
> +        popoverContentElement.style.width = "280px";

Styles that don't change should be specified in CSS.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:263
> +        columns.duration.aligned = "right";

Style: recently we've been moving to defining "columns" data in a single statement:

let columns = {
    description: {
        width = "30%"
    },
    graph: {
        width = "40%"
    },
    duration: {
        width = "30%",
        aligned = "right"
    }
};

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:268
> +        dataGrid._headerWrapperElement.style.display = "none";

It would be better not to access private class data. Elsewhere we use:

dataGrid.element.classList.add("no-header");

but ideally this would be a property of the grid.

[Johan K. Jensen]

Created attachment 285176 [details]
Patch

[Johan K. Jensen]

Created attachment 285187 [details]
Patch

[Johan K. Jensen]

Created attachment 285189 [details]
Image of the popover

Image of the popover added to the waterfall, describing various parts of the request. More detailed version to come.

[Johan K. Jensen]

Comment on attachment 285187 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/ResourceTiming.js:78
> +        payload = payload || {};

I should probably add something like this

        // COMPATIBILITY (iOS 10): Resource Timing data was incomplete and incorrect. Do not use it.
        // iOS 7 sent a requestTime and iOS 8-9.3 sent a navigationStart time.
        if (typeof payload.requestTime === "number" || typeof payload.navigationStart === "number")
            payload = {};

as per Joe’s comment on the issue for sending the new resource timing payload: https://bugs.webkit.org/show_bug.cgi?id=160095#c4

[Build Bot]

Comment on attachment 285187 [details]
Patch

Attachment 285187 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1802201

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285192 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285187 [details]
Patch

Attachment 285187 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1802208

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285194 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285187 [details]
Patch

Attachment 285187 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1802209

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285195 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Johan K. Jensen]

Looks like I forgot to include the new ResourceTiming.js in UserInterface/Test.html. 😅

[Blaze Burg]

Comment on attachment 285187 [details]
Patch

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

Please post a new patch with test fixes.

> Source/WebInspectorUI/ChangeLog:7
> +

Usually we try to put a high level summary here. For example,

"This patch adds a popover to network tab's resource rows. It shows times for each part of the resource load. The data comes from ..."

> Source/WebInspectorUI/UserInterface/Models/ResourceTiming.js:26
> +WebInspector.ResourceTiming = class ResourceTiming extends WebInspector.Object

The name here bothers me. Timing for what? A name like ResourceLoadTiming or ResourceTimingData would be better IMO. Then the member for Resource class could be _timingData.

> Source/WebInspectorUI/UserInterface/Models/ResourceTiming.js:28
> +    constructor(resource, startTime, domainLookupStart, domainLookupEnd, connectStart, connectEnd, secureConnectionStart, requestStart, responseStart)

I think this should take a data object, then we can destructure it into member variables. Passing 10 arguments is really annoying.

> Source/WebInspectorUI/UserInterface/Models/ResourceTiming.js:47
> +            this._domainLookupStart = NaN;

You can do a multiple assignment here.

if (...)
  this._domainLookupStart = this._domainLookupEnd = NaN;

> Source/WebInspectorUI/UserInterface/Models/ResourceTiming.js:70
> +        // FIXME: Get an actual responseEnd-timestamp from the backend.

Please add a bug number, file one if there isn't one yet.

> Source/WebInspectorUI/UserInterface/Models/ResourceTiming.js:83
> +        let startTime = payload.startTime;

This can be combined with the call to constructor if we pass an object.

let data = {
domainLookupStart : offsetToTimestamp(payload.domainLookupStart),
...
}

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:248
> +        recordBar.element.addEventListener("mouseenter", this._mouseoverRecordBarBound);

You can use a WebInspector.EventListener to toggle this listener on and off easily, and avoid caching your own bound version.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:316
> +        dataGrid.layout();

Er, please name this local 'popoverDataGrid'. I got confused seeing this forced layout, then figuring out what this.dataGrid was.

[Johan K. Jensen]

Created attachment 287521 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 287521 [details]
Patch

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

r- for the questions and a few things that need to be done (resize handler)

> Source/WebInspectorUI/UserInterface/Models/SegmentTimelineRecord.js:42
> +    constructor(segment)
> +    {
> +        super(WebInspector.TimelineRecord.Type.Network);
> +
> +        this._segment = segment;
> +    }
> +
> +    // Public
> +
> +    get segment() { return this._segment; }
> +    get updatesDynamically() { return false; }
> +    get usesActiveStartTime() { return false; }
> +    get startTime() { return this._segment.start; }
> +    get activeStartTime() { return this._segment.end; }
> +    get endTime() { return this._segment.end; }

You should be able to get almost everything here if you use the super class implementation more:

    super(WebInspector.TimelineRecord.Type.Network, this._segment.start, this._segment.end)

And then the only getter you need is:

    get segment() { return this._segment; }

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:254
> +    didAddRecordBar(recordBar)
> +    {
> +        this._mouseoverRecordBarBound = this._mouseoverRecordBar.bind(this, recordBar);
> +        recordBar.element.addEventListener("mouseenter", this._mouseoverRecordBarBound);
> +    }
> +
> +    didRemoveRecordBar(recordBar)
> +    {
> +        recordBar.element.removeEventListener("mouseenter", this._mouseoverRecordBarBound);
> +        this._mouseoverRecordBarBound = null;
> +    }

Why is this needed? I would expect this could just put the handler on the element and that would be it. Are recordBar elements are reused?

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:283
> +            if (resource.failed)
> +                text.innerHTML = WebInspector.UIString("This resource failed loading.");
> +            else if (resource.urlComponents.scheme === "data")
> +                text.innerHTML = WebInspector.UIString("This resource was loaded with the 'data' scheme.");
> +            else
> +                text.innerHTML = WebInspector.UIString("This resource was cached.");

Nit: "text" is a poor name for a variable containing a span element. "descriptionElement" would be better.
Nit: Use "text.textContent" instead of "text.innerHTML". You are just setting a string not markup.

    - if a resource load failed couldn't that mean a 401, etc. So there should be timing data.
    - "This resource was cached" sounds like a result, perhaps "This resource was served from the cache" would be better.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:312
> +            let addTimingRow = (title, start, end, hideGraph) => {
> +                let data = {
> +                    description: title,
> +                    duration: Number.secondsToMillisecondsString(end-start, true)
> +                };
> +                if (!hideGraph) {
> +                    let record = new WebInspector.SegmentTimelineRecord({start, end});
> +                    data.graph = new WebInspector.TimelineRecordBar([record], WebInspector.TimelineRecordBar.RenderMode.Normal);
> +                }

Could be written easier to follow:

    let addTimingRow = (description, start, end, hideGraph) => {
        let duration = Number.secondsToMillisecondsString(end - start, true);
        let graph = null;
        if (!hideGraph) {
           let record = ...;
           graph = ...;
        }

        let data = {description, duration, graph};
        ...
    }

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:317
> +            addTimingRow(WebInspector.UIString("Stalled"), resource.timingData.startTime, resource.timingData.domainLookupStart || resource.timingData.connectStart || resource.timingData.requestStart);

I still don't know the best name for this. Brainstorming some names:

    Queued, Stalled, Scheduling, Prepare

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:332
> +            this.dataGrid._popover = new WebInspector.Popover();

Style: new WebInspector.Popover;

It looks like this popover will need a windowResizeHandler to reposition itself if the user resizes a detached inspector window while the popover is showing. See other examples of "windowResizeHandler".

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingPopoverDataGridNode.js:53
> +    createCellContent(columnIdentifier, cell)

Nit: You could add a // Protected comment preceding this.

[Johan K. Jensen]

Created attachment 287535 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 287535 [details]
Patch

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

I've already done a pass on this but I think Matt would be the best reviewer for it. He may catch something I don't. Matt could you finish the review?

> Source/WebInspectorUI/ChangeLog:10
> +        Reviewed by Joseph Pecoraro.

You shouldn't but Reviewed by <name> if the only comments so far have been r- from that reviewer!

By leaving this OOPS, commit-queue could insert the reviewer name if a patch is set r+ and cq+.

[Matt Baker]

Comment on attachment 287535 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:30
> +        Add property to hide header.

Super nit: might be more clear if it read "Add property to control header visibility, so grid clients aren't forced to manipulate internal grid styles."

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.css:44
> +    border-top: 1px solid hsl(0, 0%, 65%);

Use border-top: 1px solid var(--border-color);

--border-color is equal to hsl(0, 0%, 70%) which is close enough.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:244
> +    didAddRecordBar(recordBar)

This should be moved to the // Protected section.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:250
> +    didRemoveRecordBar(recordBar)

Ditto.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:283
> +                descriptionElement.textContent = WebInspector.UIString("This resource failed loading.");

"This" feels superfluous. The description should just be "Resource failed to load."

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:285
> +                descriptionElement.textContent = WebInspector.UIString("This resource was loaded with the 'data' scheme.");

"Resource was loaded with the 'data' scheme."

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:287
> +                descriptionElement.textContent = WebInspector.UIString("This resource was served from the cache.");

"Resource was served from the cache."

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:309
> +                let duration = Number.secondsToMillisecondsString(end-start, true);

Replacing the boolean literal with a variable would make this read better:
const higherResolution = true;

Style: include spaces between binary operators and their operands: end - start.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:331
> +            popoverDataGrid.layout();

layout() is a protected method of View and shouldn't be called directly. Use updateLayout() instead.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingPopoverDataGridNode.js:42
> +    get secondsPerPixel() { return 0.01; }

This should be this._resource.duration / graphCellElement.offsetWidth. In practice however TimelineRecordBar will never use it, since the synthesized timeline records don't use inactive start time, so it really doesn't matter (other than preventing as assert in TimelineRecordBar.refresh). I think we should still return an accurate value though, for the reason mentioned in the next comment.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingPopoverDataGridNode.js:49
> +        let endPadding = (this._resource.lastTimestamp - this._resource.firstTimestamp) * 0.03;

Is the magic 3% here to account for the minimum width of the TimelineRecordBar? If so it should be WebInspector.TimelineRecordBar.MinimumWidthPixels * this.secondsPerPixel.

The secondsPerPixel issue can be addressed by normalizing the times for the synthesized TimelineRecord objects used for the resource timing parts. If all the segments have start/end times relative to a resource start time of 0 and end time of 1, and the width of the graph column is hardcoded (instead of a percent), seconds per pixel can be a const.

[Matt Baker]

Comment on attachment 287535 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:295
> +                    width: "40%"

Since the width doesn't change, set to a constant value:
width: `${WebInspector.ResourceTimelineDataGridNode.PopoverGraphColumnWidthPixels}px`

and add:
WebInspector.ResourceTimelineDataGridNode.PopoverGraphColumnWidthPixels = 110;

Now we can use this to calculate secondsPerPixel, which lets us calculate the padded end time!

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:329
> +            addTimingRow(WebInspector.UIString("Total time"), resource.timingData.startTime, resource.timingData.responseEnd, true);

"Total time" should be a plain DataGridNode.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingPopoverDataGridNode.js:28
> +    constructor(data, resource, includesGraph, graphDataSource)

Per our in-person follow up discussion:

- Remove `includesGraph` from the constructor parameters, as it will always be true for these popover grid nodes, and have ResourceTimelineDataGridNode supply a graphDataSource.
- Add constructor parameters for description, startTime, and endTime, which allows this class to synthesize a TimelineRecord and override the `records` getter in the base class.

By doing so the base class can handle the creation of the node's TimelineRecordBar automatically, which is its job to begin with!

[Johan K. Jensen]

Created attachment 287801 [details]
Patch

Thanks for the thorough review Matt.

Here’s a new revision with some of the changes we talked about.
I’m stil a little unsure about the best way to handle the ‘fake’ graphDataSource. Do you have any comments on that?

Currently the popover always tries to be above or below the record bar. I experimented with having the description-popover be on the left, but it didn’t feel right. The popover currently animates its content/position if it can keep the arrow on the same edge. When switching between above/left/below it feels more janky than when it primarily switches between above/below.

[Matt Baker]

Comment on attachment 287801 [details]
Patch

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

Getting very close! r- until the popover issues are resolved.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:156
> +    didAddRecordBar(recordBar)

To skip record bars for things that aren't resources, use:

if (recordBar.records.length && recordBar.records[0].type !== WebInspector.TimelineRecord.Type.Network)
    return;

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:158
> +        this._mouseoverRecordBarBound = this._mouseoverRecordBar.bind(this, recordBar);

This doesn't seem right. There can be multiple bars per data grid node, and this will overwrite the bound listener used by didRemoveRecordBar as soon as the next record bar is added.

Style: _mouseoverRecordBarBound should be named for the event and use "Listener" as a suffix instead of "Bound": _mouseEnterRecordBarListener

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:159
> +        recordBar.element.addEventListener("mouseenter", this._mouseoverRecordBarBound);

We should also handle mouseleave, and dismiss the popover. This should happen after a brief delay to avoid rapid present/dismiss/present cycles. Since another record bar could be hovered before the timeout function is called, _mouseEnterRecordBar should call cancelTimeout.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:283
> +                descriptionElement.textContent = WebInspector.UIString("Resource failed loading.");

Reads better as: "Resource failed to load."

[Johan K. Jensen]

Created attachment 288195 [details]
Patch

[Matt Baker]

Comment on attachment 288195 [details]
Patch

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

r=me, with a couple tiny changes before landing. Nice work!

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:609
> +localizedStrings["Resource failed loading."] = "Resource failed loading.";

localizedStrings.js needs to be updated to reflect the change to this string.

> Source/WebInspectorUI/UserInterface/Views/NetworkGridContentView.js:214
> +            const graphDataSource = this;

Nit: Just pass `this` as it is. We typically only use const to name booleans arguments.

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:237
> +            const graphDataSource = this;

Ditto.

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:215
> +        const graphDataSource = this;

Ditto.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:165
> +        this._mouseEnterRecordBarListener = this._mouseoverRecordBar.bind(this);

Add an assert before making this assignment, since we expect this path to only be taken once:

console.assert(!this._mouseEnterRecordBarListener);

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:271
> +        let recordBar = WebInspector.TimelineRecordBar.fromElement(event.target);

console.assert(recordBar);

[Johan K. Jensen]

Created attachment 288209 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 288209 [details]
Patch

Clearing flags on attachment: 288209

Committed r205578: <http://trac.webkit.org/changeset/205578>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Baker]

(In reply to comment #0)
> Show a breakdown of the different resource loading phases (App Cache, DNS,
> Secure Connection Start, etc) in the Network waterfall:
> 
> 1) Split the "inactive" portion of a resource's timeline record bar into
> colored segments

This will be done as a follow up: https://bugs.webkit.org/show_bug.cgi?id=161755.