Closed Bug 402278 Opened 16 years ago Closed 16 years ago

Download activity summary in the status bar

Categories

(Toolkit :: Downloads API, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta3

People

(Reporter: madhava, Assigned: Mardak)

References

(Depends on 1 open bug)

Details

(Keywords: late-l10n)

Attachments

(6 files, 8 obsolete files)

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.
Flags: wanted-firefox3+
Attached patch Patch for Bug #402278 (obsolete) — Splinter Review
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.
Attachment #294448 - Flags: ui-review?(beltzner)
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.
OS: Mac OS X → All
Hardware: PC → All
Hmm, Okay.  Will hold up until completion of bug #394516.

Thanks Ed!
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.. ?
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?
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
or as pike suggested..

activeStatus=One active download;#1 active downloads
pausedStatus=One paused download;#1 paused downloads
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
ahhhhhh 13 hours... it's friday night where I am.. Ill get on it right now.
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?
Hold on an hour, I'm working on this right now...
activeString=One active download;#1 active downloads
pausedString=One paused download;#1 paused downloads

do I add those to intl.properties?
(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
Attached patch v2 (obsolete) — Splinter Review
Attachment #294448 - Attachment is obsolete: true
Attachment #299260 - Flags: ui-review?(beltzner)
Attachment #299260 - Flags: review?(mano)
Attachment #294448 - Flags: ui-review?(beltzner)
do we want to use the same string for the download manager's title?
Attachment #299260 - Attachment is patch: true
Attachment #299260 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #299260 - Flags: review?(mano) → review-
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
Attachment #299260 - Flags: ui-review?(beltzner)
(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 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.
Attachment #299260 - Flags: ui-review+
Attached image screenshot (obsolete) —
A quick screenshot. :) Other text can be "One paused download" or "2 paused downloads" or "2 active downloads (5 minutes remaining)" for example.
Attachment #299279 - Flags: ui-review?(madhava)
(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.
(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 on attachment 299279 [details]
screenshot

We'll want the icon in there eventually, too.
Attachment #299279 - Flags: ui-review?(madhava) → ui-review+
Attached patch v2.1 (obsolete) — Splinter Review
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.
Assignee: nobody → edilee
Attachment #299260 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #299318 - Flags: review?(mano)
Attached patch v2.2 (obsolete) — Splinter Review
Whoops.. forgot a semicolon and a "this."
Attachment #299318 - Attachment is obsolete: true
Attachment #299319 - Flags: review?(mano)
Attachment #299318 - Flags: review?(mano)
Looks real good! I really hope it makes the upcoming beta release!!!
Attached patch v2.3 stringsSplinter Review
r=beltzner

Splitting off strings to land for l10n freeze
Attachment #299371 - Flags: review+
Attachment #299371 - Flags: approval1.9?
Comment on attachment 299371 [details] [diff] [review]
v2.3 strings

uir+a=beltzner
Attachment #299371 - Flags: ui-review+
Attachment #299371 - Flags: approval1.9?
Attachment #299371 - Flags: approval1.9+
Attached patch v2.3 code (obsolete) — Splinter Review
Now with more icon and tooltip!
Attachment #299319 - Attachment is obsolete: true
Attachment #299372 - Flags: review?(mano)
Attachment #299319 - Flags: review?(mano)
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
Attached image screenshot of v2.3 (obsolete) —
Seems like we have more time now that the strings are in for the l10n freeze.. why not add in a tooltip and icon! ;)
Attachment #299279 - Attachment is obsolete: true
Attachment #299382 - Flags: ui-review?(madhava)
Attached image screenshot of v2.3
Now with less tiffness..
Attachment #299382 - Attachment is obsolete: true
Attachment #299383 - Flags: ui-review?(madhava)
Attachment #299382 - Flags: ui-review?(madhava)
Comment on attachment 299372 [details] [diff] [review]
v2.3 code

>+let DownloadMonitorPanel = {
This also needs to implement the QueryInterface ;)
Attached patch v2.4 code (obsolete) — Splinter Review
(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.
Attachment #299372 - Attachment is obsolete: true
Attachment #299390 - Flags: review?(mano)
Attachment #299372 - Flags: review?(mano)
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.
Attachment #299390 - Flags: review?(mano) → review+
Just talked with mano online - after reading bug 410894 comment 31, he agreed to let the let's in :)
Attached patch v2.5 codeSplinter Review
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. ;)
Attachment #299390 - Attachment is obsolete: true
Attachment #299512 - Flags: review+
Attachment #299512 - Flags: approval1.9?
Attachment #299383 - Flags: ui-review?(madhava) → ui-review+
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.
Attachment #299512 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: late-l10n
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M11
Attached patch bustage fixSplinter Review
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
Attachment #299525 - Flags: review?(mano)
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 on attachment 299525 [details] [diff] [review]
bustage fix

r=mano.
Attachment #299525 - Flags: review?(mano) → review+
Depends on: 414203
I filed bug 414210 so we actually include this in the Proto theme :-)
Did this cause the platform-wide Ts / Tp / Txul regression?
(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.
Depends on: 414266
Depends on: 414375
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Depends on: 414461
Mardak/Stephend, please file a bug to get the litmus cases filed.
(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...
A little unwieldy, but it'll do:

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

in-litmus+
Flags: in-litmus? → in-litmus+
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!
Should the download indicator be to the left of the page load progress bar?
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...
(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.
(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.
Depends on: 415750
Depends on: 418524
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.