245725 - Download manager does not calculate speed accurately/correctly

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Gilad Avidan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040530 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040530 Firefox/0.8.0+

I believe, although I am not sure of this, that the download manager calculates
the download speed simply by dividing the sum amount of data by the time
required to download it. 

This approach is much too simple and must be changed for two reasons:
1. *Very* inaccurate. Imagine calculating the current speed of your car by
dividing the distance you've gone since you started driving, by the time of the
drive. Yes, it would calculate the *average* speed, but not the current speed.
If you go at 10MPH for an hour, and then suddenly kick down to 90MPH, your
current speed by the calculation above will be much closer to 10MPH than to 90. 
You can see that for downloads with a fluctuating speed this can be problematic.

2. The other, worse problem (which maybe should be a different bug, if so tell
me), is that if you click on a link and spend a lot of time deciding where to
save it, FF starts downloading it immediately (e.g. counting the distance
driven) but fails to start the timer. What this means is that if it gets through
10MB of a file when you decide where to save, then wait 2 seconds and look at
the download manager, the manager thinks it was only downloading for 2 seconds,
thus calculating a speed of 5120KBps. You may have been downloading at 1.5KBps
for all it cares. The speed goes down, of course, but still this is very stupid
and can be avoided easily.

Suggestion for methods of calculation:
1. Just give the user the raw data. Show him what you downloaded in the current
second. If you downloaded 55KB, show a speed of 55KBps.

2. Use the method mentioned above, but for 5 or 10 seconds. E.g. if you
downloaded 400KBs in 10 seconds, show a speed of 40KBPs.

Enjoy

Reproducible: Always
Steps to Reproduce:
1. Click on a link for a large file
2. When download dialog opens wait for a while (10-20 secs)
3. Decide where to save the file
4. Look at the speed in the download manager.

Actual Results:  
Speed is calculated inaccurately as described above.

Expected Results:  
Speed should be accurate.

Comment 1

[Mike Connor [:mconnor]]

most downloaders I've seen use 30-45 seconds as a window in which to calculate
current speed.  The problem is that the window isn't persistent so its harder to
be accurate with this.

Still, I'm sure there's a way of figuring this out without being unduly bloaty.

Comment 2

[Gilad Avidan]

(In reply to comment #1)
If we want to refresh the speed every 3 seconds, why don't we show the average
rate for the past 3 seconds?

Comment 3

[Adam Zey]

I much prefer a floating average for the last n seconds rather than only
updating every n seconds.

If you're doing a 3-second average (Anything over 5 strikes me as too
inaccurate), then update it constantly with the average over the last 3 seconds.

Averaging the entire download is extremely undesirable, as if a download slows
from a high speed to a very low speed, the browser still reports the high speed
as the download speed, which is no longer correct.

Comment 4

[Dimitrios]

IIRC, NS4.x was using the moving average method mentioned above.

Comment 5

[Ali Ebrahim]

*** Bug 250238 has been marked as a duplicate of this bug. ***

Comment 6

[Bill Mason]

*** Bug 251678 has been marked as a duplicate of this bug. ***

Comment 7

[Worcester12345]

Download Manager Tweak might be a good way to address this and most other
Download Manager bugs, then file new bugs after that if necessary.

Comment 8

[Peter van der Woude [:Peter6]]

*** Bug 254786 has been marked as a duplicate of this bug. ***

Comment 9

[Justin Kerk]

There is a seamonkey bug for this, bug 81857, that may have some useful concepts
and code.

Comment 10

[Phil Ringnalda (:philor)]

*** Bug 269910 has been marked as a duplicate of this bug. ***

Comment 11

[Sam Watkins]

What happens is while you're browsing where to save the file, firefox is already
downloading it in the background.  After you choose where to save the file, the
download manager opens.

The download manager is apparently assuming that the download started when the
download manager opened, but in fact a chunk of the file has already been
downloaded.  This leads to impossibly high transfer rates being reported.

Comment 12

[blujay]

