Closed Bug 237693 Opened 16 years ago Closed 13 years ago

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

Categories

(Toolkit :: Downloads API, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9alpha6

People

(Reporter: raccettura, Assigned: Mardak)

References

Details

Attachments

(2 files, 7 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
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 ;-)
Status: NEW → ASSIGNED
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.
Attachment #144065 - Attachment is obsolete: true
QA Contact: ali → download.manager
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.
The last patch actually looks pretty good.  Any reason why you obsoleted it?
Shawn: to be totally honest, I don't even remember... it's been a while.

Feel free to do what you want with it.
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.
Attached patch v2 (obsolete) — Splinter Review
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
Assignee: robert → edilee
Attachment #268862 - Flags: review?
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
Attachment #268862 - Flags: review? → review+
Summary: Download Manager Should Understand when it breaks the MB barrier → Download Manager Should Understand when it breaks the KB,MB,GB barrier
Attached patch v3 (obsolete) — Splinter Review
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.
Attachment #268862 - Attachment is obsolete: true
Attachment #268867 - Flags: review?(sdwilsh)
Comment on attachment 268867 [details] [diff] [review]
v3

Even better :)
Attachment #268867 - Flags: review?(sdwilsh)
Attachment #268867 - Flags: review?(enndeakin)
Attachment #268867 - Flags: review+
>+      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.
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 on attachment 268867 [details] [diff] [review]
v3

I'll post a new patch that displays "# of # <byte/KB/MB/GB>" type stuff.
Attachment #268867 - Attachment is obsolete: true
Attachment #268867 - Flags: review?(enndeakin)
Attached patch v4 (progress and rate) (obsolete) — Splinter Review
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.
Attachment #269325 - Flags: review?(sdwilsh)
Attachment #269325 - Flags: review?(enndeakin)
Attached patch v4.1 alternate string processing (obsolete) — Splinter Review
Almost same as v4 except I changed the numbers on the string replacement so there's less duplicate code. (see interdiff)
How does _replaceInsert handle the case where you are trying to replace something that doesn't exist?
(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
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
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 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.
Attachment #269344 - Flags: review+
Attachment #269325 - Flags: review?(sdwilsh)
Blocks: 385733
Blocks: 385734
Attached patch v5 (obsolete) — Splinter Review
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.
Attachment #269325 - Attachment is obsolete: true
Attachment #269344 - Attachment is obsolete: true
Attachment #269654 - Flags: review?(enndeakin)
Attachment #269325 - Flags: review?(enndeakin)
Attachment #269654 - Flags: review?(enndeakin) → review?(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
(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
(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.
Attached patch v6 (obsolete) — Splinter Review
(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..
Attachment #269654 - Attachment is obsolete: true
Attachment #270051 - Flags: review?(sdwilsh)
Attachment #269654 - Flags: review?(sdwilsh)
Attachment #270051 - Flags: review?(sdwilsh) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 alpha6
Version: unspecified → Trunk
Attached patch v6 as in cvsSplinter Review
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.
Attachment #270051 - Attachment is obsolete: true
Flags: in-litmus?
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
Status: RESOLVED → VERIFIED
Summary: Download Manager Should Understand when it breaks the KB,MB,GB barrier → Download Manager should understand when it breaks the KB, MB, GB barriers
in-litmus+

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

Flags: in-litmus? → in-litmus+
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.