Closed Bug 474492 Opened 15 years ago Closed 15 years ago

Update the downloads manager UI

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(fennec1.0b2+)

VERIFIED FIXED
Tracking Status
fennec 1.0b2+ ---

People

(Reporter: madhava, Assigned: mfinkle)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 477628
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0+
tracking-fennec: 1.0+ → 1.0b2+
Assignee: nobody → mark.finkle
Attached patch WIP 1 (obsolete) — Splinter Review
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.
Attached patch patch (obsolete) — Splinter Review
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
Attachment #371524 - Attachment is obsolete: true
Attachment #372812 - Flags: review?(gavin.sharp)
We also support:
* Sorting the downloads list
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).
(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.
(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 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.
Attachment #372812 - Flags: review?(gavin.sharp) → review+
(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.
(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
Followup is bug 489223
(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.
Oops, I misunderstood. You wanted the initWithPath side of the _getLocalFile method to be removed. Removed that code and everything still works.
Updated patch based on review comments. Carrying r+ forward. I'll check this version in.
Attachment #372812 - Attachment is obsolete: true
Attachment #373879 - Flags: review+
nit: s/dlmgr/nsIDLMGR/, or something?
(In reply to comment #14)
> nit: s/dlmgr/nsIDLMGR/, or something?

Done. nsIDLMgr
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 436070
verified with beta3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: