Closed Bug 289540 Opened 19 years ago Closed 17 years ago

download manager takes only one listener

Categories

(Toolkit :: Downloads API, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: myk, Assigned: sdwilsh)

Details

Attachments

(2 files)

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.
this isnt a bug its a feature request.
(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
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?
Flags: blocking1.8b4? → blocking1.8b4-
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...
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!)
- Javascript Component (Download statusbar specific)
I think this would be important to get into Firefox 3.  Setting flags accordingly.
Flags: blocking-firefox3?
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]
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.
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...  
Attached patch v1.0Splinter Review
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++.
Attachment #265893 - Flags: review?(mconnor)
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+
(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
(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
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!
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
Flags: in-testsuite?
This may already be in the testsuite - at least partially.  What I don't call is removeListener, but that could be an easy fix.
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?
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.
(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.

(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.
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. :)
A null request should never happen here...
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
a lot of our tests use more than one listener, so in-testsuite+
Flags: in-testsuite? → in-testsuite+
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.