Closed Bug 1168376 Opened 9 years ago Closed 7 years ago

Show transferred size in request summary instead of decompressed size

Categories

(DevTools :: Netmonitor, enhancement, P3)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox54 fixed)

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: sebo, Assigned: leonardo.couto, Mentored)

References

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(2 files, 4 obsolete files)

Incorrect requests size summary
32.20 KB, image/png
Details
1168376-5.patch
21.24 KB, patch
Honza
: review+
Details | Diff | Splinter Review 
The Network panel shows the decompressed size of all network requests in the summary at the bottom as well as within the performance analysis.

More important for network analyses is the size of bytes actually transferred. This size should be shown within the summary instead of the decompressed size. And within the performance analysis the size should be shown additionally to the decompressed size.

Sebastian
I imagine the decoded size is currently used because its always available even when loading from the http cache.

It would be nice to summarize the total transferred and decoded size, though.
As of Firefox 51.0 there seems to be no progress on this issue?

Currently Google Chrome shows the compressed size in their tools, surely we can have both sizes in the tool.
Isn't this dup of bug 1219556?

Honza
Flags: needinfo?(sebastianzartner)
Priority: -- → P3
Bug 1219556 and others are about the size of individual requests, where we do show both sizes today.

This bug is about the summary line near the filter box for all requests, which currently has decompressed size only.

Feels like it's pretty easy to add...?
Flags: needinfo?(sebastianzartner)
Summary: Show transferred size of network requests instead of decompressed size → Show transferred size in request summary instead of decompressed size
It's not a duplicate and it's still an issue.

The size shown in the summary and the performance analysis is still showing the uncompressed size, not the transferred size. I've attached a screenshot to illustrate that. The red framed numbers are referring to the orange framed sizes, but they are expected to refer to the green framed sizes.

Sebastian
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Feels like it's pretty easy to add...?

Mark it as good-first-bug?

Sebastian
Great, thanks for the comments!

So, here is what should be done here:

1) The summary located in Netmonitor's toolbar (in front of the filter box) should also display the transferred size (bytes actually transferred over the wire). The associated tooltip should be updated too.

The Summary should look like as follows:
11 requests, 916,13 KB (transferred: 420,2 KB), 1.32 s

If it turns out to be too long we can skip the word 'transferred' and use it only in the associated tooltip.

2) The Performance View (aka statistics view) should display both: the size and transferred size. This view is about statistics data so, both sizes should be available for analysis.

- The table should have a new column showing transferred size
- Individual columns should have headers so, it's clear what is size and what is transferred size
- The summary at the bottom should have a new field: Transferred Size: XXX KB


Honza
Mentor: odvarko
Keywords: good-first-bug
More details for anyone interested fixing this bug.

1) The Netmonitor's summary is implemented as React component here:
https://dxr.mozilla.org/mozilla-central/rev/fbdfcecf0c774d2221f11aed5a504d5591c774e0/devtools/client/netmonitor/components/summary-button.js#20

2) The tooltip is done using 'title' attribute in the same file:
https://dxr.mozilla.org/mozilla-central/rev/fbdfcecf0c774d2221f11aed5a504d5591c774e0/devtools/client/netmonitor/components/summary-button.js#34

3) The Statistics View is implemented (again as React component) here:
https://dxr.mozilla.org/mozilla-central/rev/fbdfcecf0c774d2221f11aed5a504d5591c774e0/devtools/client/netmonitor/statistics-view.js#30

The stats data (table + headers) should be customized in this file

Honza
Great summary, Honza! That's exactly what I'd expect.

