Closed Bug 1317646 Opened 7 years ago Closed 7 years ago

Netmonitor: move code for formatting request timings and content sizes to format-utils.js

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Iteration:
53.5 - Jan 23
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: jsnajdr, Assigned: steveck)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

Bug 1317646 - Netmonitor: move code for formatting request timings and content sizes to format-utils.js,
59 bytes, text/x-review-board-request
jsnajdr
: review+
Details 
Current netmonitor code duplicates constants REQUEST_TIME_DECIMALS and CONTENT_SIZE_DECIMALS at several places, and the string formatting code that uses them is also duplicated at many places.

Extract them into functions exported from format-utils.js.
Blocks: netmonitor-html
Whiteboard: [netmonitor][triage]
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
Priority: -- → P2
Whiteboard: [netmonitor][triage] → [netmonitor]
Assignee: nobody → schung
Status: NEW → ASSIGNED
Comment on attachment 8825703 [details]
Bug 1317646 - Netmonitor: move code for formatting request timings and content sizes to format-utils.js,

https://reviewboard.mozilla.org/r/103812/#review104534

Very nice patch, thanks for working on this! There are a few issues that should be easy to fix.

Note that Ricky is doing some changes in bug 1317648 (Headers panel) that could cause conflicts: formatting the headers size, HEADERS_SIZE_DECIMALS constant.

I think that having separate constants for CONTENT_SIZE_DECIMALS (2) and HEADERS_SIZE_DECIMALS (3) doesn't make much sense - we can unify on one with value "2", can't we?

Please sumbit a try run if you didn't already.

::: devtools/client/netmonitor/components/request-list-header.js:134
(Diff revision 3)
>      if (normalizedTime > 60000) {
>        normalizedTime /= 60000;
>        divisionScale = "minute";
>      } else if (normalizedTime > 1000) {
>        // If the division is greater than 1 second.
> -      normalizedTime /= 1000;
>        divisionScale = "second";
>      }

The normalizedTime variable is not needed anymore, and doesn't need to be divided by 60000. Just use the millisecondTime value to determine the divisionScale.

::: devtools/client/netmonitor/components/summary-button.js:7
(Diff revision 3)
>  const {
> -  CONTENT_SIZE_DECIMALS,
> -  REQUEST_TIME_DECIMALS,
> -} = require("../constants");
> +  getSizeWithDecimals,
> +  getTimeWithDecimals,
> +} = require("../utils/format-utils");

nit: move this import after the other relative imports (from ../something). The convention is to first import the "shared" modules (like react etc) and then "local" modules, mostly with relative path.

::: devtools/client/netmonitor/statistics-view.js:119
(Diff revision 3)
> -    size: value => {
> -      let string = L10N.numberWithDecimals(value / 1024, CONTENT_SIZE_DECIMALS);
> +    size: value => getFormattedSize(value),
> +    time: value => getFormattedTime(value)

Use getSizeWithDecimals/getTimeWithDecimals here, like you do for commonChartTotals.

All these values are displayed under each other as table columns, so it's better if they have the same units.

If values in the same table column have different units (B, kB, s, ms), it's harder to understand the information.

::: devtools/client/netmonitor/utils/format-utils.js:9
(Diff revision 3)
>  // Constants for formatting bytes.
>  const BYTES_IN_KB = 1024;
>  const BYTES_IN_MB = Math.pow(BYTES_IN_KB, 2);
>  const BYTES_IN_GB = Math.pow(BYTES_IN_KB, 3);
>  const MAX_BYTES_SIZE = 1000;
>  const MAX_KB_SIZE = 1000 * BYTES_IN_KB;
>  const MAX_MB_SIZE = 1000 * BYTES_IN_MB;

This looks wrong: the contants for comparing (bytes < MAX_BYTES_SIZE) are powers of 1000, but the constants for dividing (kb = bytes / BYTES_IN_KB) are powers of 1024?

They should all be powers of 1024. I.e., remove the MAX_*_SIZE and use BYTES_IN_* for everything.

Otherwise, weird things happen, like 1000 bytes being formatted as 0.98 KB.

Consult the "bytes" package on NPM (https://www.npmjs.com/package/bytes). It formats 1000 bytes as "1000B".

I know that the bug is not introduced by this patch, but it's a great opportunity to fix it here.

::: devtools/client/netmonitor/utils/format-utils.js:58
(Diff revision 3)
> + * Get a human-readable string from a number of time, with the ms, s, or min
> + * value.
> + */
> +function getFormattedTime(ms) {
> +  if (ms < MAX_MILLISECOND) {
> +    return L10N.getFormatStr("networkMenu.millisecond", ms);

nit: even the millisecond time can have decimals. For example, run "performance.now()" in console. It will return a DOMHighResTimeStamp, which is a number in millisecond units with microsecond precision.

Better to round it here to integer, with: ms | 0
Attachment #8825703 - Flags: review?(jsnajdr)
Iteration: --- → 53.5 - Jan 23
Priority: P2 → P1
Comment on attachment 8825703 [details]
Bug 1317646 - Netmonitor: move code for formatting request timings and content sizes to format-utils.js,

https://reviewboard.mozilla.org/r/103812/#review104836

::: devtools/client/netmonitor/utils/format-utils.js:24
(Diff revision 3)
> +const MAX_SECOND = 60 * MAX_MILLISECOND;
> +
>  const CONTENT_SIZE_DECIMALS = 2;
> +const REQUEST_TIME_DECIMALS = 2;
> +
> +function getSizeWithDecimals(size) {

There is a HEADERS_SIZE_DECIMALS defined in details-view.js

http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/details-view.js#367

So this method should be renamed to getContentSizeWithDecimals() to avoid naming conflicts with headers size case.

Don't worry about getHeadersSizeWithDecimals(), I'm going to move getHeadersSizeWithDecimals() to format-utils.js in the patch of bug 1317648.
Comment on attachment 8825703 [details]
Bug 1317646 - Netmonitor: move code for formatting request timings and content sizes to format-utils.js,

https://reviewboard.mozilla.org/r/103812/#review104534

> This looks wrong: the contants for comparing (bytes < MAX_BYTES_SIZE) are powers of 1000, but the constants for dividing (kb = bytes / BYTES_IN_KB) are powers of 1024?
> 
> They should all be powers of 1024. I.e., remove the MAX_*_SIZE and use BYTES_IN_* for everything.
> 
> Otherwise, weird things happen, like 1000 bytes being formatted as 0.98 KB.
> 
> Consult the "bytes" package on NPM (https://www.npmjs.com/package/bytes). It formats 1000 bytes as "1000B".
> 
> I know that the bug is not introduced by this patch, but it's a great opportunity to fix it here.

I think the comment for getFormattedSize explain why they had MAX_*_SIZE here. It's just to avoid some display number over 1000 like "1016 KB" . The size computation is still using BYTES_IN_xx, so I guess it's fine.
Comment on attachment 8825703 [details]
Bug 1317646 - Netmonitor: move code for formatting request timings and content sizes to format-utils.js,

https://reviewboard.mozilla.org/r/103812/#review105602

Looks great, thanks for working on this!
Attachment #8825703 - Flags: review?(jsnajdr) → review+
Thanks for the review!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/725d82b3c790
Netmonitor: move code for formatting request timings and content sizes to format-utils.js, r=jsnajdr
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/725d82b3c790
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.