1338284 - Make KB unit on summary and chart view dynamic

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Leonardo Couto]

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

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report Leonardo.

I just assigned the bug to you. Let me know if you have any questions.

Honza

Comment 2

[Leonardo Couto]

Thanks :) I will try to attach a patch in the next few days.

Comment 3

[Leonardo Couto]

Attachment: 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.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 6

[Leonardo Couto]

Thanks ! I will do the fixes and add a new patch :)

Comment 7

[Leonardo Couto]

Attachment: 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 :)

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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.

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try looks good (with the current patch)

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

Honza

Comment 11

[Leonardo Couto]

Attachment: 1338284-3.patch

- Change name of REQUEST_TIME_DECIMALS to REQUEST_DECIMALS
- Remove const NO_DECIMALS

Comment 12

[Leonardo Couto]

(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 !

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8839241 [details] [diff] [review]
1338284-3.patch

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

LGTM Thanks!

Honza

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddc511090d07
Make KB unit on summary and chart view dynamic. r=Honza

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ddc511090d07

Comment 17

[Md. Majedul islam]

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

[Md. Rahimul Islam]

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

Comment 19

[Md. Majedul islam]

As per Comment 17 and Comment 18, I am marking this bug as verified fixed.