389982 - Download Manager is not friendly to embedders or extensions

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Shawn Wilsher :sdwilsh]

The download manager is currently very unfriendly to embedders.  For starters, we open a chrome url with it; for parenting, we look for a browser window at times; and for extension authors, we force it to open in a window.

We can fix this by providing an interface to show the UI, and allow people to override it.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This doesn't seem to regress any existing behavior, and allows for embedders and extension authors to override this and control it how they see fit.

Comment 2

[Asaf Romano (gone)]

Hrm, shouldn't the new interface be used as a service? Is ifreq the usual way to expose windows to embedders.

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> Hrm, shouldn't the new interface be used as a service?
No other UI launchers are, and I'm not sure of the benefit of doing so (other than not having to create instance every time).

Is ifreq the usual way to expose windows to embedders.
Yes because they may not have a DOM window to pass in (they may have their own thing).  Some older interfaces just have nsISupports, but biesi has told me that nsIInterfaceRequester is much cleaner.

Comment 4

[Asaf Romano (gone)]

(In reply to comment #3)
> (In reply to comment #2)
> > Hrm, shouldn't the new interface be used as a service?
> No other UI launchers are, and I'm not sure of the benefit of doing so (other
> than not having to create instance every time).

which launchers? isn't that reason enough given that the download manager itself is a service (as in, you cannot have two DMs open).

Comment 5

[Shawn Wilsher :sdwilsh]

nsIHelperAppLauncherDialog and nsIContentDispatchChooser to list off the ones I know of off-hand.  I do see your argument for doing it as a service though.  I can do that.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

service instead

Comment 7

[Shawn Wilsher :sdwilsh]

<mconnor> sdwilsh: I'd say so
<mconnor> for not using the timer

(answers Mano's question on irc)

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Updated to trunk.

I also moved the JS source file to a more sensible location.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 278471 [details] [diff] [review]
v1.2

r=mano with the custom factory removed; Expect regressions from the timer removal... :-/

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

for checkin

Comment 11

[Shawn Wilsher :sdwilsh]

for whatever reason that last attachment isn't actually what I had locally...

I'm checking in without the factory stuff.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> Expect regressions from the timer removal... :-/
Fully expected sadly :(

Checking in toolkit/components/downloads/public/Makefile.in;
new revision: 1.7; previous revision: 1.6
Checking in toolkit/components/downloads/public/nsIDownloadManager.idl;
new revision: 1.20; previous revision: 1.19
Checking in toolkit/components/downloads/public/nsIDownloadManagerUI.idl;
initial revision: 1.1
Checking in toolkit/components/downloads/src/Makefile.in;
new revision: 1.15; previous revision: 1.14
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.109; previous revision: 1.108
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
new revision: 1.36; previous revision: 1.35
Checking in toolkit/components/downloads/src/nsDownloadManagerUI.js;
initial revision: 1.1
Checking in toolkit/components/downloads/src/nsDownloadProxy.h;
new revision: 1.21; previous revision: 1.20

Comment 13

[Shawn Wilsher :sdwilsh]

I'm requesting dev-doc-needed here since extension authors can override this implementation and create their own UI or put the existing UI in a tab.  Not sure if we really need to document it though :/

Comment 14

[Steve England [:stevee]]

I believe this caused bug 393982

Comment 15

[Eric Shepherd [:sheppy]]

What documentation beyond what's linked to from

http://developer.mozilla.org/en/docs/Firefox_3_for_developers#Download_manager

is needed?  I'd like some input on that before I determine what more I should write up.

Comment 16

[Shawn Wilsher :sdwilsh]

Basically, if an add-on author wants to provide a different UI for the download manager (that overrides the default one), they need to implement nsIDownloadManagerUI.

At some point, I plan on looking over all the docs for the download manager and sprucing up the empty spots still...

Comment 17

[Eric Shepherd [:sheppy]]

The nsIDownloadManagerUI interface now has reference docs:

http://developer.mozilla.org/en/docs/nsIDownloadManagerUI

Marking as complete.