Bug 147897 – Web Inspector: Network Tab - Improve graphical representation of network waterfall

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

[Iraê]

Created attachment 258742 [details]
Current Safari "Timelines" UI, which mixes network and many other browser events

More and more we see market share shifting from desktop to mobile and we should be able to be more productive in analyzing and improving network performance on our webpages. We at Yahoo have been using alternative tools and different browser to debug and find room for improvement in some websites that are iOS Safari only, and having better ways of debug that on WebKit Inspector would be a huge boost in productivity.

Also, this would make possible to do field testing, with actual Apple devices and real mobile carriers through remote debugging devices connected via USB, instead of using a number of simulators or restricting ourselves to testing on Android devices.

I am attaching a number of images that gather some tools that are currently available for visualizing network traffic and I will be explaining some improvements I think is worthwhile to the Webkit Inspector.

About the existing UI:

  1. Webkit inspector does have a waterfall chart, but currently there is no numbers attached to the UI, we can see examples on Firefox and Charles Waterfall examples.
  2. Once I select a resource on Webkit Inspector, there is not timing detail information available.
  3. No tooltip available when hovering resources.
  4. Resources are mixed with DOM events, Drawing events and etc.
  5. If I click the "Network" row, a lot of data is now available but the chart is gone.

The Network tab is a much appreciated recente addition, but here are some comments:

  1. There is no visual way of visualizing the data
  2. Having a tab is handy, but this is precisely the same data already available on the "Timelines > Network" UI
  3. Tooltips would be helpful
  4. Exporting as HAR archive is a nice feature from other vendors, for archival purposes and remote team communication

We think that adding the waterfall chart on the newly created Network tab and adding some of the items above would be an awesome and very welcome feature for the web community.

[Iraê]

Created attachment 258743 [details]
Webkit current UI for Network tab, lacks some of the mentioned features

[Iraê]

Created attachment 258744 [details]
Firefox waterfall network graph has inline timings and more colors, which help spotting improvements at a glance

[Iraê]

Created attachment 258745 [details]
Chrome Network tab has tooltips and detail "sidebar" with precise information without dismissing the waterfall chart

[Iraê]

Created attachment 258746 [details]
Charles proxy [paid product] allows us to debug certain network conditions on actual iOS devices, but setup is very complicated

[Iraê]

I mentioned HAR support in the description and there is already an issue for that. Added myself to CC list there. https://bugs.webkit.org/show_bug.cgi?id=146692

[Matt Baker]

We should break the feature requests listed here into separate issues. I recommend limiting the scope of this bug to adding the waterfall view to the Network tab and Network Timeline data grids.

[Radar WebKit Bug Importer]

<rdar://problem/27482198>

[Matt Baker]

This bug has been split into two parts:

Web Inspector: Waterfall view should be visible in Network tab and Network Timeline
https://bugs.webkit.org/show_bug.cgi?id=160061

Web Inspector: Show resource timing details in Network waterfall
https://bugs.webkit.org/show_bug.cgi?id=160062

[Matt Baker]

Showing segments for each resource timing data point is being tracked as: https://bugs.webkit.org/show_bug.cgi?id=161755.

[Joseph Pecoraro]

*** Bug 161755 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

Created attachment 323503 [details]
[IMAGE] Waterfall Table

[Joseph Pecoraro]

Created attachment 323504 [details]
[IMAGE] Waterfall Popover - All Parts

[Joseph Pecoraro]

Created attachment 323505 [details]
[IMAGE] Waterfall Popover - Hovering the Response

[Joseph Pecoraro]

Created attachment 323506 [details]
[IMAGE] Waterfall Popover - Some Parts

[Joseph Pecoraro]

Attached first round of pictures for a waterfall and detail popover for the new network tab. This incorporates the latest Resource Timing data we have.

[Joseph Pecoraro]

Created attachment 323507 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 323511 [details]
[PATCH] Proposed Fix

Addressed an issue when showing detail views.

[Joseph Pecoraro]

Created attachment 323852 [details]
[PATCH] Proposed Fix

Rebased.

[Devin Rousso]

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

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

I haven't been able to apply this yet, but here are some NIT/Style comments.  Overall, it looks fine.  One general point of feedback is that some of your single-body if statements could use additional newlines before/after to make things easier to read, but that's just what I prefer.

