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)

defect

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)

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
Thanks :) I will try to attach a patch in the next few days.
Attached patch 1338284.patch (obsolete) — Splinter Review
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
Thanks ! I will do the fixes and add a new patch :)
Attached patch 1338284-2.patch (obsolete) — Splinter Review
- 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)
Attached patch 1338284-3.patchSplinter Review
- Change name of REQUEST_TIME_DECIMALS to REQUEST_DECIMALS
- Remove const NO_DECIMALS
Attachment #8838847 - Attachment is obsolete: true
Attachment #8839241 - Flags: review?(odvarko)
(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
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
https://hg.mozilla.org/mozilla-central/rev/ddc511090d07
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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]
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]
As per Comment 17 and Comment 18, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.