report: use KiB instead of KB #10870

connorjclark · 2020-05-29T06:14:09Z

Chrome DevTools is changing its usage of KB (JEDEC, 1 KB = 1024 bytes) to the unambiguous KiB (IEC, 1 KiB = 1024 bytes). We want Lighthouse to be in alignment.

Some web.dev docs that need to update too:

and https://web.dev/performance-budgets-101/ (cc @khempenius)

We don't use displayUnit: 'kb' for anything. Do we want to just update all instances of it to kib? I imagine this would break 0 users.

I added a title to the byte divs ... yay or nay?

patrickhulce

maybe this patch too? displayUnit: 'kb' is the only other thing I think we might want to make a note of to change soon so that if we eventually introduce kB it's not so confusing.

diff --git a/lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js b/lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js
index 1add4d238..7f8297be4 100644
--- a/lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js
+++ b/lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js
@@ -26,7 +26,7 @@ const MAX_RESOURCE_SIZE_TO_ENCODE = 2000 * 1024;
 const JPEG_QUALITY = 0.92;
 const WEBP_QUALITY = 0.85;
 
-const MINIMUM_IMAGE_SIZE = 4096; // savings of <4 KB will be ignored in the audit anyway
+const MINIMUM_IMAGE_SIZE = 4096; // savings of <4 KiB will be ignored in the audit anyway
 
 const IMAGE_REGEX = /^image\/((x|ms|x-ms)-)?(png|bmp|jpeg)$/;
 
diff --git a/lighthouse-core/lib/i18n/README.md b/lighthouse-core/lib/i18n/README.md
index 8ec3779d9..58e8de4b7 100644
--- a/lighthouse-core/lib/i18n/README.md
+++ b/lighthouse-core/lib/i18n/README.md
@@ -268,12 +268,12 @@ CTC is a name that is distinct and identifies this as the Chrome translation for
     This is also the point at which ICU is replaced by values. So this...
 
     ```javascript
-    message = "Total size was {totalBytes, number, bytes} KB"
+    message = "Total size was {totalBytes, number, bytes} KiB"
     sent_values = {totalBytes: 10240}
     ```
 
     Becomes...
 
     ```javascript
-    message = "Total size was 10 KB"
+    message = "Total size was 10 KiB"
     ```
