237693 - Download Manager should understand when it breaks the KB, MB, GB barriers

Status: VERIFIED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[Robert Accettura [:raccettura]]

Every so often, if your on a LAN, or if you have a beefy network connection, you
can break the MB barrier (downloading over 1024KB/sec).  Download manager should
then show the rate in terms of MB, rather than KB.

This is not to be confused with Mbits/sec, which is different.

I have an untested patch to implement this.  From what I can tell, it does work,
but haven't verified it, because my 802.11b network won't quite allow for it. 
Next time I have the bandwidth I'llt ry to verify the functionality, but if
somone else wants to test for me... I wouldn't mind.

Comment 1

[Robert Accettura [:raccettura]]

Attachment: Patch v1

Untested Patch.

It shouldn't break anything (I can verify), but I don't know for certain it
fixes the enhancement request of the patch... but I think it does ;-)

Comment 2

[Robert Accettura [:raccettura]]

Comment on attachment 144065 [details] [diff] [review]
Patch v1

Bad math on this patch.  I'll take a better stab at this later.

It's minor enhancement, so no rush.

Comment 3

[Robert Accettura [:raccettura]]

feel free to take this if your working on download manager.  Considering speed improvements over the years, I now see > 1MB/sec downloads.  Would be a nice bit of polish.

Comment 4

[Shawn Wilsher :sdwilsh]

The last patch actually looks pretty good.  Any reason why you obsoleted it?

Comment 5

[Robert Accettura [:raccettura]]

Shawn: to be totally honest, I don't even remember... it's been a while.

Feel free to do what you want with it.

Comment 6

[Ed Lee :Mardak]

beltzner: Should the UI switch to MBps at 1024 or 1000?

For KBps 999, 1000, 1023, 1024
1024: 999KB/s, 1000KB/s, 1023KB/s, 1.0MB/s
1000: 999KB/s, 1.0MB/s, 1.0MB/s, 1.0MB/s

(The last 3 for 1000 are if we round up 1000/1024 = 0.97 to 1.0. 1024*.9 ~= 921 => smallest KBps to even show 0.9MB/s)

Robert: I can take the bug/update the patch if you want.

Comment 7

[Ed Lee :Mardak]

Attachment: v2

Use MB/s to avoid 4 digit KB/s.

Renamed |kRate| to various names because its name makes it looks like a constant, but it's reused in all sorts of ways.

Btw Robert: missed a tiny line updating "#3" (units) in the else case

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 268862 [details] [diff] [review]
v2

>+      var tenRate = Math.round(rate * 10);
nit: I think this variable name could be better.  How about something like roundedRate

Comment 9

[Ed Lee :Mardak]

Attachment: v3

Refactor the code to automatically pick from bytes, KB, MB, or GB. Adding another unit would just need a new string and array element in _units.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 268867 [details] [diff] [review]
v3

Even better :)

Comment 11

[Robert Accettura [:raccettura]]

