darin introduced nsITransfer in the ftp upload patch, and the methods on it suffice for exthandler (which, really, only requires the init method). note that this is the only user of nsIDownload in "gecko". (the other one is in xpfe/ - contentAreaUtils (for which nsITransfer suffices), and download-manager) and of course there's similar code in the embedding apps and firefox, which could likely also use nsITransfer. hence, I suggest moving nsIDownload into download-manager.

Sounds like a plan to me.

Also, nsITransfer should inherit from nsIWebProgressListener. It probably only needs methods/attributes init, listener, observer - the rest can stay on nsIDownload. it should get its own idl file.

note to myself <biesi> make sure that nsExternalHelperAppService.cpp does not use nsIDownload, only nsITransfer

>the rest can stay on nsIDownload. or really, "move to nsIDownload"

first patch. It works properly as far as I can tell. Beyond the matter of this bug the patch also makes displayName readonly, and changes nsIProgressDialog to inherit from nsIDownload (nsITransfer was not enough). I'll make separate bug for nsIDownload move from ./uriloader/base into download-manager.

Comment on attachment 169341 [details] [diff] [review] first patch uriloader/base/nsITransfer.idl +interface nsILocalFile; this forward decl seems unnecessary now... + * INTERFACES THAT NEED TO BE IMPLEMENTED: + * nsITransfer + * nsIDownload actually nsIDownload no longer needs to be implemented mailnews/base/src/nsMessenger.cpp + mWebProgressListener = do_QueryInterface(tr); since nsITransfer now inherits from nsIWPL, it seems like you could just do: mWebProgressListener = tr; right? same in uriloader/exthandler/nsExternalHelperAppService.cpp with: + listener = do_QueryInterface(tr); but maybe it would be better to make mWebProgressListener a comptr<nsITransfer>? that would save a few QIs. that nsIProgressDialog inherits from nsIDownload is kinda suboptimal, since it's also used for uploads, but I don't think that's much of a problem. thanks a lot for making this patch! r- for now, but with the above changes this looks good

