Closed
Bug 1338284
Opened 7 years ago
Closed 7 years ago
Make KB unit on summary and chart view dynamic
Categories
(DevTools :: Netmonitor, defect, P2)
DevTools
Netmonitor
Tracking
(firefox54 verified)
VERIFIED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | verified |
People
(Reporter: leonardo.couto, Assigned: leonardo.couto, Mentored)
Details
Attachments
(1 file, 2 obsolete files)
7.21 KB,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
The 'KB' units used on the requests summary and in the chart view are currently hard coded on the translations: https://dxr.mozilla.org/mozilla-central/rev/f505911eb333d5ae8c2bf5c44f7b85add6450b53/devtools/client/locales/en-US/netmonitor.properties A best approach would be use the 'getFormattedSize' function: https://dxr.mozilla.org/mozilla-central/rev/f505911eb333d5ae8c2bf5c44f7b85add6450b53/devtools/client/netmonitor/utils/format-utils.js#38 Reference: https://bugzilla.mozilla.org/show_bug.cgi?id=1168376#c24
Comment 1•7 years ago
|
||
Thanks for the report Leonardo. I just assigned the bug to you. Let me know if you have any questions. Honza
Assignee: nobody → leonardo.couto
Priority: -- → P2
Assignee | ||
Comment 2•7 years ago
|
||
Thanks :) I will try to attach a patch in the next few days.
Assignee | ||
Comment 3•7 years ago
|
||
I'm limiting the displaying of the bytes with 2 decimals since the default had no decimals limit. Let me know if we should ignore decimals on the bytes, display 2 decimals as is with the KB or keep unlimited.
Attachment #8838275 -
Flags: review?(odvarko)
Comment 4•7 years ago
|
||
Comment on attachment 8838275 [details] [diff] [review] 1338284.patch Review of attachment 8838275 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this! Please see my inline comments. Honza ::: devtools/client/netmonitor/components/toolbar.js @@ +93,3 @@ > .replace("#1", count) > + .replace("#2", getFormattedSize(contentSize / 1024)) > + .replace("#3", getFormattedSize(transferredSize / 1024)) Do not divide by 1024. It already happens inside getFormattedSize. @@ +96,1 @@ > .replace("#4", getTimeWithDecimals(millis / 1000)); It would be great if we also introduce `getFormattedTime`. Can be done as part of this bug report or as part of new bug (I'll let you to decide) ::: devtools/client/netmonitor/test/browser_net_footer-summary.js @@ +69,5 @@ > + L10N.numberWithDecimals(requestsSummary.contentSize / 1024, 2) > + )) > + .replace("#3", getFormattedSize( > + L10N.numberWithDecimals(requestsSummary.transferredSize / 1024, 2) > + )) This looks weird. The `getFormattedSize` expects a number (bytes) and calls `getSizeWithDecimals` and `numberWithDecimals``internally. Also, there are test failures where running this test.
Attachment #8838275 -
Flags: review?(odvarko) → review-
Comment 5•7 years ago
|
||
(In reply to Leonardo Couto from comment #3) > I'm limiting the displaying of the bytes with 2 decimals since the default > had no decimals limit. Let me know if we should ignore decimals on the > bytes, display 2 decimals as is with the KB or keep unlimited. * There should be no decimals in case the size is in Bytes * There should be two decimals max. We might want to change it to just one (to save space) but, let's see after the patch is fixed. * There should be no decimals in case of '0', i.e. Instead of '12.0 KB' we should render just '12 KB' Honza
Assignee | ||
Comment 6•7 years ago
|
||
Thanks ! I will do the fixes and add a new patch :)
Assignee | ||
Comment 7•7 years ago
|
||
- Update with changes based on the last review. - Introduce dynamic time units. - Add a check for values with only 0s on decimals. Let me know if I am the right path :)
Attachment #8838275 -
Attachment is obsolete: true
Attachment #8838847 -
Flags: review?(odvarko)
Comment 8•7 years ago
|
||
Comment on attachment 8838847 [details] [diff] [review] 1338284-2.patch Review of attachment 8838847 [details] [diff] [review]: ----------------------------------------------------------------- Works great now! R+, but please see my two small comments. Thanks, Honza ::: devtools/client/netmonitor/utils/format-utils.js @@ +21,1 @@ > const REQUEST_TIME_DECIMALS = 2; This is now used in both `getSizeWithDecimals` as well as `getTimeWithDecimals` so, it could be renamed to e.g. `REQUEST_DECIMALS`. Honza @@ +21,2 @@ > const REQUEST_TIME_DECIMALS = 2; > +const NO_DECIMALS = 0; This constant is used only in 1 `formatDecimals` and I think it can be removed. Just use `0` in `formatDecimals` function.
Attachment #8838847 -
Flags: review?(odvarko) → review+
Comment 9•7 years ago
|
||
One more thing: The 'transferred' label (bug 1168376) counts also requests that has been loaded from cache. I believe that these should not be included in the sum. Can you please file a bug for it? Honza
Flags: needinfo?(leonardo.couto)
Comment 10•7 years ago
|
||
Try looks good (with the current patch) https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e084a2bdd2115924344761a9485b3e01b20e648 Honza
Assignee | ||
Comment 11•7 years ago
|
||
- Change name of REQUEST_TIME_DECIMALS to REQUEST_DECIMALS - Remove const NO_DECIMALS
Attachment #8838847 -
Attachment is obsolete: true
Attachment #8839241 -
Flags: review?(odvarko)
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #9) > The 'transferred' label (bug 1168376) counts also requests that has been > loaded from cache. I believe that these should not be included in the sum. > Can you please file a bug for it? Opened here: https://bugzilla.mozilla.org/show_bug.cgi?id=1341107 Can I have it assigned for me ? Thanks !
Flags: needinfo?(leonardo.couto)
Comment 13•7 years ago
|
||
Comment on attachment 8839241 [details] [diff] [review] 1338284-3.patch Review of attachment 8839241 [details] [diff] [review]: ----------------------------------------------------------------- LGTM Thanks! Honza
Attachment #8839241 -
Flags: review?(odvarko) → review+
Updated•7 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 14•7 years ago
|
||
(In reply to Leonardo Couto from comment #12) > (In reply to Jan Honza Odvarko [:Honza] from comment #9) > > The 'transferred' label (bug 1168376) counts also requests that has been > > loaded from cache. I believe that these should not be included in the sum. > > Can you please file a bug for it? > > Opened here: https://bugzilla.mozilla.org/show_bug.cgi?id=1341107 > Can I have it assigned for me ? Thanks ! Done Thank you! Honza
Keywords: checkin-needed
Comment 15•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ddc511090d07 Make KB unit on summary and chart view dynamic. r=Honza
Keywords: checkin-needed
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ddc511090d07
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 17•7 years ago
|
||
I have reproduced this bug with nightly 54.0a1 (2017-02-09) on Ubuntu 16.04(64 Bit). The bug's fix is now verified on Latest beta 54.0b9. Build ID : 20170518175637 User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0 [testday-20170519]
Comment 18•7 years ago
|
||
I have successfully reproduced this bug with Nightly 54.0a1 (2017-02-09) (32-bit) on windows 10(32bit) this bug is verified fix with latest Release 54.0 (32-bit) Build ID: 20170608105825 Mozilla/5.0 (Windows NT 10.0; rv:54.0) Gecko/20100101 Firefox/54.0
QA Whiteboard: [bugday-20170621]
Comment 19•7 years ago
|
||
As per Comment 17 and Comment 18, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•