> Source/WebInspectorUI/ChangeLog:40
> +        (WI.NetworkTableContentView.prototype._populateTransferSizeCell):

This isn't modified.

> Source/WebInspectorUI/ChangeLog:43
> +        (WI.NetworkTableContentView.prototype._updateEntryForResource):

Ditto (40).

> Source/WebInspectorUI/ChangeLog:47
> +        (WI.NetworkTableContentView.prototype._restoreSelectedRow):

Ditto (40).

> Source/WebInspectorUI/ChangeLog:48
> +        (WI.NetworkTableContentView.prototype._tableNameColumnDidChangeWidth):

Ditto (40).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:745
> +localizedStrings["Resource does not have timing data"] = "Resource does not have timing data";

I think we typically use "has no" instead of "does not have".

    WI.UIString("Resource has no timing data")

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:904
> +localizedStrings["TCP"] = "TCP";

Does this deserve a localizedString?  I would think that TCP is not something that should be translated.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:104
> +    top: 0px;
> +    bottom: 0px;

NIT: the "px" are not necessary.

    position: absolute;
    top: 0;
    bottom: 0;
    overflow: hidden;

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:151
> +    height: 6px;
> +    min-width: 2px;

NIT: I usually put width before height.

    position: absolute;
    top: 7px;
    min-width: 2px;
    height: 6px;

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:163
> +    z-index: 2;

NIT: I consider z-index to be part of the positioning "group" (e.g. position, top, right, bottom, left, and z-index), so I'd put it before height.

    top: 1px;
    z-index: 2;
    height: 18px;

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:167
> +    background-color: var(--network-queue-color);

NIT: I typically put "style" properties (ones that are more visual than functional, such as color vs positioning) later.

    min-width: 3px;
    -webkit-margin-start: 1px;
    background-color: var(--network-queue-color);

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:440
> +            block.style[styleAttribute] = startOffset + "px";
> +            block.style.width = width + "px";

NIT: you can use `setProperty` to make this slightly nicer.

    block.style.setProperty(styleAttribute, startOffset + "px");
    block.style.setProperty("width", width + "px");

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:445
> +        let pad = 10 * secondsPerPixel;

NIT: maybe `padSeconds` as a better name (like below)?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:447
> +        mouseBlock.addEventListener("mousedown", this._handleMousedownWaterfall.bind(this, mouseBlock, entry));

How common is this pattern?  I usually find it easier to read to have an inline arrow function instead of a bind-with-arguments.

    mouseBlock.addEventListener("mousedown", (event) => {
        this._handleMousedownWaterfall(mouseBlock, entry, event);
    });

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:465
> +            appendBlock(startTime, requestStart, "queue");
> +        if (connectStart)

Style: add an extra newline to distinguish if-else blocks.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1366
> +            return contentElement;

Instead of doing an early return, you could have the next section just be an else and therefore have one unified return.

    if (!resource.hasResponse() || !resource.timingData.startTime || !resource.timingData.responseEnd)
        contentElement.textContent = WI.UIString("Resource has no timing data");
    else {
        let breakdownView = new WI.ResourceTimingBreakdownView(resource);
        contentElement.appendChild(breakdownView.element);
        breakdownView.updateLayout();
    }

    return contentElement;

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1391
> +                return;

NIT: should return null (eslint).

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1397
> +                return;

Ditto (1391).

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1408
> +        let preferredEdges = isRTL ? [WI.RectEdge.MAX_Y, WI.RectEdge.MIN_Y, WI.RectEdge.MAX_X] : [WI.RectEdge.MAX_Y, WI.RectEdge.MIN_Y, WI.RectEdge.MIN_X];

Could you just push the last item depending on `isRTL`?

    let preferredEdges = [WI.RectEdge.MAX_Y, WI.RectEdge.MIN_Y];
    if (isRTL)
        preferredEdges.push(WI.RectEdge.MAX_X);
    else
        preferredEdges.push(WI.RectEdge.MIN_X);

> Source/WebInspectorUI/UserInterface/Views/Popover.js:596
> +    Normal: "popover-background-normal",

NIT: perhaps "Default" is a better word than "Normal".  Using "Normal" makes me think that "White" is abnormal, or wrong somehow.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.css:39
> +    top: 12px;