Sebastian
I will give this bug a try, I hope it's ok, my first time around here :D
(In reply to Leonardo Couto from comment #10)
> I will give this bug a try, I hope it's ok, my first time around here :D
OK, great!

Feel free to attach a patch in progress to get early feedback...

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> (In reply to Leonardo Couto from comment #10)
> > I will give this bug a try, I hope it's ok, my first time around here :D
> OK, great!
> 
> Feel free to attach a patch in progress to get early feedback...
> 
> Honza

Cool ! I will try to push something until tomorrow, if I hit any dead end I will post here or ask in the channel, thanks :D
Attached patch 1168376.patch (obsolete) — — Splinter Review 
This patch contains all the ui changes related to transferred size except the specific headers on the chart view, if some reviewers could confirm if this is ok I can add it and what else is needed :)
Attachment #8833611 - Flags: review+
Attachment #8833611 - Flags: feedback+
Comment on attachment 8833611 [details] [diff] [review]
1168376.patch

To ask for review, you need to set the flag to ? and name a person that should review your changes. + means that the review is positive.
Honza, can you please do the review?

Sebastian
Attachment #8833611 - Flags: review?(odvarko)
Attachment #8833611 - Flags: review+
Attachment #8833611 - Flags: feedback+
(In reply to Sebastian Zartner [:sebo] from comment #14)
> To ask for review, you need to set the flag to ? and name a person that
> should review your changes. + means that the review is positive.

Sorry, didn't realized that, thanks !
Comment on attachment 8833611 [details] [diff] [review]
1168376.patch

Review of attachment 8833611 [details] [diff] [review]:
-----------------------------------------------------------------

Leonardo, I can't apply the patch on latest source. There are two conflicts,
can you please rebase?

Honza

unable to find 'devtools/client/netmonitor/components/summary-button.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/components/summary-button.js.rej
unable to find 'devtools/client/netmonitor/statistics-view.js' for patching
4 out of 4 hunks FAILED -- saving rejects to file devtools/client/netmonitor/statistics-view.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh size
Attachment #8833611 - Flags: review?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #16)
> Leonardo, I can't apply the patch on latest source. There are two conflicts,
> can you please rebase?

Sure, I will do it today yet, no problem.
(In reply to Jan Honza Odvarko [:Honza] from comment #16)
> Comment on attachment 8833611 [details] [diff] [review]
> unable to find 'devtools/client/netmonitor/components/summary-button.js' for
> patching
> 1 out of 1 hunks FAILED -- saving rejects to file
> devtools/client/netmonitor/components/summary-button.js.rej
> unable to find 'devtools/client/netmonitor/statistics-view.js' for patching
> 4 out of 4 hunks FAILED -- saving rejects to file
> devtools/client/netmonitor/statistics-view.js.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh size

Ok, I was about to rebase when I noticed the summary-button and statistics-view were deleted on the latest revisions. Is this correct ?
If yes probably I need to check exactly what was changed in the structure and recode the feature right ? :(
(In reply to Leonardo Couto from comment #18)
> Ok, I was about to rebase when I noticed the summary-button and
> statistics-view were deleted on the latest revisions. Is this correct ?
> If yes probably I need to check exactly what was changed in the structure
> and recode the feature right ? :(

Yes, the panel is under refactoring. Wwe are switching to React and removing XUL and doing clean up, sorry about the inconvenience. The good news is that the code base is a lot better now!

* The stats panel is here: netmonitor/components/statistics-panel.js 
* The summary button is now in the toolbar: netmonitor/components/toolbar.js 

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #19)
> (In reply to Leonardo Couto from comment #18)
> > Ok, I was about to rebase when I noticed the summary-button and
> > statistics-view were deleted on the latest revisions. Is this correct ?
> > If yes probably I need to check exactly what was changed in the structure
> > and recode the feature right ? :(
> 
> Yes, the panel is under refactoring. Wwe are switching to React and removing
> XUL and doing clean up, sorry about the inconvenience. The good news is that
> the code base is a lot better now!
> 
> * The stats panel is here: netmonitor/components/statistics-panel.js 
> * The summary button is now in the toolbar: netmonitor/components/toolbar.js 
> 
> Honza

Cool ! Btw, really liked the approach of use directly the react-dom without JSX :)
I will recode the solution until tomorrow with the new structure. Can I have this bug assigned to me ? ( don't know how this flow is done usually )
(In reply to Leonardo Couto from comment #20)
> Cool ! Btw, really liked the approach of use directly the react-dom without
> JSX :)
> I will recode the solution until tomorrow with the new structure. Can I have
Great!

> this bug assigned to me ? ( don't know how this flow is done usually )
Done

Honza
Assignee: nobody → leonardo.couto
Great :) Thanks !
Status: NEW → ASSIGNED
Attached patch 1168376-2.patch (obsolete) — — Splinter Review 
This patch adds transferred size on the net monitor considering the new components changes, including the individual headers on the chart view.
Attachment #8833611 - Attachment is obsolete: true
Attachment #8835208 - Flags: review?(odvarko)
Attachment #8835208 - Flags: feedback?(odvarko)
Comment on attachment 8835208 [details] [diff] [review]
1168376-2.patch

Review of attachment 8835208 [details] [diff] [review]:
-----------------------------------------------------------------

Great work here!

Just one little comment related to localization. Please fix and I'll R+

Two more comments:

1) You don't have to ask for feedback if you are asking for review.
2) It would be great if 'KB' unit isn't hard-coded but, generated automatically by 'getFormattedSize' [1]. This should be done in separate bug, could you please file one for it?

Thanks!

Honza



[1] https://dxr.mozilla.org/mozilla-central/rev/f505911eb333d5ae8c2bf5c44f7b85add6450b53/devtools/client/netmonitor/utils/format-utils.js#38

::: devtools/client/locales/en-US/netmonitor.properties
@@ +151,5 @@
>  # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
>  # This label is displayed in the network table footer providing concise
>  # information about all requests. Parameters: #1 is the number of requests,
> +# #2 is the size, #3 is the transferred size, #4 is the number of seconds.
> +networkMenu.summary=One request, #2 KB (transferred: #3 KB), #4 s;#1 requests, #2 KB (transferred: #3 KB), #4 s

Please rename 'networkMenu.summary' to 'networkMenu.summary2'. Changing the key ensures that translators will get notified about the string (value) change.
Attachment #8835208 - Flags: review?(odvarko)
Attachment #8835208 - Flags: review-
Attachment #8835208 - Flags: feedback?(odvarko)
Cool !

1) I will fix the locale. 
2) About the 'KB', you mean to open a new bug for it and solve it there ?
3) I will change the locale key.

Thanks for your time to review :) !
(In reply to Leonardo Couto from comment #25)
> Cool !
> 
> 1) I will fix the locale. 
> 2) About the 'KB', you mean to open a new bug for it and solve it there ?

Yes please!


> 3) I will change the locale key.
> 
> Thanks for your time to review :) !

