Closed
Bug 241082
Opened 21 years ago
Closed 20 years ago
exthandler should require just nsITransfer, not nsIDownload
Categories
(Core Graveyard :: File Handling, defect)
Core Graveyard
File Handling
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Biesinger, Assigned: zbraniecki)
References
Details
Attachments
(3 files, 3 obsolete files)
31.82 KB,
patch
|
Details | Diff | Splinter Review | |
1.64 KB,
patch
|
Details | Diff | Splinter Review | |
2.17 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 2•20 years ago
|
||
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.
Assignee | ||
Comment 3•20 years ago
|
||
note to myself <biesi> make sure that nsExternalHelperAppService.cpp does not use nsIDownload, only nsITransfer
Assignee: file-handling → gandalf
Reporter | ||
Comment 4•20 years ago
|
||
>the rest can stay on nsIDownload.
or really, "move to nsIDownload"
Assignee | ||
Comment 5•20 years ago
|
||
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.
Attachment #169341 -
Flags: superreview?(darin)
Attachment #169341 -
Flags: review?(cbiesinger)
Reporter | ||
Comment 6•20 years ago
|
||
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
Attachment #169341 -
Flags: review?(cbiesinger) → review-
Assignee | ||
Comment 7•20 years ago
|
||
(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?
Assignee | ||
Comment 9•20 years ago
|
||
fixes issues from Comment 6 beside mWebProgressListener in nsExternalHelperAppService.
Attachment #169341 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #169673 -
Flags: review?(cbiesinger)
Reporter | ||
Comment 10•20 years ago
|
||
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?
Attachment #169673 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Comment 11•20 years ago
|
||
deals with cocoa, camino, powerplant and xpfe. Biesi: I can make the const PRUnichar* -> const nsAString* as another bug, ok?
Attachment #169673 -
Attachment is obsolete: true
Attachment #169943 -
Flags: review?(cbiesinger)
Reporter | ||
Updated•20 years ago
|
Attachment #169943 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Updated•20 years ago
|
Attachment #169943 -
Flags: superreview?(darin)
Comment 12•20 years ago
|
||
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.)
Attachment #169943 -
Flags: superreview?(darin) → superreview+
Reporter | ||
Comment 13•20 years ago
|
||
(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...)
Assignee | ||
Comment 14•20 years ago
|
||
(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.
Updated•20 years ago
|
Attachment #169341 -
Flags: superreview?(darin)
Comment 15•20 years ago
|
||
> 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.
Assignee | ||
Comment 16•20 years ago
|
||
fixes two comment issues.
Attachment #169943 -
Attachment is obsolete: true
Reporter | ||
Comment 17•20 years ago
|
||
this patch is required in addition to the above one to fix xpfe build bustage
Reporter | ||
Comment 18•20 years ago
|
||
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)
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
![]() |
||
Comment 19•20 years ago
|
||
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).
Assignee | ||
Comment 20•20 years ago
|
||
ugh. I feel very ashamed. Sorry guys for giving you additional work because of my lack of knowledge
Reporter | ||
Comment 21•20 years ago
|
||
>Should we think about supporting multiple "global" >download observers? (e.g., maybe use a category instead?) this is now Bug 277129
Assignee | ||
Comment 22•20 years ago
|
||
and we have also bug 277130 for nsIDownload move and bug 277140 for const PRUnichar* -> const nsAString*
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•