Closed Bug 741654 Opened 10 years ago Closed 2 years ago

Add progress indicator to download manager


(Firefox for Android Graveyard :: General, defect)

Not set


(blocking-fennec1.0 -)

Tracking Status
blocking-fennec1.0 --- -


(Reporter: bnicholson, Unassigned)




(3 files, 1 obsolete file)

The download manager added in bug 695178 is a bare-bones implementation that only shows completed downloads. We can include active downloads and show a progress indicator.
Depends on: 695178
blocking-fennec1.0: --- → ?
out of scope
blocking-fennec1.0: ? → -
Brian, Should I take this bug as well?
(In reply to Thiyag Krishna (:thiyag) from comment #2)
> Brian, Should I take this bug as well?

Sure! Feel free to take any unassigned bugs you'd like to work on (we can always reassign them later if necessary).

How should I proceed with this, Should I use a "fake" progress bar(expand some element's length) based on actual progress from Gecko? I am not sure(I may be wrong) if we can have a native progress bar within our download's page? Should I use a XUL progressmeter?
Thiyag, are you still planning on implementing this feature? No rush - just wanted to check your status.
(In reply to Brian Nicholson (:bnicholson) from comment #6)
> Thiyag, are you still planning on implementing this feature? No rush - just
> wanted to check your status.

Hi, Brian, My apologies for not updating this, I will work on this in a few days if it is not blocking anything/anyone else,I have been held up with work. Android 14 minimum requirement created build issues and my dev environment no longer works, I will get it fixed and will update with a patch ASAP.
(In reply to Brian Nicholson (:bnicholson) from comment #6)
> Thiyag, are you still planning on implementing this feature? No rush - just
> wanted to check your status.

Brian, my dev environment is still toasted and I will be flying out of the states for a month this friday.. if someone wants to take this up in the meantime, they can.. I will check back once I am back or look at something else which may need assistance once I am back.. sorry for dragging this so far without any output..
Assignee: nobody → markcapella
Attached patch Patch (v1) WIP (obsolete) — Splinter Review
This looked like an interesting project, so I've been tinkering with it. There's no mentor listed so I'm asking margaret for feedback as she's been helping me with other items.

I'm not really looking for nit / polish kind of help, but more high-level / general-thoughts / comments... 

I do notice areas for improvement, and ways in which this changes behaviour from a user perspective. For example, I'm trying to find a problem where some paused downloads, (but not others), fail on resume, yet after failure, can be successfully downloaded via. retry.

(Seems to involve .gz files).
Attachment #664404 - Flags: feedback?(margaret.leibovic)
Attached image Sample Screen
Brian might be able to mentor too
Comment on attachment 664404 [details] [diff] [review]
Patch (v1) WIP

Hmmm .... yah I may to chat with someone later ... testing with various .zip .exe .tar.gz and .tgz files works ok ... but this one gives me trouble when I try to resume: (attached screenshot) ... something specific?

Well duh - tried without my patch on the queue and this one file fails on resume here also ... guess I'll flag brian for feedback also as we've not spoken before.
Attachment #664404 - Flags: feedback?(bnicholson)
Attached patch Patch (v2)Splinter Review
Sorry for the email flood - the first patch needed rebasing due to m-c changes, and I took out my debug code ...
Attachment #664404 - Attachment is obsolete: true
Attachment #664404 - Flags: feedback?(margaret.leibovic)
Attachment #664404 - Flags: feedback?(bnicholson)
Attachment #665862 - Flags: feedback?(margaret.leibovic)
Attachment #665862 - Flags: feedback?(bnicholson)
Comment on attachment 665862 [details] [diff] [review]
Patch (v2)

Review of attachment 665862 [details] [diff] [review]:

This looks like a good start. I didn't try applying the patch, but here are some comments I came up with while looking at it. I feel like it may be useful to check out what desktop Firefox does for the progress bar in the downloads panel, just to make sure there aren't any gotchas we may miss.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +181,5 @@
>          next = next.nextSibling;
>        // Move the item
>        this._list.insertBefore(aItem, next);
>      } catch (ex) {
> +      console.log("Error: _moveDownloadAfterActive() : " + ex);

You should use Cu.reportError here (not your fault, but good to fix :)

@@ +361,5 @@
>    getDownloads: function dl_getDownloads() {
>      this._dlmgr = Cc[";1"].getService(Ci.nsIDownloadManager);
> +    if (!this._progressAlert) {
> +      this._progressAlert = new AlertDownloadProgressListener();
> +      this._dlmgr.addListener(this._progressAlert);

This method takes a nsIDownloadProgressListener [1], but the AlertDownloadProgressListener you created down below doesn't implement all the methods it should [2]. You also don't need to make a whole new object for the progress listener methods. Instead, you could just add them to the Downloads object. Here's an example of some desktop code that does that:


@@ +528,5 @@
> +    var elements = list.querySelectorAll("li");
> +
> +    for (var i = 0; i < elements.length; i++)
> +      if (elements[i].querySelector(".title").getAttribute("value") ==
> +          aDownload.displayName)

This seems like a really fragile way to get the download. What if two downloads have the same display name? Also, iterating through all the elements seems like a lot of extra work to be doing on every progress change event.
Attachment #665862 - Flags: feedback?(margaret.leibovic) → feedback+
Comment on attachment 665862 [details] [diff] [review]
Patch (v2)

+    for (var i = 0; i < elements.length; i++)
+      if (elements[i].querySelector(".title").getAttribute("value") ==
+          aDownload.displayName)
+        Downloads._updateDownloadRow(elements[i]);

Note that we already have a _getElementForDownload() method that returns an element for a given download. I don't know how often progress change events fire, but as Margaret said, this could be expensive. If necessary, you could use setTimeout() to batch events and update the elements less frequently.
Attachment #665862 - Flags: feedback?(bnicholson) → feedback+
Haven't touched this in awhile, releasing it ...
Assignee: markcapella → nobody
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly]( an issue can be reported at the [Fenix GitHub project]( If you want to discuss your report please use [Mozilla's chat]( server and join the [#fenix]( channel.
Closed: 2 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.