Add progress indicator to download manager

NEW
Unassigned

Status

()

7 years ago
5 years ago

People

(Reporter: bnicholson, Unassigned)

Tracking

unspecified
ARM
Android
Points:
---

Firefox Tracking Flags

(blocking-fennec1.0 -)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Depends on: 695178

Updated

7 years ago
blocking-fennec1.0: --- → ?
out of scope
blocking-fennec1.0: ? → -
Brian, Should I take this bug as well?
(Reporter)

Comment 3

7 years ago
(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).
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?
(Reporter)

Comment 6

6 years ago
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
Status: NEW → ASSIGNED
Created attachment 664404 [details] [diff] [review]
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).
Attachment #664404 - Flags: feedback?(margaret.leibovic)
Created attachment 664406 [details]
Sample Screen
Created attachment 664407 [details]
Sample Screen with Resume Error
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?

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.
Attachment #664404 - Flags: feedback?(bnicholson)
Created attachment 665862 [details] [diff] [review]
Patch (v2)

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 15

6 years ago
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.
Attachment #665862 - Flags: feedback?(margaret.leibovic) → feedback+
(Reporter)

Comment 16

6 years ago
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
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.