diff --git a/lighthouse-core/report/html/renderer/details-renderer.js b/lighthouse-core/report/html/renderer/details-renderer.js
index 958a5db7d..bd162c87a 100644
--- a/lighthouse-core/report/html/renderer/details-renderer.js
+++ b/lighthouse-core/report/html/renderer/details-renderer.js
@@ -75,6 +75,7 @@ class DetailsRenderer {
    */
   _renderBytes(details) {
     // TODO: handle displayUnit once we have something other than 'kb'
+    // Note that 'kb' is historical and actually represents KiB.
     const value = Util.i18n.formatBytesToKiB(details.value, details.granularity);
     const text = this._renderText(value);
     text.setAttribute('title', Util.i18n.formatNumber(details.value));

docs/performance-budgets.md

khempenius · 2020-05-29T16:26:31Z

<3 this change. Doc updates: GoogleChrome/web.dev#3070

brendankenny

LGTM2

brendankenny · 2020-05-29T16:23:07Z

lighthouse-core/audits/byte-efficiency/total-byte-weight.js

-  /** Used to summarize the total byte size of the page and all its network requests. The `{totalBytes}` placeholder will be replaced with the total byte sizes, shown in kilobytes (e.g. 142 KB) */
-  displayValue: 'Total size was {totalBytes, number, bytes}\xa0KB',
+  /** Used to summarize the total byte size of the page and all its network requests. The `{totalBytes}` placeholder will be replaced with the total byte sizes, shown in kibibytes (e.g. 142 KiB) */
+  displayValue: 'Total size was {totalBytes, number, bytes}\xa0KiB',


I was going to say we should really transition these to Intl.NumberFormat with its new support for localization of numbers+unit, but it only supports kilobyte, not kibibyte :(

brendankenny · 2020-05-29T16:39:52Z

lighthouse-core/report/html/renderer/details-renderer.js

-    return this._renderText(value);
+    const value = Util.i18n.formatBytesToKiB(details.value, details.granularity);
+    const text = this._renderText(value);
+    text.setAttribute('title', Util.i18n.formatNumber(details.value));


wait, why do this? You mentioned doing this in the PR description but didn't say why :) I don't have any objection just wondering why it's beneficial.

I appreciate when tools do this with labels I'm unfamiliar with. If I'm unsure if KiB means 1000 or 1024, a quick hover to see the real value relative to the elided value tells me right away.

also it's nice to just be able to see the real size

also it's nice to just be able to see the real size

note it's not always a real size :) (estimated opportunities and sometimes already rounded to various degrees).

Should it += ' bytes' then?

alrighty I made formatBytes. @exterkamp says this should be i18n'd but the other value rendering functions here don't do that yet so I'm gonna defer it here too.

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

exterkamp

LGTM 4 i18n

exterkamp · 2020-05-29T19:14:55Z

lighthouse-core/report/html/renderer/i18n.js

    const kbs = this._numberFormatter.format(Math.round(size / 1024 / granularity) * granularity);
-    return `${kbs}${NBSP2}KB`;
+    return `${kbs}${NBSP2}KiB`;


@connorjclark and I talked about this.

This needs to be i18n'd in the future. Half our "KiB" are translated, and half aren't. We should be consistent and allow these to be translated as well.

This problem exists today, so not blocking for this PR, but it's a TODO.

brendankenny · 2020-05-29T19:22:12Z

lighthouse-core/report/html/renderer/details-renderer.js

+    // Note that 'kb' is historical and actually represents KiB.
+    const value = Util.i18n.formatBytesToKiB(details.value, details.granularity);
+    const text = this._renderText(value);
+    text.setAttribute('title', Util.i18n.formatBytes(details.value));


nit:

Suggested change

text.setAttribute('title', Util.i18n.formatBytes(details.value));

textEl.title = Util.i18n.formatBytes(details.value);

is slightly better here

brendankenny · 2020-05-29T19:22:24Z

lighthouse-core/report/html/renderer/details-renderer.js

-    return this._renderText(value);
+    // Note that 'kb' is historical and actually represents KiB.
+    const value = Util.i18n.formatBytesToKiB(details.value, details.granularity);
+    const text = this._renderText(value);


report: use KiB instead of KB

4192060

connorjclark requested a review from a team as a code owner May 29, 2020 06:14

connorjclark requested review from brendankenny and removed request for a team May 29, 2020 06:14

googlebot added the cla: yes label May 29, 2020

devtools-bot assigned brendankenny May 29, 2020

devtools-bot added the waiting4reviewer label May 29, 2020

patrickhulce approved these changes May 29, 2020

View reviewed changes

docs/performance-budgets.md Outdated Show resolved Hide resolved

khempenius mentioned this pull request May 29, 2020

Lighthouse docs: switch from kilobytes to kibibytes GoogleChrome/web.dev#3070

Merged

brendankenny approved these changes May 29, 2020

View reviewed changes

Update docs/performance-budgets.md

52737b2

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview May 29, 2020 18:51 View deployment

connorjclark added 2 commits May 29, 2020 12:01

patrick

3afe63a

Merge branch 'kib' of github.com:GoogleChrome/lighthouse into kib

fe6b0ed

vercel bot deployed to Preview May 29, 2020 19:02 View deployment

bytes

d72f58e

vercel bot deployed to Preview May 29, 2020 19:11 View deployment

exterkamp approved these changes May 29, 2020

View reviewed changes

brendankenny reviewed May 29, 2020

View reviewed changes

title

45cead6

vercel bot deployed to Preview May 29, 2020 19:25 View deployment

connorjclark merged commit 41175fe into master Jun 4, 2020

connorjclark deleted the kib branch June 4, 2020 02:30

hsablonniere mentioned this pull request Nov 1, 2020

Article: What's a kilobyte anyway hsablonniere/hsablonniere.com#21

Open

connorjclark mentioned this pull request Apr 6, 2022

i18n: localize units in report formatter #13830

Merged

vio mentioned this pull request Sep 25, 2022

Use Mebibyte for file size display value relative-ci/bundle-stats#2714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: use KiB instead of KB #10870

report: use KiB instead of KB #10870

connorjclark commented May 29, 2020 •

edited

Loading

patrickhulce left a comment

khempenius commented May 29, 2020

brendankenny left a comment

brendankenny May 29, 2020

brendankenny May 29, 2020

connorjclark May 29, 2020

brendankenny May 29, 2020

connorjclark May 29, 2020

exterkamp left a comment

exterkamp May 29, 2020

brendankenny May 29, 2020

brendankenny May 29, 2020

	text.setAttribute('title', Util.i18n.formatBytes(details.value));
	textEl.title = Util.i18n.formatBytes(details.value);

report: use KiB instead of KB #10870

report: use KiB instead of KB #10870

Conversation

connorjclark commented May 29, 2020 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

khempenius commented May 29, 2020

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented May 29, 2020 •

edited

Loading