474492 - Update the downloads manager UI

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

Wireframes for this section here:
https://wiki.mozilla.org/Mobile/UI/Designs/TouchScreen/workingUI/downloads_concepts

uses the same vertically-pannable list with rows that expand when tapped model as the add-ons manager and the prefs panel.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 1

This patch adds the downloads manager directly into the Fennec UI. It is mostly complete. I need to add support for alerts and some additional error checking.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

Here is a decent reviewable patch. It supports:
* Basic downloading progress, with pause & cancel
* Resume paused downloads
* Retry failed or canceled downloads
* Completed downloads can be found-on-disk, opened and removed
* Completed downloads can open the page where the download was initiated in the browser

We don't currently display parental blocked downloads. Do we want to?
*  can be removed

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

We also support:
* Sorting the downloads list

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 372812 [details] [diff] [review]
patch

>+  get visible() {
>+    let wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
>+    let browser = wm.getMostRecentWindow("navigator:browser");
>+    return (browser != null);
>+  },
This isn't going to be accurate though, right?  Is there a way to make sure that that particular view is open?

Also, I've seen SeaMonkey, and now you guys re-implement the DownloadProgressListener because it's not quite abstracted enough in toolkit.  Might I suggest someone file a bug to abstract that out in a more usable way (like DownloadUtils.jsm; probably even move it there).

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #4)
> (From update of attachment 372812 [details] [diff] [review])
> >+  get visible() {
> >+    let wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
> >+    let browser = wm.getMostRecentWindow("navigator:browser");
> >+    return (browser != null);
> >+  },
> This isn't going to be accurate though, right?  Is there a way to make sure
> that that particular view is open?

Good point. I can add a better check.

> Also, I've seen SeaMonkey, and now you guys re-implement the
> DownloadProgressListener because it's not quite abstracted enough in toolkit. 
> Might I suggest someone file a bug to abstract that out in a more usable way
> (like DownloadUtils.jsm; probably even move it there).

Probably a good idea. Thanks for taking a look at this.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #4)

> This isn't going to be accurate though, right?  Is there a way to make sure
> that that particular view is open?

Added changes locally waiting for more review comments. Switched to:

+  get visible() {
+    let wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
+    let browser = wm.getMostRecentWindow("navigator:browser");
+    if (browser) {
+      return browser.DownloadsView.visible;
+    }
+    return false;
+  },

Which uses this in DownloadsView:

+  get visible() {
+    let panel = document.getElementById("panel-container");
+    let items = document.getElementById("panel-items");
+    if (panel.hidden == false && items.selectedItem.id == "downloads-container")
+      return true;
+    return false;
+  },

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 372812 [details] [diff] [review]
patch

>diff --git a/chrome/content/bindings/downloads.xml b/chrome/content/bindings/downloads.xml

>+  <binding id="download-base" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">

Add a field for Components.interfaces.nsIDownloadManager?

>+  <binding id="download-downloading" extends="chrome://browser/content/bindings/downloads.xml#download-base">
>+   <content orient="vertical">
>+      <xul:hbox align="start">

This doesn't make a lot of sense... can you remove the hbox and change to orient="horizontal"? Applies to the other bindings as well.

>diff --git a/chrome/content/downloads.js b/chrome/content/downloads.js

