Download manager doesn't display on suiterunner builds

VERIFIED FIXED

Status

VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: standard8, Assigned: neil)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Download manager doesn't display on suiterunner builds.

In the first instance I'd like to get the xpfe version working. We may then wish to look at hooking in the toolkit backend, but that should be a separate bug (as long as the xpfe version works ;-) ).

Comment 1

12 years ago
We should definately look into a possibility to build the toolkit component for it (even if we may not use it), so that we can build the same stuff from tookit as XULRunner, to make it easy for us to completely change over to that architecture some time later.
building both is hard, as both register for the same set of contract IDs...
Toolkit/xulrunner apps are forced to include /toolkit/mozapps/downloads which means you get nsHelperAppDlg.js (which registers its own component) whether they like it or not.

I think it may just be best to see if we can get our front-end working with toolkit backend and overriding theirs in appropriate places.

I've got no idea if this will work or not, but I'll start looking at it soon.
Created attachment 233636 [details] [diff] [review]
WIP Patch v1

This is still very much a work in progress patch. It is highly non-functional at the moment.

What it does:

- Stops suiterunner building xpfe/components/download-manager
- Start suiterunner building toolkit/components/downloads
- Copies the downloadmanager.{js,xul} from xpfe into suite and starts to rework them to work with toolkit's downloads component. Note that it actually overrides some of toolkit's manager that is built in toolkit/mozapps/downloads

What it doesn't do:

- Most of the pause/resume/progress display in the actual download manager just doesn't work. I haven't worked out why yet, but it probably just needs some calls sorting out.
- Property dialogs for the downloads are the toolkit ones - containing very little information.

The property dialog I think may give me some trouble as I can't see an easy way of hooking into the progress for a download. xpfe did this via a nsDownload implementation and providing a hook for a progress dialog, this was all done internally.

