* SUMMARY Dashboard weight/size should show transferred size not total resources size. This would be more useful for performance analysis.
Created attachment 265440 [details] patch
<rdar://problem/23525542>
Created attachment 265442 [details] patch
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.
Created attachment 265728 [details] patch
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.
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!
It would be great to finish this up and get it landed.
<rdar://problem/23443930>
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?
We have LayoutTests/inspector/page/main-frame-resource.html
> 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.