Download Manager is not friendly to embedders or extensions

RESOLVED FIXED in mozilla1.9alpha8

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9alpha8
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 274310 [details] [diff] [review]
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.
Attachment #274310 - Flags: review?(mano)
Summary: Download Manager is not friendly to embedder → Download Manager is not friendly to embedders
Hrm, shouldn't the new interface be used as a service? Is ifreq the usual way to expose windows to embedders.
(Assignee)

Comment 3

11 years ago
(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.
(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).
(Assignee)

Comment 5

11 years ago
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.
(Assignee)

Comment 6

11 years ago
Created attachment 275079 [details] [diff] [review]
v1.1

service instead
Attachment #274310 - Attachment is obsolete: true
Attachment #275079 - Flags: review?(mano)
Attachment #274310 - Flags: review?(mano)
(Assignee)

Comment 7

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

(answers Mano's question on irc)
(Assignee)

Comment 8

11 years ago
Created attachment 278471 [details] [diff] [review]
v1.2

Updated to trunk.

I also moved the JS source file to a more sensible location.
Attachment #275079 - Attachment is obsolete: true
Attachment #278471 - Flags: review?(mano)
Attachment #275079 - Flags: review?(mano)
(Assignee)

Updated

11 years ago
Summary: Download Manager is not friendly to embedders → Download Manager is not friendly to embedders or extensions
Comment on attachment 278471 [details] [diff] [review]
v1.2

r=mano with the custom factory removed; Expect regressions from the timer removal... :-/
Attachment #278471 - Flags: review?(mano) → review+
(Assignee)

Comment 10

11 years ago
Created attachment 278487 [details] [diff] [review]
v1.3

for checkin
Attachment #278471 - Attachment is obsolete: true
(Assignee)

Comment 11

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

I'm checking in without the factory stuff.
(Assignee)

Comment 12

11 years ago
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

11 years ago
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 :/
Keywords: dev-doc-needed
(Assignee)

Updated

11 years ago
Blocks: 393938
(Assignee)

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-

Updated

11 years ago
Depends on: 393982
I believe this caused bug 393982

Updated

11 years ago
Blocks: 381157

Updated

11 years ago
Depends on: 394228
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.
(Assignee)

Comment 16

11 years ago
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...
The nsIDownloadManagerUI interface now has reference docs:

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

Marking as complete.
Keywords: dev-doc-needed → dev-doc-complete
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.