Bug 178323 – Web Inspector: Network Tab - Metrics Detail View

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

[Joseph Pecoraro]

Network Tab: Metrics Detail View

Include resource details such as:

  • Transfer Size information (Header + Body bytes)
  • Resource Size information (Compression, MIME)
  • Timing information (ResourceTiming, like Waterfall)

[Joseph Pecoraro]

<rdar://problem/34071929>

[Joseph Pecoraro]

Created attachment 323856 [details]
[IMAGE] Metrics - Data

[Joseph Pecoraro]

Created attachment 323857 [details]
[IMAGE] Metrics - No Data

[Joseph Pecoraro]

Created attachment 323858 [details]
[IMAGE] Metrics - Loading

[Joseph Pecoraro]

Created attachment 323859 [details]
[IMAGE] Metrics - Loading Done

[Joseph Pecoraro]

Created attachment 323860 [details]
[IMAGE] Metrics - Compression Warning

[Joseph Pecoraro]

Created attachment 323862 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

Cool! Compression on 16 bytes is unlikely. Maybe the warning (here and elsewhere in the Inspector) should have a minimum resource size too?

[Joseph Pecoraro]

(In reply to Timothy Hatcher from comment #8)
> Cool! Compression on 16 bytes is unlikely. Maybe the warning (here and
> elsewhere in the Inspector) should have a minimum resource size too?

Hah! Yes, good point. I'll look at a minimum size of 1kb.

[Joseph Pecoraro]

Created attachment 323917 [details]
[PATCH] Proposed Fix

This tweaks styles slightly and sets a 1kb minimum on warning case.

[Devin Rousso]

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

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

> Source/WebInspectorUI/UserInterface/Base/MIMETypeUtilities.js:148
> +}

Style: missing semicolon.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:89
> +        this._contentBrowser.navigationBar.selectedNavigationItem = this._previewNavigationItem;

Should this use `_metricsNavigationItem`?

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:91
> +        this._resourceContentView.showRequest();

Should this use `_metricsContentView`?

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:96
> +        this._contentBrowser.navigationBar.selectedNavigationItem = this._previewNavigationItem;

Ditto (89).

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:98
> +        this._resourceContentView.showResponse();

Ditto (91).

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:120
> +        this._detailsNavigationItem = new WI.RadioButtonNavigationItem("metrics", WI.UIString("Metrics"));

I think this should be renamed to `_metricsNavigationItem`.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:411
>          let goToButton = detailsElement.appendChild(WI.createGoToArrowButton());
> -        goToButton.addEventListener("click", this._goToRequestDataClicked.bind(this));
> +        goToButton.addEventListener("click", () => { this._delegate.headersContentViewGoToRequestData(this); });
>          this._appendKeyValuePair(detailsElement, WI.UIString("Request Data"), goToButton);

Should this only be added if `_delegate` has a `headersContentViewGoToRequestData` member?

    if (this._delegate.headersContentViewGoToRequestData) {
        ...
    }

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:-489
> -    _goToRequestDataClicked(event)
> -    {
> -        if (this._delegate)
> -            this._delegate.headersContentViewGoToRequestData(this);
> -    }

I think you should keep this function.  My criteria for inlining an event listener is if the callback function needs to access something local that is not easily distinguishable if the callback were to be a bound member function.  In this case, there is nothing unique about anything used by `_goToRequestDataClicked`, so it can be used for all `goToButton` instances.

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:47
> +    margin-bottom: 10px;

Style: I put `margin` (spacing) before `font` (presentation).

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:86
> +    margin-top: 2px;

Ditto (47).

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:96
> +body[dir=ltr] .resource-metrics > .content > section.network table > tr > td.label {
> +    text-align: right;
> +}
> +
> +body[dir=rtl] .resource-metrics > .content > section.network table > tr > td.label {
> +    text-align: left;
> +}

You can use `text-align: start` and eliminate any `body[dir=*]` selectors.

    .resource-metrics > .content > section.network table > tr > td.label {
        text-align: start;
    }

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:100
> +    margin: 13px 1px 0 1px;

Ditto (47).

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:101
> +    display: inline-block;

Style: I put `display` (visibility) before `margin` (spacing).

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:113
> +    bottom: -1px;

Style: I put `bottom` (positioning) before `height` (sizing).

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:120
> +    width: 10px;
> +    height: 10px;

Style: I put `width`/`height` (sizing) before `margin` (spacing).

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:134
> +    list-style-type: none;
> +    padding: 0;
> +    margin: 0;

Style: I put `margin`/`padding` (spacing) before `list-*` (presentation).

    margin: 0;
    padding: 0;
    list-style-type: none;

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:140
> +    padding: 1px 0;
> +    position: relative;
> +    height: 20px;

