Closed
Bug 1468366
Opened 6 years ago
Closed 6 years ago
Transferred column shows "0 GB" instead of "0 B" (same issue applies in status bar)
Categories
(DevTools :: Netmonitor, defect, P2)
Tracking
(firefox63 verified)
VERIFIED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox63 | --- | verified |
People
(Reporter: nawuvi, Assigned: vincent)
Details
Attachments
(4 files)
when no data has been transferred, it's (strangely) displayed as "0 GB" instead of "0 B" -- this is also inconsistent with the Size column which (correctly) shows "0 B"
Updated•6 years ago
|
Product: Firefox → DevTools
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → vi.le
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 1•6 years ago
|
||
Thank you for the report! '0 GB' is displayed because NaN is sent to getFormattedSize and gets formatted to GB here https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/utils/format-utils.js#58 Honza, shall we display '0 B' or SIZE_UNAVAILABLE '-'?
Flags: needinfo?(odvarko)
Comment 2•6 years ago
|
||
(In reply to Vincent Lequertier from comment #1) > Honza, shall we display '0 B' or SIZE_UNAVAILABLE '-'? SIZE_UNAVAILABLE should be displayed in such case. Thanks for helping with this one! Honza
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8986280 [details] Bug 1468366 - Initialize headersSize to null in the devtools network monitor; https://reviewboard.mozilla.org/r/251654/#review258154 The patch looks good, thanks! Do we have a test case I could use to verify that the issue has been fixed? (note that the size is displayed on three places: Size and Transferred size columns and status bar) Honza
Updated•6 years ago
|
Flags: needinfo?(vi.le)
Comment 6•6 years ago
|
||
Thanks! Couple of notes: 1) The summary in the status bar still says 0 GB. It's wrong it should say `-` (SIZE_UNAVAILABLE) if the summary of transferred data is undefined. Or correct summary. See the screenshot, it should be: 519 B 2) The tooltip says `-`. It isn't much helpful, what if we displayed a label saying: `Transferred size is not available`? Honza See the attached screenshot. Honza
Flags: needinfo?(odvarko)
Comment 7•6 years ago
|
||
I am setting priority to P2, it would be great to have this fixed. Honza
Priority: -- → P2
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #4) > Do we have a test case I could use to verify that the issue has been fixed? The test case from nawuvi works as intended.
Flags: needinfo?(vi.le)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #6) > 1) The summary in the status bar still says 0 GB. It's wrong it should say > `-` (SIZE_UNAVAILABLE) if the summary of transferred data is undefined. Or > correct summary. See the screenshot, it should be: 519 B Fixed! > 2) The tooltip says `-`. It isn't much helpful, what if we displayed a label > saying: `Transferred size is not available`? I agree, 'Transferred size is not available' is much more informative.
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8986280 [details] Bug 1468366 - Initialize headersSize to null in the devtools network monitor; https://reviewboard.mozilla.org/r/251654/#review259112 Thanks for the update! The last missing thing is related to localization, please see my inline comment. Honza ::: devtools/client/netmonitor/src/components/RequestListColumnTransferredSize.js:49 (Diff revision 2) > text = getFormattedSize(transferredSize); > - } else if (transferredSize === null) { > - text = SIZE_UNAVAILABLE; > } > > + const title = text == SIZE_UNAVAILABLE ? "Transferred size is not available" : text; The string needs to be localized in netmonitor.properties file Here is the place where `networkMenu.sizeUnavailable` is defined: https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/devtools/client/locales/en-US/netmonitor.properties#203 Similarly, you should define: `networkMenu.sizeUnavailable.title` Some localized strings are already loaded at the top of `RequestListColumnTransferredSize.js` file, so you might see how it's done and append the new one.
Attachment #8986280 -
Flags: review?(odvarko)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8986280 [details] Bug 1468366 - Initialize headersSize to null in the devtools network monitor; https://reviewboard.mozilla.org/r/251654/#review259112 And thanks for the review! > The string needs to be localized in netmonitor.properties file > > Here is the place where `networkMenu.sizeUnavailable` is defined: > > https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/devtools/client/locales/en-US/netmonitor.properties#203 > > > Similarly, you should define: `networkMenu.sizeUnavailable.title` > > Some localized strings are already loaded at the top of `RequestListColumnTransferredSize.js` file, so you might see how it's done and append the new one. Sorry I don't know how I missed that. Fixed ^^'
Assignee | ||
Comment 14•6 years ago
|
||
Wait. There is another issue. The root cause is in network-monitor.js
Assignee | ||
Comment 15•6 years ago
|
||
The bug is probably here https://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#640. I'd like to investigate further.
Assignee | ||
Comment 16•6 years ago
|
||
The problem comes indeed from the line mentioned in my previous comment. As this.httpActivity.headersSize is undefined (because it is not initialized in createOrGetActivityObject [0]), response.transferredSize is NaN. The right fix for this bug is in my opinion to initialize headersSize to null in the httpActivity constructor so response.transferredSize will not be NaN. Note that this [1] comment explains the issue better than I do. Thanks atlanto! If we initialize headersSize to null in the httpActivity constructor, this bug is resolved as we will get '0 B' instead of '0 GB'. So I think we should remove the Number.isNaN checks in my patch and do that instead. Honza, what do you think? Vincent [0] https://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#1241 [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1401741#c10
Flags: needinfo?(odvarko)
Comment 17•6 years ago
|
||
(In reply to Vincent Lequertier from comment #16) > The problem comes indeed from the line mentioned in my previous comment. As > this.httpActivity.headersSize is undefined (because it is not initialized in > createOrGetActivityObject [0]), response.transferredSize is NaN. The right > fix for this bug is in my opinion to initialize headersSize to null in the > httpActivity constructor so response.transferredSize will not be NaN. Note > that this [1] comment explains the issue better than I do. Thanks atlanto! > > If we initialize headersSize to null in the httpActivity constructor, this > bug is resolved as we will get '0 B' instead of '0 GB'. So I think we should > remove the Number.isNaN checks in my patch and do that instead. Honza, what > do you think? Agree, this sounds like better approach to the issue. Thanks for the analysis! Honza
Flags: needinfo?(odvarko)
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8986280 [details] Bug 1468366 - Initialize headersSize to null in the devtools network monitor; https://reviewboard.mozilla.org/r/251654/#review260610 Just removing myself from the review for now (waiting for the update) Honza
Attachment #8986280 -
Flags: review?(odvarko)
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8986280 [details] Bug 1468366 - Initialize headersSize to null in the devtools network monitor; https://reviewboard.mozilla.org/r/251654/#review261794 Thanks for the patch! Works for me. R+ Honza
Attachment #8986280 -
Flags: review?(odvarko) → review+
Assignee | ||
Comment 21•6 years ago
|
||
Push to try here https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e54024e7f6553c970e55be8f6cfaa1f1ce4555d
Comment 22•6 years ago
|
||
@Vincent: to make sure the patch is committed in m-c you need to set checkin-needed flag in Tracking/Keywords field. (I am doing it just now for you) Honza
Keywords: checkin-needed
Assignee | ||
Comment 23•6 years ago
|
||
I just wanted to check if everything is alright in the try push before setting the checkin-needed flag ^^ Thanks for your help!
Comment 24•6 years ago
|
||
(In reply to Vincent Lequertier from comment #23) > I just wanted to check if everything is alright in the try push before > setting the checkin-needed flag ^^ Great Thanks for working on this issue! Honza
Comment 25•6 years ago
|
||
Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/64734ec3245d Initialize headersSize to null in the devtools network monitor; r=Honza
Keywords: checkin-needed
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/64734ec3245d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Updated•6 years ago
|
Flags: qe-verify+
Comment 27•6 years ago
|
||
I have managed to reproduce this issue using Firefox 62.0a1 (BuildId:20180612220131) on Windows 10 64bit. This is verified fixed using Firefox 63.0b7 (BuildId:20180917143811) on Windows 10 64bit, macOS 10.9.5 and Ubuntu 16.04 64bit.
You need to log in
before you can comment on or make changes to this bug.
Description
•