As progress dialogs aren't implemented yet while I'm developing the new download manager in bug 472001, I couldn't implement the option to open a progress dialog as "Properties" or "More Information" for (active) downloads yet, but we probably should re-implement this once this new UI has landed.

From bug 483241 Comment #11 by Neil: > At the moment we can only launch progress dialogs for new downloads, and even > the old download manager could only launch progress dialogs for active > downloads, so even if you implemented that limited feature it wouldn't hurt to > default gEndTime to Date.now() when the dialog opens. I'm thinking that perhaps > the download manager could pass the end time as a second parameter so that you > could open a progress dialog for a finished download (to find out the blocked > reason, perhaps!) I think we should look into this when working on the bug here.

Here's a patch to implement this feature again. In addition to the obvious addition of the command, it does the following: * Hand the parent to the progress window from the component * Fix the component to throw and not call the progress window when no ID given * Fetch the end time from the tree view if called from dlmgr * Switch back the window ID of the dlmgr so it picks up the correct icon again The try-catch removal and window ID change are not really the core scope of this bug, but they're very small and things either I stumbled over in this work or was asked about by users (those people actually notice when the window icon is wrong).

Comment on attachment 393008 [details] [diff] [review] implement "properties" in new dlmgr >+ <menuitem id="dlMenu_properties" >+ label="&cmd.properties.label;" >+ accesskey="&cmd.properties.accesskey;" >+ command="cmd_properties"/> [The old download manager's Enter action would open properties if it couldn't open the item. Not sure whether that's ideal or whether we should have a separate key or just use the menu.] >+ if (opener && opener.gDownloadTreeView && opener.gDownloadTreeView.rowCount > 0) { Not sure that we can rely on opener here; might want to use dmui.recentWindow either here or in dmui itself (instead of parent) when opening the progress dialog. > var download = null; >- try { >- let dm = Cc["@mozilla.org/download-manager;1"]. >- getService(Ci.nsIDownloadManager); >- download = dm.getDownload(aID); >- } catch (ex) {} >+ let dm = Cc["@mozilla.org/download-manager;1"]. >+ getService(Ci.nsIDownloadManager); >+ download = dm.getDownload(aID); Nit: don't need to pre-init download any more. Also, these files seem to prefer var to let ;-)

Or another alternative is for download manager to open progress dialog directly (via openDialog) rather than going through dmui, thus allowing it to pass a third argument (the end time).

OK, I decided I couldn't cheat my own rules and added testing for this function now ;-) dmui.recentWindow is not exported to outside the component, so I used the window mediator directly to get the download manager window from the progress window to make this more robust. I split out the call to the progress window into its own function in downloadmanager.js for two reasons: 1) it can be overridden by the selection test, 2) an extension can override it and create its own properties window. The tests are on one hand an addition to the single/multiple selection test, verifying that it's called only on single selections right now, and a new, more in-depth test (based on test_clear_button_disabled.xul code) that actually calls up properties for both an inactive and an active download and even looks for correct setting of the end time in the progress window.

Comment on attachment 394716 [details] [diff] [review] implement "properties" in new dlmgr, v2, with tests >--- /dev/null >+++ b/suite/common/downloads/tests/chrome/test_open_properties.xul >+ // force rebuilding of tree to generate a ui-done event on the DM >+ dmview.initTree(); Actually, ignore those two lines, they're not needed, I had an earlier bug in the test. I removed those locally and the test still works fine.

Comment on attachment 394716 [details] [diff] [review] implement "properties" in new dlmgr, v2, with tests > var params = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray); >- var download = null; >+ let parent = null; >- let dm = Cc["@mozilla.org/download-manager;1"]. >+ let dm = Cc["@mozilla.org/download-manager;1"]. >+ var download = dm.getDownload(aID); > let reason = Cc["@mozilla.org/supports-PRInt16;1"]. > var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"]. Argh!

Pushed as http://hg.mozilla.org/comm-central/rev/ee0aee8dbc9a with the var/let mess cleaned up in the lines I touched.

Comment on attachment 394716 [details] [diff] [review] implement "properties" in new dlmgr, v2, with tests >+<!ENTITY cmd.properties.label "Propertiesâ¦"> >+<!ENTITY cmd.properties.accesskey "s"> Aargh, I just realised that, because this command's job is to open the Properties dialog, it shouldn't have an ellipsis...

(In reply to comment #10) > (From update of attachment 394716 [details] [diff] [review]) > >+<!ENTITY cmd.properties.label "Propertiesâ¦"> > >+<!ENTITY cmd.properties.accesskey "s"> > Aargh, I just realised that, because this command's job is to open the > Properties dialog, it shouldn't have an ellipsis... Erm, why not? Any command that opens a dialog should, right?