Style: ordering.

    position: relative;
    height: 20px;
    padding: 1px 0;

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:151
> +    min-width: 1px;
> +    top: 4px;

Ditto (138).

    top: 4px;
    min-width: 1px;

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:158
> +    color: black;
> +    display: inline-block;
> +    min-width: 70px;

Ditto (138).

    display: inline-block;
    min-width: 70px;
    color: black;

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:167
> +body[dir=ltr] .resource-metrics > .content > section.timing > ul > li > .row-label {
> +    text-align: right;
> +}
> +
> +body[dir=rtl] .resource-metrics > .content > section.timing > ul > li > .row-label {
> +    text-align: left;
> +}

Ditto (90).

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:189
> +    color gray;

Missing colon.

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:67
> +            let subtitleElement = parentElement.appendChild(document.createElement("div"));
> +            subtitleElement.className = "subtitle";
> +            subtitleElement.textContent = subtitle;

Is this necessary?  I don't see "subtitle" anywhere in the screenshots.

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:88
> +            let value1 = headerRow.appendChild(document.createElement("td"));

Style: value1Element

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:91
> +            let value2 = bodyRow.appendChild(document.createElement("td"));

Style: value2Element

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:136
> +        resourceSection.classList.add("large");

NIT: can you add this to the `className` on line 133?

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:140
> +        resourceComponents.imageElement.width = "32";
> +        resourceComponents.imageElement.height = "32";

Can you move these to CSS with the `.large` selector?

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:246
> +            appendGoToArrow(this._sendingHeaderBytesElement, () => { this._delegate.metricsContentViewGoToHeaders(this); });

This function is used multiple times without changing at all, so you could probably make it a member variable.

    this._boundDelegateMetricsContentViewGoToHeaders = () => { this._delegate.metricsContentViewGoToHeaders(this); };

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:248
> +            appendGoToArrow(this._sendingBodyBytesElement, () => { this._delegate.metricsContentViewGoToRequestBody(this); });

Ditto (246).

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:250
> +            appendGoToArrow(this._receivingHeaderBytesElement, () => { this._delegate.metricsContentViewGoToHeaders(this); });

Ditto (246).

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:252
> +            appendGoToArrow(this._receivingBodyBytesElement, () => { this._delegate.metricsContentViewGoToResponseBody(this); });

Ditto (246).

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:291
> +            let p = this._timingSection.appendChild(document.createElement("p"));

Style: this could use a better name, even `paragraphElement`.

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:300
> +        listElement.classList.add("waterfall"); // Include waterfall block styles.

Style: use `className`.

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:316
> +            block.style[property] = startOffset + "px";

NIT: use `setProperty`.

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

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:328
> +            timeLabel.style[property] = positionOffset + "px";

Ditto (316).

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:89
>> +        this._contentBrowser.navigationBar.selectedNavigationItem = this._previewNavigationItem;
> 
> Should this use `_metricsNavigationItem`?

For all 4 of this this is "going to the request/response body" so it should go to the Preview and show the request/response.

>> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:120
>> +        this._detailsNavigationItem = new WI.RadioButtonNavigationItem("metrics", WI.UIString("Metrics"));
> 
> I think this should be renamed to `_metricsNavigationItem`.

Oops, yes it should have.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:411
>>          this._appendKeyValuePair(detailsElement, WI.UIString("Request Data"), goToButton);
> 
> Should this only be added if `_delegate` has a `headersContentViewGoToRequestData` member?
> 
>     if (this._delegate.headersContentViewGoToRequestData) {
>         ...
>     }

This is a required delegate and a required delegate method. No need for us to if check things that should be required.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:-489
>> -    }
> 
> I think you should keep this function.  My criteria for inlining an event listener is if the callback function needs to access something local that is not easily distinguishable if the callback were to be a bound member function.  In this case, there is nothing unique about anything used by `_goToRequestDataClicked`, so it can be used for all `goToButton` instances.

Since they all have to `bind(this)` we aren't really saving anything.

>> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:96
>> +}
> 
> You can use `text-align: start` and eliminate any `body[dir=*]` selectors.
> 
>     .resource-metrics > .content > section.network table > tr > td.label {
>         text-align: start;
>     }

I learned this in the last patch review! Thanks

>> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:189
>> +    color gray;
> 
> Missing colon.

Oops!

>> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:67
>> +            subtitleElement.textContent = subtitle;
> 
> Is this necessary?  I don't see "subtitle" anywhere in the screenshots.

These subtitles are the "Bytes Sent" / "Bytes Received" etc.

>> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:300
>> +        listElement.classList.add("waterfall"); // Include waterfall block styles.
> 
> Style: use `className`.

Oops, yep.

