239006 - Download manager doesn't account for filesize when presenting combined percentages

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P3)

Description

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

I was downloading two files - one of them was about 25.2 MB and the second one 
was 42.8 MB (99%).

When the smaller file was nearly completely (99%) downloaded, the second one was
downloaded in about 64%.

Firefox's Download Manager showed "82% of two files - Downloads" in the title bar.

Size of downloaded parts: 52.6 MB
Full size of the files: 25.2 + 42.8 = 68 MB

52.6/68 = 0.774 = 77.4%

Not 82%.

It looks as if the value  "82%" was calculcated by simply adding 99 + 64 and
dividing it by 2. This is obviously wrong...

This bug was reproduced under many Firefox builds under Linux, I don't know if
it appears on other platforms.

A screenshot follows.

Comment 1

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

Attachment: Screenshot

Comment 2

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

This is an important issue (and if not fixed, can make people think "Mozilla
can't count" ;))

Requesting this to be a blocker for 0.9.

Comment 3

[Mike Connor [:mconnor]]

its minor and doesn't need to block 0.9.  But, putting on the 1.0 list and taking.

Comment 4

[Ali Ebrahim]

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

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

-ing but will take patch.

Comment 6

[N. Thomas Posthuma]

Attachment: Additional expample of bug from similar Download Manager code

Screenshot. 23.8 Kb.

Comment 7

[(not currently active) Ted Mielczarek]

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

Comment 8

[Jesse Ruderman]

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

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

taking.
patch is on a way

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

not so easy :(
The easiest way I see to do this is to get nsDownload::GetTransferInformation 
(http://lxr.mozilla.org/aviarybranch/source/toolkit/components/downloads/src/
nsDownloadManager.cpp#1923) scriptable. 

Without this I was trying to get MaxBytes and CurrBytes from listener, but I 
find it very dirty and i won't even try to patch it this way.

Second screenshot is a different problem. In some situations already downloaded/
aborted downloads are not removed from gActiveDownloads.
I will try to figure it out tommorow, but if anyone can reproduce it, it would 
be very helpfull to get info when such ghosts are left.

Comment 11

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

The second screenshot is bug 245829.

Comment 12

[Uri Bernstein]

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

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch

ok. here comes a patch.
It deals with a problem described here, but it _does not_ touch the problem
from bug 245829. I'll patch it later and that patch will also clear
onUpdateProgress.

This patch fixes following issues:
1) Bad calculations for downloaded percents
2) 3 warnings in nsDownloadManager.cpp compilation
3) "x% of 1 file" was displayed after 1% was reached. Now it displays just
after download starts

I can also suggest to remove percentComplete from nsITransfer since it is not
used anywhere in the code after this patch and can be easly counted by
(dl.currBytes/dl.maxBytes)*100.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: minor update

minor update.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: next minor update

minor update: two more comments fixed

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: +xpfe part

added code for xpfe. thanks timeless.

Comment 17

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

Comment on attachment 166995 [details] [diff] [review]
+xpfe part

as mentioned on irc:
- you must change the IID of interfaces you change
- I'm not so happy with changing this interface since embeddors have to
implement it... on the other hand, I want to change it anyway (bug 241082),
basically move almost everything from it to nsIDownload, so maybe that's ok...

but please add it to nsIDownload, not nsITransfer.

- use a 64-bit type for sizes (unsigned long long)

also, as I'm now noticing: this patch breaks camino and the cocoa embeding
sample, as they implement nsIDownload. please fix.

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: fixed patch

patch addressing issues listed in Comment 17

Comment 19

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

Comment on attachment 167428 [details] [diff] [review]
fixed patch

./uriloader/base/nsIDownload.idl
you must change the nsITransfer IID if you change it.

+     * The percentage of transfer completed;

please end sentences with a '.' :-)

+    readonly attribute PRUInt64 currBytes;

hm... this works? xpidl shows no warning for this?

(the I should be lowercase.)

I think I'd prefer "unsigned long long" here, but that doesn't really matter


toolkit/mozapps/downloads/content/downloads.js
you need a firefox/toolkit peer to OK the toolkit/ changes

+  // we're not downloading anything at the moment but we already downloaded
something

please wrap lines at 80 chars. there's also a missing comma after "moment".

