Closed Bug 382825 Opened 13 years ago Closed 13 years ago

Add a retryDownload method to the download manager backend

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha6

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 1 obsolete file)

There are a few places where we want to retry downloads, and we end up having to re-implement it in every place.  This would be a useful method to have as a part of nsIDownloadManager.
Flags: blocking-firefox3?
Blocks: 382513
One question is "do we want to figure out the MIMEInfo for this?" since it is not stored in the database.  It wouldn't be terribly difficult to do (in fact, I'd almost say do that with nsDownload if mMIMEInfo is null).  Thoughts on the matter?
Attached patch v1.0 (obsolete) — Splinter Review
This turned out to be a lot more work than expected :/

We've got the idl changes, back-end, and tests all in one patch (in a semi-sane order too!)
Attachment #266964 - Flags: review?(cbiesinger)
Comment on attachment 266964 [details] [diff] [review]
v1.0

>Index: toolkit/components/downloads/test/unit/test_bug_382825.js

>+function run_test()

>+  try {
>+    var thread = Cc["@mozilla.org/thread-manager;1"]
>+                 .getService().currentThread;
>+
>+    while (!httpserv.isStopped())
>+      thread.processNextEvent(true);
>+
>+    // get rid of any pending requests
>+    while (thread.hasPendingEvents())
>+      thread.processNextEvent(true);
>+  } catch (e) {
>+    print(e);
>+  }

This might make it easier to write the code, but it's hardly good style to nest event processing loops this way (xpcshell already includes a loop like this that runs until every do_test_pending call is matched by a do_test_finished call).  As this may not be the right place/time to address this, I think you should rethrow if the event loop processing code throws an exception.
Comment on attachment 266964 [details] [diff] [review]
v1.0

In GetDownloadFromDB, I don't think you should set mMaxBytes to 0 when it's unknown. Set it to -1 instead.

should retryDownload make sure that it's not an XPI install? (or would we handle that correctly?)

The IDL comment should mention that it works for cancelled downloads too, not just failed ones

why are you bypassing the cache?

not great that you don't send a referrer but I guess you have no choice

toolkit/components/downloads/test/unit/test_bug_382825.js
+    do_throw("Hey!  We expect to get an excpetion with this!");

typo here (should be exception)
Attachment #266964 - Flags: review?(cbiesinger) → review+
(In reply to comment #4)
> (From update of attachment 266964 [details] [diff] [review])
> In GetDownloadFromDB, I don't think you should set mMaxBytes to 0 when it's
> unknown. Set it to -1 instead.
While I agree with you, the IDL says this:
    /**
     * The size of file in bytes.
     * Unknown size is represented by 0.
     */
    readonly attribute PRUint64 size;
So, I could update it if you want, but I wasn't about to change our behavior over this.

> should retryDownload make sure that it's not an XPI install? (or would we
> handle that correctly?)
We would if only I actually stored the download type.  I don't right now, and that's a problem (I think), but should probably be addressed in a follow-up bug.  In fact, the whole bit with installs is just odd, and there's some strange behavior that I still cannot figure out with the old code.

> The IDL comment should mention that it works for cancelled downloads too, not
> just failed ones
Fair enough.

> why are you bypassing the cache?
I probably shouldn't be...

> not great that you don't send a referrer but I guess you have no choice
I think that is one of our possible changes for the future.

> typo here (should be exception)
oops
Attached patch v1.1Splinter Review
Addresses comments, and gets UI file I forgot to grab last time.

Waldo, I'd like to address that in a new bug since that will require a decent re-factoring of my tests cases for the DM.  I did remove the try catch block though (we had put it in trying to figure out why the tinderboxen were orange in the past).
Attachment #266964 - Attachment is obsolete: true
Attachment #267219 - Flags: review?(cbiesinger)
Attachment #267219 - Flags: review?(cbiesinger) → review+
Checking in uriloader/base/nsIDownload.idl;
new revision: 1.19; previous revision: 1.18
Checking in toolkit/components/downloads/public/nsIDownloadManager.idl;
new revision: 1.11; previous revision: 1.10
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
new revision: 1.26; previous revision: 1.25
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.83; previous revision: 1.82
Checking in toolkit/mozapps/downloads/content/downloads.js;
new revision: 1.60; previous revision: 1.59
Checking in toolkit/components/downloads/test/unit/head_download_manager.js;
new revision: 1.3; previous revision: 1.2
Checking in toolkit/components/downloads/test/unit/test_bug_382825.js;
initial revision: 1.1
Checking in toolkit/components/downloads/test/unit/test_download_manager.js;
new revision: 1.12; previous revision: 1.11
Checking in toolkit/components/downloads/test/unit/test_download_manager_migration.js;
new revision: 1.5; previous revision: 1.4

Filing follow-up bugs now.  I'll post them here momentarily.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Unit tests: Bug 383223

XPInstall stuff: Bug 383224
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Blocks: 377477
Depends on: 391869
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Product: Firefox → Toolkit
Depends on: 463855
No longer depends on: 463855
You need to log in before you can comment on or make changes to this bug.