Closed Bug 394548 Opened 14 years ago Closed 14 years ago

Store download progress in database when changing states

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(4 files, 12 obsolete files)

Attached patch v1 (obsolete) — 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)
Oh, forgot to mention this was tested by querying the table with mozStorage Explorer ;)
Blocks: 394263
(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.
(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...
(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)
Attached patch v2 (obsolete) — Splinter Review
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 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+
> 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.
Attached patch v2.1 (obsolete) — Splinter Review
> >+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
Attached file v3.sqlite (obsolete) —
Same files as that of bug 377243.
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...
Attached patch v3 (obsolete) — Splinter Review
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)
Attached image screenshot of v2.1
Picture of strangeness ;)
Comment on attachment 279537 [details] [diff] [review]
v3

r=sdwilsh
Attachment #279537 - Flags: review?(comrade693+bmo) → review+
Attachment #279537 - Flags: review?(mano)
Attached patch v4 (obsolete) — Splinter Review
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)
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 on attachment 279556 [details] [diff] [review]
v4

Can you unbitrot this please?
Attachment #279556 - Flags: review?(comrade693+bmo)
Target Milestone: --- → Firefox 3 M9
Attached patch v5 (obsolete) — Splinter Review
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)
Depends on: 395537
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.
Attached patch v5.1 (obsolete) — Splinter Review
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 on attachment 280751 [details] [diff] [review]
v5.1

schema change testcase?
Attachment #280751 - Flags: review?(comrade693+bmo)
Attached patch v5.2 (obsolete) — Splinter Review
Added schema testcase.
Attachment #279405 - Attachment is obsolete: true
Attachment #280751 - Attachment is obsolete: true
Attachment #281120 - Flags: review?(comrade693+bmo)
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?
Attached file v5.sqlite
v5.sqlite for testcase.. unless you can apply the gitstyle patch which has the binary in the v5.2 patch already ;)
Attachment #281120 - Flags: approval1.9? → approval1.9+
Attached patch patch for checkin (obsolete) — Splinter Review
Unbitrot from bug 393932
Attachment #281120 - Attachment is obsolete: true
Mardak, are you going to check this in?
This bug isn't ready for checkin because it still has an unresolved dependency.
Attached patch followup v1 (obsolete) — Splinter Review
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)
Attached patch followup v1.1 (obsolete) — Splinter Review
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)
Attached patch followup v1.2Splinter Review
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)
Blocks: 382388
Blocks: 398216
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?
Blocks: 398218
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: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Attached patch as checked inSplinter Review
Attachment #281412 - Attachment is obsolete: true
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?
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.