348386 - Download manager doesn't display on suiterunner builds

Status: VERIFIED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Mark Banner (:standard8)]

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

[Robert Kaiser]

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.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

building both is hard, as both register for the same set of contract IDs...

Comment 3

[Mark Banner (:standard8)]

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.

Comment 4

[Mark Banner (:standard8)]

Attachment: 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.

Comment 5

[[e-mail]]

Be careful, because our progress dialogs are also used for uploads...

Comment 6

[Mark Banner (:standard8)]

(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

Comment 7

[Mark Banner (:standard8)]

Attachment: 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).

Comment 8

[Robert Kaiser]

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

Comment 9

[Mark Banner (:standard8)]

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

Comment 10

[Mark Banner (:standard8)]

Attachment: 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.

Comment 11

[Robert Kaiser]

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.

Comment 12

[Mark Banner (:standard8)]

Attachment: 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.

Comment 14

[Mark Banner (:standard8)]

Note bug 380250

Comment 15

[Mark Banner (:standard8)]

Attachment: 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.

Comment 16

[[e-mail]]

Attachment: Use XPFE's Download Manager

* Re-enables the XPFE UI
* Disables a toolkit component which overwrites a Gecko component :-(

Comment 17

[Robert Kaiser]

Actually, I completely dislike add any ifdef to toolkit, we already have way too many temporary ifdefs in there...

Comment 18

[[e-mail]]

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

Comment 19

[Giacomo Magnini]

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

Comment 20

[Mark Banner (:standard8)]

(In reply to comment #16)
> Created an attachment (id=264542) [details]
> * Disables a toolkit component which overwrites a Gecko component :-(

Is that overrideHandler.js?

Comment 21

[Mark Banner (:standard8)]

(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

[Philip Chee]

> 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

[Robert Kaiser]

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

Comment 24

[Mark Banner (:standard8)]

(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

[Philip Chee]

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

Comment 26

[[e-mail]]

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

Comment 27

[Serge Gautherie (:sgautherie)]

(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 :-|

Comment 28

[Mark Banner (:standard8)]

(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 29

[Mark Banner (:standard8)]

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.

Comment 30

[Asaf Romano (gone)]

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

mpa=mano on the toolkit makefile change.

Comment 31

[Robert Kaiser]

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.

Comment 32

[Robert Kaiser]

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

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

why only the src directory, not all of toolkit dlmgr?

Comment 34

[[e-mail]]

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

Comment 35

[[e-mail]]

I actually checked my patch in a few days ago, so this is fixed.

Comment 36

[Shawn Wilsher :sdwilsh]

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

Comment 37

[Serge Gautherie (:sgautherie)]

[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 :-))