Thanks for the patch! :-)

Honza
Attached patch 1168376-3.patch (obsolete) — — Splinter Review 
Change summary translation key to ensure translators update.
Attachment #8835643 - Flags: review+
Attachment #8835643 - Flags: review+ → review?(odvarko)
Bug created for the 'KB' unit:

https://bugzilla.mozilla.org/show_bug.cgi?id=1338284

If someone can check and assign to me :) !
Comment on attachment 8835643 [details] [diff] [review]
1168376-3.patch

Review of attachment 8835643 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

R+, assuming try is green

Let me know if I should push the patch to Try server (to see if all existing tests are passing) before landin it in the tree.

Thanks for working on this, it nice new feature!

Honza
Attachment #8835643 - Flags: review?(odvarko) → review+
I rebased the last changes and tested locally but it's better to be sure on the try server I think, since is my first patch here I'm not sure of anything !

Thanks, if you have time for advice me on others things that I can help will be my pleasure :)
(In reply to Leonardo Couto from comment #31)
> I rebased the last changes and tested locally but it's better to be sure on
> the try server I think, since is my first patch here I'm not sure of
> anything !
Sure, here is the push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ffdf0f04eb1ecd29adba68ae62084887de67441

> 
> Thanks, if you have time for advice me on others things that I can help will
> be my pleasure :)
Excellent!

From comment #24:
> 2) It would be great if 'KB' unit isn't hard-coded but, generated automatically by 'getFormattedSize' > [1]. This should be done in separate bug, could you please file one for it?

This will be nice enhancement of what you just implemented. Please file a new bug, and we can start discussion about how to do it.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #32)
> This will be nice enhancement of what you just implemented. Please file a
> new bug, and we can start discussion about how to do it.

Nice, created here: https://bugzilla.mozilla.org/show_bug.cgi?id=1338284 :)

--

