Bug 150987 – Web Inspector: Dashboard weight/size should show transferred size not total resources size

• Component: Web Inspector
• Status: ASSIGNED

[Joseph Pecoraro]

* SUMMARY
Dashboard weight/size should show transferred size not total resources size.

This would be more useful for performance analysis.

[João Oliveira]

Created attachment 265440 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/23525542>

[João Oliveira]

Created attachment 265442 [details]
patch

[Joseph Pecoraro]

Comment on attachment 265442 [details]
patch

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

Looks good. As mentioned on IRC, I'd like to see some tests for WebInspector.Resource.size/transferSize. Like, do we handle transferSize of dataURLs correctly as 0 for non-main resources?

> Source/WebInspectorUI/ChangeLog:9
> +        show transferred size on Dashboard weight
> +        show resources size on Dashboard weight tooltip

Nit: Use full sentences starting with a capital and ending with a period. This is meant to be a paragraph describing the patch.

> Source/WebInspectorUI/UserInterface/Models/DefaultDashboard.js:154
> +        if (!event.target.mainResource.cached && event.target.mainResource.size)
> +            this.transferredSize = event.target.mainResource.size;

Why are we resorting to using event.target.mainResource.size here instead of mainResource's transferSize? Are we running into an issue where the main resource's transfer size is not correct?

r- until we get an answer to this question!

> Source/WebInspectorUI/UserInterface/Views/DefaultDashboardView.js:93
> +
> +        } else {
> +            sizeItem.text = "\u2014";
> +        }

Style: No need for the blank line.
Style: No braces for single statement if/else, so remove the braces for the else block.

[João Oliveira]

Created attachment 265728 [details]
patch

[Joseph Pecoraro]

Comment on attachment 265728 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Models/DefaultDashboard.js:153
> +        if (!event.target.mainResource.cached && event.target.mainResource.transferSize)
> +            this.transferredSize = event.target.mainResource.transferSize;

This still needs an explanation. Why not include the transferSize of the main resource when cached? At the least, I suspect we do a "if modified since" check on the main resource.

[Joseph Pecoraro]

Comment on attachment 265728 [details]
patch

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

>> Source/WebInspectorUI/UserInterface/Models/DefaultDashboard.js:153
>> +            this.transferredSize = event.target.mainResource.transferSize;
> 
> This still needs an explanation. Why not include the transferSize of the main resource when cached? At the least, I suspect we do a "if modified since" check on the main resource.

As was mentioned previous, r- until we get an answer to this question.

Also, I know you are working on a test for WebInspector.Resource. I think that is important for this patch so we can better guarantee `transferSize` is what we want it to be and that this feature won't regress in the future!

[Timothy Hatcher]

It would be great to finish this up and get it landed.

[Timothy Hatcher]

<rdar://problem/23443930>

[Nikita Vasilyev]

I can take a look at this.

(In reply to comment #7)
> ...
> Also, I know you are working on a test for WebInspector.Resource. I think
> that is important for this patch so we can better guarantee `transferSize`
> is what we want it to be and that this feature won't regress in the future!

Do we have any tests for WebInspector.Resource?
Where would the new tests go?

[Timothy Hatcher]

We have LayoutTests/inspector/page/main-frame-resource.html

[Joseph Pecoraro]

> Do we have any tests for WebInspector.Resource?
> Where would the new tests go?

A test for a model object like this would likely go here:

    LayoutTests/inspector/unit-tests/resource.html

We have a bunch of tests for Model/Controller objects in inspector/unit-tests.