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.

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

patch for toolkit dl manager will follow tomorrow

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.

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

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

Comment on attachment 180996 [details] [diff] [review] patch v2 +nsDownload::GetCancelable(nsICancelable** aCancelable) has three lines after it ... nix two, please

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

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 on attachment 181209 [details] [diff] [review] patch v3 sr=darin

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 on attachment 181209 [details] [diff] [review] patch v3 a=asa

checked in for 1.8b2.

This appears to have caused Bug 291846