734862 - Replace "Infinite GB/s" with a more readable localized string

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Marek Stępień [:marcoos, inactive]]

Attachment: Screenshot

Downloading an add-on on a sufficiently fast connections may report the download speed as "Infinity GB/s".

That's physically impossible, so maybe it should be tweaked so that if the calculated download speed is a JavaScript Infinity value, it should be reported as "really fast" or something, not "Infinity GB/s".

Comment 1

[Sachin Hosmani [:sachin]]

Attachment: Uses the phrase "Really fast" instead of Infinity GB/s

Should "really fast" be changed to something else?

Doesn't this bug affect everything that uses downloadUtils and not just the "downloading add-on" notification?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 739475 [details] [diff] [review]
Uses the phrase "Really fast" instead of Infinity GB/s

>diff --git a/toolkit/mozapps/downloads/DownloadUtils.jsm b/toolkit/mozapps/downloads/DownloadUtils.jsm

> let gStr = {

>+  infiniteSpeed: "Really fast",

This is a hard-coded en-US string in the code, which isn't acceptable. This needs to continue to point to a string name which is also added to the downloads.properties file.

In general, the approach of using a separate string for this case seems fine, though. We'll need to decide what that string should be exactly. For future revisions of the patch, please request review from :mak or :paolo (you can enter those names in the request field), and they can help you on deciding what the string should be.

Comment 3

[Sachin Hosmani [:sachin]]

Attachment: Solves the L10n problem.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 740694 [details] [diff] [review]
Solves the L10n problem.

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

Hello, I like your idea.
I'm not english mother tongue, but "Really fast" makes sense to me, it's not complicate to translate too. I did a short search whether we should prefer "very" to "really" but didn't find anything interesting, so I'm assuming your choice is good.

Would be nice if you could add a test to test_DownloadUtils.js

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties
@@ +43,5 @@
>  dontLeavePrivateBrowsingButton=Stay in Private Browsing Mode
>  downloadsCompleteTitle=Downloads Complete
>  downloadsCompleteMsg=All files have finished downloading. 
>  
> +infiniteSpeed=Really fast

please name this infiniteRate (for coherence with other strings) and add a LOCALIZATION NOTE for it, specifying when it's used

@@ +53,5 @@
>  
> +# LOCALIZATION NOTE (statusFormatInfiniteSpeed): — is the "em dash" (long dash)
> +# %1$S transfer progress; %2$S substitute phrase for Infinity speed; %3$S time left
> +# example: 4 minutes left — 1.1 of 11.1 GB (Really fast)
> +statusFormatInfiniteSpeed=%3$S — %1$S (%2$S)

statusFormatInfiniteRate

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +111,5 @@
>  
>      let [rate, unit] = DownloadUtils.convertByteUnits(normalizedSpeed);
> +
> +    let params;
> +    let status;

you don't need params out of the if/else scopes, so please just define it on first use in each scope.

@@ +112,5 @@
>      let [rate, unit] = DownloadUtils.convertByteUnits(normalizedSpeed);
> +
> +    let params;
> +    let status;
> +    if(rate === "Infinity") {

add space after if

@@ +113,5 @@
> +
> +    let params;
> +    let status;
> +    if(rate === "Infinity") {
> +      // Don't show 'Infinity' as speed

Better comment like
// Infinity speed doesn't make sense, rather use a localized description.

Comment 5

[Sachin Hosmani [:sachin]]

Attachment: Addresses the comments and includes a test case.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 747905 [details] [diff] [review]
Addresses the comments and includes a test case.

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

Looks good, please have a Try run so we can ensure there is no unexpected test failing, if you don't have commit access to the try server just put a needinfo on me and I will push it for you.

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +115,5 @@
> +    if (rate === "Infinity") {
> +      // Infinity download speed doesn't make sense. Show a localized phrase instead.
> +      let params = [transfer, gBundle.GetStringFromName(gStr.infiniteRate), timeLeft];
> +      status = gBundle.formatStringFromName(gStr.statusFormatInfiniteRate, params,
> +                                                params.length);

please fix indentation of the params

@@ +120,5 @@
> +    }
> +    else {
> +      let params = [transfer, rate, unit, timeLeft];
> +      status = gBundle.formatStringFromName(gStr.statusFormat, params,
> +                                                params.length);

same here

Comment 7

[Sachin Hosmani [:sachin]]

Attachment: Indentation fixed

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 752721 [details] [diff] [review]
Indentation fixed

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

usually you don't need a further review for such changes :)

Comment 9

[Sachin Hosmani [:sachin]]

Oh, okay. I thought it was habitual to ask for a review. :)

Comment 10

[Marco Bonardo [:mak]]

pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=a333ad627dd3
Since I noticed after pushing, please fix the checkin comment to report the bug number, a description of the change, and the reviewer.

Once try tests are all green, you can set checkin-needed keyword for the patch to be pushed to trunk. I will check back results later too.

Comment 11

[Marco Bonardo [:mak]]

Try results are fine, only some unrelated random failures, so could you please fix the checkin message, then set checkin-needed keyword and we should be done.
Thank you for your contribution!

Comment 12

[Sachin Hosmani [:sachin]]

Attachment: Check-in message fixed.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2fb2d87d31

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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