472001 - Tree-based UI for toolkit download manager in SeaMonkey

Status: VERIFIED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Robert Kaiser]

I'm splitting the tree-based UI for the toolkit download manager off bug 381157.

See e.g. http://home.kairo.at/blog/2009-01/more_on_the_new_download_manager for the work I'm doing on this right now.

Comment 1

[Robert Kaiser]

Adding a number of dependent UI bug reports:
bug 188010, bug 215716, bug 207448, bug 207683, bug 212896, bug 290117, bug 461618 will be fixed with the work I have in my WIP in the repo http://hg.mozilla.org/users/kairo_kairo.at/suite-dlmgr-ui/pushloghtml

bug 132112 and bug 171142 will need to be implemented later, but based on this work instead of the old xpfe download manager.

Comment 2

[Robert Kaiser]

Attachment: patch v1: add files for new downloadmanager window

This patch has identical files to http://hg.mozilla.org/users/kairo_kairo.at/suite-dlmgr-ui/rev/b81a7c8ada24 except for the chrome references being changed from "chrome://suitedlmgrui/" to "chrome://communicator/" and the jar.mn changes.

The patch adds all the UI to be available from chrome://communicator/content/downloads/downloadmanager.xul but doesn't call it anywhere yet (needs the main work to switch to toolkit dlmgr).
It can't be tested in SeaMonkey in this state (i.e. without the main switch), but I'd like to get some code review as soon as possible - this is completely new code, though in parts heavily based on toolkit dlmgr, xpfe dlmgr and SeaMonkey places history code as well as a large number of things looked up in MDC and even xulplanet (where MDC lacks documents, e.g. for nsITreeBoxObject).

I tried to fit with our usual code style as far as I know it - I'm happy about all review comments, this time it's code I really understand as I (re)wrote everything myself ;-)

Comment 3

[Robert Kaiser]

Attachment: screen shot of main download manager window

Neil, as you are the UI tsar but probably can't test this in its current state, here's a screen shot for ui-reviewing the basic look of the window - it shows a list filtered by "mo" (i.e. sea*mo*nkey matching as well as files from bugzilla.*mo*zilla.org and startrek*mo*vie.com) in "unsorted" order, with one file currently downloading and one paused download, and IIRC the shown-by-default columns, probably not all with their default widths though.

Comment 4

[Robert Kaiser]

As a note, I have meanwhile taken another look at the tree columns and hidden status by default, removed the width attributes (flex is enough), made the action columns fixed, added tooltips with the full titles, and made the action column headers show a play and cancel icon. I'll include this in the next patch, once I also have some review comments to address.

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 355308 [details]
screen shot of main download manager window

My largest concern here with this UI, is as it relates to search.

I don't see searches that match against anything other than file-leaf-name to be intuitive in this UI. While I *do* want search to match everything, I think a dropdown to "Match All", "Match Filename" "Match Domain", "Match ..." type options may make sense for us. And failing said dropdown in initial ver here, we may want to only match on leafname.

But I'll leave that up to Neil.

Comment 6

[[e-mail]]

Comment on attachment 355307 [details] [diff] [review]
patch v1: add files for new downloadmanager window

JavaScript code review only:

>+    let state = aDownload.state;
>+    switch (state) {
Nit: variable only used once.

>+let nsIDM = Components.interfaces.nsIDownloadManager;
const nsIDownloadManager

>+let gDownloadTree;
s/\<let\>/var/g pretty please?
("let" makes no sense in the global scope anyway.)

>+let gDownloadManager = Components.classes["@mozilla.org/download-manager;1"]
>+                                 .createInstance(nsIDM);
Surely that's a service?

>+  // Append controller on all our interactive controls
I don't think that makes sense. You could add it to the window (but don't forget to remove it on unload.)

>+  if (gDownloadStatus)
>+    gDownloadTree.focus();
Are you planning a downloads sidebar? (If so then this makes sense)

>+    let parent = file.parent.QueryInterface(Components.interfaces.nsILocalFile);
>+    if (!parent)
>+      return;
This makes no sense - QueryInterface never returns null.

>+      openExternal(parent);
Not defined? Also, not used by openDownload?

>+    let row = new Object;
(I prefer var row = {};)

>+function onTreeClick(aEvent)
Since ActionPlay and ActionStop are cycler cells, the view will get cycleCell notifications which you should use instead.

>+    return fileUrl.file.clone().QueryInterface(Components.interfaces.nsILocalFile);
Not sure what the clone is for, and can't remember whether you need the QI.

>+    var f = new nsLocalFile(aPathOrUrl);
Not defined?

>+    // we can even enable some commands when we have no selection
>+    let ignoreSelection = (aCommand == "cmd_selectAll" ||
>+                           aCommand == "cmd_clearList");
I think we could do a better job of checking the selection count as many of the commands only work with single selection.

