239111 - Rework Pause/Resume handling in Download Manager

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: patch

Comment 2

[[e-mail]]

Comment on attachment 145036 [details] [diff] [review]
patch

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

>+NS_IMETHODIMP
>+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 ( this.observer ) {
>                 if ( pausing ) {
>-                    this.request.suspend();
>+                    this.observer.observe( this, "onpause", "" );
>                 } else {
>-                    this.request.resume();
>+                    this.observer.observe( this, "onresume", "" );
>                 }
>             }
this.observer.observe( this, pausing ? "onpause" : "onresume", null );

Comment 3

[[e-mail]]

Comment on attachment 145036 [details] [diff] [review]
patch

> NS_IMETHODIMP
>+nsDownloadManager::PauseDownload(nsIDownload* aDownload)
>+{
>+  return NS_STATIC_CAST(nsDownload*, aDownload)->Suspend();
>+}
Oh, and a null check here would be nice :-)

Comment 4

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

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

Comment 5

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

Attachment: patch v2

I added an assertion that aDownload is not null...

Comment 6

[[e-mail]]

Comment on attachment 145556 [details] [diff] [review]
patch v2

> NS_IMETHODIMP
>+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.

Comment 7

[Darin Fisher]

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.

Comment 8

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

darin: no... it's targeted for 1.8alpha. I'll ask someone else for sr in that
case, thanks for the notice.

Comment 9

[Boris Zbarsky [: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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, do NS_ENSURE_ARG_POINTER(aDownload);

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

is this code going to be replaced by a call to pauseDownload/resumeDownload?

Comment 11

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

(In reply to comment #10)
> +            if ( this.observer ) {
> +                this.observer.observe( 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.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well, it's somewhat ugly to cause something to happen by directly notifying an
observer. I think observers are generally just supposed to ... observe.

Comment 13

[[e-mail]]

It's not really an observer, it's really a convenience interface.

If you prefer we could cheat and unwrap the JS object...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 145556 [details] [diff] [review]
patch v2

ok

Comment 15

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

checked in, using NS_ENSURE_ARG_POINTER