402278 - Download activity summary in the status bar

Status: VERIFIED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[Madhava Enros [:madhava]]

Attachment: v1 mockup of download summary information in the status bar

It would be a useful accompaniment to the Downloads Manager if there were a summary of current download activity in the Firefox statusbar.  Clicking on this summary could launch or bring to the foreground the Downloads Manager itself.  This area would simply not be present if no download activity were in progress.

See the accompanying mockup for an idea of how this could look and work.

A note about the mockup:
The rolled-up summary "time remaining" would be the greatest time remaining for any individual active (non-paused) download, given that they're happening in parallel.

Comment 1

[Michael Schonfeld]

Attachment: Patch for Bug #402278

The patch creates a new statusbar panel that shows the download's status as described here with.  In order to accomplish this, it uses a new function called dlMonPanelListener.  The function uses another function called getRemainingTime in order to get the localed remaining time string, which is also being used by download.js.

Comment 2

[Ed Lee :Mardak]

Thanks for the patch Michael.

There's some high level issues that might change the implementation quite a bit:

1) Localization -- "one" vs multiple; see bug 394516 for the same issue but with "1 day" vs "2 days".
2) Code reuse -- the download manager should probably be showing the same string for its title (or at least a similar one). So what you've got with the new file is probably a good start. We might need to move more stuff out.
3) Performance -- Ts and others. There's a delayed startup of the download manager service just above where you added the listener and call to updateDownloadsMonitorPanel. This should also be delayed to not init the download manager until later. Also, there's the updating of the status itself, but that probably isn't too bad compared to the existing updates.

sdwilsh: Alternatively to (2), if there's some way to keep the download manager UI around without it being shown.. perhaps it could be in charge of updating the status bar by setting some attributes? This could also help with avoiding repopulating the download list every time it's opened.

So when the download manager UI updates its title, it would do the same for the status bar.

Comment 3

[Michael Schonfeld]

Hmm, Okay.  Will hold up until completion of bug #394516.

Thanks Ed!

Comment 4

[Ed Lee :Mardak]

Michael: Just a heads up on bug 394516.. I've moved a lot of download manager utility functions to a javascript module at resource://gre/modules/DownloadUtils.jsm which should help implementing this feature if we want to get this in for Firefox 3... beta3 :x?

sdwilsh: You mentioned something along the lines of an extension maybe.. ?

Comment 5

[Michael Schonfeld]

So, we're gonna need to create 4 new strings:
1. One active download
2. #1 active downloads
3. One paused download
4. #1 paused downloads

Could I simply create them in the downloads.properties? Or, are we facing a big localization & plurals problem with these strings?

Comment 6

[Ed Lee :Mardak]

Plurals can be handled with the PluralForm module I added in the other bug.

status=#1 #2 #3
active=active
paused=paused
downloads=download;downloads

or alternatively
activeStatus=#1 active #2
pausedStatus=#1 paused #2
downloads=download;downloads

Comment 7

[Ed Lee :Mardak]

or as pike suggested..

activeStatus=One active download;#1 active downloads
pausedStatus=One paused download;#1 paused downloads

Comment 8

[Ed Lee :Mardak]

A heads up.. there's a string freeze in 13 hours. Think we can pump out a patch in time?

sdwilsh: Would we want this?

Michael: You'll want to use the two modules I created recently..

Cu.import("resource://gre/modules/DownloadUtils.jsm");
Cu.import("resource://gre/modules/PluralForm.jsm");

After you do what you've done to figure out the longest transfer time.
// get the time left message to show in ()s for active downloads
let [timeLeft, newLast] = DownloadUtils.getTimeLeft(maxTransferTime, gLastTime);

// get the status message for active or paused
let status = PluralForm.get(numDownloads, active ? activeString : pausedString);
// put in the number of downloads for #1 for both paused and active
status = replaceInsert(status, 1, numDownloads);
// for active, put in the time left in the ()s
status = replaceInsert(status, 2, timeLeft);