>+      <treecol id="ActionPlay" cycler="true"
>+               label="&col.actionPlay.label;"
>+               width="1*" flex="1"
Avoid flex on cycler columns. Also * is an illegal width character.
(I can't remember what we do in mailnews; fixed="true" perhaps?)

>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
Not used in this file (but it could be).

>+Components.utils.import("resource://gre/modules/PluralForm.jsm");
Confusingly, it's a different file that actually uses PluralForm.

>+function DownloadTreeView(aDownloadManager) {
>+  this._tree = null;
>+  this._selection = null;
Nulls, numbers and strings can go on the prototype.

>+    //append all row properties to the cell
>+    this.getRowProperties(aRow, aProperties);
I thought this happened automatically.

>+        return file.nativeLeafName || file.leafName;
I thought nativeLeafName was [noscript]

>+    // Tell the tree we added 1 row at index 0
>+    this._tree.rowCountChanged(0, 1);
>+
>+    // Update the selection
>+    this.selection.adjustSelection(0, 1);
rowCountChanged should call adjustSelection automatically.

>+      // Only add the referrer if it's not null
I don't see the point of this check; the consumer already null-checks.

>+      if (idx in this._dlList &&
Won't this always be true?

>+    // Abort if there's already something cached
IHMO _selectionCache should be null if nothing is cached.

>+    let row;
>+    for each (let dlid in this._selectionCache) {
>+      // Find out what row this is now and if possible, add it to the selection
>+      row = this._getIdxForID(dlid);
var row = this._getIdxForID(dlid);

Comment 7

[Robert Kaiser]

(In reply to comment #6)
> (From update of attachment 355307 [details] [diff] [review])
> >+let nsIDM = Components.interfaces.nsIDownloadManager;
> const nsIDownloadManager

It's used so often (for its constants) that I liked going with the shorter version better as a lot of lines would grow significantly longer.

> >+let gDownloadTree;
> s/\<let\>/var/g pretty please?
> ("let" makes no sense in the global scope anyway.)

Oh, hrm. If I'd have known you dislike "the better var" (as Brendan described "let" in presentations) I'd have saved a number of changes I did to copied code...

> >+let gDownloadManager = Components.classes["@mozilla.org/download-manager;1"]
> >+                                 .createInstance(nsIDM);
> Surely that's a service?

If it isn't then https://developer.mozilla.org/En/NsIDownloadManager is wrong. I copied this either from there or toolkit code.

> >+  if (gDownloadStatus)
> >+    gDownloadTree.focus();
> Are you planning a downloads sidebar? (If so then this makes sense)

Why doesn't it makes sense without it? IIRC, our old DM window does the same.

> >+    let parent = file.parent.QueryInterface(Components.interfaces.nsILocalFile);
> >+    if (!parent)
> >+      return;
> This makes no sense - QueryInterface never returns null.

Interesting, toolkit has it this way...

> >+      openExternal(parent);
> Not defined? Also, not used by openDownload?

Ah, right, the toolkit version defines that function for the three lines openDownload and showDownload share here, and I inlined that in the former but forgot to do so in the latter.

> >+function onTreeClick(aEvent)
> Since ActionPlay and ActionStop are cycler cells, the view will get cycleCell
> notifications which you should use instead.

Note to self: That's as far as I managed to address the comments locally now (excluding var/let and nsIDM for the moment).

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> > >+let gDownloadManager = Components.classes["@mozilla.org/download-manager;1"]
> > >+                                 .createInstance(nsIDM);
> > Surely that's a service?
> 
> If it isn't then https://developer.mozilla.org/En/NsIDownloadManager is wrong.
> I copied this either from there or toolkit code.
*ugh* and fixed.  It's safe to createInstance on it because we have a special constructor, but still - *ugh*

Comment 9

[Philip Chee]

(In reply to comment #7)
>> (From update of attachment 355307 [details] [diff] [review] [details])
>>> +let nsIDM = Components.interfaces.nsIDownloadManager;
>> const nsIDownloadManager

> It's used so often (for its constants) that I liked going with the shorter
> version better as a lot of lines would grow significantly longer.

nsIDownloadManager is consistent with the rest of Suite and makes it more readable. Who knows what nsIDeeEm means after plowing through several thousand lines of code.

> >+let gDownloadTree;
> s/\<let\>/var/g pretty please?
> ("let" makes no sense in the global scope anyway.)

Oh, hrm. If I'd have known you dislike "the better var" (as Brendan described
"let" in presentations) I'd have saved a number of changes I did to copied
code...

"let" is fine when you need the better scoping rules that it adheres to however in the root of a function let has the same scope as a var and internally the JS engine converts top level "let"s into "var"s anyway. Also I've used lets in for and while loops and inside if blocks and Neil hasn't complained (yet).

Comment 10

[Philip Chee]

Questions from the peanut gallery:

http://forums.mozillazine.org/viewtopic.php?p=5400545#p5400545

It looks fairly close to the existing download manager but a few questions:

- will you still be able to select "open progress dialog" ?
- is the percentage field still available (hidden in the screenshot) ?
- will you still be able to delete entries via the delete key ?
- can the search bar be removed as it isn't very useful for a download manager ?

Comment 11

[Justin Wood (:Callek)]

(In reply to comment #9)
> (In reply to comment #7)
> >> (From update of attachment 355307 [details] [diff] [review] [details] [details])
> >>> +let nsIDM = Components.interfaces.nsIDownloadManager;
> >> const nsIDownloadManager
> 
> > It's used so often (for its constants) that I liked going with the shorter
> > version better as a lot of lines would grow significantly longer.
> 
> nsIDownloadManager is consistent with the rest of Suite and makes it more
> readable. Who knows what nsIDeeEm means after plowing through several thousand
> lines of code.

I'd be personally happier with |const nsIDM| at global scope, and given the file this is in, I feel it is quite understandable.

(In reply to comment #10)
> Questions from the peanut gallery:
> 
> http://forums.mozillazine.org/viewtopic.php?p=5400545#p5400545
> 
> It looks fairly close to the existing download manager but a few questions:
> 
> - will you still be able to select "open progress dialog" ?

If not trivial to add, and I will add it before this ver of the DM is added to a public alpha/beta. I also should note I envision Progress Dialoges being the "More Info" feature here, so its label may change, even for users who don't have progress dialoges shown as their primary UI for new DM's.  But thats future, I'm just hoping to match Suite atm.

> - will you still be able to delete entries via the delete key ?

If not I'll also be glad to look into this, but KaiRo can surely investigate as well if he feels up to it.

If there are further Q's ask away, I have not given the code a glance yet, as I'm sure everyone will appreciate me spending most of my time at this moment working on getting new patch for actual switch up.

Comment 12

[Robert Kaiser]

(In reply to comment #6)
> (From update of attachment 355307 [details] [diff] [review])
>> +    return fileUrl.file.clone().QueryInterface(Components.interfaces.nsILocalFile);
> Not sure what the clone is for, and can't remember whether you need the QI.

I've copied this from toolkit's getLocalFileFromNativePathOrUrl, it's history might tell.

>> +    // we can even enable some commands when we have no selection
>> +    let ignoreSelection = (aCommand == "cmd_selectAll" ||
>> +                           aCommand == "cmd_clearList");
> I think we could do a better job of checking the selection count as many of the
> commands only work with single selection.

I want to make more of them work with multiple selections and probably also consolidate tracking of selected index into a single variable for both single and multiple selections, but I'd like to leave this to a followup.

>> +<treecol id="ActionPlay" cycler="true"
>> +               label="&col.actionPlay.label;"
>> +               width="1*" flex="1"
> Avoid flex on cycler columns. Also * is an illegal width character.
> (I can't remember what we do in mailnews; fixed="true" perhaps?)

Exactly. That's what I fixed in the treecol work in mentioned in comment #4.

>> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> Not used in this file (but it could be).
> 
>> +Components.utils.import("resource://gre/modules/PluralForm.jsm");
> Confusingly, it's a different file that actually uses PluralForm.

Right. Actually, things work fine with both lines excluded. I wonder though if I should add the latter import on downloadmanager.js just to be safe, what do you think?

>> +function DownloadTreeView(aDownloadManager) {
>> +  this._tree = null;
>> +  this._selection = null;
> Nulls, numbers and strings can go on the prototype.

Hmm, but isn't it nicer to keep initializing all those member vars in one single place instead of spreading them between here and the prototype?

>> +    //append all row properties to the cell
>> +    this.getRowProperties(aRow, aProperties);
> I thought this happened automatically.

In my testing, that wasn't the case, and I added this call because of that.

>> +    // Abort if there's already something cached
> IHMO _selectionCache should be null if nothing is cached.

I personally prefer the empty array so that the var type never changes, but maybe that's just my thinking in stronger-typed terms. If you want me to make it null, that should be easy to change. :)

Comment 13

[Robert Kaiser]

(In reply to comment #10)
> Questions from the peanut gallery:
> 
> http://forums.mozillazine.org/viewtopic.php?p=5400545#p5400545

Of course, a screenshot by itself desn't really explain everything it's doing ;-)

> It looks fairly close to the existing download manager but a few questions:

The goal is to stay quite similar to the old one but also extend it (not to copy it, as a note).

> - will you still be able to select "open progress dialog" ?

Progress dialogs are a separate thing and being done by Callek, the current implementation of the download manager window also doesn't make the progress dialog available as "Properties" of a download row, that's something we should add again as a followup (I simply can't do it without having Callek's patch).

> - is the percentage field still available (hidden in the screenshot) ?

Yes.

> - will you still be able to delete entries via the delete key ?

That's not implemented in this first version, but should be quite easy to be added as a followup.

> - can the search bar be removed as it isn't very useful for a download manager
> ?

There are no plans for this right now, we can look into this later (and it's not that useless, IMHO).

Comment 14

[Robert Kaiser]

(In reply to comment #8)
> (In reply to comment #7)
> > If it isn't then https://developer.mozilla.org/En/NsIDownloadManager is wrong.
> > I copied this either from there or toolkit code.
> *ugh* and fixed.  It's safe to createInstance on it because we have a special
> constructor, but still - *ugh*

Thanks. I just looked and saw that toolkit is correct, so I indeed had that wrong line from MDC - corrected in my version now as well.

New patch will follow soon, I just need to clear up the "nsIDM" and let vs. var points.

Comment 15

[[e-mail]]

(In reply to comment #7)
>(In reply to comment #6)
>>(From update of attachment 355307 [details] [diff] [review])
>>>+let nsIDM = Components.interfaces.nsIDownloadManager;
>>const nsIDownloadManager
>It's used so often (for its constants) that I liked going with the shorter
>version better as a lot of lines would grow significantly longer.
Yes, but suite style is to use the full constant name wherever possible.

>>>+  if (gDownloadStatus)
>>>+    gDownloadTree.focus();
>>Are you planning a downloads sidebar? (If so then this makes sense)
>Why doesn't it makes sense without it? IIRC, our old DM window does the same.
Oh well, I guess it must have copied it from History, where it does make sense.

(In reply to comment #12)
>(In reply to comment #6)
>> (From update of attachment 355307 [details] [diff] [review])
>>> +    return fileUrl.file.clone().QueryInterface(Components.interfaces.nsILocalFile);
>> Not sure what the clone is for, and can't remember whether you need the QI.
> I've copied this from toolkit's getLocalFileFromNativePathOrUrl, it's history
> might tell.
You do need the QI. And nsIFileURL does ask you to clone, too.

>>> +<treecol id="ActionPlay" cycler="true"
>>> +               label="&col.actionPlay.label;"
>>> +               width="1*" flex="1"
>>Avoid flex on cycler columns. Also * is an illegal width character.
>>(I can't remember what we do in mailnews; fixed="true" perhaps?)
>Exactly. That's what I fixed in the treecol work in mentioned in comment #4.
Ah, sorry for overlooking that.

>>> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>> Not used in this file (but it could be).
>> 
>>> +Components.utils.import("resource://gre/modules/PluralForm.jsm");
>> Confusingly, it's a different file that actually uses PluralForm.
>Right. Actually, things work fine with both lines excluded. I wonder though if
>I should add the latter import on downloadmanager.js just to be safe, what do
>you think?
I think it would be more obvious as to what's happening, yes.

>>> +function DownloadTreeView(aDownloadManager) {
>>> +  this._tree = null;
>>> +  this._selection = null;
>> Nulls, numbers and strings can go on the prototype.
>Hmm, but isn't it nicer to keep initializing all those member vars in one
>single place instead of spreading them between here and the prototype?
Well, it's less "real" code to init them in the prototype.

>>> +    //append all row properties to the cell
>>> +    this.getRowProperties(aRow, aProperties);
>> I thought this happened automatically.
> In my testing, that wasn't the case, and I added this call because of that.
You're right, I checked the tree code.

>>> +    // Abort if there's already something cached
>> IHMO _selectionCache should be null if nothing is cached.
>I personally prefer the empty array so that the var type never changes, but
>maybe that's just my thinking in stronger-typed terms. If you want me to make
>it null, that should be easy to change. :)
Well, in many object-oriented languages, null is a special case of object :-)

Comment 16

[Robert Kaiser]

Attachment: patch v2: address code-review nits, improve tree header display

Comment 17

[[e-mail]]

Comment on attachment 356094 [details] [diff] [review]
patch v2: address code-review nits, improve tree header display

>+    window.updateCommands("tree-select");
Should be in addDownload, not here.

>+  var colID = column.getAttribute("id");
Nit: column.id

>+    sortDirection = document.getElementById("menu_SortDescending").checked
>+                    ? "descending"
>+                    : "ascending";
For History we actually reset the sort direction when switching column.

>+    for (let node = document.getElementById("Name"); node; node = node.nextSibling) {
>+      if (node.getAttribute("sortActive") == "true") {
>+        colID = node.id;
>+        column = node;
>+      }
>+    }
Use getSortedColumn

>+  if (colID != "unsorted" && column.getAttribute("cycler") == "true")
Nit: I think I'd prefer column &&

>+  // Clear attributes on previously sorted column
>+  for (let node = document.getElementById("Name"); node; node = node.nextSibling) {
>+    if (node.getAttribute("sortActive") == "true" && node.id != colID)
>+      node.removeAttribute("sortActive");
>+    if (node.getAttribute("sortDirection") && node.id != colID)
>+      node.removeAttribute("sortDirection");
>+  }
Nit: don't really need these checks because we'll fix things later.

>+    sortDirection = column.getAttribute("sortDirection") == "ascending"
>+                    ? "descending"
>+                    : "ascending";
Nit: Odd ?: wrapping.

>+  // Actuall sort the tree view
Nit: typo

>+  if (colID != "unsorted") {
Nit: if (column)

>+    // On Vista and above, we rely on native security prompting for
>+    // downloaded content.
Bah, I don't have a Vista build to try this with

>+  } else
>+    gDownloadStatus.label = "";
Not keen on } else without {

>+  if (aPathOrUrl.substring(0,7) == "file://") {
Nit: regexp?

>+    // XXX it's possible that using a null char-set here is bad
I doubt it, the bug only affects Linux, which we assume is UTF-8 anyway.

>+function replaceInsert(aText, aIndex, aValue)
Since this only has the one caller I think this should be moved nearer to it.

>+    // we can even enable some commands when we have no selection
>+    var ignoreSelection = (aCommand == "cmd_selectAll" ||
>+                           aCommand == "cmd_clearList");
I'm not sure that this check is worthwhile as the patch stands, as cmd_copyLocation is the only case without its own selectionCount check, so you might as well check there instead, even if you find you do need to restore this for the extra multiple selection handling code.

>+        var download = gDownloadManager.getDownload(selItemData.dlid);
>+        return selectionCount == 1 &&
>+               selItemData.isActive &&
>+               selItemData.state != nsIDownloadManager.DOWNLOAD_PAUSED &&
>+               download.resumable;
Perhaps add download.resumable to selItemData? You use it twice.

>+        // XXX handling multiple selection would be nice
Do we need these comments checked in? ;-)

>+    // we can even enable some commands when we have no selection
>+    var ignoreSelection = (aCommand == "cmd_selectAll" ||
>+                           aCommand == "cmd_clearList");
Not used?

>+      case "cmd_openReferrer":
>+        if (selItemData.referrer)
>+          openUILink(selItemData.referrer);
>+        // Otherwise, open the source
>+        else
>+          openUILink(selItemData.uri);
Surely this command is disabled if there is no referrer?

>+        gSearchBox.doCommand();
Why not call gDownloadTreeView.searchView directly?

>+  onEvent: function(aEvent){
>+    switch (aEvent) {
>+    case "tree-select":
>+      this.onCommandUpdate();
>+    }
>+  },
Not sure this version gets called, but never mind.

>+      </menu>
>+
>+      <menu id="menu_Edit">
Nit: Inconsistent spacing.

>+  QueryInterface: XPCOMUtils.generateQI([nsITreeView,
>+                                         Components.interfaces.nsISupports]),
generateQI includes nsISupports

>+  get selection() {
>+    return this._selection;
>+  },
>+
>+  set selection(val) {
>+    return this._selection = val;
>+  },
Don't need this, just declare a selection property.

>+    // active/notActive
We have a word for not active, it's "inactive" :-) s///g please

>+    switch (aColumn.id) {
>+      case "Name":
>+        return "moz-icon://" + this._dlList[aRow].file + "?size=16";
Hardly seems worth a switch for one case when an if will do (xN).

>+        if (dl.isActive)
>+          return (dl.maxBytes >= 0) ? nsITreeView.PROGRESS_NORMAL
>+                                    : nsITreeView.PROGRESS_UNDETERMINED;
>+        else
>+          return nsITreeView.PROGRESS_NONE;
>+    }
>+    return nsITreeView.PROGRESS_NONE;
Not only do you have else after return, but you're returning the value anyway.

>+            return this._dlbundle.getFormattedString("pausedpct",
>+              [DownloadUtils.getTransferTotal(dl.currBytes,
>+                                              dl.maxBytes)]);
I'm not sure it's worth including the percentage just in this case. Those users who really need it can always show the appropriate column.

>+    // Init lastSec for calculations of remaining time
>+    attrs.lastSec = Infinity;
Move to the object literal?

>+    // Data has changed, so re-sorting might be needed
>+    this.sortView("", "");
This is going to be very expensive :-( Maybe only do it for a state change?

>+      "SELECT id, target, name, source, state, startTime, endTime, referrer, " +
>+            "currBytes, maxBytes, state IN (?1, ?2, ?3, ?4, ?5) isActive " +
AS isActive
(Omitting the AS is lazy sqlite syntax)

>+    this._statement.bindInt32Parameter(0, nsIDownloadManager.DOWNLOAD_NOTSTARTED);
>+    this._statement.bindInt32Parameter(1, nsIDownloadManager.DOWNLOAD_DOWNLOADING);
>+    this._statement.bindInt32Parameter(2, nsIDownloadManager.DOWNLOAD_PAUSED);
>+    this._statement.bindInt32Parameter(3, nsIDownloadManager.DOWNLOAD_QUEUED);
>+    this._statement.bindInt32Parameter(4, nsIDownloadManager.DOWNLOAD_SCANNING);
Since these are always the same, I'm tempted to suggest using
state IN (" + nsIDownloadManager.DOWNLOAD_NOTSTARTED + ... + ") AS isActive"

>+      attrs.progress = attrs.isActive
>+                         ? this._dm.getDownload(attrs.dlid).percentComplete
>+                         : 100;
attrs.progress = attrs.isActive ?
                 this._dm.getDownload(attrs.dlid).percentComplete : 100;
Or
attrs.progress = 
    attrs.isActive ? this._dm.getDownload(attrs.dlid).percentComplete : 100;

>+      var matchSearch = true;
>+      for each (let term in this._searchTerms)
>+        if (combinedSearch.search(term) == -1)
>+          matchSearch = false;
No point doing this loop for active downloads.

>+    this._lastListIndex = 0; // we'll prepend other downloads with --!
Maybe we should reverse the sort order of the initial query instead?

>+    this._searchTerms = aInput.replace(/^\s+|\s+$/g, "")
Nit: .trim()

>+      // Re-sort in already selected/cached order
>+      aColumnID = this._listSortCol;
Maybe use getSortedColumn?

>+treechildren::-moz-tree-image(ActionPlay) {
>+  list-style-image: none;
>+  -moz-image-region: auto;
>+}
This should already be the default...

>+treechildren::-moz-tree-image(ActionStop) {
>+  list-style-image: none;
>+  -moz-image-region: auto;
>+}
And this is worse, because all downloads are either active or inactive.

Comment 18

[Robert Kaiser]

(In reply to comment #17)
> >+  // Clear attributes on previously sorted column
> >+  for (let node = document.getElementById("Name"); node; node = node.nextSibling) {
> >+    if (node.getAttribute("sortActive") == "true" && node.id != colID)
> >+      node.removeAttribute("sortActive");
> >+    if (node.getAttribute("sortDirection") && node.id != colID)
> >+      node.removeAttribute("sortDirection");
> >+  }
> Nit: don't really need these checks because we'll fix things later.

Where? How? In my testing, I needed this, or we'd end up with multiple rows showing sort indicators.

> >+    // XXX it's possible that using a null char-set here is bad
> I doubt it, the bug only affects Linux, which we assume is UTF-8 anyway.

In that case, should I just kill that copied comment?

> >+function replaceInsert(aText, aIndex, aValue)
> Since this only has the one caller I think this should be moved nearer to it.

I so much dislike it altogether, it would be so nice if we'd be able to use .getFormattedString() or so with plural forms and wouldn't need yet another way of doing things here. :(

> >+        var download = gDownloadManager.getDownload(selItemData.dlid);
> >+        return selectionCount == 1 &&
> >+               selItemData.isActive &&
> >+               selItemData.state != nsIDownloadManager.DOWNLOAD_PAUSED &&
> >+               download.resumable;
> Perhaps add download.resumable to selItemData? You use it twice.

You mean adding it right to the tree view's internal list of data?

> >+        gSearchBox.doCommand();
> Why not call gDownloadTreeView.searchView directly?

I guess because the onCommand() could in theory do more than just adjusting the view for this search. If we switch to directly calling the tree view here, we probably should point the oncommand attribute to that as well and kill the local function that calls there (toolkit does e.g. update clear list button state there, but we handle this with our controller).

> >+    // Data has changed, so re-sorting might be needed
> >+    this.sortView("", "");
> This is going to be very expensive :-( Maybe only do it for a state change?

The problem is that even without a state change, we want resorting when values for active downloads change - say we're sorting for download speed, and the speed changes for our multiple active downloads (same for progress etc. changing differently).
We could possibly go and only re-sort active downloads, but that makes the sort code more complicated and needs us to track state changes more carefully around here, and active/inactive change then need us to re-sort inactive downloads as well. If we really like to go and do that optimization, I think that should be done in a followup.

> >+      "SELECT id, target, name, source, state, startTime, endTime, referrer, " +
> >+            "currBytes, maxBytes, state IN (?1, ?2, ?3, ?4, ?5) isActive " +
> AS isActive
> (Omitting the AS is lazy sqlite syntax)

Ah, and I already wondered about that when I copied this statement from toolkit. ;-)

> >+    this._statement.bindInt32Parameter(0, nsIDownloadManager.DOWNLOAD_NOTSTARTED);
> >+    this._statement.bindInt32Parameter(1, nsIDownloadManager.DOWNLOAD_DOWNLOADING);
> >+    this._statement.bindInt32Parameter(2, nsIDownloadManager.DOWNLOAD_PAUSED);
> >+    this._statement.bindInt32Parameter(3, nsIDownloadManager.DOWNLOAD_QUEUED);
> >+    this._statement.bindInt32Parameter(4, nsIDownloadManager.DOWNLOAD_SCANNING);
> Since these are always the same, I'm tempted to suggest using
> state IN (" + nsIDownloadManager.DOWNLOAD_NOTSTARTED + ... + ") AS isActive"

Hmm, I think the statement is more readable the way it is right now, but I can do that if you insist.

> >+    this._lastListIndex = 0; // we'll prepend other downloads with --!
> Maybe we should reverse the sort order of the initial query instead?

Hmm, and then have an ascending sort for the initial order be a descending sort for the list index member? That avoids us using negative numbers there, but not sure if it would be more logical and need less commenting to be understood.

> >+      // Re-sort in already selected/cached order
> >+      aColumnID = this._listSortCol;
> Maybe use getSortedColumn?

Hmm, good idea, though we'd need to assume that null getSortedColumn means "unsorted" - and can we be sure that the tree's sorted column is always set accurately?

Comment 19

[Justin Wood (:Callek)]

(In reply to comment #18)
> (In reply to comment #17)
> > >+    // Data has changed, so re-sorting might be needed
> > >+    this.sortView("", "");
> > This is going to be very expensive :-( Maybe only do it for a state change?
> 
> The problem is that even without a state change, we want resorting when values
> for active downloads change - say we're sorting for download speed, and the
> speed changes for our multiple active downloads (same for progress etc.
> changing differently).
> We could possibly go and only re-sort active downloads, but that makes the sort
> code more complicated and needs us to track state changes more carefully around
> here, and active/inactive change then need us to re-sort inactive downloads as
> well. If we really like to go and do that optimization, I think that should be
> done in a followup.

Still did not do a code review, but we can probably get around the expensiveness via a few ways... (all of which I feel should be ok in followup)

1) Keep track of which columns require "realtime" re-sort, and only resort if those change state (and we are sorting on them). And we can avoid those checks all together if those columns hidden.
2) Only resort the ONE row we are receiving state change for. By using something similar to hg bisect with it (we already have updated sort for all other rows, and this potential changed order would be good, we can even test against both-sides of it in sort-order to see if a change is even needed)
3) disable sort on the "perf heavy" columns. Like transfer rate, and possibly progress.
...other options (again I feel unless it causes UI to be completely unresponsive in common case that a new bug is ok)

Comment 20

[Robert Kaiser]

Callek:
I think your option 2) sounds good to me, but as we both said, it's probably best to look for doing that in a followup.

Comment 21

[[e-mail]]

(In reply to comment #18)
>(In reply to comment #17)
>>>+  // Clear attributes on previously sorted column
>>>+  for (let node = document.getElementById("Name"); node; node = node.nextSibling) {
>>>+    if (node.getAttribute("sortActive") == "true" && node.id != colID)
>>>+      node.removeAttribute("sortActive");
>>>+    if (node.getAttribute("sortDirection") && node.id != colID)
>>>+      node.removeAttribute("sortDirection");
>>>+  }
>>Nit: don't really need these checks because we'll fix things later.
>Where? How? In my testing, I needed this, or we'd end up with multiple rows
>showing sort indicators.
Oh, you still need the loop, but just remove all the attributes unconditionally.

>>>+    // XXX it's possible that using a null char-set here is bad
>>I doubt it, the bug only affects Linux, which we assume is UTF-8 anyway.
>In that case, should I just kill that copied comment?
Sure.

>>>+function replaceInsert(aText, aIndex, aValue)
>>Since this only has the one caller I think this should be moved nearer to it.
>I so much dislike it altogether, it would be so nice if we'd be able to use
>.getFormattedString() or so with plural forms and wouldn't need yet another way
>of doing things here. :(
That might be possible, something like this:
>downloadsTitlePercent=%2$S%% of 1 file - Download Manager;%2$S%% of %1$S files - Download Manager

>>>+        var download = gDownloadManager.getDownload(selItemData.dlid);
>>>+        return selectionCount == 1 &&
>>>+               selItemData.isActive &&
>>>+               selItemData.state != nsIDownloadManager.DOWNLOAD_PAUSED &&
>>>+               download.resumable;
>>Perhaps add download.resumable to selItemData? You use it twice.
>You mean adding it right to the tree view's internal list of data?
Right.

>>>+        gSearchBox.doCommand();
>>Why not call gDownloadTreeView.searchView directly?
>I guess because the onCommand() could in theory do more than just adjusting the
>view for this search. If we switch to directly calling the tree view here, we
>probably should point the oncommand attribute to that as well and kill the
>local function that calls there (toolkit does e.g. update clear list button
>state there, but we handle this with our controller).
Calling the local function would be fine.

>>state IN (" + nsIDownloadManager.DOWNLOAD_NOTSTARTED + ... + ") AS isActive"
>Hmm, I think the statement is more readable the way it is right now, but I can
>do that if you insist.
Well, actually my preference would be for a comment and the numeric values, but do it whichever way you like best :-)

>>>+    this._lastListIndex = 0; // we'll prepend other downloads with --!
>>Maybe we should reverse the sort order of the initial query instead?
>Hmm, and then have an ascending sort for the initial order be a descending sort
>for the list index member? That avoids us using negative numbers there, but not
>sure if it would be more logical and need less commenting to be understood.
Well, starting at 0, counting up, then resetting to 0 and counting down isn't entirely the most obviously logical way either. If you don't want to use a descending sort for the list index, then just make them all negative ;-)

>>>+      // Re-sort in already selected/cached order
>>>+      aColumnID = this._listSortCol;
>>Maybe use getSortedColumn?
>Hmm, good idea, though we'd need to assume that null getSortedColumn means
>"unsorted" - and can we be sure that the tree's sorted column is always set
>accurately?
It should be, it looks for a column with the sortDirection attribute.

(In reply to comment #19)
>(In reply to comment #18)
>>(In reply to comment #17)
>>>>+    // Data has changed, so re-sorting might be needed
>>>>+    this.sortView("", "");
>>>This is going to be very expensive :-( Maybe only do it for a state change?
>>The problem is that even without a state change, we want resorting when values
>>for active downloads change - say we're sorting for download speed, and the
>>speed changes for our multiple active downloads (same for progress etc.
>>changing differently).
>2) Only resort the ONE row we are receiving state change for. By using
>something similar to hg bisect with it (we already have updated sort for all
>other rows, and this potential changed order would be good, we can even test
>against both-sides of it in sort-order to see if a change is even needed)
A trick I've just thought of here is that we know that the list is probably sorted, and the one row we are receiving state change for is the only one that might need to move, so we only need to invalidate the range between its old and new positions. Or only invalidate the whole tree if the row moved.

Comment 22

[Robert Kaiser]

(In reply to comment #21)
> A trick I've just thought of here is that we know that the list is probably
> sorted, and the one row we are receiving state change for is the only one that
> might need to move, so we only need to invalidate the range between its old and
> new positions. Or only invalidate the whole tree if the row moved.

So you're more concerned about the tree invalidation than the actual array sort? Or is that meant to be just in addition to the strategy of only resorting the one element we are updating? In any case, I'd really like to split this off to a followup, so that's the only comment not addressed (other than the constants in the statement) in the upcoming v3 patch.

Comment 23

[Robert Kaiser]

Attachment: (wrong patch v3: address more code-review comments)

This patch addresses the review comments and additionally adds another use of the .resumable member of our download array items in that it adds a property for it and we only display pause/resume icons if that's set. When I tested with "downloads" from a local file:// URL on an offline system, I realized that we displayed a pause icon there even if we cannot pause or resume such a download - this case if fixed with this new property.

Comment 24

[Robert Kaiser]

Attachment: patch v3: address more code-review comments

Sorry, had the wrong chrome paths (from the FF extension) in the last attachment, this is the real v3 (also fixing the CSS comment as pointed out on IRC).

Comment 25

[Robert Kaiser]

Comment on attachment 357896 [details] [diff] [review]
patch v3: address more code-review comments

>+    while (this._statement.executeStep()) {
>+      // Try to get the attribute values from the statement
>+      var attrs = {

I wonder if I should make this a let instead of var, actually.

(in the same loop)
>+      if (attrs.isActive) {
>+        var dld = this._dm.getDownload(attrs.dlid);

I converted this one to a let locally, I hope that's OK.

Comment 26

[[e-mail]]

Comment on attachment 357896 [details] [diff] [review]
patch v3: address more code-review comments

>+    if (sortedColumn && sortedColumn.id == colID) {
>+      sortDirection = sortedColumn.element.getAttribute("sortDirection");
>+    }
>+    else {
>+      sortDirection = "ascending";
>+    }
I'm not sure this logic is right. I need to go back and test it. With menus ;-)

>+      if (sysInfo.getProperty("name").match(/^Windows/) &&
/^Windows/.test(...)

>+  var title;
>+  if (base == 0)
>+    title = dlbundle.getFormattedString("downloadsTitleFiles",
>+                                        [numActiveDownloads]);
Hmm... in theory we could fetch this title, but never use it because neither the mean nor number of downloads has changed.

>+  if (aPathOrUrl.match(/^file:\/\//)) {
/^file:\/\//.test(aPathOrUrl)

>+<?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?>
>+<?xul-overlay href="chrome://global/content/editMenuOverlay.xul"?>
So, what's in editMenuOverlay.xul that utilityOverlay.xul doesn't have?

Comment 27

[[e-mail]]

Comment on attachment 357896 [details] [diff] [review]
patch v3: address more code-review comments

>+  // If the target is a menuitem, handle it and forward to a column
>+  if (colID.match(/^menu_SortBy/)) {
>+    colID = colID.replace(/^menu_SortBy/, "");
>+    column = document.getElementById(colID);
>+    var sortedColumn = gDownloadTree.columns.getSortedColumn();
>+    if (sortedColumn && sortedColumn.id == colID) {
>+      sortDirection = sortedColumn.element.getAttribute("sortDirection");
>+    }
>+    else {
>+      sortDirection = "ascending";
>+    }
The original XPFE history window made it so both the menuitems and column headers would (assuming you kept toggling the same column each time) toggle through ascending/descending/unsorted (actually, the current history window omits unsorted, that's a mistake on my part.) While this code doesn't do that, that doesn't necessarily make it incorrect; making it so that the currently selected menuitem does nothing is an equally valid interpretation.

>+  if (!sortDirection) {
>+    // If not set above already, toggle the current direction
>+    sortDirection = column.getAttribute("sortDirection") == "ascending" ?
>+                    "descending" : "ascending";
For some reason this doesn't seem to work; the column headers refused to cycle and the only way I could get a descending sort was via the descending menuitem.

Comment 28

[Robert Kaiser]

(In reply to comment #27)
> The original XPFE history window made it so both the menuitems and column
> headers would (assuming you kept toggling the same column each time) toggle
> through ascending/descending/unsorted (actually, the current history window
> omits unsorted, that's a mistake on my part.) While this code doesn't do that,
> that doesn't necessarily make it incorrect; making it so that the currently
> selected menuitem does nothing is an equally valid interpretation.

So, should we change it for consistency (now or in a followup?) or leave it as-is?

> For some reason this doesn't seem to work; the column headers refused to cycle
> and the only way I could get a descending sort was via the descending menuitem.

I know the reason, what I suspected when I read this comment is correct: The change to always deleting those attributes made us end up there without the attribute. Shifting the two blocks should care for that.

Comment 29

[Robert Kaiser]

(In reply to comment #26)
> >+  var title;
> >+  if (base == 0)
> >+    title = dlbundle.getFormattedString("downloadsTitleFiles",
> >+                                        [numActiveDownloads]);
> Hmm... in theory we could fetch this title, but never use it because neither
> the mean nor number of downloads has changed.

What case would we have where we need to show this title instead of the one already set but have no change in mean or number? Or is what you want to say that we also should only fetch it if we actually need to change the title?

> >+  if (aPathOrUrl.match(/^file:\/\//)) {
> /^file:\/\//.test(aPathOrUrl)

I'm changing all String.match(/regexp/) to /regexp/.test(String) in my patch - I heard this is faster, right?

> >+<?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?>
> >+<?xul-overlay href="chrome://global/content/editMenuOverlay.xul"?>
> So, what's in editMenuOverlay.xul that utilityOverlay.xul doesn't have?

IMHO, we should consequently move to using editMenuOverlay, sharing it with the other toolkit apps, and finally go and remove the duplicated stuff from utilityOverlay. And at least cMenu_selectAll is something that only editMenuOverlay has.

Comment 30

[[e-mail]]

(In reply to comment #28)
> So, should we change it for consistency (now or in a followup?) or leave it as-is?
Well, I can't really claim consistency :-)

(In reply to comment #29)
> Or is what you want to say
> that we also should only fetch it if we actually need to change the title?
Well, I was wondering how difficult that would be to achieve.

> I'm changing all String.match(/regexp/) to /regexp/.test(String) in my patch -
> I heard this is faster, right?
Right; match() returns an array, test() only a boolean.

> cMenu_selectAll is something that only editMenuOverlay has.
Not exactly the most inspiring menuitem id though ;-)
[Other windows don't seem to attempt to share context menuitems]

Comment 31

[Robert Kaiser]

Attachment: patch v3.1: some additional small fixes

This patch adds a few small fixes per the latest comments - it looks like this is becoming a quite stable state now.

Comment 32

[[e-mail]]

Comment on attachment 359916 [details] [diff] [review]
patch v3.1: some additional small fixes

The small fixes look fine but I admit I didn't test this version.

Comment 33

[Robert Kaiser]

Attachment: add tests, v0

This is a first version of the tests ported from the toolkit/mozapps UI to our new UI. The browser test is quite simple and just ensures that showManager() works.
The chrome tests currently result in this:
*** 130 INFO Passed: 93
*** 131 INFO Failed: 0
*** 132 INFO Todo:   8

I filed bugs for the items I marked TODO or where I needed to change what is being tested because we still fail to implement something.
I also filed a bug for porting/creating even more tests, though there's no low-hanging fruit for porting any more.

This patch is basically working fine, but I'm marking it "v0" because it's still full of Cc/Ci/Cr stuff.

Comment 34

[Robert Kaiser]

Attachment: patch v3.2: fixes for running tests

This patch mainly has updates that fix running the tats attached here as well as the generic tests on toolkit - this includes correcting some functionality that was somewhat broken before (mostly uncovered by tests for that functionality).

Requesting code review for those changes, will ask for sr when Callek has a new backend patch this can be tested with (i have it running with a slightly modified backend patch).

Comment 35

[Robert Kaiser]

Attachment: download manager UI tests, v1

OK, here's the tests without any Cc/Ci stuff in them. Followups filed for the TODOs that are still in there (I still like to add the tests for those so that we have the test code around).

Comment 36

[[e-mail]]

Comment on attachment 371928 [details] [diff] [review]
patch v3.2: fixes for running tests

>+    switch (aTopic) {
>+      case "download-manager-remove-download":
>+        // A null subject here indicates "remove multiple", so we just rebuild.
>+        if (!aSubject) {
>+          // Rebuild the default view
>+          gDownloadTreeView.initTree();
>+          break;
>+        }
>+
>+        // Otherwise, remove a single download
>+        var id = aSubject.QueryInterface(Components.interfaces.nsISupportsPRUint32);
>+        gDownloadTreeView.removeDownload(id);
I wonder why they didn't simply pass the nsIDownload itself as the subject...
Anyway, one issue is that you're using implicit conversions to get your Uint32 data value out of your subject, and I don't like that coding style, so...
if (aSubject instanceof Components.interfaces.nsISupportsPRUint32)
  // We have a single download.
  gDownloadTreeView.removeDownload(aSubject.data);
else
  // Rebuild the view.
  gDownloadTreeView.initTree();
r=me with that fixed.

>+    <key id="key_close"/>
>+    <key id="key_quit"/>
>+    <!-- Edit Menu -->
>+    <key id="key_cut"/>
>+    <key id="key_copy"/>
>+    <key id="key_delete"/>
>+    <key id="key_delete2"/>
>+    <key id="key_selectAll"/>
>+    <!-- Search Box -->
>+    <key id="key_search_focus"
>+         command="cmd_search_focus"
>+         key="&search.key;"
>+         modifiers="accel"/>
>+    <key keycode="VK_ESCAPE" oncommand="window.close();"/>
Ugh :-(

>+      let matchSearch = true;
Previous patch correctly used var here and other places ;-)

Comment 37

[[e-mail]]

Comment on attachment 372075 [details] [diff] [review]
download manager UI tests, v1

Forgot to actually tick the box ;-)

Comment 38

[[e-mail]]

D'oh, that was a different patch...

Comment 39

[[e-mail]]

Comment on attachment 372075 [details] [diff] [review]
download manager UI tests, v1

>diff --git a/suite/common/downloads/tests/browser/browser_nsISuiteDownloadManagerUI.js b/suite/common/downloads/tests/browser/browser_nsISuiteDownloadManagerUI.js
>new file mode 100644
Unused? r- because of this.

>+  // The window doesn't open once we call show, so we need to wait a little bit
[This should have used the observer]

>+      let win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
This doesn't need a QI, GlobalChromeWindows have class info.

>+    ok(!dmui.visible, "DMUI closes with ESC key");
Would rather not have this ;-)

>+  // Close the DM UI if it is already open
>+  let dm_win = getDMWindow();
>+  if (dm_win) dm_win.close();
Why is this a different style to the other tests?

Comment 40

[Robert Kaiser]

(In reply to comment #36)
> >+    <key keycode="VK_ESCAPE" oncommand="window.close();"/>
> Ugh :-(

I actually thought (and still think) it's probably a good idea, not just because toolkit's version does it, but if you insist, I can live without it.

> >+      let matchSearch = true;
> Previous patch correctly used var here and other places ;-)

I only changed this one because it's inside a while loop and it sounded like a good idea to have it scoped only to that, and even more in the combinedSearch case that only exists inside it's if block now. I think those two changes are reasonable, right? (I didn't touch any others, judging by an interdiff).


(In reply to comment #39)
> (From update of attachment 372075 [details] [diff] [review])
> >diff --git a/suite/common/downloads/tests/browser/browser_nsISuiteDownloadManagerUI.js b/suite/common/downloads/tests/browser/browser_nsISuiteDownloadManagerUI.js
> >new file mode 100644
> Unused? r- because of this.

Erm, this file is all that's needed to actually run that test, and from what I saw, it runs fine in the browser-chrome suite.

> >+  // The window doesn't open once we call show, so we need to wait a little bit
> [This should have used the observer]
> 
> >+      let win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> This doesn't need a QI, GlobalChromeWindows have class info.

I copied both of those from the toolkit tests, if/when we change those, we should care that bugs against their tests are at least filed as well.

> >+  // Close the DM UI if it is already open
> >+  let dm_win = getDMWindow();
> >+  if (dm_win) dm_win.close();
> Why is this a different style to the other tests?

This test seems to be the only one using utils.js, which nicely abstracts some things the other tests are doing manually. I can either change this test to also not use utils.js or make the others use it as well (in which case I'd very much like to leave this to a followup).

Comment 41

[Justin Wood (:Callek)]

Attachment: integration

This is the patch (on top of my current DLMGR backend patch) that makes the Tree-Based UI Work. Also makes use of the Bug 483781 pref so we can be sure to pass those tests as well.

Comment 42

[[e-mail]]

(In reply to comment #40)
> (In reply to comment #36)
> > >+    <key keycode="VK_ESCAPE" oncommand="window.close();"/>
> > Ugh :-(
> I actually thought (and still think) it's probably a good idea, not just
> because toolkit's version does it, but if you insist, I can live without it.
It's very unusual for a window with menus and toolbars to close on Escape. (It always bugs me that the standalone message window does. Maybe I should file a bug and try and convince Mnyromyr to change it.)

> > >+      let matchSearch = true;
> > Previous patch correctly used var here and other places ;-)
> I only changed this one because it's inside a while loop and it sounded like a
> good idea to have it scoped only to that, and even more in the combinedSearch
> case that only exists inside it's if block now. I think those two changes are
> reasonable, right? (I didn't touch any others, judging by an interdiff).
In fact it was an interdiff that hid the fact that it's inside a loop!

> (In reply to comment #39)
> > (From update of attachment 372075 [details] [diff] [review] [details])
> > >diff --git a/suite/common/downloads/tests/browser/browser_nsISuiteDownloadManagerUI.js b/suite/common/downloads/tests/browser/browser_nsISuiteDownloadManagerUI.js
> > >new file mode 100644
> > Unused? r- because of this.
> Erm, this file is all that's needed to actually run that test, and from what I
> saw, it runs fine in the browser-chrome suite.
OK, but I don't see this requiring browser chrome to run, so is it possible to convert this into a mochichrome test? (A browser-chrome test would make more sense if you were, say, testing the toDownloadManager function.)

> I copied both of those from the toolkit tests, if/when we change those, we
> should care that bugs against their tests are at least filed as well.
The first is definitely a bug that should be filed against toolkit, because they should not be relying on random timeouts. The second one I'd like fixed here, even if toolkit decide to ignore it in their version.

> > >+  // Close the DM UI if it is already open
> > >+  let dm_win = getDMWindow();
> > >+  if (dm_win) dm_win.close();
> > Why is this a different style to the other tests?
> This test seems to be the only one using utils.js, which nicely abstracts some
> things the other tests are doing manually. I can either change this test to
> also not use utils.js or make the others use it as well (in which case I'd very
> much like to leave this to a followup).
Sure, standardise on not using utils.js and then convert them all later.

Comment 43

[[e-mail]]

Comment on attachment 372458 [details] [diff] [review]
integration

>         behavior = prefs.getIntPref(PREF_DM_BEHAVIOR);
>+        if (prefs.getBoolPref(PREF_FORCE_TOOLKIT_UI))
>+          behavior = 0;
No point changing the behaviour and then changing it back again...

Comment 44

[Justin Wood (:Callek)]

(In reply to comment #43)
> (From update of attachment 372458 [details] [diff] [review])
> >         behavior = prefs.getIntPref(PREF_DM_BEHAVIOR);
> >+        if (prefs.getBoolPref(PREF_FORCE_TOOLKIT_UI))
> >+          behavior = 0;
> No point changing the behaviour and then changing it back again...

I only did it this way so I could use the same try/catch block, it is MUCH more likely that the getBoolPref will throw from not being set than the PREF_DM_BEHAVIOR will. I'd rather not need to introduce two try-catch blocks here if possible. setting an int is relatively cheap js wise, while try-catch is much more expensive.

Comment 45

[Robert Kaiser]

Comment on attachment 372458 [details] [diff] [review]
integration

This works nicely with my patch and makes us execute the toolkit UI tests successfully with the patch to bug 483781 which I'm attaching right now.

Comment 46

[Robert Kaiser]

Attachment: patch v3.3: applies cleanly on top of Callek's patches

This is a version of the main patch that applies cleanly on top of Callek's patches, works well with the integration patch here and fixes the one real review comment from Neil.
It doesn't take care of the ESC key, I'm still somewhat undecided on removing it or not.

We still have another round to go though, and as the patches are starting to come together now, we have patches for the full line and the UI can be tested with those, I'm requesting sr now. We can clear any remaining issues along with that.
Neil, if your final word on close-on-ESC is "no", please state it in sr and it's pretty easy to remove.

Comment 47

[Robert Kaiser]

Attachment: download manager UI tests, v2

(In reply to comment #42)
> OK, but I don't see this requiring browser chrome to run, so is it possible to
> convert this into a mochichrome test? (A browser-chrome test would make more
> sense if you were, say, testing the toDownloadManager function.)

We should at least also test opening the progress dialogs, I'm not yet 100% sure what the differences between chrome and browser tests is, and toolkit has this as browser tests. Additionally, we already have chrome tests that open and close the manager, so I left this.

All other comments on tests should be addressed in this patch.

Comment 48

[[e-mail]]

Comment on attachment 373041 [details] [diff] [review]
download manager UI tests, v2

A couple of nits:
In one place you use dom_win where everwhere else you use win
Random mixture of var and let; I'll let(!) you choose

Comment 49

[[e-mail]]

(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 372458 [details] [diff] [review] [details])
> > >         behavior = prefs.getIntPref(PREF_DM_BEHAVIOR);
> > >+        if (prefs.getBoolPref(PREF_FORCE_TOOLKIT_UI))
> > >+          behavior = 0;
> > No point changing the behaviour and then changing it back again...
> I only did it this way so I could use the same try/catch block, it is MUCH more
> likely that the getBoolPref will throw from not being set than the
> PREF_DM_BEHAVIOR will. I'd rather not need to introduce two try-catch blocks
> here if possible. setting an int is relatively cheap js wise, while try-catch
> is much more expensive.
Assuming we're not setting a default value for the pref then we're usually going to throw, so we'll always take the expensive path anyway.

Comment 50

[[e-mail]]

Comment on attachment 373001 [details] [diff] [review]
patch v3.3: applies cleanly on top of Callek's patches

No ESC, and that's my final anwser ;-)

Comment 51

[[e-mail]]

Oh, one thing I forgot to ask... are you planning on adding a Properties menu and button to open a progress dialog from the download manager?

Comment 52

[Robert Kaiser]

Attachment: patch v3.4: everything addressed, ready for checkin

This patch has the ESC key removed, everything else identical to last patch, ready for checkin when Callek's backend patch lands, carrying forward r+sr.

(In reply to comment #51)
> Oh, one thing I forgot to ask... are you planning on adding a Properties menu
> and button to open a progress dialog from the download manager?

Yes, that's bug 474620.

(In reply to comment #48)
> Random mixture of var and let; I'll let(!) you choose

You mean I should go through all those declarations I copied from toolkit, analyze them and decide on one way? D'Oh! Though, I guess the test files are already far enough changed from the toolkit tests that making porting test fixes over isn't realistic any more in any case... :-/

Comment 53

[Justin Wood (:Callek)]

Attachment: alternate integration patch

Neil, this (hopefully) addresses your concerns with the first version of the integration patch. If you like the first one better we can still go with that.

Comment 54

[Robert Kaiser]

Comment on attachment 374905 [details] [diff] [review]
alternate integration patch

Works fine here, passes all tests I have here.

Comment 55

[Robert Kaiser]

Attachment: download manager UI tests, v2.1: ready for checkin

Here's the download UI tests, with the nits fixed and ready for checkin. regarding declarations, I changed all top-level and directly-in-function let calls to var, and did let (!) all other declarations (in loops/blocks) unchanged. Carrying forward the review.

Comment 56

[[e-mail]]

(In reply to comment #41)
> This is the patch (on top of my current DLMGR backend patch) that makes the
> Tree-Based UI Work. Also makes use of the Bug 483781 pref so we can be sure to
> pass those tests as well.
Is there a reason that showManager needs to show the toolkit UI? Otherwise I would just suggest making show open the toolkit UI directly.

Comment 57

[Robert Kaiser]

(In reply to comment #56)
> Is there a reason that showManager needs to show the toolkit UI? Otherwise I
> would just suggest making show open the toolkit UI directly.

The main reason is extensions here. If the user installs an extension that make they hyper-super-duper thing out of the toolkit UI, it should be able to set that pref and make us show that one instead of our own. On the other hand, any extension that just has a button or such to open "the download manager" should end up showing our UI just by getting the generic toolkit contract ID and call the generic .show() function.

Comment 58

[[e-mail]]

Comment on attachment 374905 [details] [diff] [review]
alternate integration patch

Let's say I like this one less ;-)

Comment 59

[[e-mail]]

Comment on attachment 372458 [details] [diff] [review]
integration

>                   "Download:Manager",
IMHO this should just be the empty string.
(Window watcher names are set in XUL.)

Comment 60

[Robert Kaiser]

Main patch pushed as http://hg.mozilla.org/comm-central/rev/aea638bfac91
Integration pushed as http://hg.mozilla.org/comm-central/rev/12636893a8e3
Tests pushed as http://hg.mozilla.org/comm-central/rev/5d9f0b2e8544

Comment 77

[Robert Kaiser]

Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query