Closed
Bug 402278
Opened 16 years ago
Closed 16 years ago
Download activity summary in the status bar
Categories
(Toolkit :: Downloads API, enhancement)
Toolkit
Downloads API
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)
39.54 KB,
image/png
|
Details | |
1.62 KB,
patch
|
Mardak
:
review+
beltzner
:
ui-review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
19.25 KB,
image/png
|
beltzner
:
ui-review+
|
Details |
8.59 KB,
patch
|
Mardak
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
3.09 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
13.62 KB,
image/png
|
Details |
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.
Updated•16 years ago
|
Flags: wanted-firefox3+
Comment 1•16 years ago
|
||
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)
Assignee | ||
Comment 2•16 years ago
|
||
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
Comment 3•16 years ago
|
||
Hmm, Okay. Will hold up until completion of bug #394516. Thanks Ed!
Assignee | ||
Comment 4•16 years ago
|
||
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•16 years ago
|
||
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?
Assignee | ||
Comment 6•16 years ago
|
||
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
Assignee | ||
Comment 7•16 years ago
|
||
or as pike suggested.. activeStatus=One active download;#1 active downloads pausedStatus=One paused download;#1 paused downloads
Assignee | ||
Comment 8•16 years ago
|
||
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•16 years ago
|
||
ahhhhhh 13 hours... it's friday night where I am.. Ill get on it right now.
Assignee | ||
Comment 10•16 years ago
|
||
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•16 years ago
|
||
Hold on an hour, I'm working on this right now...
Comment 12•16 years ago
|
||
activeString=One active download;#1 active downloads pausedString=One paused download;#1 paused downloads do I add those to intl.properties?
Assignee | ||
Comment 13•16 years ago
|
||
(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•16 years ago
|
||
Attachment #294448 -
Attachment is obsolete: true
Attachment #299260 -
Flags: ui-review?(beltzner)
Attachment #299260 -
Flags: review?(mano)
Attachment #294448 -
Flags: ui-review?(beltzner)
Comment 15•16 years ago
|
||
do we want to use the same string for the download manager's title?
Updated•16 years ago
|
Attachment #299260 -
Attachment is patch: true
Attachment #299260 -
Attachment mime type: application/octet-stream → text/plain
Comment 16•16 years ago
|
||
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-
Assignee | ||
Comment 17•16 years ago
|
||
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)
Assignee | ||
Comment 18•16 years ago
|
||
(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•16 years ago
|
||
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+
Assignee | ||
Comment 20•16 years ago
|
||
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)
Comment 21•16 years ago
|
||
(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•16 years ago
|
||
(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.
Reporter | ||
Comment 23•16 years ago
|
||
Comment on attachment 299279 [details]
screenshot
We'll want the icon in there eventually, too.
Attachment #299279 -
Flags: ui-review?(madhava) → ui-review+
Assignee | ||
Comment 24•16 years ago
|
||
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)
Assignee | ||
Comment 25•16 years ago
|
||
Whoops.. forgot a semicolon and a "this."
Attachment #299318 -
Attachment is obsolete: true
Attachment #299319 -
Flags: review?(mano)
Attachment #299318 -
Flags: review?(mano)
Comment 26•16 years ago
|
||
Looks real good! I really hope it makes the upcoming beta release!!!
Assignee | ||
Comment 27•16 years ago
|
||
r=beltzner Splitting off strings to land for l10n freeze
Attachment #299371 -
Flags: review+
Attachment #299371 -
Flags: approval1.9?
Comment 28•16 years ago
|
||
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+
Assignee | ||
Comment 29•16 years ago
|
||
Now with more icon and tooltip!
Attachment #299319 -
Attachment is obsolete: true
Attachment #299372 -
Flags: review?(mano)
Attachment #299319 -
Flags: review?(mano)
Assignee | ||
Comment 30•16 years ago
|
||
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
Assignee | ||
Comment 31•16 years ago
|
||
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)
Assignee | ||
Comment 32•16 years ago
|
||
Now with less tiffness..
Attachment #299382 -
Attachment is obsolete: true
Attachment #299383 -
Flags: ui-review?(madhava)
Attachment #299382 -
Flags: ui-review?(madhava)
Comment 33•16 years ago
|
||
Comment on attachment 299372 [details] [diff] [review] v2.3 code >+let DownloadMonitorPanel = { This also needs to implement the QueryInterface ;)
Assignee | ||
Comment 34•16 years ago
|
||
(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 35•16 years ago
|
||
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+
Comment 36•16 years ago
|
||
Just talked with mano online - after reading bug 410894 comment 31, he agreed to let the let's in :)
Assignee | ||
Comment 37•16 years ago
|
||
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?
Updated•16 years ago
|
Attachment #299383 -
Flags: ui-review?(madhava) → ui-review+
Comment 38•16 years ago
|
||
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+
Assignee | ||
Comment 39•16 years ago
|
||
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
Assignee | ||
Comment 40•16 years ago
|
||
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)
Assignee | ||
Comment 41•16 years ago
|
||
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•16 years ago
|
||
Comment on attachment 299525 [details] [diff] [review] bustage fix r=mano.
Attachment #299525 -
Flags: review?(mano) → review+
I filed bug 414210 so we actually include this in the Proto theme :-)
Comment 44•16 years ago
|
||
Did this cause the platform-wide Ts / Tp / Txul regression?
Assignee | ||
Comment 45•16 years ago
|
||
(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.
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?
Comment 47•16 years ago
|
||
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!
Reporter | ||
Comment 51•16 years ago
|
||
Should the download indicator be to the left of the page load progress bar?
Comment 52•16 years ago
|
||
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•16 years ago
|
||
(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.
Updated•15 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•