741655 - Add more controls to download manager

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

The download manager added in bug 695178 is a bare-bones implementation that only contains completed downloads. We can add active downloads with the ability to pause/resume/cancel them, and also retry failed or canceled downloads.

Comment 1

[Thiyag Krishna (:thiyag)]

Brian,
I can take this up if no one else is assigned. I have never contributed before, so I could use all the help I can get.

I am assuming that the new changes will be to the following file based on bug 695178 ?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutDownloads.js

Comment 2

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

Thiyag

You assume correctly. You can also find some JS we used in the download manager in XUL version of Fennec here:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/downloads.js#397

That file has lots of code and I point to the methods used to implement cancel, pause, resume and retry.

Comment 3

[Thiyag Krishna (:thiyag)]

I have Pause/Resume/Cancel/Retry working now. I need to selectively show/hide the menu items based on the current state of the download.

Shall I use mozDownload={state} in the template to track the current download state(done/cancelled/active etc.) and use it in the code? Should I rename it to mozDownloadState or something similar or use it as-is?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutDownloads.js#14

I will then use something like  contextmenus.SelectorContext("li[mozDownload='active']") etc. to decide if the menu item gets displayed. Please let me know if that is not the right approach.

Also, visually where should the download state(Downloading/Cancelled etc.) be displayed; below the filesize/host? Will it part of the download progress bug 741654?
It looks like this currently: https://bug695178.bugzilla.mozilla.org/attachment.cgi?id=612313

Comment 4

[Brian Nicholson (:bnicholson)]

