If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make KB unit on summary and chart view dynamic

VERIFIED FIXED in Firefox 54

Status

()

Firefox
Developer Tools: Netmonitor
P2
normal
VERIFIED FIXED
7 months ago
3 months ago

People

(Reporter: Leonardo Couto, Assigned: Leonardo Couto, Mentored)

Tracking

unspecified
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox54 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 months ago
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
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 months ago
Thanks :) I will try to attach a patch in the next few days.
(Assignee)

Comment 3

7 months ago
Created attachment 8838275 [details] [diff] [review]
1338284.patch

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 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-
(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 months ago
Thanks ! I will do the fixes and add a new patch :)
(Assignee)

Comment 7

7 months ago
Created attachment 8838847 [details] [diff] [review]
1338284-2.patch

- 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 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+
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)
Try looks good (with the current patch)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e084a2bdd2115924344761a9485b3e01b20e648

Honza
(Assignee)

Comment 11

7 months ago
Created attachment 8839241 [details] [diff] [review]
1338284-3.patch

- 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 months 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 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+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(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 months 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 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ddc511090d07
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54

Comment 17

4 months 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

3 months 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

3 months ago
As per Comment 17 and Comment 18, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
You need to log in before you can comment on or make changes to this bug.