Bug 147897 - Web Inspector: Network Tab - Improve graphical representation of network waterfall
Summary: Web Inspector: Network Tab - Improve graphical representation of network wate...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
: 161755 (view as bug list)
Depends on: 160061 160062 161755
Blocks:
  Show dependency treegraph
 
Reported: 2015-08-11 13:20 PDT by Iraê
Modified: 2017-10-19 19:13 PDT (History)
10 users (show)

See Also:


Attachments
Current Safari "Timelines" UI, which mixes network and many other browser events (323.46 KB, image/png)
2015-08-11 13:20 PDT, Iraê
no flags Details
Webkit current UI for Network tab, lacks some of the mentioned features (327.04 KB, image/png)
2015-08-11 13:22 PDT, Iraê
no flags Details
Firefox waterfall network graph has inline timings and more colors, which help spotting improvements at a glance (1.35 MB, image/png)
2015-08-11 13:23 PDT, Iraê
no flags Details
Chrome Network tab has tooltips and detail "sidebar" with precise information without dismissing the waterfall chart (672.06 KB, image/png)
2015-08-11 13:24 PDT, Iraê
no flags Details
Charles proxy [paid product] allows us to debug certain network conditions on actual iOS devices, but setup is very complicated (648.95 KB, image/png)
2015-08-11 13:25 PDT, Iraê
no flags Details
[IMAGE] Waterfall Table (176.87 KB, image/png)
2017-10-11 19:03 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Waterfall Popover - All Parts (197.74 KB, image/png)
2017-10-11 19:03 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Waterfall Popover - Hovering the Response (197.94 KB, image/png)
2017-10-11 19:03 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Waterfall Popover - Some Parts (185.16 KB, image/png)
2017-10-11 19:04 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (49.03 KB, patch)
2017-10-11 19:25 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (49.10 KB, patch)
2017-10-11 19:52 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (46.90 KB, patch)
2017-10-15 17:09 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (46.58 KB, patch)
2017-10-16 12:14 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Iraê 2015-08-11 13:20:47 PDT
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.
Comment 1 Iraê 2015-08-11 13:22:08 PDT
Created attachment 258743 [details]
Webkit current UI for Network tab, lacks some of the mentioned features
Comment 2 Iraê 2015-08-11 13:23:16 PDT
Created attachment 258744 [details]
Firefox waterfall network graph has inline timings and more colors, which help spotting improvements at a glance
Comment 3 Iraê 2015-08-11 13:24:16 PDT
Created attachment 258745 [details]
Chrome Network tab has tooltips and detail "sidebar" with precise information without dismissing the waterfall chart
Comment 4 Iraê 2015-08-11 13:25:18 PDT
Created attachment 258746 [details]
Charles proxy [paid product] allows us to debug certain network conditions on actual iOS devices, but setup is very complicated
Comment 5 Iraê 2015-08-11 16:27:53 PDT
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
Comment 6 Matt Baker 2016-07-21 16:09:05 PDT
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.
Comment 7 Radar WebKit Bug Importer 2016-07-21 16:11:31 PDT
<rdar://problem/27482198>
Comment 8 Matt Baker 2016-07-21 16:40:12 PDT
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
Comment 9 Matt Baker 2016-09-08 13:33:10 PDT
Showing segments for each resource timing data point is being tracked as: https://bugs.webkit.org/show_bug.cgi?id=161755.
Comment 10 Joseph Pecoraro 2017-10-11 19:00:28 PDT
*** Bug 161755 has been marked as a duplicate of this bug. ***
Comment 11 Joseph Pecoraro 2017-10-11 19:03:09 PDT
Created attachment 323503 [details]
[IMAGE] Waterfall Table
Comment 12 Joseph Pecoraro 2017-10-11 19:03:26 PDT
Created attachment 323504 [details]
[IMAGE] Waterfall Popover - All Parts
Comment 13 Joseph Pecoraro 2017-10-11 19:03:45 PDT
Created attachment 323505 [details]
[IMAGE] Waterfall Popover - Hovering the Response
Comment 14 Joseph Pecoraro 2017-10-11 19:04:04 PDT
Created attachment 323506 [details]
[IMAGE] Waterfall Popover - Some Parts
Comment 15 Joseph Pecoraro 2017-10-11 19:05:20 PDT
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.
Comment 16 Joseph Pecoraro 2017-10-11 19:25:40 PDT
Created attachment 323507 [details]
[PATCH] Proposed Fix
Comment 17 Joseph Pecoraro 2017-10-11 19:52:12 PDT
Created attachment 323511 [details]
[PATCH] Proposed Fix

Addressed an issue when showing detail views.
Comment 18 Joseph Pecoraro 2017-10-15 17:09:20 PDT
Created attachment 323852 [details]
[PATCH] Proposed Fix

Rebased.
Comment 19 Devin Rousso 2017-10-15 21:17:57 PDT
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;
    }
Comment 20 Joseph Pecoraro 2017-10-16 12:14:55 PDT
Created attachment 323926 [details]
[PATCH] Proposed Fix
Comment 21 Joseph Pecoraro 2017-10-16 12:15:27 PDT
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.
Comment 22 BJ Burg 2017-10-19 15:00:55 PDT
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.
Comment 23 Joseph Pecoraro 2017-10-19 18:54:07 PDT
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.
Comment 24 Joseph Pecoraro 2017-10-19 18:55:03 PDT
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.
Comment 25 Joseph Pecoraro 2017-10-19 19:13:02 PDT
<https://trac.webkit.org/changeset/223734/webkit>