toolkit has the same functions defined in its nsDownload (http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/downloads/src/nsDownloadManager.h&rev=1.20&mark=216-219#216) however they won't work (they've removed the usage of the listener in the implementation)  and aren't currently accessible from outside toolkit.

My proposal would be to create a nsIToolkitDownload (or something similar), which would define the four functions linked to above (as two attributes of course), and fix the listener implementation so that it works for progress dialogs. We could then hook into it and use it for our progress dialog display.

I'd like to get some SeaMonkey thoughts on this before proposing it directly for toolkit, as I've probably missed something that would actually be easier.
(Assignee)

Comment 5

12 years ago
Be careful, because our progress dialogs are also used for uploads...
(In reply to comment #5)
> Be careful, because our progress dialogs are also used for uploads...
> 
That shouldn't be a problem because we use a progress dialog from the /embedding, and create our own instance of it here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/browser/resources/content/navigator.js&rev=1.594&mark=2577#2556
Created attachment 234564 [details] [diff] [review]
WIP Patch v2

Changes in this patch:

- All of download manager's columns are filled in. Though some are incorrect (progress doesn't display finished/canceled etc.
- Progress is updated.

Main items still to do:

- Work out a reasonable way of overriding toolkit's progress dialog so that we can use our ones (currently with the patch we'll display toolkit's version).
- Sort out the progress column (so that finished/cancelled etc are displayed).
Attachment #233636 - Attachment is obsolete: true

Comment 8

12 years ago
Comment on attachment 234564 [details] [diff] [review]
WIP Patch v2

>Index: suite/common/jar.mn
>===================================================================
>@@ -1,3 +1,15 @@

That first hunk is an unrelated dirty hack, right? It looks very wrong, so I count it as just that.

>+   content/communicator/downloadmanager/downloadmanager.xul         (downloadmanager/downloadmanager.xul)
>+   content/communicator/downloadmanager/downloadmanager.js          (downloadmanager/downloadmanager.js)
>+   content/communicator/downloadmanager/DownloadProgressListener.js (downloadmanager/DownloadProgressListener.js)

Please put those in a correct alphabetical order, i.e. between directory and history (and the .js before the .xul) - when entries in this file are odered, it's much easier to find them later :)

>Index: suite/locales/jar.mn
>===================================================================
>+  locale/@AB_CD@/communicator/downloadmanager/downloadmanager.dtd           (%chrome/common/downloadmanager/downloadmanager.dtd)
>+  locale/@AB_CD@/communicator/downloadmanager/downloadmanager.properties    (%chrome/common/downloadmanager/downloadmanager.properties)

Same as above for where to add the lines.


You forgot to show us the actually added UI files (those mentioned in the two hunks I cited) in the patch... And the bookmarks stuff is unrelated as well, right? That looks like it belongs to the migration patch...
(In reply to comment #8)
> (From update of attachment 234564 [details] [diff] [review] [edit])
> >Index: suite/common/jar.mn
> >===================================================================
> >@@ -1,3 +1,15 @@
> 
> That first hunk is an unrelated dirty hack, right? It looks very wrong, so I
> count it as just that.

Yes, quite right.

> You forgot to show us the actually added UI files (those mentioned in the two
> hunks I cited) in the patch... And the bookmarks stuff is unrelated as well,
> right? That looks like it belongs to the migration patch...

Yep looks like I need better patch control ;-) Anyway, the patch has progressed a little since the v2, so I'll post a new one soon.
Created attachment 242214 [details] [diff] [review]
WIP Patch v3

I'm not going to be look at this patch for the next month or so at least, so if anyone wants to pick it up feel welcome. I'm happy to answer questions if anyone is picking up this patch (standard8 on irc).

He's the basic idea:

- Use toolkit's back-end for the download manager
- Override toolkit's front-end using similar code to the current xpfe front-end
- Due to overriding the front-end we also have to override the download progress dialog, and because toolkit's nsDownload is essentially private, we're having to provide a new public interface to it within toolkit.

Its very much sort-of works. The progress dialogs do come up and display, but the buttons don't work. There's also quite a bit of work to do on the download manager front-end to bring that up to scratch as well.
Attachment #234564 - Attachment is obsolete: true
Assignee: bugzilla → download-manager
Keywords: helpwanted

Comment 11

12 years ago
Could someone pick this up again?

The "basic idea" of the last post sounds reasonable to me: We want to use toolkit back-end where reasonable and keep our front-end as near as possible to the "old" SeaMonkey front-end, I think.
Created attachment 262919 [details] [diff] [review]
Simpler method?

I had a slightly different idea about how to approach this. Most of what is in this patch is build config changes so that we stop building xpfe download manager and start building toolkits.

These are the main points in this patch:

- Used toolkit's download.xul as a base (copied and then overrode the original).
- Created a tree instead of a rich text list

The basic concept is to use as much as possible of toolkit's js code, rather than re-writing xpfe to fit toolkit.

The bit I've got stuck on is that toolkit's dm uses an xml extension (http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/download.xml) of a richlistitem to do a lot of work (provide attributes/functions etc).

I think we'd either need to implement something similar for a tree cell (if that's possible) or use css to fake the rich list box into a tree if that's possible.

This is where my xml knowledge lets me down, and I haven't really got time to learn it at the moment so if someone else is able to use this as a starting point, it'd be really good.
Blocks: 379214
No longer blocks: 379214
Duplicate of this bug: 379214
Created attachment 264496 [details] [diff] [review]
Use Toolkit's Download Manager

As discussed on irc, this patch lets us use toolkit's download manager for suiterunner. I'll raise a separate bug later for implementing our own version of it.

Checking Neil's happy with it first - though we may need to decide on some of the prefs I've left in - before asking for build-config reviews.
Assignee: download-manager → bugzilla
Attachment #242214 - Attachment is obsolete: true
Attachment #262919 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #264496 - Flags: superreview?(neil)
Blocks: 380441
(Assignee)

Comment 16

12 years ago
Created attachment 264542 [details] [diff] [review]
Use XPFE's Download Manager

* Re-enables the XPFE UI
* Disables a toolkit component which overwrites a Gecko component :-(
Attachment #264542 - Flags: review?(bugzilla)

Comment 17

12 years ago
Actually, I completely dislike add any ifdef to toolkit, we already have way too many temporary ifdefs in there...
(Assignee)

Comment 18

12 years ago
Oh, and of course for those people who don't like download managers, toolkit doesn't support download progress dialogs.

Comment 19

12 years ago
(In reply to comment #18)
> Oh, and of course for those people who don't like download managers, toolkit
> doesn't support download progress dialogs.
That really sucks... :(
(In reply to comment #16)
> Created an attachment (id=264542) [details]
> * Disables a toolkit component which overwrites a Gecko component :-(

Is that overrideHandler.js?
(In reply to comment #19)
> (In reply to comment #18)
> > Oh, and of course for those people who don't like download managers, toolkit
> > doesn't support download progress dialogs.
> That really sucks... :(
> 
The WIP Patch v3 on this bug gave toolkit support for progress dialogs again (the get/set functions are already in there just no interfaces/hooks)... so I think we could get toolkit's DM & xpfe's download progress dialogs fairly easily...

Thoughts? I'd really pref not to disable more of toolkit at the moment.

Comment 22

12 years ago
> Thoughts? I'd really pref not to disable more of toolkit at the moment.

consider this a temporary fix, until you can get a tree view working with the toolkit download manager.

Comment 23

12 years ago
Sure using the old xpfe code would be intended as a temporary measure, but so would be using the toolkit download manager - just that the latter would make the pressure on existing and possible developers higher to find a solution, which is something I consider to be very good on trunk.
And then, we already have way too many temporary ifdefs in toolkit - which we were only allowed to place there as a short-lived temporary measure.

Neil, if you want to add that additional ifdef in toolkit, be sure to get review from toolkit folks (only removing temporary ifdefs from there probably can go with reviews from our people as well, I guess).
(In reply to comment #23)
> Sure using the old xpfe code would be intended as a temporary measure, but so
> would be using the toolkit download manager - just that the latter would make
> the pressure on existing and possible developers higher to find a solution,
> which is something I consider to be very good on trunk.

I have to say I agree with Robert here - using toolkit's download manager as the interim measure would encourage us more to do a proper switch. I'm already fairly sure that with a smallish change to toolkit we could get our progress dialogs back again.

I think with a temporary xpfe DM a 'blocker' which is move to toolkit's backend is much less likely to get fixed than a temporary toolkit DM with a 'blocker' that is give us back our nice UI, and this is also one of the things we'll need to start doing and be happy with if we ever want to get to running with xulrunner.

Comment 25

12 years ago
(In reply to comment #23)
You've convinced me. Objection withdrawn.
(Assignee)

Comment 26

12 years ago
(In reply to comment #20)
>(In reply to comment #16)
>>Created an attachment (id=264542)
>>* Disables a toolkit component which overwrites a Gecko component :-(
>Is that overrideHandler.js?
No, nsHelperAppDlg.js

(In reply to comment #21)
>The WIP Patch v3 on this bug gave toolkit support for progress dialogs again
Actually it doesn't, that's handled by nsDownloadProxy.h ... and that still supposes you can get toolkit review for your changes, which is doubtful.
(In reply to comment #0)
> In the first instance I'd like to get the xpfe version working. We may then

As a SeaMonkey user/tester (= not developer),
Neil's patch seems trivial enough to apply then remove eventually,
and might help giving "suiterunner" a try while it's still "experimental":
D.M. working, and as we are used to.

> wish to look at hooking in the toolkit backend, but that should be a separate
> bug (as long as the xpfe version works ;-) ).

That would also give time for the Core/Toolkit bugs on which bug 380441 depends to get fixed.
I would suggest to (re)consider switching to Mark's patch when that happens.

"Or" review requests could be issued for both patches now and see what Toolkit people say :-|
(In reply to comment #26)
> (In reply to comment #21)
> >The WIP Patch v3 on this bug gave toolkit support for progress dialogs again
> Actually it doesn't, that's handled by nsDownloadProxy.h ... and that still
> supposes you can get toolkit review for your changes, which is doubtful.

If toolkit isn't going to accept additional hooks that would make using the platform easier for other application and extensions (which we would be supplying the code for), then IMHO based on some of the recent blogs, they wouldn't be supporting the Mozilla "platform" and hence I think would probably be able to complain that MoCo/Fo aren't doing what I see they've committed to. How far that gets us remains to be seen, but we should at least try in the first instance.
Comment on attachment 264542 [details] [diff] [review]
Use XPFE's Download Manager

I'm giving r+ on the basis that this patch works (though mine does as well, just with different results). I think the SeaMonkey Council should agree on which way to go before landing either patch.
Attachment #264542 - Flags: review?(bugzilla) → review+
Comment on attachment 264542 [details] [diff] [review]
Use XPFE's Download Manager

mpa=mano on the toolkit makefile change.

Comment 31

12 years ago
Comment on attachment 264542 [details] [diff] [review]
Use XPFE's Download Manager

OK, then let's go with this temporarily until the toolkit download stuff settles down and we find a better way to deal with all that in a followup bug.

>Index: toolkit/mozapps/downloads/Makefile.in
>===================================================================
>+ifndef MOZ_SUITE
> DIRS = src
>+endif

Could you add a comment like this, please:

# XXX Suite isn't ready to build this just yet

We're using those comments everywhere we do ifdef out stuff in toolkit, so that we can find them easily enough to remove them in the future.
Attachment #264496 - Flags: superreview?(neil)

Comment 32

12 years ago
Neil, looks like you forgot to note the bug number in your checkin.

Anyways, I guess this bug should be marked FIXED now as download manager does display now and that was the subject of this report.

We should file a followup on getting something working on the toolkit backend once the rework of that has been done.
As a note, the FF UI becomes completely JS-driven without using templates now, and we probably can drive a <tree> by JS in roughly the same way...

Updated

12 years ago
Blocks: 381157
Blocks: 381159

Updated

12 years ago
No longer blocks: 381159
Assignee: bugzilla → neil
Status: ASSIGNED → NEW
why only the src directory, not all of toolkit dlmgr?
(Assignee)

Comment 34

12 years ago
(In reply to comment #33)
>why only the src directory, not all of toolkit dlmgr?
We're already excluding the rest of toolkit dlmgr. See Mark's patch.
(Assignee)

Comment 35

12 years ago
I actually checked my patch in a few days ago, so this is fixed.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(In reply to comment #28)
> If toolkit isn't going to accept additional hooks that would make using the
> platform easier for other application and extensions (which we would be
> supplying the code for), then IMHO based on some of the recent blogs, they
> wouldn't be supporting the Mozilla "platform" and hence I think would probably
> be able to complain that MoCo/Fo aren't doing what I see they've committed to.
> How far that gets us remains to be seen, but we should at least try in the
> first instance.

Please file bugs/alert me on issues of the api.  I'm touching the code now, so it's best to get my attention on it while it is still fresh in my head.
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/2007052105 SeaMonkey/1.5a] (suiterunner, tinderbox-builds) (W2Ksp4)

V.Fixed, both progress dialog and D.M. window :-))
Status: RESOLVED → VERIFIED
Keywords: helpwanted
You need to log in before you can comment on or make changes to this bug.