Closed
Bug 289540
Opened 19 years ago
Closed 17 years ago
download manager takes only one listener
Categories
(Toolkit :: Downloads API, enhancement)
Toolkit
Downloads API
Tracking
()
RESOLVED
FIXED
People
(Reporter: myk, Assigned: sdwilsh)
Details
Attachments
(2 files)
6.58 KB,
application/x-javascript
|
Details | |
14.30 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
The download manager component only registers a single listener at a time, so extensions that provide alternative interfaces to downloads can't listen for download progress without taking progress updates away from the standard download manager. It should be possible to register multiple listeners with the download manager component so that both the standard download manager and an alternative interface to it can get progress updates simultaneously.
Comment 1•19 years ago
|
||
this isnt a bug its a feature request.
Comment 2•19 years ago
|
||
(In reply to comment #1) > this isnt a bug its a feature request. And it's marked as such. What's your point?
Assignee: bugs → nobody
QA Contact: aebrahim-bmo → download.manager
Version: unspecified → Trunk
Reporter | ||
Comment 3•19 years ago
|
||
If we want to unleash the creative energies of our extension developers towards better interfaces for downloads, this bug would be important to address.
Flags: blocking1.8b4?
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4-
Comment 4•17 years ago
|
||
Hmm, is there no one interested in this? I think it would greatly ease Firefox extensions - and I guess it might help us with bug 348386 as well...
Comment 5•17 years ago
|
||
For the Download Statusbar extension I had recently written a "Download Info Broadcaster" javascript component. It took control of the one available listener and when it got a progress update it passed it to both the built-in firefox manager and my download statusbar extension. Right now it is very much specific for the extension, but it could be generalized for other purposes. Like adding the ability for other things to "register" and receive the updates. It worked/works great but I actually decided not to use it. For awhile the built-in download manager had enough problems that I felt it was necessary to use the listener and calculate everything myself. Now I can get enough (accurate) information from the nsiDownload interface that I don't need the listener anymore. http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsIDownload.idl I'll attach my "Download info broadcaster" in case anyone else wants to run with it, but I don't expect to need it for my extension (in the foreseeable future!)
Comment 6•17 years ago
|
||
- Javascript Component (Download statusbar specific)
Reporter | ||
Comment 7•17 years ago
|
||
I think this would be important to get into Firefox 3. Setting flags accordingly.
Flags: blocking-firefox3?
Comment 8•17 years ago
|
||
Not a blocker, but definitely wanted. Connor says that he's gonna assign an intern who can probably take this.
Assignee: nobody → sdwilsh
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Assignee | ||
Comment 9•17 years ago
|
||
I planned on taking this anyway, but hey, if you want to assign it to me, sure ;)
Status: NEW → ASSIGNED
I think this is edging on blocker, since the demand for DL-related add-ons is high and growing, and this makes them very hard to write effectively. I think sdwilsh will get it sorted, though, so I needn't make a federal case out of it.
Comment 11•17 years ago
|
||
Since this isn't a blocker for Ffx 3, it probably shouldn't block bug 372972. Is there another DL Manager bug that it should depend on, or does this actually stand on its own as a one-off bug/rfe? Just trying to keep track of these...
Assignee | ||
Comment 12•17 years ago
|
||
Ask and you shall receive. Of course, since mconnor volunteered me for this, he gets to review it ;) I'm not sure if this is the best way to do this, but it's simple and it works quite nicely. Patch includes fixes for UI, tests, public interface, and C++.
Assignee | ||
Updated•17 years ago
|
Attachment #265893 -
Flags: review?(mconnor)
Comment 13•17 years ago
|
||
Comment on attachment 265893 [details] [diff] [review] v1.0 >+ void NotifyListenersOnDownloadStateChange(PRInt16, nsIDownload*); >+ void NotifyListenersOnProgressChange(nsIWebProgress*, nsIRequest*, PRInt64, >+ PRInt64, PRInt64, PRInt64, nsIDownload*); >+ void NotifyListenersOnStateChange(nsIWebProgress*, nsIRequest*, PRUint32, >+ nsresult, nsIDownload*); name the args here, no? otherwise, woo, this works for me. Make sure to ping some of the download extension authors to try using the new code in a5.
Attachment #265893 -
Flags: review?(mconnor) → review+
Assignee | ||
Comment 14•17 years ago
|
||
(In reply to comment #13) > (From update of attachment 265893 [details] [diff] [review]) > >+ void NotifyListenersOnDownloadStateChange(PRInt16, nsIDownload*); > >+ void NotifyListenersOnProgressChange(nsIWebProgress*, nsIRequest*, PRInt64, > >+ PRInt64, PRInt64, PRInt64, nsIDownload*); > >+ void NotifyListenersOnStateChange(nsIWebProgress*, nsIRequest*, PRUint32, > >+ nsresult, nsIDownload*); > > name the args here, no? I could if you really want me to. > otherwise, woo, this works for me. Make sure to ping some of the download > extension authors to try using the new code in a5. I've been updating the Firefox 3 for Developers page: http://developer.mozilla.org/en/docs/Firefox_3_for_developers#Download_Manager
Comment 15•17 years ago
|
||
(In reply to comment #14) >http://developer.mozilla.org/en/docs/Firefox_3_for_developers#Download_Manager Might be more useful if you add links to the relevant bugs or to LXR/MXR or to the .idl
Assignee | ||
Comment 16•17 years ago
|
||
Hey! It's better than the password/login manager :p I did plan on adding bug links though once this bug landed. Linking the idls is a good idea too. Thanks!
Assignee | ||
Comment 17•17 years ago
|
||
Before checking in, I did add the names of the arguments in the .h file. Checking in toolkit/components/downloads/public/nsIDownloadManager.idl; new revision: 1.10; previous revision: 1.9 Checking in toolkit/components/downloads/src/nsDownloadManager.h; new revision: 1.25; previous revision: 1.24 Checking in toolkit/components/downloads/src/nsDownloadManager.cpp; new revision: 1.81; previous revision: 1.80 Checking in toolkit/components/downloads/test/unit/test_download_manager.js; new revision: 1.11; previous revision: 1.10 Checking in toolkit/mozapps/downloads/content/downloads.js; new revision: 1.58; previous revision: 1.57
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 18•17 years ago
|
||
This may already be in the testsuite - at least partially. What I don't call is removeListener, but that could be an easy fix.
Comment 19•17 years ago
|
||
I've added new documentation for nsIDownloadProgressListener but they're pretty sketchy since I don't entirely understand how some of the nsIWebProgressListener concepts carry over to it. http://developer.mozilla.org/en/docs/nsIDownloadProgressListener So I have a few questions if anyone can answer them: 1. What does the aRequest parameter mean in this case? Is it the request that initiated the download? What does it mean if it's NULL? 2. What sort of status messages are sent to onStatusChange()? 3. Is aStatus on these functions intended to be the nsIWebProgressListener-style state information, or the download status? The terms "state" and "status" are being used when talking about both, so I'm confused here. 4. What does onLocationChange() mean in the context of a download? What might cause this to be called?
Assignee | ||
Comment 20•17 years ago
|
||
I understand your confusion. The only method that differs from nsIWebProgressListener is onDownloadStateChange. Here, state refers to nsIDownload::state, which is one of the DOWLOAD_* constants in nsIDownloadManager. As for the other questions, I don't understand nsIWebProgressListener enough to answer those. Maybe biesi can.
Comment 21•17 years ago
|
||
(In reply to comment #19) > 1. What does the aRequest parameter mean in this case? Is it the request that > initiated the download? What does it mean if it's NULL? Yeah, it's the channel (nsIChannel) for the download. > 2. What sort of status messages are sent to onStatusChange()? Hm, doesn't look like that gets called at all. Should be removed I guess. > 3. Is aStatus on these functions intended to be the > nsIWebProgressListener-style state information, or the download status? The > terms "state" and "status" are being used when talking about both, so I'm > confused here. For onStateChange, it is one of the states from nsITransport and nsISocketTransport for things like "Resolving Host", "Connecting to Host", etc. There's also the download state stuff that sdwilsh explained. > 4. What does onLocationChange() mean in the context of a download? What might > cause this to be called? Doesn't look like that ever gets called, should probably be removed as well.
Assignee | ||
Comment 22•17 years ago
|
||
(In reply to comment #21) > > 2. What sort of status messages are sent to onStatusChange()? > > Hm, doesn't look like that gets called at all. Should be removed I guess. > > > 4. What does onLocationChange() mean in the context of a download? What might > > cause this to be called? > > Doesn't look like that ever gets called, should probably be removed as well. Filed Bug 393922.
Comment 23•17 years ago
|
||
Thanks all, I've made appropriate updates to the doc. Still would like to know what it means when the aRequest parameter is null though, if anyone knows. :)
Comment 24•17 years ago
|
||
A null request should never happen here...
Updated•16 years ago
|
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Assignee | ||
Comment 25•16 years ago
|
||
a lot of our tests use more than one listener, so in-testsuite+
Flags: in-testsuite? → in-testsuite+
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•