download manager takes only one listener

RESOLVED FIXED

Status

()

Toolkit
Downloads API
--
enhancement
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: myk, Assigned: sdwilsh)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.8b5 -
blocking1.9 -
wanted1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

13 years ago
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

12 years ago
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
(Reporter)

Comment 3

12 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

12 years ago
Flags: blocking1.8b4? → blocking1.8b4-

Comment 4

10 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

10 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

10 years ago
Created attachment 263758 [details]
Download Info Broadcaster

 - Javascript Component (Download statusbar specific)
(Reporter)

Comment 7

10 years ago
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]
(Assignee)

Comment 9

10 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

10 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

10 years ago
Created attachment 265893 [details] [diff] [review]
v1.0

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

10 years ago
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+
(Assignee)

Comment 14

10 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

10 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

10 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

10 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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
(Assignee)

Comment 18

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

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

10 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.
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]
(Assignee)

Comment 25

9 years ago
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.