399817 - Resume downloads marked as auto-resume when starting download manager

Status: VERIFIED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[Ed Lee :Mardak]

Attachment: v1

Adds a resume/retry method which we could potentially use for ResumeDownload as well.

A separate bug should be filed to init the download manager when starting the app.. similar to session restore. So if we always restart dlmgr, we won't the code in session restore either.

Comment 1

[Shawn Wilsher :sdwilsh]

Comment on attachment 284883 [details] [diff] [review]
v1

> nsresult
>+nsDownloadManager::ResumeRetry(nsDownload *aDl)
>+{
>+  // Keep a reference incase we need to cancel the download
nit: in case

>+  nsRefPtr<nsDownload> dl = aDl;
>+
>+  // Try to resume the active download
>+  nsresult rv = dl->Resume();
>+
>+  // If not, try to retry the download
>+  if (NS_FAILED(rv)) {
>+    // First cancel the download so it's no longer active
>+    rv = CancelDownload(dl->mID);
>+
>+    // Then retry it
>+    if (NS_SUCCEEDED(rv))
>+      rv = RetryDownload(dl->mID);
In what case would Cancel fail exactly?

>+nsresult
>+nsDownloadManager::ResumeAllDownloads(PRBool aCheckAuto)
>+{
>+  nsresult ret = NS_OK;
>+  for (PRInt32 i = mCurrentDownloads.Count() - 1; i >= 0; --i) {
>+    nsRefPtr<nsDownload> dl = mCurrentDownloads[i];
>+
>+    // Resume everything or only those that should auto resume
>+    if (!aCheckAuto || dl->ShouldAutoResume()) {
I'd like it if we just returned early if aCheckAuto is false (this logic is a bit confusing now, although technically correct.

>+    "WHERE (state = ?1 AND LENGTH(entityID) > 0) "
>+      "OR autoStart != 0"), getter_AddRefs(stmt));
I would appreciate it if you bound the enum value here for code readability.

>   /**
>+   * First try to resume the download and then retry it if necessary.
>+   */
nit: "First try to resume the download, and if that fails, retry it."
also, @param aDl...
>+  nsresult ResumeRetry(nsDownload *aDl);

Comment 2

[Ed Lee :Mardak]

Attachment: v2

(In reply to comment #1)
> >+  // Keep a reference incase we need to cancel the download
> nit: in case
> >   /**
> >+   * First try to resume the download and then retry it if necessary.
> >+   */
> nit: "First try to resume the download, and if that fails, retry it."
> also, @param aDl...
Fixed and added other @param comments.

> >+    // First cancel the download so it's no longer active
> >+    rv = CancelDownload(dl->mID);
> >+
> >+    // Then retry it
> >+    if (NS_SUCCEEDED(rv))
> >+      rv = RetryDownload(dl->mID);
> In what case would Cancel fail exactly?
Well, CancelDownload can return failure... if SetState fails, which is important to succeed because that method will eventually remove the download from mCurrentDownloads.

> >+    // Resume everything or only those that should auto resume
> >+    if (!aCheckAuto || dl->ShouldAutoResume()) {
> I'd like it if we just returned early if aCheckAuto is false (this logic is a
> bit confusing now, although technically correct.
It might be doing something other than what you're expecting actually.. I've updated the comments here and @param.

> >+    "WHERE (state = ?1 AND LENGTH(entityID) > 0) "
> >+      "OR autoStart != 0"), getter_AddRefs(stmt));
> I would appreciate it if you bound the enum value here for code readability.
Bound to DONT_RESUME.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 284891 [details] [diff] [review]
v2

>   /**
>+   * First try to resume the download, and if that fails, retry it.
>+   *
>+   * @param aDl: The download to resume and/or retry.
nit: that is not a javadoc style comment.  Lose the : please.
>+   */
>+  nsresult ResumeRetry(nsDownload *aDl);
>+
>+  /**
>    * Pause all active downloads and indicate if they should try to auto-resume
>    * when the download manager starts again.
>    *
>@@ -163,6 +170,15 @@ protected:
>    *                     marked as auto-resume.
>    */
>   nsresult PauseAllDownloads(PRBool aAutoResume);
>+
>+  /**
>+   * Resume all paused downloads unless we're only supposed to do the automatic
>+   * ones; in that case, try to retry them as well if resuming doesn't work.
>+   *
>+   * @param aCheckAuto: If false, all downloads will be resumed; otherwise,
>+   *                    only those that are marked as auto-resume will resume.
ditto
>+   */
>+  nsresult ResumeAllDownloads(PRBool aCheckAuto);
OK, this is still bloody confusing, but I get it now, and we need to rework this.  How about |nsresult ResumeAllDownloads(PRBool aOnlyResumeAuto);|, which isn't great still, but it's way easier to understand that what you currently have.

Comment 4

[Ed Lee :Mardak]

Attachment: v2.1

(In reply to comment #3)
> >+   * @param aDl: The download to resume and/or retry.
> nit: that is not a javadoc style comment.  Lose the : please.
> >+   * @param aCheckAuto: If false, all downloads will be resumed; otherwise,
> >+   *                    only those that are marked as auto-resume will resume.
> ditto
Lost.

> >+  nsresult ResumeAllDownloads(PRBool aCheckAuto);
> OK, this is still bloody confusing, but I get it now, and we need to rework
> this.  How about |nsresult ResumeAllDownloads(PRBool aOnlyResumeAuto);|, which
> isn't great still, but it's way easier to understand that what you currently
> have.
I've changed it to aResumeAll so that there isn't the negation in front of the flag. I think one source of confusion is that I'm adding this flag when I don't need to.. but for both PauseAll and ResumeAll, we could potentially use them for a UI-level pause all and resume all as well.. both of which shouldn't touch the auto-resume stuff.

Comment 5

[Shawn Wilsher :sdwilsh]

Can you file follow-up(s) to add those methods to the interface at least - we don't need UI, but can provide it in case an add-on wants to.

Comment 6

[Ed Lee :Mardak]

The interface should only have void PauseAllDownloads() and void ResumeAllDownloads() and we would actually implement them as..

NS_IMETHODIMP
nsDownloadManager::ResumeAllDownloads()
{
  return ResumeAllDownloads(PR_TRUE);
}

because the interface doesn't need to specify anything about auto-resume.. ?

Comment 7

[Shawn Wilsher :sdwilsh]

Sounds about right to me.  Trivial to add, and gives add-on authors something nice to work with.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 284971 [details] [diff] [review]
v2.1

>+    if (aResumeAll || dl->ShouldAutoResume()) {
>+      // Reset auto-resume before retrying so that it gets into the DB
could you give a slightly more elaborate comment as to why you set this.  It's not immediately obvious, so the comment will be nice.

r=sdwilsh

Comment 9

[Ed Lee :Mardak]

Attachment: v2.2

>+    if (aResumeAll || dl->ShouldAutoResume()) {
>+      // Reset auto-resume before retrying so that it gets into the DB
could you give a slightly more elaborate comment as to why you set this.  It's
not immediately obvious, so the comment will be nice.
Elaborated. Mentioned how/when/where it gets set and why it gets set.

Comment 10

[Ed Lee :Mardak]

I accidentally created a testcase for this earlier by setting a download to autoresume=1 and start the download manager service.. the last update time will be different from the entry originally in the .sqlite file..

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.143; previous revision: 1.142
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.55; previous revision: 1.54
done

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007120304 Minefield/3.0b2pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007120304 Minefield/3.0b2pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007120305 Minefield/3.0b2pre

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=5021
https://litmus.mozilla.org/show_test.cgi?id=4992

in-litmus+