(In reply to comment #11)
> What happens is while you're browsing where to save the file, firefox is already
> downloading it in the background.  After you choose where to save the file, the
> download manager opens.
> 
> The download manager is apparently assuming that the download started when the
> download manager opened, but in fact a chunk of the file has already been
> downloaded.  This leads to impossibly high transfer rates being reported.
> 

Sam is completely correct.  This should be a simple bug to fix.  Please fix it!
 It makes the time remaining indicator pretty much useless for any download that
takes less than several minutes.

Comment 13

[Mike Kowalski]

*** Bug 274607 has been marked as a duplicate of this bug. ***

Comment 14

[Mike Kowalski]

*** Bug 274987 has been marked as a duplicate of this bug. ***

Comment 15

[Worcester12345]

needs aviary landing keyword

Comment 16

[Fini A. Alring]

I just discovered a fiesty bugger regarding download manager, that will make the
gui show wrong values..

I have found that download manager (Firefox 1.0/WinXP Pro) will download the
data while im still deciding where to put it. Last time it had downloaded 14MB
of 43MB when I finally chose a location. But the progress bar started on 0%
(instead of ~33%) and the speedmeter told that the file was downloading at 10x
the speed of my Internet connection ( I wish it was true! But in reality it was
more likely because it 'thought' it had downloaded the 14MB in a millisecond ;) ).

So the fix here would be to consider in the gui (progressbar and speedmeter) the
data that has already been downloaded, before the user chooses the files
destination..

Comment 17

[Thomas Bertels]

Attachment: Fix to take into account the data already downloaded