>+      while ((rate >= 1000) && (unitIndex < this._units.length - 1)) {

This is completely unscientific since I'm just glancing at this... I wonder if this will have an impact on cpu consumption for downloads.

Comment 12

[Ryan Jones-Ward [:sciguyryan]]

Maybe you could modify the _formatKBytes function to cover both download speeds and file sizes at the same time? That would probably make the code a little smaller if there were just a single function too format the download speed / size.

Comment 13

[Ed Lee :Mardak]

Comment on attachment 268867 [details] [diff] [review]
v3

I'll post a new patch that displays "# of # <byte/KB/MB/GB>" type stuff.

Comment 14

[Ed Lee :Mardak]

Attachment: 113 MB of 1.0 GB at 3.3 MB/sec

Comment 15

[Ed Lee :Mardak]

Attachment: v4 (progress and rate)

Updated patch to figure out which units to use for not only rate but also progress.

(In reply to comment #11)
> >+      while ((rate >= 1000) && (unitIndex < this._units.length - 1)) {
> 
> I wonder if this will have an impact on cpu consumption for downloads.

Note that download updates are once every 500ms, and the number of units isn't many - right now 4. Even if there was a "smarter" technique that tracks the units it used last time, the additional logic and code size probably would probably outweigh the benefits.

For example, if we start with "bytes" and need to convert to "MB", even with a shortcut knowing that we should probably use MB (while reserving compensation code if it happens not to be MB), the bytes need to be converted to MB by doing the division.

Comment 16

[Ed Lee :Mardak]

Attachment: v4.1 alternate string processing

Almost same as v4 except I changed the numbers on the string replacement so there's less duplicate code. (see interdiff)

Comment 17

[Shawn Wilsher :sdwilsh]

How does _replaceInsert handle the case where you are trying to replace something that doesn't exist?

Comment 18

[Ed Lee :Mardak]

(In reply to comment #17)
> How does _replaceInsert handle the case where you are trying to replace
> something that doesn't exist?

_replaceInsert basically does string.replace(/#4/, 'thing for #4')

(I copied the idea from the time display; it drops off the 0 hours by not having #1)

longTimeFormat=#1:#2:#3
shortTimeFormat=#2:#3

Comment 19

[Shawn Wilsher :sdwilsh]

Hmm, I'm wondering if it would make more sense to use this performance wise:
http://www.xulplanet.com/references/xpcomref/ifaces/nsIStringBundle.html#method_formatStringFromName

Comment 20

[Ed Lee :Mardak]

Or maybe something similar to..

http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/content/downloads.js#462

title = strings.getFormattedString("downloadsTitleMultiple", [mean, numActiveDownloads]);

..where the string is..

downloadsTitleMultiple=%S%% of %S files - Downloads

But that would mean the statusFormat strings would become something like..

statusFormatSameUnits=%S of %S %S
statusFormatDiffUnits=%S %S of %S %S
statusFormatNoTotal=%S %S

..which runs into similar localization issues if we didn't have those strings and just concatenated the individual components together.


But I suppose taking a step back, are we really worried about performance in this part of the application? For comparison, I believe Safari updates download status once every second and doesn't bother displaying seconds for "time left" unless it's less than a minute to go.

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 269344 [details] [diff] [review]
v4.1 alternate string processing

I like this version better.

nit: you've got a few lines over 80 chars that you should probably wrap.

Comment 22

[Ed Lee :Mardak]

Attachment: v5

fix nits and cleanup
- Rename statusFormat* to transfer*
- Check for total <= 0 not just non-zero (total can be negative)
- Remove repeated lines of replaceInsert and consolidate
- Rename convertByte's parameter to |aByte| (other methods to be fixed in bug 385734)
- Explicitly use 1 and 0 instead of using true/false.

Comment 23

[Shawn Wilsher :sdwilsh]

Comment on attachment 269654 [details] [diff] [review]
v5

>-    var status = this._statusFormat;
>+    let status = this._statusFormat;
What is the advantage of using let here (and in other places)?

>+        rate = "??.?";
Hmm, I think this might be best to have localized.

>+  // converts an number of bytes to the appropriate unit that results in a
nit: a number

Comment 24

[Ed Lee :Mardak]

(In reply to comment #23)
> (From update of attachment 269654 [details] [diff] [review])
> >-    var status = this._statusFormat;
> >+    let status = this._statusFormat;
> What is the advantage of using let here (and in other places)?
Just being consistent and using "let"s instead of "var"s as it's more consistent with what C++ does when declaring variables. "let" has block scope if you use it |let () { block }| or |let () singline|, or in this case.. |{ let; block }|.

"var" will spill out of block scope, e.g.:

if (true) { let x = 5; } typeof x == 'undefined';
if (true) { var y = 5; } typeof y == 'number';

> >+        rate = "??.?";
> Hmm, I think this might be best to have localized.
The individual "?" or the whole "??.?"? We don't localize the number we display for progress/rate which we choose to be "#.#" or "##.#" or "###". I suppose we might need to do "#1#2#3" and define what the decimal point is being "." or "," or others in various localizations... But that seems kinda wasteful to generate a number string one number at a time.

Perhaps a new bug for
decimalPoint=.
unknownNumber=?

rate = rate.replace('.', decimalPoint)
and
rate = unknownNumber + unknownNumber + decimalPoint + unknownNumber

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> "var" will spill out of block scope, e.g.:
> 
> if (true) { let x = 5; } typeof x == 'undefined';
> if (true) { var y = 5; } typeof y == 'number';
Wow, I didn't realize that was the case.

> > >+        rate = "??.?";
> > Hmm, I think this might be best to have localized.
> The individual "?" or the whole "??.?"?
no no, just a string for unknown rate.

Comment 26

[Ed Lee :Mardak]

Attachment: v6

(In reply to comment #23)
> >+        rate = "??.?";
> Hmm, I think this might be best to have localized.
Or completely remove it! ;) New version doesn't check if aDownload.speed is 0 because it's not going to be 0 unless just starting - in that case display "0 bytes/sec" (no decimal points)

> >+  // converts an number of bytes to the appropriate unit that results in a
> nit: a number
fixed

Additionally, changed "unknown file size" to "unknown time left" because the message is used if aDownload.speed is 0 or if the file size isn't known. But then again, the common case is to have an unknown file size if it does show the message..

Comment 27

[Shawn Wilsher :sdwilsh]

Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties;
new revision: 1.4; previous revision: 1.3
Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
new revision: 1.13; previous revision: 1.12

Comment 28

[Ed Lee :Mardak]

Attachment: v6 as in cvs

There was a slight problem with the actual checked in patch, but this has been fixed with the checkin for bug 385733. Nothing extra needs to be done.

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9a9pre) Gecko/2007110212 Minefield/3.0a9pre and Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007110212 Minefield/3.0a9pre

Comment 30

[Stephen Donner [:stephend] Not actively reading bugmail]

in-litmus+

https://litmus.mozilla.org/show_test.cgi?id=4993