(In reply to comment #6) > that nsIProgressDialog inherits from nsIDownload is kinda suboptimal, since > it's also used for uploads, but I don't think that's much of a problem. Do you have any ideas about better inheritance here?

not offhand, no...

fixes issues from Comment 6 beside mWebProgressListener in nsExternalHelperAppService.

Comment on attachment 169673 [details] [diff] [review] second patch uriloader/base/Makefile.in indentation is wrong (but you know this already) uriloader/base/nsIDownload.idl + /** * The source of the transfer. indentation of /** is wrong toolkit/components/downloads/src/nsDownloadManager.cpp xpfe needs no changes?

deals with cocoa, camino, powerplant and xpfe. Biesi: I can make the const PRUnichar* -> const nsAString* as another bug, ok?

Comment on attachment 169943 [details] [diff] [review] third patch >Index: uriloader/base/nsITransfer.idl >+ * started, and Init will be called on it and an observer will be set. s/Init/nsITransfer::init/ >+ * INTERFACES THAT NEED TO BE IMPLEMENTED: s/NEED TO/MUST/ >+#define NS_DOWNLOAD_CONTRACTID "@mozilla.org/download;1" So, there may be only one class (nsIFactory) registered under this ContractID? Should we think about supporting multiple "global" download observers? (e.g., maybe use a category instead?) Also, why is the word "download" being used here instead of "transfer"? Should the registered class also implement nsIDownload? >Index: uriloader/base/nsIDownload.idl >+ /** >+ * The amount of kbytes downloaded so far. >+ */ >+ readonly attribute PRUint64 amountTransferred; amountTransferred still feels like something that could be on nsITransfer, but ok. I suppose these are just informational fields that the uriloader never really needs to access? do you intend to eventually move nsIDownload out of the uriloader module? >Index: browser/base/content/contentAreaUtils.js >+ var tr = Components.classes["@mozilla.org/download;1"].createInstance(Components.interfaces.nsITransfer); funny that this ContractID mentions "download" when it should maybe be "transfer" instead. this would be our opportunity to change the ContractID since we are changing the interfaces. or maybe the ContractID is fine if we are saying that these transfers are only downloads. >Index: uriloader/exthandler/nsExternalHelperAppService.cpp >+ nsCOMPtr<nsITransfer> tr(do_QueryInterface(mWebProgressListener)); Since nsITransfer inherits from nsIWebProgressListener, perhaps it would make sense to change mWebProgressListener to mTransfer to avoid the QI here (and perhaps in other places). >Index: toolkit/components/downloads/src/nsDownloadManager.cpp >+ internalDownload->SetDisplayName(displayName.get()); >+nsDownload::SetDisplayName(const PRUnichar* aDisplayName) can this be changed to take a |const nsAString&| instead? sr=darin (nothing major to prevent this from landing, but I'd appreciate it if you could respond to my comments here by either rev'ing the patch or filing a followup bug.)

(In reply to comment #12) > >+#define NS_DOWNLOAD_CONTRACTID "@mozilla.org/download;1" > > So, there may be only one class (nsIFactory) registered under this > ContractID? well, that's what our current architecture is... I wouldn't mind extending this (I think this could even be used to provide a hook for external dl managers) > Should the registered class also implement nsIDownload? I want to move nsIDownload into xpfe/toolkit, and freeze nsITransfer. that'd make nsITransfer an embedding interface, and nsIDownload nothing an embeddor should know about. > amountTransferred still feels like something that could be on > nsITransfer, but ok. I suppose these are just informational > fields that the uriloader never really needs to access? do > you intend to eventually move nsIDownload out of the uriloader > module? and per the above, I don't really want to make embeddors implement something that nobody will ever ask for. that said. an additional variable may not hurt, so if you want it there, it could be put there. > funny that this ContractID mentions "download" when it should maybe > be "transfer" instead. this would be our opportunity to change the > ContractID since we are changing the interfaces. > > or maybe the ContractID is fine if we are saying that these transfers > are only downloads. that's what I was thinking (that it's only for downloads); we can change it if you want though. > >Index: uriloader/exthandler/nsExternalHelperAppService.cpp > Since nsITransfer inherits from nsIWebProgressListener, perhaps it > would make sense to change mWebProgressListener to mTransfer to > avoid the QI here (and perhaps in other places). That's what I thought too, but it doesn't work. The helper app dialog can be the listener here, via nsIHelperAppLauncher::setWebProgressListener; and it does not implement nsITransfer (unless we want to require that...)

(In reply to comment #12) > >+nsDownload::SetDisplayName(const PRUnichar* aDisplayName) > > can this be changed to take a |const nsAString&| instead? Yes. But I really prefer to do this in separate patch because there are probably more places where we can make this switch. But if you require this switch here, I will make it.

> Yes. But I really prefer to do this in separate patch because there are probably > more places where we can make this switch. But if you require this switch here, > I will make it. It's up to you, but I figured that since you were changing all of the SetDisplayName functions that it might make sense to include another change.

fixes two comment issues.

this patch is required in addition to the above one to fix xpfe build bustage

I checked in this patch. toolkit's nsDownloadProxy needed similar changes as xpfe's. this was even a codesize win: Zdiff:-764 mZdiff:-40 (from luna, SeaMonkey)

This is needed to fix the camino bustage (with the change in this bug, the class in question ended up with ambiguous inheritance from nsIWebProgressListener).

ugh. I feel very ashamed. Sorry guys for giving you additional work because of my lack of knowledge

>Should we think about supporting multiple "global" >download observers? (e.g., maybe use a category instead?) this is now Bug 277129

and we have also bug 277130 for nsIDownload move and bug 277140 for const PRUnichar* -> const nsAString*