activeStatus=One active download (#2);#1 active downloads (#2)
pausedStatus=One paused download;#1 paused downloads

Comment 9

[Michael Schonfeld]

ahhhhhh 13 hours... it's friday night where I am.. Ill get on it right now.

Comment 10

[Ed Lee :Mardak]

Michael, I should be able to put something together with the new modules and all.

Were you able to test this to make sure things were working when you uploaded the patch?

Comment 11

[Michael Schonfeld]

Hold on an hour, I'm working on this right now...

Comment 12

[Michael Schonfeld]

activeString=One active download;#1 active downloads
pausedString=One paused download;#1 paused downloads

do I add those to intl.properties?

Comment 13

[Ed Lee :Mardak]

(In reply to comment #12)
> do I add those to intl.properties?
Same place you had them before.

diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties+
+# Downloads Monitor Panel
+activeDownloads=One active download (#2);#1 active downloads (#2)
+pausedDownloads=One paused download;#1 paused downloads

Comment 14

[Michael Schonfeld]

Attachment: v2

Comment 15

[Michael Schonfeld]

do we want to use the same string for the download manager's title?

Comment 16

[Asaf Romano (gone)]

Comment on attachment 299260 [details] [diff] [review]
v2

>Index: browser/base/content/browser.js
>===================================================================
>@@ -109,16 +109,18 @@ var gSanitizeListener = null;
> 
> var gAutoHideTabbarPrefListener = null;
> var gBookmarkAllTabsHandler = null;
> 
> #ifdef XP_MACOSX
> var gClickAndHoldTimer = null;
> #endif
> 
>+var dlLastSec = {};

Global variables should be prefixed with "g", but shouldn't this live inside dlPanelMonitor?

>+
> /**
> * We can avoid adding multiple load event listeners and save some time by adding
> * one listener that calls all real handlers.
> */
> 
> function pageShowEventHandlers(event)
> {
>   // Filter out events that are not about the document load we are interested in
>@@ -1059,16 +1061,22 @@ function delayedStartup()
> 
>   // Initialize the download manager some time after the app starts so that
>   // auto-resume downloads begin (such as after crashing or quitting with
>   // active downloads) and speeds up the first-load of the download manager UI.
>   // If the user manually opens the download manager before the timeout, the
>   // downloads will start right away, and getting the service again won't hurt.
>   setTimeout(function() Cc["@mozilla.org/download-manager;1"].
>                         getService(Ci.nsIDownloadManager), 10000);
>+  
>+  // Initialize the downloads monitor panel listener.
>+  var downloadMgr = Cc["@mozilla.org/download-manager;1"].
>+                       getService(Ci.nsIDownloadManager)
>+  downloadMgr.addListener(dlPanelMonitor);
>+  dlPanelMonitor.init();

As noted earlier, this should be done in the delayed method above your code.

> }
> 
> function BrowserShutdown()
> {
>   try {
>     FullZoom.destroy();
>   }
>   catch(ex) {
>@@ -5915,8 +5923,81 @@ var gIdentityHandler; 
>  * Returns the singleton instance of the identity handler class.  Should always be
>  * used instead of referencing the global variable directly or creating new instances
>  */
> function getIdentityHandler() {
>   if (!gIdentityHandler)
>     gIdentityHandler = new IdentityHandler();
>   return gIdentityHandler;    
> }
>+
>+var dlPanelMonitor = {

I would bother calling this DownloadPanelMonitor.

>+init: function() {

weird indent.

Please name scoped functions (DMP_functionname).

>+  Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
>+  Components.utils.import("resource://gre/modules/PluralForm.jsm");
>+  this.updateStatusbar();
>+},

nit: add an empty line between functions.

>+updateStatusbar: function() {
>+  var dlMonPanel = document.getElementById("download-monitor");
>+  var dlMgr =  Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);

Maybe we should cache gDownloadManager?

>+  var brstrbundle = document.getElementById("bundle_browser");
>+  
>+  if (dlMgr.activeDownloadCount == 0) {
>+    dlMonPanel.hidden = true;
>+    return;
>+  }


>+  
>+  dlMonPanel.hidden = false;
>+  var dls = dlMgr.activeDownloads;
>+  var numberOfPausedDownloads = 0;
>+  var longestRemaining = 0;
>+  var longestDlId;
>+  
>+  // Find the download with the longest remaining time, then
>+  // let LongestRemaining assume it's remaining time, and longestDlId
>+  // it's id number.
>+  while (dls.hasMoreElements()) {
>+    let dl = dls.getNext();
>+    dl.QueryInterface(Ci.nsIDownload);

or let dl = dls.getNext().QueryInterface(Ci.nsIDownload);

>+    if (dl.state == dlMgr.DOWNLOAD_DOWNLOADING) {
>+      if (dl.speed > 0 && dl.size > 0) {
>+      let currRemaining = Math.ceil((dl.size - dl.amountTransferred) / dl.speed);
>+        if (currRemaining > longestRemaining) {
>+          longestRemaining = currRemaining;
>+          longestDlId = dl.id;
>+        }
>+      }
>+      else
>+        longestRemaining = -1;
>+    }
>+    else if (dl.state == dlMgr.DOWNLOAD_PAUSED)
>+      numberOfPausedDownloads++;
>+  }
>+
>+  // Use the getTimeLeft util to obtain the remaining time string. then,
>+  // parse it and display the end result.
>+  [timeLeftText, newLastSec] = DownloadUtils.getTimeLeft(longestRemaining,
>+                                                         dlLastSec[longestDlId]);
>+  dlLastSec[longestDlId] = newLastSec;
>+  var numDownloads = dlMgr.activeDownloadCount - numberOfPausedDownloads;
>+  if (numberOfPausedDownloads == dlMgr.activeDownloadCount) {
>+    active = false;
>+    numDownloads = numberOfPausedDownloads;
>+  }
>+  else
>+    active = true;
>+        
>+  let status = PluralForm.get(numDownloads, active ?
>+                brstrbundle.getString("activeStatus") :
>+                brstrbundle.getString("pausedStatus"));

We should probably cache these strings as members of DownloadMonitorPanel given how often this method might be called.

>+  status = status.replace("#1", numDownloads);
>+  status = status.replace("#2", timeLeftText);
>+  dlMonPanel.label = status;
>+},
>+  // call updateDownloadsMonitorPanel whenever there's
>+  // a change in a download's progess or state

progress.

>+  onProgressChange: function () {
>+    this.updateStatusbar();
>+  },
>+  onDownloadStateChange: function () {
>+    this.updateStatusbar();
>+  },
>+};
>\ No newline at end of file

Add a new line :)

>Index: browser/locales/en-US/chrome/browser/browser.properties
>===================================================================
> identity.identified.state_and_country=%S, %S
> identity.identified.title_with_country=%S (%S)
> 
> identity.unknown.title=Identity Unknown
> identity.unknown.body=This web site does not supply identity information.
> 
> identity.encrypted=Your connection to this web site is encrypted to prevent eavesdropping.
> identity.unencrypted=Your connection to this web site is not encrypted.
>+
>+# Downloads Monitor Panel
>+activeStatus=One active download (#2);#1 active downloads (#2)
>+pausedStatus=One paused download;#1 paused downloads
>\ No newline at end of file

ditto.

Comment 17

[Ed Lee :Mardak]

Comment on attachment 299260 [details] [diff] [review]
v2

mano: I'll take the review ;) And probably sdwilsh too.
ui-r? would probably be for madhava after we can get a screenshot of this

>+var dlLastSec = {};
This can be a plain number instead of an object. We don't need to track the times of each individual download -- we only care about the biggest time anyway.
something like..
let gDownloadLastTime = Infinity;

>   setTimeout(function() Cc["@mozilla.org/download-manager;1"].
>                         getService(Ci.nsIDownloadManager), 10000);
>+  
>+  // Initialize the downloads monitor panel listener.
>+  var downloadMgr = Cc["@mozilla.org/download-manager;1"].
>+                       getService(Ci.nsIDownloadManager)
>+  downloadMgr.addListener(dlPanelMonitor);
>+  dlPanelMonitor.init();
This is okay for testing, but we'll probably want to combine it with the setTimeout eventually because otherwise we load the download manager service at startup which will impact start time Ts.


>+var dlPanelMonitor = {
nit: let
>+init: function() {
nit: indent this whole block and { on new line
>+  Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
>+  Components.utils.import("resource://gre/modules/PluralForm.jsm");
these two can go anywhere.. sdwilsh: should this just sit at the top of the file?
>+  this.updateStatusbar();
>+},
>+updateStatusbar: function() {
nit: indent block and { on new line
>+  var dlMonPanel = document.getElementById("download-monitor");
>+  var dlMgr =  Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
nit: let instead of var
>+  var brstrbundle = document.getElementById("bundle_browser");
this can be moved down to when we actually need to get the string
>+  
>+  if (dlMgr.activeDownloadCount == 0) {
>+    dlMonPanel.hidden = true;
We should reset gDownloadLastTime to Infinity here
>+    return;
>+  }
>+  
>+  dlMonPanel.hidden = false;
>+  var dls = dlMgr.activeDownloads;
>+  var numberOfPausedDownloads = 0;
>+  var longestRemaining = 0;
>+  var longestDlId;
nit: lets instead of var
variable names..
how about numPaused instead of numberOfPausedDownloads
longestRemaining -> maxTime
we don't need to track longestDlId when only tracking the longest time of all
>+  
>+  // Find the download with the longest remaining time, then
>+  // let LongestRemaining assume it's remaining time, and longestDlId
>+  // it's id number.
>+  while (dls.hasMoreElements()) {
>+    let dl = dls.getNext();
>+    dl.QueryInterface(Ci.nsIDownload);
>+    if (dl.state == dlMgr.DOWNLOAD_DOWNLOADING) {
>+      if (dl.speed > 0 && dl.size > 0) {
>+      let currRemaining = Math.ceil((dl.size - dl.amountTransferred) / dl.speed);
nit: indent
currRemaining -> currTime
No need for Math.ceil(), keep it as a double
>+        if (currRemaining > longestRemaining) {
>+          longestRemaining = currRemaining;
>+          longestDlId = dl.id;
no need for download id tracking
>+        }
>+      }
>+      else
nit: } else
>+        longestRemaining = -1;
>+    }
>+    else if (dl.state == dlMgr.DOWNLOAD_PAUSED)
nit: } else if {
>+      numberOfPausedDownloads++;
add the matching }
>+  }
>+
>+  // Use the getTimeLeft util to obtain the remaining time string. then,
>+  // parse it and display the end result.
>+  [timeLeftText, newLastSec] = DownloadUtils.getTimeLeft(longestRemaining,
>+                                                         dlLastSec[longestDlId]);
need to declare the timeLeftText variable... let's do
let timeLeft;
[timeLeft, gDownloadLastTime] = ...getTime(maxTime, gDownloadLastTime);
>+  dlLastSec[longestDlId] = newLastSec;
no need for this because we can update gDownloadLastTime in one go
>+  var numDownloads = dlMgr.activeDownloadCount - numberOfPausedDownloads;
nit: let
>+  if (numberOfPausedDownloads == dlMgr.activeDownloadCount) {
>+    active = false;
>+    numDownloads = numberOfPausedDownloads;
>+  }
>+  else
>+    active = true;
>+        
>+  let status = PluralForm.get(numDownloads, active ?
>+                brstrbundle.getString("activeStatus") :
>+                brstrbundle.getString("pausedStatus"));
How about..
let status = "activeStatus";
let numDownloads = ...;
if (..) {
  status = "pausedStatus";
  numDown.. = ..;
}
let (bundle = document.getElementById("bundle_browser")) {
  status = PluralForm.get(numDownloads, bundle.getString(status));
}
>+  status = status.replace("#1", numDownloads);
>+  status = status.replace("#2", timeLeftText);
>+  dlMonPanel.label = status;
>+},
>+  // call updateDownloadsMonitorPanel whenever there's
>+  // a change in a download's progess or state
>+  onProgressChange: function () {
>+    this.updateStatusbar();
>+  },
>+  onDownloadStateChange: function () {
>+    this.updateStatusbar();
>+  },
>+};

>+# Downloads Monitor Panel
>+activeStatus=One active download (#2);#1 active downloads (#2)
>+pausedStatus=One paused download;#1 paused downloads
Actually.. lets call these activeDownloads and pausedDownloads to help localizers. We'll need to add in some LOCALIZATION NOTES about #1, #2

Comment 18

[Ed Lee :Mardak]

(In reply to comment #16)
> >+var dlLastSec = {};
> Global variables should be prefixed with "g", but shouldn't this live inside
> dlPanelMonitor?
Oh indeed. Neat.

> >+  var dlMonPanel = document.getElementById("download-monitor");
> >+  var dlMgr =  Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> Maybe we should cache gDownloadManager?
That too.

> >+  var brstrbundle = document.getElementById("bundle_browser");
..
> >+  let status = PluralForm.get(numDownloads, active ?
> >+                brstrbundle.getString("activeStatus") :
> >+                brstrbundle.getString("pausedStatus"));
> We should probably cache these strings as members of DownloadMonitorPanel given
> how often this method might be called.
More neat stuff.

Just checking mano..

let DownloadMonitorPanel = {
  get activeStr()
  {
    delete this.activeStr;
    return this.activeStr = document.getElementById("bundle_browser").getString("activeDownloads");
  },
  get pausedStr() ..,
  lastTime: Infinity,
  dlmgr: Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager),
  DMP_updateStatusbar: function() ..
};

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 299260 [details] [diff] [review]
v2

>+# Downloads Monitor Panel
>+activeStatus=One active download (#2);#1 active downloads (#2)
>+pausedStatus=One paused download;#1 paused downloads

ui-r+ on these, though I think using the number "1" instead of "One" will look a little better in this case, as that's what we use on the title of the download manager, etc.

>\ No newline at end of file

nit: I think you need a newline at the end of the file :)

If you split the strings out into their own patch I'll be happy to approve them for landing in advance of the string freeze.

Comment 20

[Ed Lee :Mardak]

Attachment: screenshot

A quick screenshot. :) Other text can be "One paused download" or "2 paused downloads" or "2 active downloads (5 minutes remaining)" for example.

Comment 21

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> ui-r+ on these, though I think using the number "1" instead of "One" will look
> a little better in this case, as that's what we use on the title of the
> download manager, etc.
Proper grammar dictates that we should spell out one (although we should spell out any number less than and equal to ten too).

(In reply to comment #15)
> do we want to use the same string for the download manager's title?
Which string are you talking about?

(In reply to comment #17)
> >+        }
> >+      }
> >+      else
> nit: } else
> >+        longestRemaining = -1;
> >+    }
> >+    else if (dl.state == dlMgr.DOWNLOAD_PAUSED)
> nit: } else if {
*bops you*
Most browser code is styled like:
}
else {
- or -
}
else if (...) {

Edward - we aren't sufficient as reviewers since this is in browser/ land.  With that said, we should totally be looking at this.

Comment 22

[Shawn Wilsher :sdwilsh]

(In reply to comment #20)
> Created an attachment (id=299279) [details]
> screenshot
> 
> A quick screenshot. :) Other text can be "One paused download" or "2 paused
> downloads" or "2 active downloads (5 minutes remaining)" for example.
That looks to be missing the icon in the original mockup.

Comment 23

[Madhava Enros [:madhava]]

Comment on attachment 299279 [details]
screenshot

We'll want the icon in there eventually, too.

Comment 24

[Ed Lee :Mardak]

Attachment: v2.1

I've updated the patch for you Michael, thanks for the patch.

(In reply to comment #16)
> >+var dlLastSec = {};
> Global variables should be prefixed with "g", but shouldn't this live inside
> dlPanelMonitor?
Moved inside.
> >   setTimeout(function() Cc["@mozilla.org/download-manager;1"].
> >                         getService(Ci.nsIDownloadManager), 10000);
> >+  
> >+  // Initialize the downloads monitor panel listener.
> >+  var downloadMgr = Cc["@mozilla.org/download-manager;1"].
> >+                       getService(Ci.nsIDownloadManager)
> >+  downloadMgr.addListener(dlPanelMonitor);
> >+  dlPanelMonitor.init();
> As noted earlier, this should be done in the delayed method above your code.
Put into the delayed-delayed startup.

> I would bother calling this DownloadPanelMonitor.
Renamed to DownloadMonitorPanel

> >+init: function() {
> weird indent.
> Please name scoped functions (DMP_functionname).
Done. Done.

> >+},
> 
> nit: add an empty line between functions.
Newlined.

> >+  var dlMgr =  Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> Maybe we should cache gDownloadManager?
Cached.

> or let dl = dls.getNext().QueryInterface(Ci.nsIDownload);
Neato.

> >+  let status = PluralForm.get(numDownloads, active ?
> >+                brstrbundle.getString("activeStatus") :
> >+                brstrbundle.getString("pausedStatus"));
> We should probably cache these strings as members of DownloadMonitorPanel given
> how often this method might be called.
Cached when init is called.
> >\ No newline at end of file
> Add a new line :)
Fixed.

I've also addressed my own comments and cleaned up the code and comments.

Comment 25

[Ed Lee :Mardak]

Attachment: v2.2

Whoops.. forgot a semicolon and a "this."

Comment 26

[Michael Schonfeld]

Looks real good! I really hope it makes the upcoming beta release!!!

Comment 27

[Ed Lee :Mardak]

Attachment: v2.3 strings

r=beltzner

Splitting off strings to land for l10n freeze

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 299371 [details] [diff] [review]
v2.3 strings

uir+a=beltzner

Comment 29

[Ed Lee :Mardak]

Attachment: v2.3 code

Now with more icon and tooltip!

Comment 30

[Ed Lee :Mardak]

Strings are in.

Checking in browser/locales/en-US/chrome/browser/browser.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v  <--  browser.dtd
new revision: 1.91; previous revision: 1.90
done
Checking in browser/locales/en-US/chrome/browser/browser.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v  <--  browser.properties
new revision: 1.59; previous revision: 1.58
done

Comment 31

[Ed Lee :Mardak]

Attachment: screenshot of v2.3

Seems like we have more time now that the strings are in for the l10n freeze.. why not add in a tooltip and icon! ;)

Comment 32

[Ed Lee :Mardak]

Attachment: screenshot of v2.3

Now with less tiffness..

Comment 33

[Shawn Wilsher :sdwilsh]

Comment on attachment 299372 [details] [diff] [review]
v2.3 code

>+let DownloadMonitorPanel = {
This also needs to implement the QueryInterface ;)

Comment 34

[Ed Lee :Mardak]

Attachment: v2.4 code

(In reply to comment #33)
> >+let DownloadMonitorPanel = {
> This also needs to implement the QueryInterface ;)
Used XPCOMUtils.generateQI. Added comment headers and such. Moved Cu.import to outside/top.

Comment 35

[Asaf Romano (gone)]

Comment on attachment 299390 [details] [diff] [review]
v2.4 code


>+let Ci = Components.interfaces;
>+let Cu = Components.utils;

var!

>+let DownloadMonitorPanel = {

ditto.

>+  //////////////////////////////////////////////////////////////////////////////
>+  //// DownloadMonitorPanel Member Variables
>+
>+  _DM: null,
>+  _panel: null,
>+  _activeStr: null,
>+  _pausedStr: null,
>+  _lastTime: Infinity,
>+
>+  //////////////////////////////////////////////////////////////////////////////
>+  //// DownloadMonitorPanel Public Methods
>+
>+  /**
>+   * Initialize the status panel and member variables
>+   */
>+  DMP_init: function() {

That should be init: function DPM_init() {, same for all other scoped functions.

>+    // Initialize "private" member variables
>+    this._DM = Cc["@mozilla.org/download-manager;1"].
>+               getService(Ci.nsIDownloadManager);

Let's do gDownloadManger in the global scope instead, set in in the delayed timer and use it everywhere.

>+    let numPaused = 0;
>+    let maxTime = -Infinity;
>+    let dls = this._DM.activeDownloads;
>+    while (dls.hasMoreElements()) {
>+      let dl = dls.getNext().QueryInterface(Ci.nsIDownload);
>+      if (dl.state == this._DM.DOWNLOAD_DOWNLOADING) {
>+        // Figure out if this download takes longer
>+        if (dl.speed > 0 && dl.size > 0)
>+          maxTime = Math.max(maxTime, (dl.size - dl.amountTransferred) / dl.speed);
>+        else
>+          maxTime = -1;
>+      }
>+      else if (dl.state == this._DM.DOWNLOAD_PAUSED) {
>+        numPaused++;
>+      }

nit: unnecessary braces.

r=mano otherwise.

Comment 36

[Shawn Wilsher :sdwilsh]

Just talked with mano online - after reading bug 410894 comment 31, he agreed to let the let's in :)

Comment 37

[Ed Lee :Mardak]

Attachment: v2.5 code

r=mano

(In reply to comment #35)
> >+let Ci = Components.interfaces;
> >+let Cu = Components.utils;
> var!
> >+let DownloadMonitorPanel = {
> ditto.
Kept as let! let is the new var! Except in some switch cases without {}s..

> >+  _DM: null,
removed as we're using gDownloadManager now

> >+  DMP_init: function() {
> That should be init: function DPM_init() {, same for all other scoped
> functions.
Switched for both init and updateStatus.

> >+    // Initialize "private" member variables
> >+    this._DM = Cc["@mozilla.org/download-manager;1"].
> >+               getService(Ci.nsIDownloadManager);
> Let's do gDownloadManger in the global scope instead, set in in the delayed
> timer and use it everywhere.
Done.

> >+      else if (dl.state == this._DM.DOWNLOAD_PAUSED) {
> >+        numPaused++;
> >+      }
> nit: unnecessary braces.
Gone. I suppose it doesn't look as bad to have some branches with {} and others without when the else starts on its own line. ;)

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 299512 [details] [diff] [review]
v2.5 code

a=beltzner

Please add the late-l10n keyword when you check this in.

LOCALIZERS: this patch calls strings which were checked in before the l10n freeze.

Comment 39

[Ed Lee :Mardak]

Thanks a bunch for the patch Michael!

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.939; previous revision: 1.938
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.408; previous revision: 1.407
done
Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.159; previous revision: 1.158
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.108; previous revision: 1.107
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.153; previous revision: 1.152
done

Comment 40

[Ed Lee :Mardak]

Attachment: bustage fix

Some reason gDownloadManager was causing mochitests to fail. I've checked this in for the bustage fix, but I don't like it. I don't know why it failed to begin with and why this fixes it.

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.941; previous revision: 1.940
done

I tried switching things to var in both browser.js and downloads.js but that did nothing.

There were a bunch of these errors:
JavaScript error: chrome://mozapps/content/downloads/downloads.js, line 356: gDownloadManager is null

Comment 41

[Ed Lee :Mardak]

browser.js and downloads.js were being loaded together:
<sdwilsh> because of the mac menu bar
<gavin|> via macBrowserOverlay.xul

<brendan> let is the same as var [at top level]
<brendan> Mardak: you are in trouble due to bug 346749 at least
<brendan> each top-level prog has an implicit block around it
<brendan> so when bug 346749 is fixed, you can get away with let x = ... in two or more files where ... differ among the files, and there's no #include like merging of sources
<brendan> Mardak: bug mrbkap to get to fixing bug 346749 soon ;-)

Comment 42

[Asaf Romano (gone)]

Comment on attachment 299525 [details] [diff] [review]
bustage fix

r=mano.

Comment 43

[Stephen Donner [:stephend] Not actively reading bugmail]

I filed bug 414210 so we actually include this in the Proto theme :-)

Comment 44

[Reed Loden [:reed]]

Did this cause the platform-wide Ts / Tp / Txul regression?

Comment 45

[Ed Lee :Mardak]

(In reply to comment #44)
> Did this cause the platform-wide Ts / Tp / Txul regression?
Most likely no? It doesn't even get loaded until 10 seconds after delayed startup.

Comment 46

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre

-and-

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre

Tested:

* Pause
* Resume
* Cancel
* Retry

I need Litmus cases for this.

Comment 47

[Mike Beltzner [:beltzner, not reading bugmail]]

Mardak/Stephend, please file a bug to get the litmus cases filed.

Comment 48

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #47)
> Mardak/Stephend, please file a bug to get the litmus cases filed.

That's what the "in-litmus?" flag in this bug is for...

Comment 49

[Stephen Donner [:stephend] Not actively reading bugmail]

A little unwieldy, but it'll do:

https://litmus.mozilla.org/show_test.cgi?id=5164

in-litmus+

Comment 50

[Stephen Donner [:stephend] Not actively reading bugmail]

My total bad; https://litmus.mozilla.org/show_test.cgi?id=5165 covers the functions in comment 46 that I glossed over in my eagerness to write Litmus cases for the rest of the functionality.  All set, I think!

Comment 51

[Madhava Enros [:madhava]]

Attachment: odd location of page load progress bar

Should the download indicator be to the left of the page load progress bar?

Comment 52

[Alfred Kayser]

The downloadMonitor icon is 32x32, but it is forced resized to 16x16.
It would be better to resize the icon image itself to 16x16, as it will be then better rendered, use less memory, less cpu...

Comment 53

[Shawn Wilsher :sdwilsh]

(In reply to comment #52)
> The downloadMonitor icon is 32x32, but it is forced resized to 16x16.
> It would be better to resize the icon image itself to 16x16, as it will be then
> better rendered, use less memory, less cpu...
That's a theme bug.  Please file it against the appropriate theme.

Comment 55

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #51)
> Created an attachment (id=300073) [details]
> odd location of page load progress bar
> 
> Should the download indicator be to the left of the page load progress bar?

Spun your question off as bug 415718.