Closed Bug 239111 Opened 20 years ago Closed 20 years ago

Rework Pause/Resume handling in Download Manager


(SeaMonkey :: Download & File Handling, enhancement, P1)



(Not tracked)



(Reporter: Biesinger, Assigned: Biesinger)




(1 file, 1 obsolete file)

o) Implement nsIObserver on nsDownload
o) Set that object as the progress dialog's observer
(this is just cleanup)

o) Instead of pausing the request using nsIRequest directly in the progress
dialog, have it handled by nsDownload
o) Implement new methods pauseDownload and resumeDownload on nsIDownloadManager

preliminary work for cross-session download resuming.
Attached patch patch (obsolete) — Splinter Review
Assignee: download-manager → cbiesinger
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha
Attachment #145036 - Flags: review?(
Comment on attachment 145036 [details] [diff] [review]

>+// nsIWebProgressListener
Wrong interface, methinks :-)

>+nsDownload::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* aData)
>+  if (strcmp(aTopic, "oncancel") == 0) {
>+    SetDialog(nsnull);
>+    nsAutoString path;
>+    nsresult rv = mTarget->GetPath(path);
>+    // XXX why can't nsDownload cancel itself without help from the dl manager?
>+    if (NS_SUCCEEDED(rv))
>+      mDownloadManager->CancelDownload(NS_ConvertUCS2toUTF8(path));
>+  }
Please comment why you ignore the return value. I assume you plan to return it,
which is why you didn't inline rv?

>+  else if (strcmp(aTopic, "onpause") == 0) {
>+    return Suspend();
>+  }
>+  else if (strcmp(aTopic, "onresume") == 0) {
>+    return Resume();
>+  }
Please put these early returns first, so you don't need to use else.
>+  return NS_OK;

>+		   public nsIObserver
Spaces, not tabs, please.

>             var string = pausing ? "resume" : "pause";
>             this.dialogElement( "pauseResume" ).label = this.getString(string);
>-            // If we have a request, suspend/resume it.
>-            if ( this.request ) {
>+            // If we have an observer, tell it to suspend/resume
>+            if ( ) {
>                 if ( pausing ) {
>-                    this.request.suspend();
>+           this, "onpause", "" );
>                 } else {
>-                    this.request.resume();
>+           this, "onresume", "" );
>                 }
>             } this, pausing ? "onpause" : "onresume", null );
Attachment #145036 - Flags: review?( → review-
Comment on attachment 145036 [details] [diff] [review]

>+nsDownloadManager::PauseDownload(nsIDownload* aDownload)
>+  return NS_STATIC_CAST(nsDownload*, aDownload)->Suspend();
Oh, and a null check here would be nice :-)
(In reply to comment #3)
> >+nsDownloadManager::PauseDownload(nsIDownload* aDownload)
> Oh, and a null check here would be nice :-)

Hmm, why? People should not pass null to this function. but if you insist...
Attached patch patch v2Splinter Review
I added an assertion that aDownload is not null...
Attachment #145036 - Attachment is obsolete: true
Attachment #145556 - Flags: review?(
Comment on attachment 145556 [details] [diff] [review]
patch v2

>+nsDownloadManager::PauseDownload(nsIDownload* aDownload)
>+  NS_ASSERTION(aDownload, "Unexpected null download object");
>+  return NS_STATIC_CAST(nsDownload*, aDownload)->Suspend();
Personally I'd prefer this to say NS_ENSURE_ARG_POINTER(aDownload); [or if
(!aDownload) return NS_ERROR_NULL_POINTER; if you want to save a few bytes in
debug] because as far as I can tell no public API should crash even ones only
available using UniversalXPConnect privileges. No doubt your superreviewer will
correct me if I'm wrong and this construct is in fact permissible.
Attachment #145556 - Flags: review?( → review+
Attachment #145556 - Flags: superreview?(darin)
Is this targeting 1.7 final?  If not, then I might need to punt until after I
finish all of my 1.7 tasks, which could be a couple weeks.
darin: no... it's targeted for 1.8alpha. I'll ask someone else for sr in that
case, thanks for the notice.
Attachment #145556 - Flags: superreview?(darin) → superreview?(bzbarsky)
I'm afraid I'll also not be able to get to this any time in the near future; I'm
finishing up the srs that have been in my queue for a while, but not doing any
more srs until at least end of summer.
Attachment #145556 - Flags: superreview?(bzbarsky) → superreview?(tor)
Attachment #145556 - Flags: superreview?(tor) → superreview?(roc)
Yeah, do NS_ENSURE_ARG_POINTER(aDownload);

+            if ( ) {
+       this, pausing ? "onpause" : "onresume" ,
"" );

is this code going to be replaced by a call to pauseDownload/resumeDownload?
(In reply to comment #10)
> +            if ( ) {
> +       this, pausing ? "onpause" : "onresume" ,
> "" );
> is this code going to be replaced by a call to pauseDownload/resumeDownload?

this code doesn't know anything about a download manager... so, no, it is not
going to do that. that file is also used for ftp upload now... and that does not
use the download manager.
Well, it's somewhat ugly to cause something to happen by directly notifying an
observer. I think observers are generally just supposed to ... observe.
It's not really an observer, it's really a convenience interface.

If you prefer we could cheat and unwrap the JS object...
Comment on attachment 145556 [details] [diff] [review]
patch v2

Attachment #145556 - Flags: superreview?(roc) → superreview+
checked in, using NS_ENSURE_ARG_POINTER
Closed: 20 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.