Restore correct speed and remaining time. The speed is still calcuted since the
start of the download (it isn't the average of the X last seconds) and the
remaining time too of course.
I'll perhaps make an other patch to use the average of the X last seconds, but
it's just an enhancement.

Comment 18

[Thomas Bertels]

Comment on attachment 178737 [details] [diff] [review]
Fix to take  into account the data already downloaded

>Index: toolkit/mozapps/downloads/content/DownloadProgressListener.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/content/DownloadProgressListener.js,v
>retrieving revision 1.4.12.1.2.1
>diff -u -r1.4.12.1.2.1 DownloadProgressListener.js
>--- toolkit/mozapps/downloads/content/DownloadProgressListener.js	28 Aug 2004 21:43:00 -0000	1.4.12.1.2.1
>+++ toolkit/mozapps/downloads/content/DownloadProgressListener.js	27 Mar 2005 19:45:52 -0000
>@@ -9,9 +9,9 @@
>   this._statusFormatKBMB = aStringBundle.getString("statusFormatKBMB");
>   this._statusFormatKBKB = aStringBundle.getString("statusFormatKBKB");
>   this._statusFormatMBMB = aStringBundle.getString("statusFormatMBMB");
>-  this._statusFormatUnknownMB = aStringBundle.getString("statusFormatUnknownMB");
>-  this._statusFormatUnknownKB = aStringBundle.getString("statusFormatUnknownKB");
>-  this._remain = aStringBundle.getString("remain");
>+  this._statusFormatUnknownMB = aStringBundle.getString("statusFormatUnknownMB");
>+  this._statusFormatUnknownKB = aStringBundle.getString("statusFormatUnknownKB");
>+  this._remain = aStringBundle.getString("remain");
>   this._unknownFilesize = aStringBundle.getString("unknownFilesize");
>   this._longTimeFormat = aStringBundle.getString("longTimeFormat");
>   this._shortTimeFormat = aStringBundle.getString("shortTimeFormat");
>@@ -25,6 +25,8 @@
>   rateChangeLimit: 0,
>   priorRate: 0,
>   lastUpdate: -500,
>+  aCurTotalProgressFix: 0,
>+  Difference: 0,
>   doc: null,
>   onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus, aDownload)
>   {
>@@ -39,6 +41,11 @@
>   onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress, aMaxSelfProgress,
>                               aCurTotalProgress, aMaxTotalProgress, aDownload)
>   {
>+	// Fix to get real download rate and remaining time
>+	if (this.Difference == 0)
>+		this.Difference = aCurTotalProgress;
>+	this.aCurTotalProgressFix = aCurTotalProgress - this.Difference;
>+  
>     var overallProgress = aCurTotalProgress;
>     // Get current time.
>     var now = (new Date()).getTime();
>@@ -56,7 +63,7 @@
>     this.elapsed = now - (aDownload.startTime / 1000);
>     var rate; // aCurTotalProgress/sec
>     if (this.elapsed)
>-      rate = (aCurTotalProgress * 1000) / this.elapsed;
>+      rate = (this.aCurTotalProgressFix * 1000) / this.elapsed;
>     else
>       rate = 0;

Comment 19

[Daniel Brooks [:db48x]]

Comment on attachment 178737 [details] [diff] [review]
Fix to take  into account the data already downloaded

mconner would be a more likely reviewer

Comment 20

[Mike Connor [:mconnor]]

Comment on attachment 178737 [details] [diff] [review]
Fix to take  into account the data already downloaded

>-  this._statusFormatUnknownMB = aStringBundle.getString("statusFormatUnknownMB");
>-  this._statusFormatUnknownKB = aStringBundle.getString("statusFormatUnknownKB");
>-  this._remain = aStringBundle.getString("remain");
>+  this._statusFormatUnknownMB = aStringBundle.getString("statusFormatUnknownMB");
>+  this._statusFormatUnknownKB = aStringBundle.getString("statusFormatUnknownKB");
>+  this._remain = aStringBundle.getString("remain");

I'm guessing this is a line ending change, which seems unnecessary/out of
scope, so it shouldn't really be here.

>+  aCurTotalProgressFix: 0,

this is only used in onProgressChange, and recalculated there, so it doesn't
need to be persisted.  Also, the "a" prefix denotes an argument, i.e. function
foo (aValue), and thus isn't correct here. This should be be something more
accurate, like realCurrentProgress, and only used in the function.

>+	// Fix to get real download rate and remaining time
>+	if (this.Difference == 0) {
>+		this.Difference = aCurTotalProgress;
>+	this.aCurTotalProgressFix = aCurTotalProgress - this.Difference;
>+  

spaces, not tabs please.

Comment 21

[Thomas Bertels]

Attachment: Fix to take into account the data already downloaded - 2

I've made the requested changes.

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 181131 [details] [diff] [review]
Fix to take into account the data already downloaded - 2


>+  Difference: 0,

This should be difference, sorry, should have pointed that out (we use
intercaps for variable names pretty much everywhere in Moz code).

>+    // Fix to get real download rate and remaining time
>+    if (this.Difference == 0)
>+        this.Difference = aCurTotalProgress;
>+    var realCurrentProgress = aCurTotalProgress - this.Difference;
>+

2-space identation, like the rest of the file.

with those style nits fixed, r=me.  Thanks!

Comment 23

[Thomas Bertels]

Attachment: Fix to take into account the data already downloaded - 3

Comment 24

[Jo Hermans]

*** Bug 292658 has been marked as a duplicate of this bug. ***

Comment 25

[Jo Hermans]

(In reply to comment #23)
> Created an attachment (id=181257) [edit]
> Fix to take into account the data already downloaded - 3
> 

Note that this only fixes case 2 of comment 0.

Comment 26

[Thomas Bertels]

(In reply to comment #25)
The case 1 is more difficult to implement because we need to make Firefox
remember the data downloaded at different times.
The refresh time seems to be 500ms, so if the window used is 5 seconds, Firefox
will have to remember 10 different values or so (I don't even think about a 30
seconds window).

Comment 27

[Jo Hermans]

(In reply to comment #26)
> (In reply to comment #25)
> The case 1 is more difficult to implement because we need to make Firefox
> remember the data downloaded at different times.
> The refresh time seems to be 500ms, so if the window used is 5 seconds, Firefox
> will have to remember 10 different values or so (I don't even think about a 30
> seconds window).

No, you don't. You can also work with 'smoothed averages'. Look it up in your
statistics course. The one below smoothes over the last 10 measurements :

new_savg = (0.9 * old_savg) + (speed / 10);

Comment 28

[Thomas Bertels]

Comment on attachment 181257 [details] [diff] [review]
Fix to take into account the data already downloaded - 3

This patch doesn't handle multiple downloads. It seems to be necessary to fix
this problem at the source. Anyway, Jo, thanks for the formula.

Comment 29

[Daniel Veditz [:dveditz]]

Not blocking a security release

Comment 30

[Christian Stadler]

(In reply to comment #28)
> This patch doesn't handle multiple downloads. It seems to be necessary to fix
> this problem at the source. Anyway, Jo, thanks for the formula.

I guess, restoring the current speed, when the download-manager opens doen't
make much sense at all. Especially, when you have multiple downloads its likely,
that one download finishes earlier than another one, so even if the download
speeds have been restored, they would be inaccurate, if one or more downloads
are finished. So they either need to be restored again, if a download has
finished or a smoothed average (average of the last 5 to 10 secs or something
like that) has to be calculated and IMHO the latter is the best, if not the only
solution for this. Opinions?

Comment 31

[Asa Dotzler [:asa]]

->mconnor.

Comment 32

[Son Le]

Attachment: proposed patch v0

In similar veins to Thomas's patch, but one level lower.

The speed is calculated by dividing the downloaded amount by the time taken
(and smoothed over the last 5 readings - see comment #27).

The pref was originally for just tweaking, but I decided to leave it in anyway.

Comment 33

[Mike Connor [:mconnor]]

Comment on attachment 189053 [details] [diff] [review]
proposed patch v0

fixing bugmail (other one is not me) hopefully I'll get to this today.

Comment 34

[Mike Connor [:mconnor]]

Comment on attachment 189053 [details] [diff] [review]
proposed patch v0

first, the pref stuff is overkill and the extra member is unnecessary, just use
the hardcoded interval.

>+  PRUint32 milliseconds = PR_IntervalToMilliseconds(delta);
>+  if (milliseconds > 0) {
>+    PRFloat64 speed = (PRFloat64)(aCurTotalProgress - mLastTotalProgress) / milliseconds;
>+    // smooth average over the last 5 readings
>+    mSpeed = mSpeed * (4.0 / 5.0) + speed / 5.0;
>+  }

this actually doesn't do what you're saying it does.  It takes the current
value of mSpeed, and weights it at 80% so its really not using the last five
readings.  You'd need to store the last five values, drop the oldest, then add
and divide.  Also, this will give broken results for the first couple of
seconds (at the 400/500ms range), since if mSpeed is 0, and speed is 100,
mSpeed is then 20.

I haven't reviewed the rest of the patch yet, but skimming it looked reasonable
thus far.

Comment 35

[Jo Hermans]

(In reply to comment #34)
> this actually doesn't do what you're saying it does.  It takes the current
> value of mSpeed, and weights it at 80% so its really not using the last five
> readings.  You'd need to store the last five values, drop the oldest, then add
> and divide.  Also, this will give broken results for the first couple of
> seconds (at the 400/500ms range), since if mSpeed is 0, and speed is 100,
> mSpeed is then 20.
> 

It's just a smoothed average, but it doesn't run over the last 5 measurements,
though. Smoothes averages don't need asn arrays with prtevious measurements, so
it's useful when there are a lot of data-points. If you only want the last 5
values (2.5 seconds), then it doesn't make a difference, but if you want all
data-points of the last 30 seconds (60 values), then this formulat would be a
lot faster. I'm using a similar technique in calculating DSL speeds in
commercial software, where it's practical to keep so many data points for 10000
users or more. Here, it doesn't matter much.

To prevent the error for the first measurement, you can set mSeed equal to
speed. The second data-point will be all right.

Comment 36

[Son Le]

Attachment: patch v1

Removed pref stuff and tweaked init speed calc as suggested by Jo.

Also added fix for point no. 2 in original comment, which strictly speaking is
no longer needed, but is still good to have IMHO (more accurate start time
anyway).

Comment 37

[Son Le]

Attachment: patch v2

hmmm... noticed that the start speed was way too high when the filepicker was
up for a while. 

This patch addresses that by calculating the initial speed differently (using
the start time and current bytes downloaded). Also bumped up the sampling to 10
readings instead of 5.

The remaining issue is when a cancelled and then downloaded again, it seems the
original channel is reused and we get a reading of last download progress in a
very short time frame.

Comment 38

[Mike Connor [:mconnor]]

It's late, the patch looks reasonable, but I'll be looking at this first thing
tomorrow.

Comment 39

[Chris Beard]

is this ready to land?

Comment 40

[Jo Hermans]

*** Bug 305920 has been marked as a duplicate of this bug. ***

Comment 41

[Mike Connor [:mconnor]]

Comment on attachment 191325 [details] [diff] [review]
patch v2

I'm not sure I wholly grok the nsIDownload changes, but this looks good to me.

bz, can you take a look and sanity-check the whole thing, along with SR for the
non-toolkit bits?

Comment 42

[Boris Zbarsky [:bzbarsky]]

It'll take me a bit of time to get to this (maybe a week?  Not sure...)

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 191325 [details] [diff] [review]
patch v2

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp
>@@ -1964,33 +1967,56 @@ nsDownload::OnProgressChange64(nsIWebPro
>+  if (LL_CMP(delta, <, gInterval) && aMaxTotalProgress != -1 && aCurTotalProgress < aMaxTotalProgress)

Why bother with LL_CMP?  PRIntervalTime is a PRInt32.

Also, do we care that PRIntervalTime can wrap in about 11 hours in some cases? 
I guess that's ok because the delta will still be fine as long as your updates
come more often than that, huh?

>+  if (LL_IS_ZERO(mSpeed)) {

That makes no sense; mSpeed is not a PRInt64 or PRUint64.

>+    // (microseconds -> milliseconds)
>+    PRInt64 usecPerMsec;
>+    LL_I2L(usecPerMsec, PR_USEC_PER_MSEC);
>+    PRInt64 diff = PR_Now() - mStartTime;
>+    LL_DIV(diff, diff, usecPerMsec);
>+    LL_L2UI(elapsedMsecs, diff);

I have to wonder... why not just use nsInt64/nsUint64 instead?

>+  if (elapsedMsecs > 0) {
>+    PRFloat64 speed = (PRFloat64)(aCurTotalProgress - mLastTotalProgress) / elapsedMsecs;

And this is _definitely_ wrong if you'd using the PRInt64 types.  You can't
just subtract them, for one thing.

>+    PRFloat64 speed = (PRFloat64)(aCurTotalProgress - mLastTotalProgress) / elapsedMsecs;
>+    if (LL_IS_ZERO(mSpeed))
>+      mSpeed = speed;

So mSpeed is in bytes per msec, right?

> nsDownload::OnStateChange(nsIWebProgress* aWebProgress,
>+  // Record the start time only if it hasn't been set.
>+  if ((!mStartTime) && (aStateFlags & STATE_START))

Can't just do !mStartTime, sorry.

>Index: toolkit/mozapps/downloads/content/DownloadProgressListener.js

>@@ -133,49 +125,49 @@ DownloadProgressListener.prototype = 
>+    var rate = aDownload.speed;
>+      // rate is K bytes/sec;

Hmm... Are we really sure we want to use 1000-byte units here?	I guess that's
ok...

>Index: uriloader/base/nsIDownload.idl

>+
>+    /**
>+     * The speed of the transfer in K bytes/sec.
>+     */
>+    readonly attribute double speed;

Please document clearly that this is KB, not KiB (that is, that these are
1000-byte kilobytes).

If you're changing nsIDownload, you probably want to get review from biesi, I
suspect (since he's the one working on freezing this interface).

You also need to fix the other implementations of this.  at the very least,
there's the download manager in xpfe/components and the two embedding impls:
embedding/browser/cocoa and embedding/browser/powerplant.

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

> NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest *request, nsISupports * aCtxt)
> {
>   NS_PRECONDITION(request, "OnStartRequest without request?");
> 
>+  mTimeDownloadStarted = PR_Now();

This change is so that we set mTimeDownloadStarted before posing the dialog? 
If so, please document that?

Comment 44

[Boris Zbarsky [:bzbarsky]]

biesi, this patch changes nsIDownload, so I figured you'd want to take a look at it.

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 191325 [details] [diff] [review]
patch v2

bz: actually, I won't freeze nsIDownload... but I do want to move it to the
frontend (embeddors only need to implement nsITransfer)

you must change the nsIDownload IID, and you should also fix the other
nsIDownload implementations

why is the speed in kbytes/sec, instead of bytes/sec?

this impl forces the UI to poll the speed... I guess there's not really another
way. except... why does the backend need to calculate this?

>Please document clearly that this is KB, not KiB 

ewwww, that sucks, does it _have_ to use KB instead of KiB?

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

(to be clear: the UI should still be labelled "KB", but I do think it should
divide by 1024)

Comment 47

[Son Le]

Attachment: patch v3

This patch should address all review comments. I must have had a brain freeze
whilst doing patch v2. I'm also glad to say that the speed in this patch is
bytes/sec.

I've also updated the other impls as per bzbarsky's comment, but can't
compile/test them as such. Any pointers as to how to do this would be great.

(In reply to comment #45)
> this impl forces the UI to poll the speed... I guess there's not really 
> another way. except... why does the backend need to calculate this?

As per comment #28, it doesn't seem to work for multiple downloads.

Comment 48

[Boris Zbarsky [:bzbarsky]]

You can test the xpfe code by building Seamonkey.

You can test the embedding/browser/cocoa code by asking a Camino build buddy to
do so.  Or at least filing a followup bug on Camino to implement it (after ccing
some Camino developers here so they can look at this patch).

Comment 49

[Son Le]

Built Seamonkey and successfully tested with single and multiple downloads.

Comment 50

[Boris Zbarsky [:bzbarsky]]

It'll probably take me a week or more to get to this...

Comment 51

[Christian :Biesinger (don't email me, ping me on IRC)]

I'll try to get to this this weekend or so. Sorry, I've been swamped with too much stuff recently.

Comment 52

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 198271 [details] [diff] [review]
patch v3

+static nsInt64 gInterval((PRUint32)(500 * PR_USEC_PER_MSEC));

make this a constant:
static const nsInt64 gInterval((PRUint32)(500 * PR_USEC_PER_MSEC));

+                         mLastUpdate(PR_Now() - (PRUint32)gInterval)

why not initialize to LL_ZERO?

please do casts to double like double(foo), not (double)foo

+  mLastTotalProgress = curTotalProgress;

now this value is stored twice... (mCurrBytes is the other value)

I'd be much happier if bug 273971 was fixed before this bug.

+    else
+      // Calculate 'smoothed average' of 10 readings.
+      mSpeed = mSpeed * (9.0 / 10.0) + speed / 10.0;

please use braces around the else body here; easier to read for multi-line elses

r=me (hesitantly).


I didn't look at the toolkit/mozapps/downloads/content/DownloadProgressListener.js changes, those need seperate review from a toolkit peer, but maybe mconnor gave that already above

Comment 53

[Boris Zbarsky [:bzbarsky]]

Still looking for time to get to this.  Or rather, looking for time to get to patches that requested review a month before this one...

You may want to consider a different sr, given that biesi has looked at this patch.

Comment 54

[Son Le]

Comment on attachment 198271 [details] [diff] [review]
patch v3

new patch soon pending checkin of patch to bug 273971

Comment 55

[Son Le]

Attachment: patch v4

new updated patch now that bug 273971 has been checked in.

Comment 56

[[e-mail]]

Comment on attachment 206695 [details] [diff] [review]
patch v4

Basically fine, but I'd appreciate if you could come up with some good answers to these questions before checking in.

>+  // For the initial speed we use the start time as the download could have
>+  // progressed while the filepicker was up.
>+  if (mSpeed == 0.0)
>+    delta = now - mStartTime;
Is this supposed to be only in the toolkit version of the patch? Although, I'd prefer it instead if you could set mLastUpdate at the same time as mStartTime, that way you wouldn't need this check.

>+  if (delta < gInterval && aMaxTotalProgress != -1 && aCurTotalProgress < aMaxTotalProgress)
Does anyone know why we have this long condition? I'd have thought delta < gInterval would suffice, possibly with aCurTotalProgres != aMaxTotalProgress?

>+    if (mSpeed == 0.0)
Nit: I'm not sure that you shouldn't test something else, such as mCurrBytes.

>+      mSpeed = speed;
>+    else
>+    {
>+      // Calculate 'smoothed average' of 10 readings.
>+      mSpeed = mSpeed * (9.0 / 10.0) + speed / 10.0;
>+    }
Nit: I'm not convinced about this way of writing it, although I'm not sure whether I prefer (mSpeed * 9 + speed) / 10 or mSpeed * 0.9 + speed * 0.1 also I wouldn't have bothered with the {}s.

Comment 57

[Son Le]

Attachment: patch v4.1

(In reply to comment #56)

> Is this supposed to be only in the toolkit version of the patch? Although, I'd
> prefer it instead if you could set mLastUpdate at the same time as mStartTime,
> that way you wouldn't need this check.

ok, done.
 
> >+  if (delta < gInterval && aMaxTotalProgress != -1 && aCurTotalProgress < aMaxTotalProgress)
> Does anyone know why we have this long condition? I'd have thought delta <
> gInterval would suffice, possibly with aCurTotalProgres != aMaxTotalProgress?

I've done a bit of testing and the only time the last 2 conditions are met is
when the size is unknown. Deleting them had no noticeable impact regardless of
size so I'm incline to think they're redundant. 

> >+    if (mSpeed == 0.0)
> Nit: I'm not sure that you shouldn't test something else, such as mCurrBytes.

fixed.

> >+    else
> >+    {
> >+      // Calculate 'smoothed average' of 10 readings.
> >+      mSpeed = mSpeed * (9.0 / 10.0) + speed / 10.0;
> >+    }
> Nit: I'm not convinced about this way of writing it, although I'm not sure
> whether I prefer (mSpeed * 9 + speed) / 10 or mSpeed * 0.9 + speed * 0.1 also I
> wouldn't have bothered with the {}s.

I've changed this to

+    else {
+      // Calculate 'smoothed average' of 10 readings.
+      mSpeed = mSpeed * 0.9 + speed * 0.1;
+    }

I think the {}s make it more readable.

Comment 58

[Son Le]

> I've done a bit of testing and the only time the last 2 conditions are met is
> when the size is unknown. Deleting them had no noticeable impact regardless of
> size so I'm incline to think they're redundant. 

Forgot to add that I've removed them in patch v4.1.

Comment 59

[Son Le]

Attachment: patch v4.1 - unbitrotted

Ready for checkin.

Comment 60

[Son Le]

checked in (thanks neil!)

Comment 61

[Jan Darmochwal]

Progress dialogs in SeaMonkey now display different values (time left / download rate) than Download Manager.
Are there plans to somehow make progress dialogs use the values from nsDownloadManager.cpp or will it have to calculate its own values?
Shall I file a new bug on this?

Comment 62

[Christian :Biesinger (don't email me, ping me on IRC)]

yeah, please file a new bug on that.

Comment 63

[Jan Darmochwal]

filed new bug 326840

Should it block this one?

Comment 64

[Steffen Wilberg]

Comment on attachment 210465 [details] [diff] [review]
patch v4.1 - unbitrotted

Would this be acceptable for the 1.8.1 branch?

Comment 65

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 210465 [details] [diff] [review]
patch v4.1 - unbitrotted

No; it changes APIs.

Comment 66

[[e-mail]]

Attachment: Branch port

I had to include portions of 273971 in this patch.
Also I've only tested the xpfe code, not the toolkit code.

Comment 67

[[e-mail]]

Attachment: Branch port, diffed against trunk

For ease of review of the previous patch, this is what you get if you apply the patch to the branch but then diff against trunk.

Comment 68

[Mike Connor [:mconnor]]

Comment on attachment 214086 [details] [diff] [review]
Branch port

a181=me, assuming biesi's r=

Comment 69

[[e-mail]]

Attachment 214086 [details] [diff] checked in to the branch.

Comment 70

[Kevin Brosnan]

*** Bug 340722 has been marked as a duplicate of this bug. ***

Comment 73

[Worcester12345]

The bug that this was duped from is a Mozilla Application
Suite bug.  This is a Firefox bug. Should either one or both of these be changed to "Core"?