+  if (gLastComputedMean === false || mean != gLastComputedMean) {

well... JS lets you do this, but I'm not really a fan of variables changing
their type...

./xpfe/components/download-manager/src/nsDownloadManager.cpp

seems like you also need to change nsDownloadManager.h to change to 64-bit
types?

/camino/src/download/nsDownloadListener.mm, line 139 --
nsDownloadListener::GetPercentComplete(PRInt32 *aPercentComplete)
this file will also need changing

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: thid try

this patch fixes issues mentioned in previous comment

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: add new UUID for nsITransfer

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: add patch for ./camino/src/download

Comment 23

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

Comment on attachment 167869 [details] [diff] [review]
add patch for ./camino/src/download

r=biesi if darin sr's, and if you get the ok from a toolkit peer.

Comment 24

[Mike Connor [:mconnor]]

Comment on attachment 167869 [details] [diff] [review]
add patch for ./camino/src/download

r=me for the toolkit bits

Comment 25

[Darin Fisher]

Comment on attachment 167869 [details] [diff] [review]
add patch for ./camino/src/download

>Index: ./uriloader/base/nsIDownload.idl

> interface nsITransfer : nsISupports {

>-    readonly attribute PRInt32 percentComplete;

why is this attribute being moved?  doesn't a nsITransfer have the
concept of being partially complete?


>+    readonly attribute PRUint64 currBytes;

how about curBytes instead of currBytes (or currentBytes) :)


>Index: ./toolkit/components/downloads/src/nsDownloadManager.cpp

>+  gRDFService->GetIntLiteral(aPause ? (int)nsIDownloadManager::DOWNLOAD_PAUSED : (int)nsIDownloadManager::DOWNLOAD_DOWNLOADING, getter_AddRefs(intLiteral));

nit: break this long line into something readable at 80 chars?

Comment 26

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok. The plan is (per irc discussion):
1) currBytes -> currentBytes for scriptable
2) maxBytes -> size for scriptable
3) we have darin's acceptance for nsIDownload -> nsITransfer change
4) deal with unknown (-1) size (maxBytes)

patch is on the way

Comment 27

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: deal with unknown size

ok. here it is.
All darin's suggestions implemented.

I solved "unknown size" issue by just not counting in such files.

We're definitly screwing up gActiveDownloads and I'll make a patch for bug
241082 when we'll finish this.

Also, I changed Math.round to Math.floor because we don't want to tell somebody
that he downloaded 1% after downloading 5 MB of 1000 MB.

And additionally i moved var dl before a loop.

I hope you like it. :)

Comment 28

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

Comment on attachment 168271 [details] [diff] [review]
deal with unknown size

uriloader/base/nsIDownload.idl
please document what size is if the size is unknown (maybe also what
percentComplete is?)

toolkit/mozapps/downloads/content/downloads.js

Comment 29

[Darin Fisher]

Comment on attachment 168271 [details] [diff] [review]
deal with unknown size

sr=darin

Comment 30

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: comment idl

Add two comments

Comment 31

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

Comment on attachment 168348 [details] [diff] [review]
comment idl

ok... as it turns out, what download manager calls "mCurrBytes" is actually
kilobytes; that rounding process really does turn -1 into 0 which explains why
an unknown size is 0.

But, this means that your comments are wrong... I'd kinda prefer this interface
to expose real bytes, but ah well... just make sure that the comments don't lie
in this patch. so, unfortunately I have to review- again.

Comment 32

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: .

bytes->kbytes
currentBytes->amountTransfered

I'll remember your sugestion about using bytes here.

Comment 33

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

Comment on attachment 168360 [details] [diff] [review]
.

thanks! r=biesi, if darin is ok with this.

ah, one thing...
http://dictionary.reference.com/search?q=transfered - transferred is spelled
with two 'r'; but I wouldn't attach a new patch just for that yet

Comment 34

[Zibi Braniecki [:zbraniecki][:gandalf]]

kbytes->bytes is bug 273971

Comment 35

[Darin Fisher]

Comment on attachment 168360 [details] [diff] [review]
.

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp

>+  gRDFService->GetIntLiteral(
>+    aPause ? 
>+    (int)nsIDownloadManager::DOWNLOAD_PAUSED : 
>+    (int)nsIDownloadManager::DOWNLOAD_DOWNLOADING, getter_AddRefs(intLiteral));

s/int/PRInt32/


sr=darin

Comment 36

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: next try (transferred && PRInt32)

fixed both issues.

Comment 37

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: updated to the trunk

No change in patch, only chunk line numbers updated to trunk.

Comment 38

[Zibi Braniecki [:zbraniecki][:gandalf]]

The patch was checked in.

Hoah!

Comment 39

[Jaime Mitchell (use [e-mail] for email)]

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

Comment 40

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

Verified fixed with 1.5b2 and the latest trunk nightly build.

Comment 41

[Jo Hermans]

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