741654 - Add progress indicator to download manager

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

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.

Comment 1

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

out of scope

Comment 2

[Thiyag Krishna (:thiyag)]

Brian, Should I take this bug as well?

Comment 3

[Brian Nicholson (:bnicholson)]

(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).

Comment 4

[Thiyag Krishna (:thiyag)]

Brian/Mike,

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?

Comment 5

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

Can you use the new <progress> HTML element?
https://developer.mozilla.org/en/HTML/Element/progress
http://blog.oldworld.fr/index.php?post/2011/07/The-HTML5-progress-element-in-Firefox

Comment 6

[Brian Nicholson (:bnicholson)]

Thiyag, are you still planning on implementing this feature? No rush - just wanted to check your status.

Comment 7

[Thiyag Krishna (:thiyag)]

(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.

Comment 8

[Thiyag Krishna (:thiyag)]

(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..

Comment 9

[Mark Capella [:capella]]

Attachment: Patch (v1) WIP

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).

Comment 10

[Mark Capella [:capella]]

Attachment: Sample Screen

Comment 11

[Mark Capella [:capella]]

Attachment: Sample Screen with Resume Error

Comment 12

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

Brian might be able to mentor too

Comment 13

[Mark Capella [:capella]]

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?

http://www.digital-digest.com/software/getdownload.php?sid=1989&did=2&code=z3jCybBi&decode=d4bf0cd8be23324dbc53f2d59dea47a6

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.

Comment 14

[Mark Capella [:capella]]

Attachment: Patch (v2)

Sorry for the email flood - the first patch needed rebasing due to m-c changes, and I took out my debug code ...

Comment 15

[:Margaret Leibovic]

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["@mozilla.org/download-manager;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:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6960

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsIDownloadManager.idl#232
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsIDownloadProgressListener.idl

@@ +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.

Comment 16

[Brian Nicholson (:bnicholson)]

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.

Comment 17

[Mark Capella [:capella]]

Haven't touched this in awhile, releasing it ...

Comment 18

[BMO Automation]

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](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.