>+var DownloadsView = {

>+  _getLocalFile: function dv__getLocalFile(aPathOrUrl) {

I think we can get rid of the non-file:/// URL case, since our download DB should store those consistently.

>+  _getReferrerOrSource: function dv__getReferrerOrSource(aItem) {
>+    // Give the referrer if we have it set
>+    if (aItem.hasAttribute("referrer"))
>+      return aItem.getAttribute("referrer");
>+
>+    // Otherwise, provide the source
>+    return aItem.getAttribute("uri");

Skipping the hasAttribute() and just returning getAttribute() || getAttribute() would actually avoid a trip through xpconnect, and the getAttribute call already short-circuits based on attribute presence... unless you need to support returning "" in the referrer="" case, but that seems unlikely to be possible.

>+  _createItem: function dv__createItem(aAttrs) {

>+      let file = this._getLocalFile(aAttrs.file);
>+      item.setAttribute("path", file.nativePath || file.path);

nativePath is noscript, so it won't ever be there. I don't see what this attribute is used for, though... can we just remove it?

>+  getDownloads: function dv_getDownloads() {

>+    // Array of space-separated lower-case search terms
>+    let search = document.getElementById("downloads-search-text");
>+    this._searchTerms = search.value.replace(/^\s+|\s+$/g, "").toLowerCase().split(/\s+/);

Can use trim() instead of replace(/^\s+|\s+$/g, "").

>+    // Clear the list before adding items by replacing with a shallow copy
>+    let (empty = this._list.cloneNode(false)) {
>+      this._list.parentNode.replaceChild(empty, this._list);
>+      this._list = empty;
>+    }

Hrm, is this really better than just removing all the children? I guess downloads.js does it this way.

>+    try {
>+      this._stmt.bindInt32Parameter(0, Ci.nsIDownloadManager.DOWNLOAD_NOTSTARTED);
>+      this._stmt.bindInt32Parameter(1, Ci.nsIDownloadManager.DOWNLOAD_DOWNLOADING);
>+      this._stmt.bindInt32Parameter(2, Ci.nsIDownloadManager.DOWNLOAD_PAUSED);
>+      this._stmt.bindInt32Parameter(3, Ci.nsIDownloadManager.DOWNLOAD_QUEUED);
>+      this._stmt.bindInt32Parameter(4, Ci.nsIDownloadManager.DOWNLOAD_SCANNING);
>+    }
>+    catch (e) {
>+      // Something must have gone wrong when binding, so clear and quit
>+      this._stmt.reset();
>+      return;
>+    }

sdwilsh tells me that this try/catch is unnecessary (here and in the original downloads.js), since the bind* calls are quite unlikely to fail, especially if the stmt.reset() call earlier in this method didn't. I suppose it doesn't hurt too much to leave them.

>+  _stepDownloads: function dv__stepDownloads(aNumItems) {

>+      if (!this._stmt.executeStep()) {
>+        // Send a notification that we finished, but wait for clear list to update
>+        //updateClearListButton();

Are we planning on having a clear list button?

>+  _matchesSearch: function dv__matchesSearch(aItem) {

>+    const searchAttributes = ["target", "status", "datetime"];

>+    for each (let attr in searchAttributes)
>+      combinedSearch += aItem.getAttribute(attr).toLowerCase() + " ";

Could we store combinedSearch as a JS prop on the item in createItem? Or at least cache it here for subsequent matchesSearch calls? I suppose "status" and "datetime" can change, but we should be able to also update the cache in _updateStatus and _updateTime. Followup, perhaps.

>+    // Make sure each of the terms are found
>+    for each (let term in this._searchTerms)
>+      if (combinedSearch.search(term) == -1)

I think indexOf should be preferred over search for the string constant case, though it probably doesn't matter that much. Doesn't matter as much if we do the cache of combinedSearch, but this method could shortcircuit if this._searchTerms.length==0 right?

>+  downloadCompleted: function dv_downloadCompleted(aDownload) {

>+    notifier.showAlertNotification(URI_GENERIC_ICON_DOWNLOAD, strings.getString("alertDownloads"),
>+                                   strings.getString("alertDownloadsDone"), true, "", this);

alertDownloadsDone is "all files have finished downloading", but doesn't this method get called whenever any file is complete?

>+    let element = this.getElementForDownload(aDownload.id);
>+    element.setAttribute("progress", "100");

Hmm, are we not garanteed a final onProgressChange with percentComplete=100? I guess it doesn't really hurt to make sure.

>+  _updateStatus: function dv__updateStatus(aItem) {

>+  _updateTime: function dv__updateTime(aItem) {

Looks like the strings are included for these in the patch, but not used:

>diff --git a/locales/en-US/chrome/browser.properties b/locales/en-US/chrome/browser.properties

>+# Download Manager
>+downloadsDoneSize=#1 #2
>+donwloadsYesterday=Yesterday
>+downloadsMonthDate=#1 #2

>+  toggleMode: function dv_toggleMode() {
>+    let mode = document.getElementById("downloads-sort-mode");
>+    if (mode.value == "search") {
>+      document.getElementById("downloads-search-box").collapsed = false;

Shouldn't this call getDownloads() too, to reset the list?

>+  cancelDownload: function dv_cancelDownload(aItem) {
>+    this._dlmgr.cancelDownload(aItem.getAttribute("downloadID"));
>+    var f = this._getLocalFile(aItem.getAttribute("file"));
>+    if (f.exists())
>+      f.remove(false);

Hmm, seems odd that removing the file isn't handled by the backend.

>+  retryDownload: function dv_retryDownload(aItem) {
>+    this._removeItem(aItem);
>+    this._dlmgr.retryDownload(aItem.getAttribute("downloadID"));

I guess retryDownload triggers downloadStarted again?

>+DownloadProgressListener.prototype = {

>+    // We should eventually know the referrer at some point
>+    let referrer = aDownload.referrer;
>+    if (referrer)
>+      element.setAttribute("referrer", referrer.spec);

Worth adding a property on the element to avoid unnecessary setAttribute calls?

>+  onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress, aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress, aDownload) {

>+    // Dispatch ValueChange for a11y
>+    var event = document.createEvent("Events");
>+    event.initEvent("ValueChange", true, true);
>+    document.getAnonymousElementByAttribute(element, "anonid", "progressmeter").dispatchEvent(event);

Hmm... perhaps the binding should instead have setters that map to its progressmeter's "value" and "mode" properties, which take care of this event firing for you.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #7)
> (From update of attachment 372812 [details] [diff] [review])
> 
> Add a field for Components.interfaces.nsIDownloadManager?

Done

> >+   <content orient="vertical">
> >+      <xul:hbox align="start">
> 
> This doesn't make a lot of sense... can you remove the hbox and change to
> orient="horizontal"? Applies to the other bindings as well.

Done. Early tweaking that never got switched back

> >+  _getLocalFile: function dv__getLocalFile(aPathOrUrl) {
> 
> I think we can get rid of the non-file:/// URL case, since our download DB
> should store those consistently.

Removed URI sniffing code

> >+  _getReferrerOrSource: function dv__getReferrerOrSource(aItem) {
> >+    // Give the referrer if we have it set
> >+    if (aItem.hasAttribute("referrer"))
> >+      return aItem.getAttribute("referrer");
> >+
> >+    // Otherwise, provide the source
> >+    return aItem.getAttribute("uri");
> 
> Skipping the hasAttribute() and just returning getAttribute() || getAttribute()
> would actually avoid a trip through xpconnect, and the getAttribute call
> already short-circuits based on attribute presence... unless you need to
> support returning "" in the referrer="" case, but that seems unlikely to be
> possible.

Changed to getAttribute("referrer") || getAttribute("uri")

> >+      let file = this._getLocalFile(aAttrs.file);
> >+      item.setAttribute("path", file.nativePath || file.path);
> 
> nativePath is noscript, so it won't ever be there. I don't see what this
> attribute is used for, though... can we just remove it?

Removed "path" code. It wasn't used anywhere I could see either

> >+    // Array of space-separated lower-case search terms
> >+    let search = document.getElementById("downloads-search-text");
> >+    this._searchTerms = search.value.replace(/^\s+|\s+$/g, "").toLowerCase().split(/\s+/);
> 
> Can use trim() instead of replace(/^\s+|\s+$/g, "").

Changed

> >+      if (!this._stmt.executeStep()) {
> >+        // Send a notification that we finished, but wait for clear list to update
> >+        //updateClearListButton();
> 
> Are we planning on having a clear list button?

Not if Madhava has anything to say about it :) Removed the commented code

> >+    // Make sure each of the terms are found
> >+    for each (let term in this._searchTerms)
> >+      if (combinedSearch.search(term) == -1)
> 
> I think indexOf should be preferred over search for the string constant case,
> though it probably doesn't matter that much. Doesn't matter as much if we do
> the cache of combinedSearch, but this method could shortcircuit if
> this._searchTerms.length==0 right?

Added shortcircuit for this._searchTerms.length==0
Added combindSearch cleanup in followup bug

> >+    notifier.showAlertNotification(URI_GENERIC_ICON_DOWNLOAD, strings.getString("alertDownloads"),
> >+                                   strings.getString("alertDownloadsDone"), true, "", this);
> 
> alertDownloadsDone is "all files have finished downloading", but doesn't this
> method get called whenever any file is complete?

Yes. Changed message text to include the name of the completed file

> >+    let element = this.getElementForDownload(aDownload.id);
> >+    element.setAttribute("progress", "100");
> 
> Hmm, are we not garanteed a final onProgressChange with percentComplete=100? I
> guess it doesn't really hurt to make sure.

I saw some cases that didn't it the 100 for some reason. Added to followup bug

> >+  _updateStatus: function dv__updateStatus(aItem) {
> 
> >+  _updateTime: function dv__updateTime(aItem) {
> 
> Looks like the strings are included for these in the patch, but not used:

Ugh, forgot to use them. Changed code to use the strings

> >+  toggleMode: function dv_toggleMode() {
> >+    let mode = document.getElementById("downloads-sort-mode");
> >+    if (mode.value == "search") {
> >+      document.getElementById("downloads-search-box").collapsed = false;
> 
> Shouldn't this call getDownloads() too, to reset the list?

Not sure. But I did add code to clear the search terms textbox.

> >+  retryDownload: function dv_retryDownload(aItem) {
> >+    this._removeItem(aItem);
> >+    this._dlmgr.retryDownload(aItem.getAttribute("downloadID"));
> 
> I guess retryDownload triggers downloadStarted again?

Yep

> >+    // We should eventually know the referrer at some point
> >+    let referrer = aDownload.referrer;
> >+    if (referrer)
> >+      element.setAttribute("referrer", referrer.spec);
> 
> Worth adding a property on the element to avoid unnecessary setAttribute calls?

We use the "referrer" attribute elsewhere, but I added a check to avoid the need to call setAttribute everytime.

> >+  onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress, aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress, aDownload) {
> 
> >+    // Dispatch ValueChange for a11y
> >+    var event = document.createEvent("Events");
> >+    event.initEvent("ValueChange", true, true);
> >+    document.getAnonymousElementByAttribute(element, "anonid", "progressmeter").dispatchEvent(event);
> 
> Hmm... perhaps the binding should instead have setters that map to its
> progressmeter's "value" and "mode" properties, which take care of this event
> firing for you.

Added to followup bug. I wonder if add-ons might make use of the progress for some reason. Probably not.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #8)

> > >+  _getLocalFile: function dv__getLocalFile(aPathOrUrl) {
> > 
> > I think we can get rid of the non-file:/// URL case, since our download DB
> > should store those consistently.
> 
> Removed URI sniffing code

Ugh. After some testing, I had to add it back. Many of the paths passed in were of the form "file:///"

> > >+    let element = this.getElementForDownload(aDownload.id);
> > >+    element.setAttribute("progress", "100");
> > 
> > Hmm, are we not garanteed a final onProgressChange with percentComplete=100? I
> > guess it doesn't really hurt to make sure.
> 
> I saw some cases that didn't it the 100 for some reason. Added to followup bug

But I didn't see it in any of my current tests, so I removed it

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Followup is bug 489223

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> Ugh. After some testing, I had to add it back. Many of the paths passed in were
> of the form "file:///"

Do you know where they were coming from? Would be nice to figure this out in a followup.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Oops, I misunderstood. You wanted the initWithPath side of the _getLocalFile method to be removed. Removed that code and everything still works.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch for checkin

Updated patch based on review comments. Carrying r+ forward. I'll check this version in.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

nit: s/dlmgr/nsIDLMGR/, or something?

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #14)
> nit: s/dlmgr/nsIDLMGR/, or something?

Done. nsIDLMgr

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

With a last minute move of the DownloadsView.init() call to BrowserUI.init() - so the listeners were hooked up regardless of whether or not the panel was ever visible.

http://hg.mozilla.org/mobile-browser/rev/86b92466d464

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

verified with beta3