(In reply to Thiyag Krishna from comment #3)
> I have Pause/Resume/Cancel/Retry working now. I need to selectively
> show/hide the menu items based on the current state of the download.
> 
> Shall I use mozDownload={state} in the template to track the current
> download state(done/cancelled/active etc.) and use it in the code? Should I
> rename it to mozDownloadState or something similar or use it as-is?
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> aboutDownloads.js#14

I think saving states in the mozDownload attribute should work fine. You don't need to rename it.

> I will then use something like 
> contextmenus.SelectorContext("li[mozDownload='active']") etc. to decide if
> the menu item gets displayed. Please let me know if that is not the right
> approach.

Yes, that sounds correct.

> Also, visually where should the download state(Downloading/Cancelled etc.)
> be displayed; below the filesize/host? Will it part of the download progress
> bug 741654?
> It looks like this currently:
> https://bug695178.bugzilla.mozilla.org/attachment.cgi?id=612313

I would put it below the filesize/host like you said.

I made an initial attempt at the download state awhile ago when bug 695178 was being worked on, but we dropped it because we wanted to keep the download manager minimal. If it helps, you can look at it here: https://bug695178.bugzilla.mozilla.org/attachment.cgi?id=611651

CC'ing Wes in case he has any thoughts.

Comment 5

[Wesley Johnston (:wesj)]

I just landed bug 747354, which will bitrot your work here, but it should be easy to "undo". I'm also going to CSS ian. I think we probably want some sort of progress bar for in progress downloads. Maybe he's got opinions on that or the right ways to expose "pause", "resume", etc. For now, I would look at how we designed them in XUL Fennec for guidance. Let me see if I can dig up ay pictures....

Comment 6

[Thiyag Krishna (:thiyag)]

Ok, I will have the download status below the filesize/host temporarily. Work in progress.. I will update it again once we have a decision.

Wes,
Regarding the code for bug 747354, Should I roll back the change to the query, so that all the required states are returned, and then for any Failed downloads, I will mark it as failed in the download mgr? I am assuming the Wi-fi disconnect issue will mark the download as DOWNLOAD_FAILED internally?

Comment 7

[Wesley Johnston (:wesj)]

(In reply to Thiyag Krishna from comment #6)
> Wes,
> Regarding the code for bug 747354, Should I roll back the change to the
> query, so that all the required states are returned, and then for any Failed
> downloads, I will mark it as failed in the download mgr? I am assuming the
> Wi-fi disconnect issue will mark the download as DOWNLOAD_FAILED internally?

Yep. I'm not sure what the Wi-Fi thing does, whether it marks the download as paused or failed, but you shouldn't have to do that.

Comment 8

[Thiyag Krishna (:thiyag)]

Attachment: Pause/Resume/Cancel/Retry added to the download manager

Preliminary patch to add additional functionality(Pause/Resume/Cancel/Retry) to the download manager. There was just enough space for the download status text below the file size/host, we might need to increase overall height. It is overflowing and hiding the bottom border. I used a 1px negative margin for now, might need something better.The UI decision for bug 741654 might also impact how this will turn out. Feedback appreciated, Thank you!

Comment 9

[Thiyag Krishna (:thiyag)]

Attachment: Download state appearance in the list

Screenshot showing how the download state appears below the filesize/host.

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 620969 [details] [diff] [review]
Pause/Resume/Cancel/Retry added to the download manager

This patch looks pretty good from the functionality/code perspective. That -1px margin is noticeable, so I think we'll want to change that. The easiest fix would be to just increase the row height slightly to fit the third entry, but I don't know how much we care about making the height be the same as the about:addons page.

There's other things we haven't talked about yet, such as whether to show "Completed" for completed downloads (desktop doesn't show this), or whether we should show the "Downloading..." string or something else until bug 695178 is implemented. Passing off to Ian.

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 620971 [details]
Download state appearance in the list

I guess it would make more sense to give him f? on the screenshot.

Comment 12

[Ian Barlow (:ibarlow)]

Attachment: UI tweaks

Hi Brian and Thiyag, this looks great. I have a couple of small tweaks and then I think we are good to go.

1. The progress text is too close to the line above, let's move it down 2px
2. The progress text should be the same grey colour as the download time 
3. The shadow at the top of the page should be removed, so that the browser header and page background all blend together.

Comment 13

[Ian Barlow (:ibarlow)]

Comment on attachment 620971 [details]
Download state appearance in the list

Added my comments above, if I could see one more screenshot with the visual changes applied I will f+

Comment 14

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

(In reply to Brian Nicholson (:bnicholson) from comment #10)
> Comment on attachment 620969 [details] [diff] [review]
> Pause/Resume/Cancel/Retry added to the download manager
> 
> This patch looks pretty good from the functionality/code perspective. That
> -1px margin is noticeable, so I think we'll want to change that. The easiest
> fix would be to just increase the row height slightly to fit the third
> entry, but I don't know how much we care about making the height be the same
> as the about:addons page.

I think we should maintain consistent row heights and either reduce the second and third row font sizes slightly... or increase the row sizes in our other in-content UIs
> 
> There's other things we haven't talked about yet, such as whether to show
> "Completed" for completed downloads (desktop doesn't show this), or whether
> we should show the "Downloading..." string or something else until bug
> 695178 is implemented. Passing off to Ian.

I think we should not show the word "Completed" for successfully downloaded files

Ian?

Comment 15

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

(In reply to Ian Barlow (:ibarlow) from comment #13)
> Comment on attachment 620971 [details]
> Download state appearance in the list
> 
> Added my comments above, if I could see one more screenshot with the visual
> changes applied I will f+

The shadow should be removed by Java code, not the webpage itself. Why isn't this already working?

Comment 16

[Ian Barlow (:ibarlow)]

> I think we should maintain consistent row heights and either reduce the
> second and third row font sizes slightly... or increase the row sizes in our
> other in-content UIs

I'd like to keep our font sizes consistent, so if we need to increase row sizes slightly across our in-content UIs to allow for slightly more generous line spacing, I'd prefer that approach. 


> I think we should not show the word "Completed" for successfully downloaded
> files

Yes, I agree

Comment 17

[Thiyag Krishna (:thiyag)]

Attachment: Downloads Screenshot update 1

Updates:
- Nudged State by 2px compared to previous screenshot
- Updated state color to "gray" which matches what is configured for date.
- No shadow at the top (no change, works as-is with latest build)
- Didn't have to increase item height (negative bottom margin solved overlay)

Comment 18

[Thiyag Krishna (:thiyag)]

(In reply to Mark Finkle (:mfinkle) from comment #15)
> The shadow should be removed by Java code, not the webpage itself. Why isn't
> this already working?

This appears to be working as expected now, no shadow is displayed, I am not sure why the screenshot had it originally, not able to replicate.


(In reply to Ian Barlow (:ibarlow) from comment #12)
> 1. The progress text is too close to the line above, let's move it down 2px
> 2. The progress text should be the same grey colour as the download time 
> 3. The shadow at the top of the page should be removed, so that the browser
> header and page background all blend together.

These are completed and a sample is attached with the previous screenshot.

(In reply to Ian Barlow (:ibarlow) from comment #16)
> I'd like to keep our font sizes consistent, so if we need to increase row
> sizes slightly across our in-content UIs to allow for slightly more generous
> line spacing, I'd prefer that approach. 

I did the "nudging" without having to increase existing height, I will update it again if you do not like what is present in the current screenshot in attachment 621903 [details].

> > I think we should not show the word "Completed" for successfully downloaded
> > files
> 
> Yes, I agree

I removed the string that was displayed on download completion, nothing is displayed once the download completes now. (sample in attachment 621903 [details])

New Question:
What should we do for "Downloading...",leave it as-is for now and eventually replace it with a progress bar(bug 741654)?

Comment 19

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

(In reply to Thiyag Krishna (:thiyag) from comment #18)

> New Question:
> What should we do for "Downloading...",leave it as-is for now and eventually
> replace it with a progress bar(bug 741654)?

Sounds like a good plan

Comment 20

[Ian Barlow (:ibarlow)]

Comment on attachment 621903 [details]
Downloads Screenshot update 1

Looks excellent :)

Comment 21

[Ian Barlow (:ibarlow)]

I really meant to f+ there, sorry!

Comment 22

[Thiyag Krishna (:thiyag)]

Attachment: Pause/Resume/Cancel/Retry added to the download manager

New patch having rebased code.
- Intentionally regresses Wes' change for bug 747354(adding him as reviewer)
- Added "Delete" menu item to Failed downloads(differs from original patch which brian f+'ed)

Pending:
- Tests??
- Localization strings

I am not sure if I can flag this for review at the moment and if it can be checked in if approved. I will probably take up the other download progress bar bug as well (741654)

Comment 23

[Brian Nicholson (:bnicholson)]

Attachment: downloads margin

In comment 12, Ian said to move the state line 2px down from where it was, which is slightly more of a margin than there is between the first and second lines. Here's a screenshot with no added margin.

Comment 24

[Brian Nicholson (:bnicholson)]

Comment on attachment 622035 [details] [diff] [review]
Pause/Resume/Cancel/Retry added to the download manager

This looks pretty good, but there are a few things that need to be updated first.

For the sake of completion, we should probably include all possible download states listed here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsIDownloadManager.idl#60

This means we should have all corresponding strings in _getStateString(), we should select all downloads in the SQL query, and we should listen for all observers: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#2383


+  margin-top: 1px;

You can get rid of this - see comment 23.


+        // if element is already in download list, update it, else add a new one
+        if (!this._getElementForDownload(download.id)) {
+            this._insertDownloadRow(download);
+            break;
+        }
+        //fall through

No need for this. You have the same check below "dl-done", so you can just let "dl-start" fall through.


+    Downloads.retryMenuItem = contextmenus.add(gStrings.GetStringFromName("downloadAction.retry"),
+                                                contextmenus.SelectorContext("li[mozDownload='" + this._dlmgr.DOWNLOAD_FAILED + "']," +
+                                                                             "li[mozDownload='" + this._dlmgr.DOWNLOAD_CANCELED + "']"),
+                                                                             // Retry shown when its failed or canceled

nit: remove a space next to "contextmenus.SelectorContext" so that it aligns with the line above it. Also, put comments above the first line rather than under the code.


+  _updateDownloadRow: function (aItem){
+    try{
+      let download = this._getDownloadForElement(aItem);

nit: space between "try" and "{"


+      });
+    } catch(ex){
+       console.log("ERROR: _updateDownloadRow(): " + ex);

nit: space before "{" on catch line

Comment 25

[Thiyag Krishna (:thiyag)]

(In reply to Brian Nicholson (:bnicholson) from comment #24)

Thanks Brian, I will update them as needed. Need inputs on how each state should be displayed on the UI and if all of the states are applicable on mobile. I tried to look too in the source, but any pointers to existing behaviour appreciated.

DOWNLOAD_NOTSTARTED : "Waiting" or ignore and wait for dl-start?
DOWNLOAD_QUEUED     : "Queued" or ignore and wait for dl-start?
DOWNLOAD_SCANNING   : "Scanning"?
DOWNLOAD_DIRTY      : This is for "Virus Detected", default to "Failed"?
DOWNLOAD_BLOCKED_PARENTAL, DOWNLOAD_BLOCKED_POLICY(Windows): "Failed" or "Blocked"?

Comment 26

[Brian Nicholson (:bnicholson)]

(In reply to Thiyag Krishna (:thiyag) from comment #25)
> (In reply to Brian Nicholson (:bnicholson) from comment #24)
> 
> Thanks Brian, I will update them as needed. Need inputs on how each state
> should be displayed on the UI and if all of the states are applicable on
> mobile. I tried to look too in the source, but any pointers to existing
> behaviour appreciated.

I doubt some of these states apply to mobile, but it doesn't hurt to have them. You can can find a list of the existing strings here:

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/downloads/downloads.properties#5

Comment 27

[Thiyag Krishna (:thiyag)]

Attachment: Pause/Resume/Cancel/Retry added to the download manager

Brian, The attached patch implements the changes requested in Comment 24. I took longer than needed as I was putting out fires at work and didnt get to look at this :) I hope I didn't hold back anything.

No context menu is currently displayed when the Scanning, Queued and Not_started states are underway. I will catch up with you on IRC tomorrow to check if I have the right context menus for each state.

Current states & menus: (Download state --> menu items)

Finished --> Open/Delete
Downloading --> Pause/Cancel
Failed --> Retry/Delete
Paused --> Resume/Cancel
Canceled --> Retry/Delete
Blocked Policy --> Retry/Delete
Blocked Parental --> Retry/Delete
Dirty --> Delete (Retry? Since the file was already infected once, showing only
                  delete)

Comment 28

[Thiyag Krishna (:thiyag)]

Attachment: Pause/Resume/Cancel/Retry added to the download manager

Brian - Updated patch as per our discussion on IRC.

- "Cancel" menu is enabled for NOT_STARTED and QUEUED.
- Dirty, blocked, show up as "Failed"
- Scanning now shows up as "Downloading..."

And a few minor fixes
- Added new function to get download size, which defaults to "Unknown size" if it is not greater than 0 (matches desktop)
- Whitespace nits
- Added MPL license to properties file

Comment 29

[Thiyag Krishna (:thiyag)]

Ian,

Right now in the downloads page, we only have a "Delete" option which deletes the file from storage. We might want a "Remove Item" kind of option which removes the item from the download list, but which does not delete the actual file. Need your feedback from a UX perspective.

Comment 30

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

(In reply to Thiyag Krishna (:thiyag) from comment #29)
> Ian,
> 
> Right now in the downloads page, we only have a "Delete" option which
> deletes the file from storage. We might want a "Remove Item" kind of option
> which removes the item from the download list, but which does not delete the
> actual file. Need your feedback from a UX perspective.

I would vote against "Remove Item" because it would leave the user with potentially large files on the device with no easy way to remove them. This is why we only have a "Delete" menu item now, and in XUL Fennec.

Comment 31

[Ian Barlow (:ibarlow)]

I agree with Mark, partly for his reason, and partly because I can't imagine most users understanding the difference between the concepts of "delete" and "remove"

Comment 32

[Adrian Tamas (:AdrianT)]

Actually there is an open enhancement request Bug 716356 to add a remove entry for Firefox XUL. 
At lest there should be a "clear download history" option in my opinion to avoid having a lot of items in the download manager which a user may still need available on the device(e.g. music, photos, documents).

Comment 33

[Brian Nicholson (:bnicholson)]

Comment on attachment 623913 [details] [diff] [review]
Pause/Resume/Cancel/Retry added to the download manager

This looks good - just a few more suggested changes to simplify things a bit.

Can you add another function, _getState(), that does the state translations (like scanning->downloading, blocked->failed, etc)? That way, _getStateString() will only need to have 1:1 matchings for each state:string. Also, this should help simplify the SelectorContexts since you won't need to have the translations in those.


>+      "SELECT id, name, source, state, startTime, endTime, referrer, " +
>+             "currBytes, maxBytes, state IN (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11) isActive " +

Since your patch handles all cases, you should be able to drop '(?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11) isActive' and just select all downloads.


>+    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);
>+    this._stmt.bindInt32Parameter(5, Ci.nsIDownloadManager.DOWNLOAD_FINISHED);
>+    this._stmt.bindInt32Parameter(6, Ci.nsIDownloadManager.DOWNLOAD_FAILED);
>+    this._stmt.bindInt32Parameter(7, Ci.nsIDownloadManager.DOWNLOAD_CANCELED);
>+    this._stmt.bindInt32Parameter(8, Ci.nsIDownloadManager.DOWNLOAD_DIRTY);
>+    this._stmt.bindInt32Parameter(9, Ci.nsIDownloadManager.DOWNLOAD_BLOCKED_POLICY);
>+    this._stmt.bindInt32Parameter(10, Ci.nsIDownloadManager.DOWNLOAD_BLOCKED_PARENTAL);

Same as above.

Comment 34

[Thiyag Krishna (:thiyag)]

Brian,
I made the changes, but for some reason, some of the time, downloads are getting stuck under the QUEUED state and only a restart allows us to start the download(restarting fennec starts the stalled download automatically when I open about:downloads). Guess I messed up something, I will look more closely into it tomorrow night and will provide an updated patch.

Comment 35

[Wesley Johnston (:wesj)]

> Since your patch handles all cases, you should be able to drop '(?1, ?2, ?3,
> ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11) isActive' and just select all downloads.

This was used before for sorting. Active downloads should be shown at the top of the list. i.e. see the XUL Fennec query:

http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/downloads.js#55

Comment 36

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #35)
> > Since your patch handles all cases, you should be able to drop '(?1, ?2, ?3,
> > ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11) isActive' and just select all downloads.
> 
> This was used before for sorting. Active downloads should be shown at the
> top of the list. i.e. see the XUL Fennec query:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/
> downloads.js#55

In that case, I'd copy the query Wes linked to here.

Comment 37

[Brian Nicholson (:bnicholson)]

And new items should follow the same ordering rules: http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/downloads.js#279

Thiyag, sorry to keep adding changes. Hopefully that will be the last one.

Comment 38

[Thiyag Krishna (:thiyag)]

Attachment: Pause/Resume/Cancel/Retry added to the download manager

Brian,
No problem! The attached patch fixes the following:

- _getState() function which will translate states to "umbrella" states
- Fixes the query to maintain XUL ordering (active downloads on the top)

I made all new downloads that become active to appear at the top of the list, including those that were originally paused/cancelled and then were resumed/retried.

Completed, Cancelled and Failed downloads are pushed below any active ones(active includes paused).

The only difference being, for paused downloads, if there are more than one paused downloads already in the list, I am making any new paused downloads to be pushed before the existing paused entries. This is to maintain the timestamp order within the paused set, please let me know if I can remove that corner case.

Comment 39

[Thiyag Krishna (:thiyag)]

Attachment: Pause/Resume/Cancel/Retry added to the download manager-v5

Brian, Updated as discussed on IRC. Removed pause case, it is treated as an active download now and will not jump within the list on Pause or Resume.

Comment 40

[Brian Nicholson (:bnicholson)]

Comment on attachment 625286 [details] [diff] [review]
Pause/Resume/Cancel/Retry added to the download manager-v5

+      case "dl-cancel":
+      case "dl-done":
       case "dl-failed":
-      case "dl-cancel":
+     
+      // For all "completed" states, move them after active downloads
+      this._moveDownloadAfterActive(this._getElementForDownload(download.id));

nit: remove the empty line and indent the code below


+    } catch (ex) {
+      console.log("ERROR: _moveDownloadAfterActive() : "+ex);
+    }

nit: add space before and after +


r+ with these changes.

Comment 41

[Brian Nicholson (:bnicholson)]

Comment on attachment 625286 [details] [diff] [review]
Pause/Resume/Cancel/Retry added to the download manager-v5

Let's have Wes look at this too since he did most of the download manager.

Comment 42

[Wesley Johnston (:wesj)]

Comment on attachment 625286 [details] [diff] [review]
Pause/Resume/Cancel/Retry added to the download manager-v5

Review of attachment 625286 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Just one real question I had about moving items below the active ones? (will r+ once its addressed) The rest are nitpicky comments.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +10,5 @@
>  
>  let gStrings = Services.strings.createBundle("chrome://browser/locale/aboutDownloads.properties");
>  
>  let downloadTemplate =
> +"<li downloadID='{id}' role='button' mozDownload='{state}'>" +

Nit: Is there a reason we can't be more explicit and use "state" here instead of mozDownload?

@@ +148,5 @@
> +      case "dl-scanning":
> +        if (!this._getElementForDownload(download.id)) {
> +          this._insertDownloadRow(download);
> +        } else {
> +          let item = this._getElementForDownload(download.id);

Nit: Might as well move this outside the if-else block. Then you can just do:

if (item)
  // insert!
else
  // udpate!

@@ +165,5 @@
> +      while (next && this._inProgress(next.getAttribute("mozDownload"))) {
> +        next = next.nextSibling;
> +      }
> +      // Move the item
> +      this._list.insertBefore(element, next);

"element" seems unnecessary, since we're not modifying aItem. But mostly stopped because this will fail if everything is in progress? We should probably do

if (next) this._list.insertBefore(aItem, next);
else this._list.appendChild(aItem);

unless I'm missing something...

@@ +374,5 @@
>    },
>  
> +  _removeItem: function dv__removeItem(aItem) {
> +    // Make sure we have an item to remove
> +    if (!aItem) {

Nit: braces aren't needed for these one line things. There's a couple more of these. Nothing big to worry about, but if you're fixing stuff...

::: mobile/android/locales/en-US/chrome/aboutDownloads.properties
@@ +7,5 @@
> +downloadAction.pause=Pause
> +downloadAction.resume=Resume
> +downloadAction.cancel=Cancel
> +downloadAction.retry=Retry
> +downloadState.downloading=Downloading...

Nit: We should use the unicode ellipsis character ( … ) in this file. If bugzilla won't give that to you, you can steal from somewhere like:

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#57

Comment 43

[Thiyag Krishna (:thiyag)]

Attachment: Pause/Resume/Cancel/Retry added to the download manager-v6

(In reply to Brian Nicholson (:bnicholson) from comment #40)
> Comment on attachment 625286 [details] [diff] [review]
> Pause/Resume/Cancel/Retry added to the download manager-v5
> 
> +      case "dl-cancel":
> +      case "dl-done":
>        case "dl-failed":
> -      case "dl-cancel":
> +     
> +      // For all "completed" states, move them after active downloads
> +     
> this._moveDownloadAfterActive(this._getElementForDownload(download.id));
> 
> nit: remove the empty line and indent the code below
> 

Done..

> 
> +    } catch (ex) {
> +      console.log("ERROR: _moveDownloadAfterActive() : "+ex);
> +    }
> 
> nit: add space before and after +
> 
> 
> r+ with these changes.

Done.. thanks, Brian.

(In reply to Wesley Johnston (:wesj) from comment #42)
> > +"<li downloadID='{id}' role='button' mozDownload='{state}'>" +
> 
> Nit: Is there a reason we can't be more explicit and use "state" here
> instead of mozDownload?
> 

Renamed to "state" and updated all references.

> > +        if (!this._getElementForDownload(download.id)) {
> > +          this._insertDownloadRow(download);
> > +        } else {
> > +          let item = this._getElementForDownload(download.id);
> 
> Nit: Might as well move this outside the if-else block. Then you can just do:
> 
> if (item)
>   // insert!
> else
>   // udpate!
> 

Done.

> > +      while (next && this._inProgress(next.getAttribute("mozDownload"))) {
> > +        next = next.nextSibling;
> > +      }
> > +      // Move the item
> > +      this._list.insertBefore(element, next);
> 
> "element" seems unnecessary, since we're not modifying aItem. But mostly
> stopped because this will fail if everything is in progress? We should
> probably do
> 
> if (next) this._list.insertBefore(aItem, next);
> else this._list.appendChild(aItem);
> 
> unless I'm missing something...
> 

Good catch! insertBefore() behaves like appendChild() if the second param is null(it adds aItem to the end of the list), so we should be covered for the "else" case. If required, I will explicitly code the else condition, but it may not be needed. I removed the unnecessary "element" and used aItem in its place within that block..


> Nit: braces aren't needed for these one line things. There's a couple more
> of these. Nothing big to worry about, but if you're fixing stuff...

Got used to including braces at work :) I removed the braces from all single-line statements I added to the code :)

> Nit: We should use the unicode ellipsis character ( … ) in this file. If
> bugzilla won't give that to you, you can steal from somewhere like:

Unicode ellipsis updated for "Downloading…" and "Starting…"

Thanks, Wes!

I am not too sure about the release schedule for this change, if this is r+, please let me know if I should tag it for check-in..

Comment 44

[Wesley Johnston (:wesj)]

Comment on attachment 626315 [details] [diff] [review]
Pause/Resume/Cancel/Retry added to the download manager-v6

Sounds good thanks. We can land this on 15 now. I'm not going to push to uplift it to 14.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60d3cf56a65c

Comment 46

[Jim Jeffery not reading bug-mail 1/2/11]

Bug is marked as ARM Android - does this only apply to that platform ?  or does it cover all platforms ?

Comment 47

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

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #46)
> Bug is marked as ARM Android - does this only apply to that platform ?  or
> does it cover all platforms ?

Only Android

Comment 48

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/60d3cf56a65c

Comment 50

[Andreea Pod]

Verified using Samsung Galaxy Tab 10.1 (Android 3.1) on:
Firefox 19.0b5 build 2 (2013-02-06), Nightly 21.0a1(2013-02-07)and Aurora 20.0a2(2013-02-07)