290648 - make nsITransfer::init take an nsICancelable, and remove the observer attribute

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, enhancement, P1)

Description

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

it sucks to have to have two codepaths in nsITransfer implementations to cancel
the download. instead, we can provide an nsICancelable which can be used to
cancel the dl.


note: this will require changes for embeddors. needs removal of implementation
of SetObserver, and a signature change of their nsITransfer::init
implementation, and a change of the type of the object being stored. also,
replacement of CancelSave() and Cancel() calls with Cancel(NS_BINDING_ABORTED)
or another failure code.

note: for the patch I will attach, if webbrowserpersist is used, callers will
have to set the transfer as the progresslistener themselves.

Comment 1

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

this should be the final API change to nsITransfer before freezing it. it would
be nice to get it done before 1.8beta2. nsITransfer is implemented by all
embeddors who want their own progress dialog for downloads (that should be
basically all of them, I assume).

Comment 2

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

Attachment: patch

patch for toolkit dl manager will follow tomorrow

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

>Index: embedding/browser/powerplant/source/UDownload.cpp
> NS_IMETHODIMP CDownload::OnProgressChange(nsIWebProgress 

>-        if (mHelperAppLauncher)
>-            mHelperAppLauncher->Cancel(NS_BINDING_ABORTED);
>-        else if (aRequest)
>+        if (aRequest)
>             aRequest->Cancel(NS_BINDING_ABORTED);

What about calling Cancel() on mCancelable?  Don't we want to do that?	The
code used to....

>Index: embedding/components/ui/progressDlg/nsIProgressDialog.idl

>+  attribute nsIObserver observer;

Document this?	Also, it's not indented like the line right above it... fix
them to be consistent?

>Index: toolkit/content/contentAreaUtils.js

>     tr.init(aSniffer.uri, persistArgs.target, null, null, null, persist);

Init() should take "" as the third arg, no?

>     tr.init(source, persistArgs.target, null, null, null, persist);

Same.

>Index: uriloader/base/nsIDownload.idl

>+     * Object that can be used to cancel the download.
>      */
>-    readonly attribute nsIWebBrowserPersist persist;
>+    readonly attribute nsICancelable cancelable;

So is this guaranteed to be non-null?  If so, are we actually enforcing that? 
If not, document?

>Index: uriloader/base/nsITransfer.idl

>+     * @param aCancelable An object that can be used to abort the download.

Is this allowed to be null?

Also, you're changing whether this method adds the nsITransfer as a progress
listener to aCancelable (which I realize has no progress listeners).  It may be
worth having a note about that; say that this method will just hold a reference
to aCancelable until the download completes (this last is also relevant for the
whole cycle-breaking thing).

> %{C++
> /**
>  * A component with this contract ID will be created each time a download is

Want to add a note here that this should move into nsEmbedCID when the
interface is frozen?

>Index: xpfe/communicator/resources/content/contentAreaUtils.js

>     tr.init((aChosenData ? aChosenData.uri : fileInfo.uri),
>             persistArgs.target, null, null, null, persist);

"" for the third arg.

>     tr.init((aChosenData ? aChosenData.uri : source),
>             persistArgs.target, null, null, null, persist);

And here.

>Index: xpfe/components/download-manager/public/nsIDownloadManager.idl

document startTime?

>+  * @param aCancelable Object that can be used to cancel the 
download

Is this allowed to be null?

>Index: xpfe/components/download-manager/src/nsDownloadManager.cpp

> nsDownload::Init(nsIURI* aSource,

>   NS_WARNING("Huh...how did we get here?!");

Want to make that a NS_NOTREACHED instead?

r=bzbarsky with those fixed.

Comment 4

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

>>+    readonly attribute nsICancelable cancelable;
>So is this guaranteed to be non-null?  If so, are we actually enforcing that? 
>If not, document?

hm... actually... this will be null once the download is finished. other than
that it should always be nonnull, since exthandler always passes a nonnull
pointer to it.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Document that.  It should be clear to people _implementing_ the interface that
they have to hold on the cancelable they're passed....

Comment 6

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

Attachment: patch v2

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

+nsDownload::GetCancelable(nsICancelable** aCancelable) has three lines after
it ... nix two, please

Comment 8

[Darin Fisher]

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

do we have to worry about reference cycles between the nsIObserver and the
nsIProgressDialog?  sometimes observers are held as weak references for this
reason.  Do we have reason to worry in this case?

Perhaps there is a similar question to be answered in regards to nsITransfer
and its nsICancelable.

nit: I don't like documenting that NS_TRANSFER_CONTRACTID will be moved to
nsEmbedCID.h when the interfaces are frozen.  How about (XXX move this to some
appropriate SDK header file when interfaces are frozen) or something like that
instead.

Comment 9

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

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

nsDownload does indeed seem to be leaked due to that cycle.

as for nsITransfer, the IDL says about nsICancelable:
+     * 		   Implementations are expected to hold a strong
+     * 		   reference to this object until the download is
+     * 		   finished, at which point they should release the
+     * 		   reference.

and the implementations do that. that should be ok, I think.

Comment 10

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

Attachment: patch v3

this should do it. seamonkey dl mgr now sets the observer to null and drops the
reference to the dialog when the download is complete. (this is the only
product that uses nsIProgressDialog)

Comment 11

[Darin Fisher]

Comment on attachment 181209 [details] [diff] [review]
patch v3

sr=darin

Comment 12

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

Comment on attachment 181209 [details] [diff] [review]
patch v3

this is the (hopefully) final API change to nsITransfer before freezing. I
tested and made sure that downloads can still be canceled, and that progress
reporting still works, which should be the only things affected by this patch.

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 181209 [details] [diff] [review]
patch v3

a=asa

Comment 14

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

checked in for 1.8b2.

Comment 15

[Peter van der Woude [:Peter6]]

This appears to have caused Bug 291846