Closed Bug 734862 Opened 9 years ago Closed 8 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: marcoos, Assigned: sachin)

Details

Attachments

(2 files, 4 obsolete files)

Attached image 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".
Severity: normal → minor
Component: Untriaged → Add-ons Manager
Product: Firefox → Toolkit
QA Contact: untriaged → add-ons.manager
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?
Assignee: nobody → sachinhosmani2
Status: NEW → ASSIGNED
Attachment #739475 - Flags: review?(gavin.sharp)
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.
Attachment #739475 - Flags: review?(gavin.sharp) → review-
Attached patch Solves the L10n problem. (obsolete) — Splinter Review
Attachment #739475 - Attachment is obsolete: true
Attachment #740694 - Flags: review?(mak77)
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.
Attachment #740694 - Flags: review?(mak77) → feedback+
Attachment #740694 - Attachment is obsolete: true
Attachment #747905 - Flags: review?(mak77)
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
Attachment #747905 - Flags: review?(mak77) → review+
Attached patch Indentation fixed (obsolete) — Splinter Review
Attachment #747905 - Attachment is obsolete: true
Attachment #752721 - Flags: review?(mak77)
Flags: needinfo?(mak77)
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 :)
Attachment #752721 - Flags: review?(mak77) → review+
Flags: needinfo?(mak77)
Oh, okay. I thought it was habitual to ask for a review. :)
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.
Flags: needinfo?(mak77)
Summary: "Downloading add-on" bubble shows "Infinite GB/s" as download speed → Replace "Infinite GB/s" with a more readable localized string
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!
Flags: needinfo?(mak77)
Attachment #752721 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fc2fb2d87d31
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.