[Joseph Pecoraro]

Created attachment 324254 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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

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

r=me.  One general comment; you have a lot of anonymous arrow functions that have the same body.  I mentioned this in the review of the previous patch, but would it be worth it to save these values to a `_bound*` variable so as to reduce duplication?  This might be useful if the go-to arrows are recreated multiple times (after refreshes), but I'm not sure how common that would be.

> Source/WebInspectorUI/ChangeLog:1
> [phone]  Joseph Pecoraro  <[e-mail]>

Update date.

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:288
> +            let p = this._timingSection.appendChild(document.createElement("p"));

Style: this could use a better name, even `paragraphElement`.

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:313
> +            block.style[property] = startOffset + "px";

NIT: use `setProperty`.

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

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:325
> +            timeLabel.style[property] = positionOffset + "px";

Ditto (313).

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #14)
> Comment on attachment 324254 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324254&action=review
> 
> r=me.  One general comment; you have a lot of anonymous arrow functions that
> have the same body.  I mentioned this in the review of the previous patch,
> but would it be worth it to save these values to a `_bound*` variable so as
> to reduce duplication?  This might be useful if the go-to arrows are
> recreated multiple times (after refreshes), but I'm not sure how common that
> would be.

I wouldn't define two identical arrow functions as a lot. Also they might not be identical in the future (one could link to the request headers, the other the response headers). I'm also not worried about this duplication. When new go-to arrows are made the old ones (and their listeners) can be GC'd. In the meantime the bound functions are tiny in size, compared to the elements we are creating. I think the readability here is better then optimizing for a few bytes.


> > Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:288
> > +            let p = this._timingSection.appendChild(document.createElement("p"));
> 
> Style: this could use a better name, even `paragraphElement`.
> 
> > Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:313
> > +            block.style[property] = startOffset + "px";
> 
> NIT: use `setProperty`.
> 
>     block.style.setProperty(property, startOffset + "px");
> 
> > Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:325
> > +            timeLabel.style[property] = positionOffset + "px";
> 
> Ditto (313).

There is enough precedence to do the opposite of these I've avoided doing them in all the reviews. It seems like just a style thing. If we can conclude that `setProperty` is actually better than a style property access then I'd be happy to make a more global change to cleanup all instances and stick to one style moving forward.

[Blaze Burg]

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

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

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:76
> +    border-right: 1px solid var(--border-color);

Is this an issue for RTL?

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:86
> +    text-align: left;

Should this be text-align: start? Or turned into an RTL rule?

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:121
> +    margin-left: 3px;

Is this an issue for RTL?

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:198
> +        if (bytes < 103)

I'm pretty sure we have a utility to format as B/KB/etc somewhere. Why add a new one?

> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:249
> +            appendGoToArrow(this._receivingBodyBytesElement, () => { this._delegate.metricsContentViewGoToResponseBody(this); });

This seems a little strange to use a delegate, but I guess it's ok?

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:76
>> +    border-right: 1px solid var(--border-color);
> 
> Is this an issue for RTL?

Nope, since its a 1px border with 10px of padding on both sides. border-right or border-left would produce the same result =)

>> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:86
>> +    text-align: left;
> 
> Should this be text-align: start? Or turned into an RTL rule?

Oops, yes it should.

>> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.css:121
>> +    margin-left: 3px;
> 
> Is this an issue for RTL?

Doh, yes I made this -webkit-margin-start.

>> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:198
>> +        if (bytes < 103)
> 
> I'm pretty sure we have a utility to format as B/KB/etc somewhere. Why add a new one?

A few reasons.

  1. This prefers KB over B for values between 0.1 KB and 1KB.
     - This is to provide mostly consistent units instead of jumping between B and KB for resources hovering around the same size.
  2. This always prefers one decimal point.
     - Same thing, to provide consistent units. Too much precision just ends up being annoying.
  3. This provides the value and suffix separately.
     - This allows the UI here to format them differently.
     - Technically the existing Utility functions could sit on top of something like this, but (1) and (2) make that difficult without making a lot of options.

>> Source/WebInspectorUI/UserInterface/Views/ResourceMetricsContentView.js:249
>> +            appendGoToArrow(this._receivingBodyBytesElement, () => { this._delegate.metricsContentViewGoToResponseBody(this); });
> 
> This seems a little strange to use a delegate, but I guess it's ok?

Yeah. I think of this as each of the Resource*ContentViews expect to be tied to NetworkResourceDetailView. So here delegate is always a DetailView. I just named it delegate for familiarity but made it required and tightly coupled with these expected methods. If we ever go away from this we could choose to add these kind of embellishments if the delegate supports them or not. So I still think a delegate approach has merit if we were to go down that road.

[Joseph Pecoraro]

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