Closed
Bug 394548
Opened 17 years ago
Closed 17 years ago
Store download progress in database when changing states
Categories
(Toolkit :: Downloads API, defect)
Toolkit
Downloads API
Tracking
()
RESOLVED
FIXED
mozilla1.9beta1
People
(Reporter: Mardak, Assigned: Mardak)
References
Details
Attachments
(4 files, 12 obsolete files)
41.75 KB,
image/png
|
Details | |
7.00 KB,
application/octet-stream
|
Details | |
1.12 KB,
patch
|
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
16.73 KB,
patch
|
Details | Diff | Splinter Review |
Useful for displaying progress of paused downloads (w/o depending on ProgressChanges), size of completed downloads, extra safety check for resuming downloads to make sure the file didn't change, etc. This is a backend-only change.. db upgrade tests in a bit.. Various notes.. NOT NULL DEFAULT on columns for existing downloads... (future issue of "what should we show for completed downloads that we don't know the size?") A newly created table doesn't need the NOT NULL DEFAULT as downloads will set the value. The sizes we get come in as PRInt64s, so no real need to convert to PRUint64 and check LL_MAXUINT. Got rid of those nsUint64s. No need to fudge <1KB files to 1KB -- the "GB patch" also handles bytes :)
Attachment #279227 -
Flags: review?(comrade693+bmo)
Assignee | ||
Comment 1•17 years ago
|
||
Oh, forgot to mention this was tested by querying the table with mozStorage Explorer ;)
Comment 2•17 years ago
|
||
(In reply to comment #0) > NOT NULL DEFAULT on columns for existing downloads... (future issue of "what > should we show for completed downloads that we don't know the size?") so, that's an issue for the UI, which you plan on doing in Bug 394263, correct? > A newly created table doesn't need the NOT NULL DEFAULT as downloads will set > the value. Yes it does - importing from the rdf. > The sizes we get come in as PRInt64s, so no real need to convert to PRUint64 > and check LL_MAXUINT. Fair enough > Got rid of those nsUint64s. dwitte will be proud > No need to fudge <1KB files to 1KB -- the "GB patch" also handles bytes :) The problem is that nsIDownload::size is the size in KB, with an unknown size represented as zero. I'd also like some comments with your new function at least.
Assignee | ||
Comment 3•17 years ago
|
||
(In reply to comment #2) > (In reply to comment #0) > > NOT NULL DEFAULT on columns for existing downloads... (future issue of "what > > should we show for completed downloads that we don't know the size?") > so, that's an issue for the UI, which you plan on doing in Bug 394263, correct? Yeah. Bug 394263 is for paused and bug 223895 is for completed. Both will probably end up using a same helper function that figures out what to display on unknown sizes. > > A newly created table doesn't need the NOT NULL DEFAULT as downloads will set > > the value. > Yes it does - importing from the rdf. Well, one way is to have AddDownloadToDB insert values for currBytes and maxBytes.. or maybe easier is just having the NOT NULL DEFAULT. > > The sizes we get come in as PRInt64s, so no real need to convert to PRUint64 > > and check LL_MAXUINT. > Fair enough We'll have to either change GetSize to check for -1 and return LL_MAXUINT or change the idl to be PRInt64. Additionally, things from JSland or in general, users of the DB are looking for -1 anyway because SQLite only supports signed 64 integers, so might as well rev the idl to signed 64. (I don't know how we would check for LL_MAXUINT from jsland because numbers that big become doubles.) > > No need to fudge <1KB files to 1KB -- the "GB patch" also handles bytes :) > The problem is that nsIDownload::size is the size in KB, with an unknown size > represented as zero. Well, for other people following size/amountTransferred are in bytes and LL_MAXUINT are "unknown". http://mxr.mozilla.org/mozilla/source/toolkit/components/downloads/public/nsIDownload.idl > I'd also like some comments with your new function at least. Sure. I'll probably rename it to SetProgressBytes to avoid SetProgressListener search conflict :p It'll need an extra check of curr == max -> percent = 100 for the 0 byte case...
Comment 4•17 years ago
|
||
(In reply to comment #3) > > > A newly created table doesn't need the NOT NULL DEFAULT as downloads will set > > > the value. > > Yes it does - importing from the rdf. > Well, one way is to have AddDownloadToDB insert values for currBytes and > maxBytes.. or maybe easier is just having the NOT NULL DEFAULT. Yeah, let's keep the tables the same regardless. > > > The sizes we get come in as PRInt64s, so no real need to convert to PRUint64 > > > and check LL_MAXUINT. > > Fair enough > We'll have to either change GetSize to check for -1 and return LL_MAXUINT or > change the idl to be PRInt64. Additionally, things from JSland or in general, > users of the DB are looking for -1 anyway because SQLite only supports signed > 64 integers, so might as well rev the idl to signed 64. (I don't know how we > would check for LL_MAXUINT from jsland because numbers that big become > doubles.) So, nsIFile::fileSize is a PRInt64, so I think that that will work. Just use long long in the idl please as it looks nicer. Also, as I recall JS converts that to -1 magically :) > > > No need to fudge <1KB files to 1KB -- the "GB patch" also handles bytes :) > > The problem is that nsIDownload::size is the size in KB, with an unknown size > > represented as zero. > Well, for other people following size/amountTransferred are in bytes and > LL_MAXUINT are "unknown". > http://mxr.mozilla.org/mozilla/source/toolkit/components/downloads/public/nsIDownload.idl I wonder if changing the type on this would be useful (like decimal of KB)
Assignee | ||
Comment 5•17 years ago
|
||
Oops, forgot to attach this last night when I posted the patch for bug 394263. zzzz 5am
Attachment #279227 -
Attachment is obsolete: true
Attachment #279336 -
Flags: review?(comrade693+bmo)
Attachment #279227 -
Flags: review?(comrade693+bmo)
Comment 6•17 years ago
|
||
Comment on attachment 279336 [details] [diff] [review] v2 > NS_IMETHODIMP >-nsDownload::GetAmountTransferred(PRUint64* aAmountTransferred) >+nsDownload::GetAmountTransferred(PRInt64* aAmountTransferred) >-nsDownload::GetSize(PRUint64* aSize) >+nsDownload::GetSize(PRInt64* aSize) nit: I prefer |type *var|, and am slowly unifying the file to this. You also need to attach your test database that you are using :)
Attachment #279336 -
Flags: review?(comrade693+bmo) → review+
Assignee | ||
Comment 7•17 years ago
|
||
> You also need to attach your test database that you are using :) (Shh.. I didn't actually test the test script :p I'm assuming I should be able to use the one you provided for download resume bug 377243?) Probably still want it as a separate attachment even if mercurial/gitstyle diffs can handle added binary files.
Assignee | ||
Comment 8•17 years ago
|
||
> >+nsDownload::GetSize(PRInt64* aSize)
> nit: I prefer |type *var|, and am slowly unifying the file to this.
Done.
Let's see what happens with gitstyle patches and binary files...
Attachment #279336 -
Attachment is obsolete: true
Assignee | ||
Comment 9•17 years ago
|
||
Same files as that of bug 377243.
Assignee | ||
Comment 10•17 years ago
|
||
I would put checkin-needed, but I haven't tested the test script. The non-test code works fine as I can check with moz_storage explorer. The test script seems like it should work...
Assignee | ||
Comment 11•17 years ago
|
||
Apparently we can miss out on the last progress update if we're stopping. Fix up the state and percent.
Attachment #279404 -
Attachment is obsolete: true
Attachment #279537 -
Flags: review?(comrade693+bmo)
Assignee | ||
Comment 12•17 years ago
|
||
Picture of strangeness ;)
Comment 13•17 years ago
|
||
Comment on attachment 279537 [details] [diff] [review] v3 r=sdwilsh
Attachment #279537 -
Flags: review?(comrade693+bmo) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #279537 -
Flags: review?(mano)
Assignee | ||
Comment 14•17 years ago
|
||
Because we might miss out on all progress updates, we should check the file for the size. This will have downloads reporting the correct size when they finish, even if they're saved from cache (like web pages).
Attachment #279537 -
Attachment is obsolete: true
Attachment #279556 -
Flags: review?(comrade693+bmo)
Attachment #279537 -
Flags: review?(mano)
Assignee | ||
Comment 15•17 years ago
|
||
Regarding v4's interdiff.. [09/04/2007 12:03:00] <Mardak> biesi: it seems like WBP is skipping out on ProgressChange and going directly to StateChange Stop [09/04/2007 12:03:18] <biesi> Mardak, you shouldn't rely on onProgressChange imo [09/04/2007 12:03:46] <Mardak> biesi: it seems like the last progressChange doesn't get sent if we're stopping either.. related to why you say we shouldnt rely on it? [09/04/2007 12:04:26] <biesi> Mardak, yes [09/04/2007 12:04:48] <biesi> 46 * This interface is used to asynchronously convey channel status and progress [09/04/2007 12:04:48] <biesi> 47 * information that is generally not critical to the processing of the channel.
Comment 16•17 years ago
|
||
Comment on attachment 279556 [details] [diff] [review] v4 Can you unbitrot this please?
Attachment #279556 -
Flags: review?(comrade693+bmo)
Updated•17 years ago
|
Target Milestone: --- → Firefox 3 M9
Assignee | ||
Comment 17•17 years ago
|
||
unbitrot with the expectation of bug 230870 changing the schema to 5 -> depends Other changes include calculating percentage based on resume size and making sure various places use "real file size" such as when we're stopping and updating the DB Some reason I've been getting 0 file sizes when using GetTargetFile's size.. it works better if I grab the size from mTempFile, but looking at exthandler code, the file should have been moved to the destination before we get the stop state change.............
Attachment #279556 -
Attachment is obsolete: true
Attachment #280437 -
Flags: review?(comrade693+bmo)
Comment 18•17 years ago
|
||
Can you enter the debugger and see what size it is (using terminal, ls -l filename)? Also, we had to clone the nsIFile to get the right size.
Assignee | ||
Comment 19•17 years ago
|
||
nsIFile seems to have solved the issue. Made the code look similar to bug 395537.
Attachment #280437 -
Attachment is obsolete: true
Attachment #280751 -
Flags: review?(comrade693+bmo)
Attachment #280437 -
Flags: review?(comrade693+bmo)
Comment 20•17 years ago
|
||
Comment on attachment 280751 [details] [diff] [review] v5.1 schema change testcase?
Attachment #280751 -
Flags: review?(comrade693+bmo)
Assignee | ||
Comment 21•17 years ago
|
||
Added schema testcase.
Attachment #279405 -
Attachment is obsolete: true
Attachment #280751 -
Attachment is obsolete: true
Attachment #281120 -
Flags: review?(comrade693+bmo)
Comment 22•17 years ago
|
||
Comment on attachment 281120 [details] [diff] [review] v5.2 r=sdwilsh
Attachment #281120 -
Flags: review?(comrade693+bmo)
Attachment #281120 -
Flags: review+
Attachment #281120 -
Flags: approval1.9?
Assignee | ||
Comment 23•17 years ago
|
||
v5.sqlite for testcase.. unless you can apply the gitstyle patch which has the binary in the v5.2 patch already ;)
Updated•17 years ago
|
Attachment #281120 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 24•17 years ago
|
||
Unbitrot from bug 393932
Attachment #281120 -
Attachment is obsolete: true
Comment 25•17 years ago
|
||
Mardak, are you going to check this in?
Comment 26•17 years ago
|
||
This bug isn't ready for checkin because it still has an unresolved dependency.
Assignee | ||
Comment 27•17 years ago
|
||
I was thinking about creating a new bug for this patch, but then it would require more reviews/approvals? But I suppose even if I attach it to this bug, it would still require that? The patch fixes a few issues: 1) warning about undefined behavior of multiple "i++" on the same line 2) potentially a display problem if you restart a resumed download that you canceled 3) main issue: DB tracks -1 as the max size when resuming, but the UI is fine I could split this off to a new followup bug or incorporate it into this bug's patch when I unbitrot it (due to bug 397967 and bug 397380)
Attachment #282840 -
Flags: review?(comrade693+bmo)
Assignee | ||
Comment 28•17 years ago
|
||
Whoops. maxBytes - fileSize instead of mMaxBytes - fileSize because mMaxBytes is based on the current resume and not the real maxBytes.
Attachment #282840 -
Attachment is obsolete: true
Attachment #282866 -
Flags: review?(comrade693+bmo)
Attachment #282840 -
Flags: review?(comrade693+bmo)
Assignee | ||
Comment 29•17 years ago
|
||
Oh, yay. #2 isn't actually a problem either. RetryDownload will always pull from the DB meaning it'll allocate a new nsDownload object which will correctly initialize mResumedAt.
Attachment #282866 -
Attachment is obsolete: true
Attachment #282875 -
Flags: review?(comrade693+bmo)
Attachment #282866 -
Flags: review?(comrade693+bmo)
Comment 30•17 years ago
|
||
Comment on attachment 282875 [details] [diff] [review] followup v1.2 r=sdwilsh
Attachment #282875 -
Flags: review?(comrade693+bmo)
Attachment #282875 -
Flags: review+
Attachment #282875 -
Flags: approval1.9?
Assignee | ||
Comment 31•17 years ago
|
||
Filed bug 398218 for the followup. Checking in toolkit/components/downloads/src/nsDownloadManager.cpp; /cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v <-- nsDownloadManager.cpp new revision: 1.130; previous revision: 1.129 done Checking in toolkit/components/downloads/src/nsDownloadManager.h; /cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v <-- nsDownloadManager.h new revision: 1.48; previous revision: 1.47 done RCS file: /cvsroot/mozilla/toolkit/components/downloads/test/schema_migration/v5.sqlite,v done Checking in toolkit/components/downloads/test/schema_migration/v5.sqlite; /cvsroot/mozilla/toolkit/components/downloads/test/schema_migration/v5.sqlite,v <-- v5.sqlite initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/components/downloads/test/schema_migration/test_migration_to_6.js,v done Checking in toolkit/components/downloads/test/schema_migration/test_migration_to_6.js; /cvsroot/mozilla/toolkit/components/downloads/test/schema_migration/test_migration_to_6.js,v <-- test_migration_to_6.js initial revision: 1.1 done Checking in toolkit/components/downloads/public/nsIDownload.idl; /cvsroot/mozilla/toolkit/components/downloads/public/nsIDownload.idl,v <-- nsIDownload.idl new revision: 1.21; previous revision: 1.20 done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Comment 32•17 years ago
|
||
Attachment #281412 -
Attachment is obsolete: true
Assignee | ||
Comment 33•17 years ago
|
||
Comment on attachment 282875 [details] [diff] [review] followup v1.2 This bug is fixed, so requesting a1.9? from bug 398218.
Attachment #282875 -
Flags: approval1.9?
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•