Closed Bug 239006 Opened 16 years ago Closed 15 years ago

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

Categories

(Toolkit :: Downloads API, defect, P3, minor)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: marcoos, Assigned: zbraniecki)

References

Details

Attachments

(3 files, 12 obsolete files)

27.23 KB, image/png
Details
23.24 KB, image/gif
Details
12.15 KB, patch
zbraniecki
: review+
zbraniecki
: superreview+
Details | Diff | Splinter Review
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.
Attached image Screenshot
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.
Flags: blocking0.9?
its minor and doesn't need to block 0.9.  But, putting on the 1.0 list and taking.
Assignee: bugs → mconnor
Severity: normal → minor
Flags: blocking1.0?
Flags: blocking0.9?
Flags: blocking0.9-
OS: Linux → All
Hardware: PC → All
Summary: Download Manager displays strange percentage values in the title bar → Download manager doesn't account for filesize when presenting combined percentages
*** Bug 240109 has been marked as a duplicate of this bug. ***
-ing but will take patch.
Flags: blocking1.0? → blocking1.0-
Status: NEW → ASSIGNED
Priority: -- → P3
*** Bug 248626 has been marked as a duplicate of this bug. ***
*** Bug 253808 has been marked as a duplicate of this bug. ***
taking.
patch is on a way
Assignee: mconnor → gandalf
Status: ASSIGNED → NEW
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.
The second screenshot is bug 245829.
*** Bug 270964 has been marked as a duplicate of this bug. ***
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch minor update (obsolete) — Splinter Review
minor update.
Attachment #166990 - Attachment is obsolete: true
Attached patch next minor update (obsolete) — Splinter Review
minor update: two more comments fixed
Attachment #166992 - Attachment is obsolete: true
Attached patch +xpfe part (obsolete) — Splinter Review
added code for xpfe. thanks timeless.
Attachment #166994 - Attachment is obsolete: true
Attachment #166995 - Flags: superreview?(darin)
Attachment #166995 - Flags: review?(cbiesinger)
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.
Attachment #166995 - Flags: review?(cbiesinger) → review-
Attachment #166995 - Flags: superreview?(darin)
Attached patch fixed patch (obsolete) — Splinter Review
patch addressing issues listed in Comment 17
Attachment #166995 - Attachment is obsolete: true
Attachment #167428 - Flags: review?(cbiesinger)
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
Attachment #167428 - Flags: review?(cbiesinger) → review-
Attached patch thid try (obsolete) — Splinter Review
this patch fixes issues mentioned in previous comment
Attachment #167428 - Attachment is obsolete: true
Attached patch add new UUID for nsITransfer (obsolete) — Splinter Review
Attachment #167866 - Attachment is obsolete: true
Attachment #167867 - Flags: review?(cbiesinger)
Attachment #167867 - Attachment is obsolete: true
Attachment #167867 - Flags: review?(cbiesinger)
Attachment #167869 - Flags: review?(cbiesinger)
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.
Attachment #167869 - Flags: superreview?(darin)
Attachment #167869 - Flags: review?(cbiesinger)
Attachment #167869 - Flags: review+
Comment on attachment 167869 [details] [diff] [review]
add patch for ./camino/src/download

r=me for the toolkit bits
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?
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
Attachment #167869 - Flags: superreview?(darin) → superreview-
Attached patch deal with unknown size (obsolete) — Splinter Review
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. :)
Attachment #167869 - Attachment is obsolete: true
Attachment #168271 - Flags: superreview?(darin)
Attachment #168271 - Flags: review?(cbiesinger)
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
Attachment #168271 - Flags: review?(cbiesinger) → review+
Comment on attachment 168271 [details] [diff] [review]
deal with unknown size

sr=darin
Attachment #168271 - Flags: superreview?(darin) → superreview+
Attached patch comment idl (obsolete) — Splinter Review
Add two comments
Attachment #168271 - Attachment is obsolete: true
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.
Attachment #168348 - Flags: review-
Attached patch . (obsolete) — Splinter Review
bytes->kbytes
currentBytes->amountTransfered

I'll remember your sugestion about using bytes here.
Attachment #168348 - Attachment is obsolete: true
Attachment #168360 - Flags: superreview?(darin)
Attachment #168360 - Flags: review?(cbiesinger)
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
Attachment #168360 - Flags: review?(cbiesinger) → review+
kbytes->bytes is bug 273971
Status: NEW → ASSIGNED
Blocks: 273971
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
Attachment #168360 - Flags: superreview?(darin) → superreview+
fixed both issues.
Attachment #168360 - Attachment is obsolete: true
Attachment #168640 - Flags: superreview+
Attachment #168640 - Flags: review+
No change in patch, only chunk line numbers updated to trunk.
Attachment #168640 - Attachment is obsolete: true
Attachment #169009 - Flags: superreview+
Attachment #169009 - Flags: review+
The patch was checked in.

Hoah!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
*** Bug 301182 has been marked as a duplicate of this bug. ***
Verified fixed with 1.5b2 and the latest trunk nightly build.
Status: RESOLVED → VERIFIED
*** Bug 355746 has been marked as a duplicate of this bug. ***
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.