Last Comment Bug 289540 - download manager takes only one listener
: download manager takes only one listener
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Download Manager (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: ---
Assigned To: Shawn Wilsher :sdwilsh
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-04-08 04:52 PDT by Myk Melez [:myk] [@mykmelez]
Modified: 2008-07-31 04:30 PDT (History)
12 users (show)
cdbordelon: blocking1.8b5-
mbeltzner: blocking1.9-
reed: wanted1.9+
sdwilsh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Download Info Broadcaster (6.58 KB, application/x-javascript)
2007-05-04 12:23 PDT, Devon Jensen
no flags Details
v1.0 (14.30 KB, patch)
2007-05-23 22:37 PDT, Shawn Wilsher :sdwilsh
mconnor: review+
Details | Diff | Review

Description Myk Melez [:myk] [@mykmelez] 2005-04-08 04:52:24 PDT
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 chris sabo 2005-06-22 20:42:36 PDT
this isnt a bug its a feature request.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-06-22 20:50:26 PDT
(In reply to comment #1)
> this isnt a bug its a feature request.

And it's marked as such. What's your point?
Comment 3 Myk Melez [:myk] [@mykmelez] 2005-08-01 11:38:09 PDT
If we want to unleash the creative energies of our extension developers towards
better interfaces for downloads, this bug would be important to address.
Comment 4 Robert Kaiser (not working on stability any more) 2007-05-04 07:36:16 PDT
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 Devon Jensen 2007-05-04 12:19:10 PDT
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 Devon Jensen 2007-05-04 12:23:32 PDT
Created attachment 263758 [details]
Download Info Broadcaster

 - Javascript Component (Download statusbar specific)
Comment 7 Myk Melez [:myk] [@mykmelez] 2007-05-07 18:53:10 PDT
I think this would be important to get into Firefox 3.  Setting flags accordingly.
Comment 8 Mike Beltzner [:beltzner, not reading bugmail] 2007-05-23 09:16:00 PDT
Not a blocker, but definitely wanted. Connor says that he's gonna assign an intern who can probably take this.
Comment 9 Shawn Wilsher :sdwilsh 2007-05-23 09:34:55 PDT
I planned on taking this anyway, but hey, if you want to assign it to me, sure ;)
Comment 10 Mike Shaver (:shaver -- probably not reading bugmail closely) 2007-05-23 10:08:42 PDT
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 cmtalbert 2007-05-23 12:36:59 PDT
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...  
Comment 12 Shawn Wilsher :sdwilsh 2007-05-23 22:37:37 PDT
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++.
Comment 13 Mike Connor [:mconnor] 2007-05-25 09:53:14 PDT
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.
Comment 14 Shawn Wilsher :sdwilsh 2007-05-25 10:00:52 PDT
(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 Philip Chee 2007-05-25 10:24:55 PDT
(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
Comment 16 Shawn Wilsher :sdwilsh 2007-05-25 10:44:10 PDT
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!
Comment 17 Shawn Wilsher :sdwilsh 2007-05-25 16:49:00 PDT
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
Comment 18 Shawn Wilsher :sdwilsh 2007-05-30 10:41:07 PDT
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 Eric Shepherd [:sheppy] 2007-08-27 12:01:40 PDT
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?
Comment 20 Shawn Wilsher :sdwilsh 2007-08-27 15:37:41 PDT
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 Christian :Biesinger (don't email me, ping me on IRC) 2007-08-27 16:09:01 PDT
(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.

Comment 22 Shawn Wilsher :sdwilsh 2007-08-27 16:27:50 PDT
(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 Eric Shepherd [:sheppy] 2007-08-27 17:45:47 PDT
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 Christian :Biesinger (don't email me, ping me on IRC) 2007-08-27 18:31:52 PDT
A null request should never happen here...
Comment 25 Shawn Wilsher :sdwilsh 2008-06-26 21:40:14 PDT
a lot of our tests use more than one listener, so in-testsuite+

Note You need to log in before you can comment on or make changes to this bug.