382825 - Add a retryDownload method to the download manager backend

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Shawn Wilsher :sdwilsh]

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.

Comment 1

[Shawn Wilsher :sdwilsh]

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?

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

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!)

Comment 3

[Jeff Walden [:Waldo]]

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 4

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

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)

Comment 5

[Shawn Wilsher :sdwilsh]

(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

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

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).

Comment 7

[Shawn Wilsher :sdwilsh]

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.

Comment 8

[Shawn Wilsher :sdwilsh]

Unit tests: Bug 383223

XPInstall stuff: Bug 383224