NIT: ordering.

    top: 12px;
    min-width: 3px;
    height: 12px;

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.css:50
> +body[dir=ltr] .resource-timing-breakdown .waterfall .block.queue,
> +body[dir=ltr] .resource-timing-breakdown .waterfall .block.request {

NIT: use :matches() instead?

    body[dir=ltr] .resource-timing-breakdown .waterfall .block:matches(.queue, .request) {

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.css:61
> +body[dir=rtl] .resource-timing-breakdown .waterfall .block.queue,
> +body[dir=rtl] .resource-timing-breakdown .waterfall .block.request {

Ditto (49).

    body[dir=rtl] .resource-timing-breakdown .waterfall .block:matches(.queue, .request) {

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.css:77
> +    text-align: left;

You can use `text-align: start` and eliminate the `[dir=ltr]` and `[dir=rtl] selectors.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:73
> +            appendBlock(startTime, requestStart, "queue");
> +        if (connectStart)

Ditto (NetworkTableContentView.js:464).

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:84
> +            let p = numbersSection.appendChild(document.createElement("p"));

This could use a better name, even paragraphElement is more descriptive.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:113
> +                appendRow(WI.UIString("DNS"), (domainLookupEnd || connectStart) - domainLookupStart, "sub", "dns");
> +            appendRow(WI.UIString("TCP"), connectEnd - connectStart, "sub", "connect");

This could use another space.

> Source/WebInspectorUI/UserInterface/Views/Table.js:251
> +                continue;
> +            let cell = row.children[columnIndex];

Ditto (ResourceTimingBreakdownView.js:112).

> Source/WebInspectorUI/UserInterface/Views/Table.js:1106
> +            headerView.element.style.setProperty(WI.resolvedLayoutDirection() === WI.LayoutDirection.RTL ? "right" : "left", offset + "px");

Nice!

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.css:64
> +    --timeline-ruler-height: 23px;

I typically put variables at the end of the rule, so as to not interfere with the styles.

    {
        top: 0;
        left: 0;
        right: 0;
        height: var(--timeline-ruler-height);

       --timeline-ruler-height: 23px;
    }

[Joseph Pecoraro]

Created attachment 323926 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/ChangeLog:40
>> +        (WI.NetworkTableContentView.prototype._populateTransferSizeCell):
> 
> This isn't modified.

It appears prepare-ChangeLog has an issue.

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:745
>> +localizedStrings["Resource does not have timing data"] = "Resource does not have timing data";
> 
> I think we typically use "has no" instead of "does not have".
> 
>     WI.UIString("Resource has no timing data")

Nice!

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:904
>> +localizedStrings["TCP"] = "TCP";
> 
> Does this deserve a localizedString?  I would think that TCP is not something that should be translated.

Hmm, I'll ask around.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:447
>> +        mouseBlock.addEventListener("mousedown", this._handleMousedownWaterfall.bind(this, mouseBlock, entry));
> 
> How common is this pattern?  I usually find it easier to read to have an inline arrow function instead of a bind-with-arguments.
> 
>     mouseBlock.addEventListener("mousedown", (event) => {
>         this._handleMousedownWaterfall(mouseBlock, entry, event);
>     });

Sure I can go this route.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:465
>> +        if (connectStart)
> 
> Style: add an extra newline to distinguish if-else blocks.

I see this as one paragraph of code, so I don't want to add newlines.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1408
>> +        let preferredEdges = isRTL ? [WI.RectEdge.MAX_Y, WI.RectEdge.MIN_Y, WI.RectEdge.MAX_X] : [WI.RectEdge.MAX_Y, WI.RectEdge.MIN_Y, WI.RectEdge.MIN_X];
> 
> Could you just push the last item depending on `isRTL`?
> 
>     let preferredEdges = [WI.RectEdge.MAX_Y, WI.RectEdge.MIN_Y];
>     if (isRTL)
>         preferredEdges.push(WI.RectEdge.MAX_X);
>     else
>         preferredEdges.push(WI.RectEdge.MIN_X);

Doesn't make it any shorter and it isn't too hard to read right now. I'll leave as is.

>> Source/WebInspectorUI/UserInterface/Views/Popover.js:596
>> +    Normal: "popover-background-normal",
> 
> NIT: perhaps "Default" is a better word than "Normal".  Using "Normal" makes me think that "White" is abnormal, or wrong somehow.

Good idea.

>> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.css:50
>> +body[dir=ltr] .resource-timing-breakdown .waterfall .block.request {
> 
> NIT: use :matches() instead?
> 
>     body[dir=ltr] .resource-timing-breakdown .waterfall .block:matches(.queue, .request) {

Good idea!

>> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.css:77
>> +    text-align: left;
> 
> You can use `text-align: start` and eliminate the `[dir=ltr]` and `[dir=rtl] selectors.

Oh snap, I didn't know about this. I can use this in later patches too.

[Blaze Burg]

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

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

r=me with one quibble about the new Table.js API

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:109
> +    top: calc(var(--navigation-bar-height) - var(--timeline-ruler-height));

The future is nice. In some ways.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:160
> +.waterfall .block.mouse {

NIt: can we improve .mouse to .mouse-tracking or something more explanatory?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1389
> +            let rowIndex = this._rowIndexForResource(entry.resource);

I am not sure about this bit. It's possible that the mouse block could extend beyond the cell frame if the cell takes up the full width. This would be more likely to be encountered if a timeline overview has applied zoom/time range to the waterfall charts.

NSTableView exposes -frameOfCellAtColumn:row: for this purpose (https://developer.apple.com/documentation/appkit/nstableview/1524517-frameofcellatcolumn?language=objc). Maybe you should add that instead of cellForRowAndColumn, since 'cell' could be almost anything and digging into it is messy, whereas the Table can calculate the frame of the cell without caring about the contents. For the purposes of this method, anchoring to the frame is fine even if the waterfall might not take it all up. Practically speaking, it's all flush to the right so there isn't much adjustment that would happen with a tighter frame, except perhaps to move the arrow a bit.

Lastly, taking a column index rather than WI.TableColumn as the argument here will ensure by construction that this method is only called on columns that are visible.

> Source/WebInspectorUI/UserInterface/Views/Table.js:1106
> +            headerView.element.style.setProperty(WI.resolvedLayoutDirection() === WI.LayoutDirection.RTL ? "right" : "left", offset + "px");

Please hoist the resolved property name.

> Source/WebInspectorUI/UserInterface/Views/Table.js:1108
> +            headerView.updateLayout(WI.View.LayoutReason.Resize);

Cool.

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:160
>> +.waterfall .block.mouse {
> 
> NIt: can we improve .mouse to .mouse-tracking or something more explanatory?

Done.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1389
>> +            let rowIndex = this._rowIndexForResource(entry.resource);
> 
> I am not sure about this bit. It's possible that the mouse block could extend beyond the cell frame if the cell takes up the full width. This would be more likely to be encountered if a timeline overview has applied zoom/time range to the waterfall charts.
> 
> NSTableView exposes -frameOfCellAtColumn:row: for this purpose (https://developer.apple.com/documentation/appkit/nstableview/1524517-frameofcellatcolumn?language=objc). Maybe you should add that instead of cellForRowAndColumn, since 'cell' could be almost anything and digging into it is messy, whereas the Table can calculate the frame of the cell without caring about the contents. For the purposes of this method, anchoring to the frame is fine even if the waterfall might not take it all up. Practically speaking, it's all flush to the right so there isn't much adjustment that would happen with a tighter frame, except perhaps to move the arrow a bit.
> 
> Lastly, taking a column index rather than WI.TableColumn as the argument here will ensure by construction that this method is only called on columns that are visible.

Here I want to dig into the cell itself (to get the mouse-tracking div) I don't think just the cell's frame would be enough. I want the popover to center over the waterfall graph and not always center it on the cell. Currently if the rect goes out of bounds of the cell this actually behaves correctly and bounds the popover within the cell.

Clients don't really know which columns are visible or not. They could figure it out (or we could vend Table API) but if Table were updated in the future to allow reordering columns it would need to be computed dynamically anyways. I think using a Column is the way to go at the API level. Table can always figure out the answer.

[Joseph Pecoraro]

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

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

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:447
> +        mouseBlock.addEventListener("mousedown", (event) => {

I am drive-by going to avoid right clicking:

    if (event.button !== 0 || event.ctrlKey)
        return;

A right click should not show the popover.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/223734/webkit>