Assuming that this bug is solved the next step usually is add the "checkin-needed" keyword to be merged on main branch right ?
(In reply to Leonardo Couto from comment #33)
> (In reply to Jan Honza Odvarko [:Honza] from comment #32)
> > This will be nice enhancement of what you just implemented. Please file a
> > new bug, and we can start discussion about how to do it.
> 
> Nice, created here: https://bugzilla.mozilla.org/show_bug.cgi?id=1338284 :)
Thanks

> 
> --
> 
> Assuming that this bug is solved the next step usually is add the
> "checkin-needed" keyword to be merged on main branch right ?
Yes, I just did that

Honza
Keywords: checkin-needed
(In reply to Jan Honza Odvarko [:Honza] from comment #34)
> Yes, I just did that

Thanks !
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1dd3438f8b5
Show transferred size in request summary. r=Honza
Keywords: checkin-needed
I will fix the issues, thanks.
Attached patch 1168376-4.patch (obsolete) — — Splinter Review 
- Fix the eslint problems.
- Update tests to consider header row on assertions.
- Update tests for summary.
- Add a few tests for header content.

The https://treeherder.mozilla.org/logviewer.html#?job_id=76870598&repo=mozilla-inbound&lineNumber=12806 I could not reproduce on my machine.
Attachment #8835643 - Attachment is obsolete: true
Attachment #8836920 - Flags: review?(odvarko)
One thing that I noticed after this review is that the empty cache data table is overflowing the screen to the right. Don't know if this should be solved on this ticket on in a new one, but anyway here some of alternatives at a first look that I thought to solve it:

1) Reduce spaces to accommodate the tables and charts.
2) Let the chart overflow how it is right now and make the line in middle draggable.
3) Put the empty cache chart on bottom and enable vertical scrolling.
4) Reduce font size, pie chart size or both.
5) ?

I quite new to this project, so I don't know what was the thought process about the chart view, so let me know what you guys think :)
Comment on attachment 8836920 [details] [diff] [review]
1168376-4.patch

Review of attachment 8836920 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/locales/en-US/netmonitor.properties
@@ +151,5 @@
>  # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
>  # This label is displayed in the network table footer providing concise
>  # information about all requests. Parameters: #1 is the number of requests,
> +# #2 is the size, #3 is the transferred size, #4 is the number of seconds.
> +networkMenu.summary2=One request, #2 KB (transferred: #3 KB), #4 s;#1 requests, #2 KB (transferred: #3 KB), #4 s

You forgot to update the localization note above.
Comment on attachment 8836920 [details] [diff] [review]
1168376-4.patch

Review of attachment 8836920 [details] [diff] [review]:
-----------------------------------------------------------------

R+ assuming Try is green.

Here is yet another try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c7ece9d7be2f264ae5047121810c53d35b623d2


The one in comment #43 looks good.

Honza

::: devtools/client/locales/en-US/netmonitor.properties
@@ +151,5 @@
>  # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
>  # This label is displayed in the network table footer providing concise
>  # information about all requests. Parameters: #1 is the number of requests,
> +# #2 is the size, #3 is the transferred size, #4 is the number of seconds.
> +networkMenu.summary2=One request, #2 KB (transferred: #3 KB), #4 s;#1 requests, #2 KB (transferred: #3 KB), #4 s

Francesco already pointed out, the localization note need to use the right string ID.
Attachment #8836920 - Flags: review?(odvarko) → review+
(In reply to Leonardo Couto from comment #41)
> One thing that I noticed after this review is that the empty cache data
> table is overflowing the screen to the right. Don't know if this should be
> solved on this ticket on in a new one, but anyway here some of alternatives
> at a first look that I thought to solve it:
> 
> 1) Reduce spaces to accommodate the tables and charts.
> 2) Let the chart overflow how it is right now and make the line in middle
> draggable.
> 3) Put the empty cache chart on bottom and enable vertical scrolling.
> 4) Reduce font size, pie chart size or both.
> 5) ?

Can you please file a new bug report and put these suggestions in to it? It should be done separately from this patch. I like these suggestions and some combination of those things would be great. Let's continue discussion about this in the new report.

Thanks!
Honza
Attached patch 1168376-5.patch — — Splinter Review 
Update localization note.
Attachment #8836920 - Attachment is obsolete: true
Attachment #8837284 - Flags: review?(odvarko)
Bug for further discussion on chart data overflow: https://bugzilla.mozilla.org/show_bug.cgi?id=1339558
Comment on attachment 8837284 [details] [diff] [review]
1168376-5.patch

Review of attachment 8837284 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!
Honza
Attachment #8837284 - Flags: review?(odvarko) → review+
Thanks, ready to land.

Honza
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/266f17250570
Show transferred size in request summary. r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/266f17250570
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Thank you for the patch, Leonardo! I verified that it's working for me in 54.0a1 2017-02-18.

I've added a note at https://developer.mozilla.org/en-US/Firefox/Releases/54#Developer_Tools and updated the performance analysis image at https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor.
Will, could you please update the nm-toolbar image?

Sebastian
Status: RESOLVED → VERIFIED
Keywords: dev-doc-needed
Flags: needinfo?(wbamberg)
Depends on: 1346272
I'm no longer working on devtools docs, so clearing the NI.
Flags: needinfo?(wbamberg)
Depends on: 1435320
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: