474060 - Show download progress in app icon in Windows 7 taskbar

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ed Lee :Mardak]

This might be Firefox specific depending on how APIs work. If we can just overlay over the app's icon independent of the actual icon, it could work from here.

Depending on how much space there is, we could track multiple files or just show an overall progress.

Comment 1

[Rob Arnold [:robarnold]]

For progress, there is ITaskbarList3::SetProgressState and ITaskbarList3::SetProgressValue

If we want an overlay icon, then there ITaskbarList3::SetOverlayIcon which requires an HICON. I'm not sure if we have convenience functions to generate an HICON so this would require more work than the progress.

Comment 2

[Rain [:rain1] (inactive)]

I hacked together an elementary extension to display the download status. It requires the Win7 SDK to build (so I'm keeping it an extension for now), but I'd like to get it in to the core code when Mozilla officially switches to the Windows 7 SDK.

Comment 3

[Rain [:rain1] (inactive)]

Taking

Comment 4

[Shawn Wilsher :sdwilsh]

Feel free to post a patch - I'll do what I can to help make sure this lands.

Comment 5

[Rain [:rain1] (inactive)]

Attachment: Part 1: Add service to set taskbar progress, v1

This adds a new interface and component, nsIWindowsTaskbarProgressService/nsWindowsTaskbarProgressService. I'm still open to other names -- I want to drop the Service at the end, but I'm not sure of toolkit naming policy.

The service is basially just a wrapper around the relevant functions in the ITaskbarList3 API. More functions could possibly be exposed, but I'm not convinced this is the place to do it.

GetHWND is taken mostly from <https://developer.mozilla.org/Talk:En/Code_snippets/Finding_Window_Handles> -- I don't have much of an idea about this code, and nothing else I tried worked.

Comment 6

[Rain [:rain1] (inactive)]

Attachment: Part 1: v1.01 (license blocks fixed)

Comment 7

[Rain [:rain1] (inactive)]

Oh, and there are no SDK checks here, as it isn't clear how that'll pan out.

Comment 8

[Rain [:rain1] (inactive)]

Attachment: Part 1, v1.02: small fix

According to the docs, HrInit() is supposed to be the first thing called.

Comment 9

[Shawn Wilsher :sdwilsh]

It almost feels like this should live in windows widget code to be honest.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 358171 [details] [diff] [review]
Part 1, v1.02: small fix

And I don't know windows code at all, so let's have jimm take a look at this.

Comment 11

[Jim Mathies [:jimm]]

Independent of this patch, we have some issues that need to be worked out. We haven't upgraded the build to support the new sdk (which is still in beta). I doubt we'll move to that until after the final comes out. For the vista sdk, we have a flag for stripping out vista related code, but that was implemented after we switched to the vista sdk officially. So I'm guessing we'll need some sort of temporary flag that turns code like this off. Then we can switch that out later to a flag similar to the vista flag once we embrace the newer sdk.

Comment 12

[Jim Mathies [:jimm]]

I need to chat with some people to figure out what we'll do. I'll split out some bugs once we figure it out and post back here. This patch will likely needs some changes to support whatever we do.

Comment 13

[Jim Mathies [:jimm]]

Well be waiting on the new NTDDI_VERSION selection build changes to get landed. (See bug 472093 for details.) At which point we'll need to wrap this code in an appropriate ifdef block.

Comment 14

[Dave Vasilevsky]

I wrote an extension to display download progress in the dock icon in Mac OS X: https://addons.mozilla.org/en-US/firefox/addon/10485/ . On OS X we have to draw the progress bar ourselves, rather than just SetProgressValue and let the OS do it. Is there any functionality that's worth abstracting into a common component?

Comment 15

[Rain [:rain1] (inactive)]

So, the listener should exist separate from the download window, and update the taskbar item for the download window if it's open, and another window (possibly activeWindow or frontmost window?) if it isn't.

I think the best option is to make the listener a JS XPCOM component in toolkit/mozapps/downloads/, and have it register with the category manager at startup.

Comment 16

[Ed Lee :Mardak]

What if multiple requests to display a progress bar? Perhaps addons update, app update, file download? Would they register with the interface instead of having each component decide if they're allowed to set a progress?

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> I think the best option is to make the listener a JS XPCOM component in
> toolkit/mozapps/downloads/, and have it register with the category manager at
> startup.
That'll hurt perf.  Right now in the browser we do a delayed start (15 seconds after delayed-startup if I recall correctly).

Comment 18

[Rain [:rain1] (inactive)]

(In reply to comment #16)
> What if multiple requests to display a progress bar? Perhaps addons update, app
> update, file download? Would they register with the interface instead of having
> each component decide if they're allowed to set a progress?

Well, if we decide to handle all of it at one place, it becomes significantly more complicated. :)

What do you think of this?
- File download progress shows up in the taskbar even if the downloads window is closed.
- Addon/app update sets the taskbar progress only for its window, and only if the window's open -- so it gets handled separately.

(In reply to comment #17)
> That'll hurt perf.  Right now in the browser we do a delayed start (15 seconds
> after delayed-startup if I recall correctly).

I see. The component should really be loaded the first time a download is started or resumed, but I can't come up with a good way to do this, other than initializing it somewhere in nsDownloadManager.cpp (like what's done with the UI right now). Is that fine?

Comment 19

[Shawn Wilsher :sdwilsh]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6764 would be the right place to put the taskbar stuff.  It's the same code that powers the status bar download meter.

Comment 20

[Rain [:rain1] (inactive)]

(In reply to comment #19)
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6764
> would be the right place to put the taskbar stuff.  It's the same code that
> powers the status bar download meter.

That sounds great. Since this needs to be independent of browser windows, how about a module, which we Cu.import inside the function at <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1342> ?

Comment 21

[Rain [:rain1] (inactive)]

Attachment: [WIP] Part 2: taskbar progress module, v0.8

The logic here is to display a "downloading" state, i.e. Normal, whenever at least one download is going on, and to display a "paused" state whenever all downloads are paused. The "downloading" state is always shown, whether the download manager UI is open or not, while other states (e.g. paused) are shown only when the download manager is open.

Tested cases:
- no downloads previously active, download is started, download manager window opens (both before and after delayed startup)
- downloads auto-resume at startup
- download manager window is manually opened (before and after delayed startup)

Still to work out:
- before delayed startup, download started, download manager UI doesn't open -- the progress doesn't show up then, but neither does the taskbar panel

Perf-wise this shouldn't be an issue, since the module is always loaded after the first time the download manager is initialized.

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 358171 [details] [diff] [review]
Part 1, v1.02: small fix

Removing request, waiting on compat. with bug 472093.

Comment 23

[Rain [:rain1] (inactive)]

Attachment: Part 1, v1.5 -- code moved to widget/, updated to use MOZ_WINSDK_TARGETVER

widget/ is probably a better place for this. This code also uses MOZ_WINSDK_TARGETVER now to indicate that it needs the Windows 7 SDK to build.

Comment 24

[Rain [:rain1] (inactive)]

Attachment: Part 1, v1.51 -- small fix

needed to wrap one more bit inside a MOZ_NTDDI_WIN7

Comment 25

[Rain [:rain1] (inactive)]

Attachment: Part 2: taskbar progress module, v1.0

This is on top of the NTDDI and taskbar progress service patches.

I've tested it on both Windows 7 and XP, and it looks like it works fine in Win7, and doesn't negatively affect XP.

Comment 26

[Ere Maijala (slow)]

Comment on attachment 374529 [details] [diff] [review]
Part 1, v1.51 -- small fix

Sorry, I'm not a super-reviewer.

Comment 27

[Rain [:rain1] (inactive)]

Comment on attachment 374529 [details] [diff] [review]
Part 1, v1.51 -- small fix

Oops, sorry, Ere.

Comment 28

[Rain [:rain1] (inactive)]

Attachment: Part 1, fixed bitrot

Comment 29

[Jim Mathies [:jimm]]

Comment on attachment 375583 [details] [diff] [review]
Part 1, fixed bitrot

This looks really clean. Do we really have to jump through all those hoops just to get the HWND of a dom window? That seemed pretty crazy.

Comment 30

[Rain [:rain1] (inactive)]

(In reply to comment #29)
> (From update of attachment 375583 [details] [diff] [review])
> Do we really have to jump through all those hoops just
> to get the HWND of a dom window? That seemed pretty crazy.

As far as I can tell, yes. :(

That code's essentially taken from <https://developer.mozilla.org/Talk:En/Code_snippets/Finding_Window_Handles>. I asked plasticmillion if he had found a better way or had written an API to do this, and he said no.

Comment 31

[Jim Mathies [:jimm]]

Comment on attachment 375464 [details] [diff] [review]
Part 2: taskbar progress module, v1.0

I'm not the right reviewer for this, you should choose someone from the browser module owners list.

http://www.mozilla.org/owners.html#firefox

Comment 32

[Rain [:rain1] (inactive)]

Comment on attachment 375464 [details] [diff] [review]
Part 2: taskbar progress module, v1.0

Requesting review from vlad for browser changes and sdwilsh for toolkit changes.

Shawn, as mentioned in comment 21, an invariant maintained in the code is that the module is always initialized only after the download manager is.

Comment 33

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 375464 [details] [diff] [review]
Part 2: taskbar progress module, v1.0

Hrm.  Two comments:

1) Why do we Cu.import DownloadTaskbarProgress.js in downloads.js?  Even with the browser import, we'll have a DownloadTaskbarProgress per window -- that seems wrong, given that the code there transfers the progress to the next available window.  This seems like it really wants to be a service component.

2) Do we have to actually add load listeners on all windows?  The only two window types that make sense to care about are toplevel browser windows and the download manager.  Let's just decorate their init code -- the download manager can take the taskbar progress on open, give it away on close; the browser can check if another window has it, and if not, take the taskbar progress.

Comment 34

[Shawn Wilsher :sdwilsh]

(In reply to comment #33)
> 1) Why do we Cu.import DownloadTaskbarProgress.js in downloads.js?  Even with
> the browser import, we'll have a DownloadTaskbarProgress per window -- that
> seems wrong, given that the code there transfers the progress to the next
> available window.  This seems like it really wants to be a service component.
Cu.import gets the same instance if it's already been imported somewhere else, so it is a service.  See https://developer.mozilla.org/en/Components.utils.import#Difference_from_mozIJSSubScriptLoader for more details.  (unless I misunderstand what you are asking)

Comment 35

[Rain [:rain1] (inactive)]

(In reply to comment #33)
> 
> 2) Do we have to actually add load listeners on all windows?  The only two
> window types that make sense to care about are toplevel browser windows and the
> download manager.  Let's just decorate their init code -- the download manager
> can take the taskbar progress on open, give it away on close; the browser can
> check if another window has it, and if not, take the taskbar progress.

Yes, that makes sense.

Comment 36

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #34)
> Cu.import gets the same instance if it's already been imported somewhere else,
> so it is a service.

Ah, I didn't know that -- ok, ignore issue #1 :)

Comment 37

[Shawn Wilsher :sdwilsh]

Comment on attachment 375464 [details] [diff] [review]
Part 2: taskbar progress module, v1.0

global nit: javadoc style comments on all methods you are adding please.
global nit: all pArameters should be prefixed with an a

>+++ b/toolkit/mozapps/downloads/content/downloads.js
>@@ -53,16 +53,25 @@ const nsLocalFile = Components.Construct
>                                            "nsILocalFile", "initWithPath");
> 
> var Cc = Components.classes;
> var Ci = Components.interfaces;
> let Cu = Components.utils;
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/DownloadUtils.jsm");
> Cu.import("resource://gre/modules/PluralForm.jsm");
>+#ifdef XP_WIN
>+// Do this at load with a timeout, so that the window is drawn correctly
>+// initially
>+window.addEventListener("load", function () {
>+  window.setTimeout(function () {
>+    Cu.import("resource://gre/modules/DownloadTaskbarProgress.jsm");
>+  }, 1);
>+}, false);
>+#endif
do we really want this for both the browser window and the download window to do this?  This should get UI review.

>+++ b/toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm
>+ * The Initial Developer of the Original Code is
>+ *  Siddharth Agarwal <[e-mail]>
nit: no indent for your name

>+  /// Reference to the download manager
>+  _downloadManager: null,
we usually use dm for the download manager everywhere else, so I'd like to keep with that consistency

>+  /// Initialize, register ourselves as a download progress listener
>+  _init: function download_taskbar_progress_init()
>+  {
>+    // We throw and bail out at this point if the service isn't available
>+    try {
>+      this._taskbarProgress = Cc["@mozilla.org/taskbar-progress-service;1"]
>+                                .getService(Ci.nsITaskbarProgressServiceWin);
>+    }
>+    catch (e) { return; }
new line for the return, and a comment why we are returning please

>+    let observerService = Cc["@mozilla.org/observer-service;1"]
>+                            .getService(Ci.nsIObserverService);
This isn't the right format here.  You want
let observerService = Cc["@mozilla.org/observer-service;1"].
                      getService(Ci.nsIObserverService);
This applies globally.

>+  _updateTaskbar: function download_taskbar_update_taskbar(window,
>+                                                           isDownloadManager)

>+    if (isDownloadManager || (!isDownloadManager && this._taskbarState ==
>+                              this._taskbarProgress.Normal))
>+    {
This brace should be the same line as the closing paren

>+    // CLear any state otherwise
nit: typo

>+    else
>+      this._clearTaskbar(window);
nit: if any part of the if block needs a brace, all parts should be braced please

>+      if (windows.hasMoreElements())
>+      {
nit: brace on previous line

>+      // Enumerate all active downloads
>+      let downloads = this._downloadManager.activeDownloads;
>+      while (downloads.hasMoreElements())
>+      {
>+        let download = downloads.getNext().QueryInterface(Ci.nsIDownload);
>+        // Only set values if we actually know the download size
>+        if (download.percentComplete < 100 && download.size > 0)
>+        {
>+          totalSize += download.size;
>+          totalTransferred += download.amountTransferred;
>+        }
>+        // We might need to display a paused state, so track this
>+        if (download.state == this._downloadManager.DOWNLOAD_PAUSED)
>+          numPaused++;
>+      }
>+
>+      // If all downloads are paused, show the progress as paused, unless we
>+      // don't have any information about sizes, in which case we don't
>+      // display anything
>+      if (numActive == numPaused)
>+      {
>+        if (totalSize == 0)
>+          this._taskbarState = this._taskbarProgress.NoProgress;
>+        else
>+          this._taskbarState = this._taskbarProgress.Paused;
>+      }
>+      // If at least one download is not paused, and we don't have any
>+      // information about download sizes, display an indeterminate indicator
>+      else if (totalSize == 0)
>+        this._taskbarState = this._taskbarProgress.Indeterminate;
>+      // Otherwise display a normal progress bar
>+      else
>+        this._taskbarState = this._taskbarProgress.Normal;
>+    }
>+
>+    this._totalSize = totalSize;
>+    this._totalTransferred = totalTransferred;
>+    this._updateTaskbar(this._activeWindow, this._activeWindow.__DT_isDM);
Is this the same logic in downloads.js?  If so, we should abstract that out into a JSM as well.

Comment 38

[Jim Mathies [:jimm]]

For parity with IE, this really should land on 1.9.1 for win7. If it's not blocking, it should be wanted at least.

Comment 39

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

Yup, you know the drill; get it on trunk and show me a safe patch (with tests, if possible, or litmus test descriptions if not) and we'll consider it for approval when you mark the patch approval-1.9.1?

Comment 40

[Jim Mathies [:jimm]]

Updates coming per comments from Vlad and Shawn Sid? I was hoping to apply this locally and see how it works now that we're past the RC.

Also, I'm curious, how does Win7 handle the display with grouped windows on the superbar and multiple windows are downloading content?

Comment 41

[Rain [:rain1] (inactive)]

(In reply to comment #40)
> Updates coming per comments from Vlad and Shawn Sid? I was hoping to apply this
> locally and see how it works now that we're past the RC.

Sorry, I've been very busy lately and haven't had any time at all. I'm interested in getting this in as well, and I'll see if I have time this weekend.

You can still apply both patches locally and play with it -- the comments look like they're mainly nits.

> 
> Also, I'm curious, how does Win7 handle the display with grouped windows on the
> superbar and multiple windows are downloading content?

Described at <http://msdn.microsoft.com/en-us/library/dd391697%28VS.85%29.aspx>, in "How the Taskbar Button Chooses the Progress Indicator for a Group".

Comment 42

[Rain [:rain1] (inactive)]

Attachment: Part 2: taskbar progress module, v2.0

(In reply to comment #33)
> 
> 2) Do we have to actually add load listeners on all windows?  The only two
> window types that make sense to care about are toplevel browser windows and the
> download manager.  Let's just decorate their init code -- the download manager
> can take the taskbar progress on open, give it away on close; the browser can
> check if another window has it, and if not, take the taskbar progress.

Done.

(In reply to comment #37)
> (From update of attachment 375464 [details] [diff] [review])

> do we really want this for both the browser window and the download window to
> do this?  This should get UI review.

OK, asking faaborg for UI review. The behaviour is described in the comments to _updateTaskbar and _updateStatus. Sorry, I can't provide try builds because the boxes don't have the Windows 7 SDK. I'll try uploading some custom builds somewhere within a few days.

> Is this the same logic in downloads.js?  If so, we should abstract that out
> into a JSM as well.

I could find two other places that enumerate through all downloads similarly: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6924> and <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/content/downloads.js#383>. What they do within the loop looks like it's pretty different though. Do you think it's worth abstracting this out? I guess one way to do that would be to calculate and return everything for use as callers see fit.

I've fixed all the other nits.

Comment 43

[Shawn Wilsher :sdwilsh]

Comment on attachment 383201 [details] [diff] [review]
Part 2: taskbar progress module, v2.0

>+++ b/toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm
>+  /**
>+   * Sets the active window, and whether it's the download window. This takes
>+   * care of clearing out the previous active window's taskbar item, updating
>+   * the taskbar, and setting an onunload listener.
>+   */
please describe what the parameters are for with java-doc style comments

>+  /**
>+   * Called when a browser window appears. This has an effect only when we
>+   * don't already have an active window.
>+   */
>+  onBrowserWindowLoad: function download_taskbar_on_browser_window_load(
>+                           aWindow)
ditto

>+  /**
>+   * Called when the download window appears. The download window will take
>+   * over as the active window.
>+   */
>+  onDownloadWindowLoad: function download_taskbar_on_download_window_load(
>+                             aWindow)
ditto

r=sdwilsh

Comment 44

[Rob Arnold [:robarnold]]

Comment on attachment 383201 [details] [diff] [review]
Part 2: taskbar progress module, v2.0

>--- a/toolkit/mozapps/downloads/content/downloads.js
>+++ b/toolkit/mozapps/downloads/content/downloads.js
>@@ -53,16 +53,27 @@ const nsLocalFile = Components.Construct
>                                            "nsILocalFile", "initWithPath");
> 
> var Cc = Components.classes;
> var Ci = Components.interfaces;
> let Cu = Components.utils;
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/DownloadUtils.jsm");
> Cu.import("resource://gre/modules/PluralForm.jsm");
>+#ifdef XP_WIN
>+// Do this at load with a timeout, so that the window is drawn correctly
>+// initially
>+window.addEventListener("load", function () {
>+  window.setTimeout(function () {
>+    Cu.import("resource://gre/modules/DownloadTaskbarProgress.jsm");
>+    if (DownloadTaskbarProgress)
>+      DownloadTaskbarProgress.onDownloadWindowLoad(window);
>+  }, 1);
>+}, false);
>+#endif

This is a hack. The Windows taskbar will send us a message when the button is ready and that's when we should do the initialization. As part of my Aero Peek implementation, I wrote an implementation of a taskbar interface that will listen for the event and fire an observable event when it happens (and let users query if it already happened). I will try to post a version of that to this bug so that the two existing patches can be rebased against it.

Comment 45

[Rain [:rain1] (inactive)]

Sounds great. I was looking for such an event but couldn't find one in a cursory search.

Comment 46

[Rob Arnold [:robarnold]]

Sid, can you mind basing your patch off of the taskbar interface in bug 501490? Jim Mathies is basing Jumplists off of that patch too.

Comment 47

[Jim Mathies [:jimm]]

Would anyone object to moving this out of downloads and into a taskbar mozapps folder? The folder would serve as a general place for taskbar related code for all platforms. I've got a jumplist jsm that needs a place that fits, so I figured why not put everything related to taskbars together.

Comment 48

[Shawn Wilsher :sdwilsh]

I think that for general purpose stuff, it makes sense to put it in a general spot, but I feel like specific stuff, like in this case downloads, should go with the code it is for.  The same applies to the tab preview stuff, which won't even live in toolkit because it is browser specific.

Comment 49

[Jim Mathies [:jimm]]

(In reply to comment #48)
> I think that for general purpose stuff, it makes sense to put it in a general
> spot, but I feel like specific stuff, like in this case downloads, should go
> with the code it is for.  The same applies to the tab preview stuff, which
> won't even live in toolkit because it is browser specific.

I really don't see it as tied to downloads really, taskbar progress can be more generic. For example I could see an email client using it to show the status of checking for new mail.

Comment 50

[Shawn Wilsher :sdwilsh]

OK - fair.  We should have some generic code that helps with taskbar progress, but the download specific stuff should live with the download manager code.  Having something in toolkit that makes it easier to do the taskbar progress stuff would be a big win for the platform.

Comment 51

[Rain [:rain1] (inactive)]

(In reply to comment #49)
> (In reply to comment #48)
> > I think that for general purpose stuff, it makes sense to put it in a general
> > spot, but I feel like specific stuff, like in this case downloads, should go
> > with the code it is for.  The same applies to the tab preview stuff, which
> > won't even live in toolkit because it is browser specific.
> 
> I really don't see it as tied to downloads really, taskbar progress can be more
> generic. For example I could see an email client using it to show the status of
> checking for new mail.

That generic code (part 1 of the fix, basically exposes the native API) is currently in widget/windows. Do you mean that I should move it out to toolkit?

I'm not sure any part of the download progress module can be generalized...

Comment 52

[Jim Mathies [:jimm]]

> That generic code (part 1 of the fix, basically exposes the native API) is
> currently in widget/windows. Do you mean that I should move it out to toolkit?
> 
> I'm not sure any part of the download progress module can be generalized...

No it's fine the way it is. Jump list need's for something in mozapps is diminishing anyway due to app specific strings and settings.

Comment 53

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #48)
> The same applies to the tab preview stuff, which
> won't even live in toolkit because it is browser specific.

Mail has tabs too, as well as SeaMonkey and Songbird. What makes you think it's browser-specific?

Comment 54

[Shawn Wilsher :sdwilsh]

(In reply to comment #53)
> Mail has tabs too, as well as SeaMonkey and Songbird. What makes you think it's
> browser-specific?
Because they'll have to have a totally different implementation because tabbrowser.xml lives in browser, so they'll have something different as well.

Comment 55

[Alex Faaborg [:faaborg] (Firefox UX)]

I don't have a windows 7 box up and running at the moment.  To expedite the ui-review could you post screen shots of IE doing a similar notification, and the patch applied?

Comment 56

[Vorenus]

Attachment: IE downloading a file in Windows 7

Alex, this is what IE looks like when it downloads a file in Windows 7. Also, for more info on taskbar usage see: http://msdn.microsoft.com/en-us/library/aa511446%28loband%29.aspx

Comment 57

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 383201 [details] [diff] [review]
Part 2: taskbar progress module, v2.0

Thanks for the details.  Approving under the assumption that we are just replicating the native platform behavior.  If that isn't the case, please post explaining the differences.

Also it will be useful to get this tested on trunk to get a sense of how it behaves.

Comment 58

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 383201 [details] [diff] [review]
Part 2: taskbar progress module, v2.0

clearing review based on rob/jim's comments

Comment 59

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 375583 [details] [diff] [review]
Part 1, fixed bitrot

sr'ing this because I think it needs integration with the (bigger) aero peek patch

Comment 60

[Rain [:rain1] (inactive)]

Attachment: Part 1, now on top of the taskbar preview patch

I've basically followed the design of the taskbar preview patch here. Rob, Jim, Vlad, does this look fine?

Comment 61

[Rain [:rain1] (inactive)]

Attachment: Part 2, v3.0

This is on top of Part 1.

Alex: the UI here is slightly different from IE's. I'll attach screenshots in a bit.

Comment 62

[Rain [:rain1] (inactive)]

Attachment: screenshot for download in progress, download window open

Comment 63

[Rain [:rain1] (inactive)]

Attachment: actual screenshot for download in progress, download window open

oops

Comment 64

[Rain [:rain1] (inactive)]

Attachment: screenshot for download paused, download progress open

Comment 65

[Rain [:rain1] (inactive)]

Attachment: screenshot for download in progress, download window closed

IE doesn't have an equivalent to this, since for a download to happen there the download window must be open. So this patch shows downloads if they're in progress even if the download manager is closed...

Comment 66

[Rain [:rain1] (inactive)]

Attachment: screenshot for download paused, download window closed

...however, it doesn't show anything if the download window is closed and all downloads are currently paused.

Alex, what do you think?

Comment 67

[Rain [:rain1] (inactive)]

(In the default configuration, the progress simply shows up on the icon.)

Comment 68

[Jim Mathies [:jimm]]

(In reply to comment #63)
> Created an attachment (id=398957) [details]
> actual screenshot for download in progress, download window open
> 
> oops

Sid, is that the console entry on the taskbar to the left of the download window, or is that the main window? Once the Jump Lists patch lands, all of these windows will be grouped together, so I'm curious how grouping will affect what you've done here based on the state of the download window.

Comment 69

[Rain [:rain1] (inactive)]

(In reply to comment #68)
> (In reply to comment #63)
> > Created an attachment (id=398957) [details] [details]
> > actual screenshot for download in progress, download window open
> > 
> > oops
> 
> Sid, is that the console entry on the taskbar to the left of the download
> window, or is that the main window?

That's the main window.

> Once the Jump Lists patch lands, all of
> these windows will be grouped together, so I'm curious how grouping will affect
> what you've done here based on the state of the download window.

It shouldn't. The taskbar is in the "Never Combine" configuration in the screenshot. If the buttons are grouped (default configuration, "Always combine, hide labels"), the download status shows up on the icon, just like the IE screenshot posted earlier.

Comment 70

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 398955 [details] [diff] [review]
Part 2, v3.0

Looks good based on described behavior and screen shots.  I'll file follow up bugs if anything doesn't seem right after having a chance to try this out for awhile.

Comment 71

[Shawn Wilsher :sdwilsh]

Comment on attachment 398955 [details] [diff] [review]
Part 2, v3.0

Most nits, but I'm r- because I'd like to run through it again.  I'm also concerned that we have a lot of private methods that can easily be accessed by outside objects that import this.  I'd prefer if they were global objects/functions in the file that were not exported.  For more detailed review comments, please see http://reviews.visophyte.org/r/398955/

on file: toolkit/mozapps/downloads/content/downloads.js line 62
> #ifdef XP_WIN
> window.addEventListener("load", function () {
>   Cu.import("resource://gre/modules/DownloadTaskbarProgress.jsm");
>   if (DownloadTaskbarProgress)
>     DownloadTaskbarProgress.onDownloadWindowLoad(window);
> }, false);
> #endif

This would be better to do in the Startup method.


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 1
> /* -*- Mode: JavaScript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* ***** BEGIN LICENSE BLOCK *****

/* -*- Mode: JavaScript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset:
2 -*-
 * vim: sw=2 ts=2 sts=2 et filetype=javascript
 * ***** BEGIN LICENSE BLOCK *****
please


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 38
> var EXPORTED_SYMBOLS = [ "DownloadTaskbarProgress" ];

nit: format like this please:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#41


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 40
> const Cc = Components.classes;
> const Ci = Components.interfaces;
> 
> const kTaskbarID = "@mozilla.org/windows-taskbar;1";

nit: add a heading for constants like this please:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#49


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 45
> var DownloadTaskbarProgress =

nit: ditto on heading comment


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 47
>   /// Reference to the taskbar

global nit: only two / for inline comments like this please


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 57
>   get _observerService() {
>     let observerService = Cc["@mozilla.org/observer-service;1"].
>                           getService(Ci.nsIObserverService);
>     this.__defineGetter__("_observerService", function() observerService);
>     return this._observerService;
>   },

nit: please use XPCOMUtils.generateLazyServiceGetter like
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#155


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 64
>   /// Initialize and register ourselves as a download progress listener

nit: real javadoc style comments please (globally)


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 145
>       let docShell = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>                        getInterface(Ci.nsIWebNavigation).
>                        QueryInterface(Ci.nsIDocShellTreeItem).treeOwner.
>                        QueryInterface(Ci.nsIInterfaceRequestor).
>                        getInterface(Ci.nsIXULWindow).docShell;

It's a shame we don't have some utility method to get the docshell :/


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 279
>   /**
>    * We observe the taskbar button becoming ready to receive progress
>    * notifications
>    */
>   observe: function download_taskbar_observe(aSubject, aTopic, aData)
>   {
>     if (aTopic == "windows-taskbar") {
>       this._observerService.removeObserver(this, aTopic);
>       this._onReady();
>     }
>   },

prefix this bit with an nsIObserver heading please since it is implementing
that (goes the same for other interfaces as well)


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 291
>   /**
>    * Called when a browser window appears. This has an effect only when we
>    * don't already have an active window.
>    */
>   onBrowserWindowLoad: function download_taskbar_on_browser_window_load(
>                            aWindow)
>   {
>     if (!this._setActiveWindowArgs && !this._activeTaskbarProgress)
>       this._setActiveWindow(aWindow, false);
>   },
> 
>   /**
>    * Called when the download window appears. The download window will take
>    * over as the active window.
>    */
>   onDownloadWindowLoad: function download_taskbar_on_download_window_load(
>                              aWindow)
>   {
>     this._setActiveWindow(aWindow, true);
>   },

These are the public API methods, yes?  I'd prefer them to be up top.


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 320
>    * @param aTaskbarProgress The taskbar progress for the window that is being
>    *                         unloaded

global formatting nit:
@param aTaskbarProgress
       The taskbar progress...


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 323
>   _onActiveWindowUnload: function download_taskbar_on_active_window_unload(

in general, our format for named functions would result in this instead:
DTP_onActiveWindowUnload
this has the benefit of having the same name (when doing a search) as the
property, so the mapping is clearer.


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 341
>   onProgressChange: function()

nit: use a named function


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 348
>   onDownloadStateChange: function()

nit: use a named function


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 359
> // Initialize the module

nit: use a heading-style comment

Comment 72

[Rob Arnold [:robarnold]]

Comment on attachment 398953 [details] [diff] [review]
Part 1, now on top of the taskbar preview patch

>+  /**
>+   * Sets the taskbar progress state and value for this window. The currentValue
>+   * and maxValue parameters are optional and should be supplied when |state| is
>+   * one of kNormal, kError or kPaused.
>+   *
>+   * @throws NS_ERROR_ILLEGAL_VALUE if state is kNoProgress or kIndeterminate,
>+   *                                and currentValue and maxValue are both not 0.
>+   */
>+  void setProgressState(in nsTaskbarProgressState state,
>+                        [optional] in unsigned long long currentValue,
>+                        [optional] in unsigned long long maxValue);
>+};

Do the paused or error states need the current/max values? The underlying API doesn't seem to even use them (of course I'm just reading the docs - maybe it does). It doesn't make sense to me to have to provide the progress amounts when I pause or error.

>+
>+// Defined in dwmapi in a header that needs a higher numbered _WINNT #define
>+#define DWM_SIT_DISPLAYFRAME 0x1

Copy & paste? This doesn't belong here.

A good chunk of the rest of the patch strikes me as being very similar to my Taskbar*Preview classes. I think we would save a good chunk of code if TaskbarProgress wasn't separate since it's essentially parallel to the WindowPreviewClass in terms of data structure (weak ptr from nsWindow) and most of the behavior. I'm not sure that we need a new interface either - nsITaskbarProgress could be merged into nsITaskbarWindowPreview.

Comment 73

[Rain [:rain1] (inactive)]

> Do the paused or error states need the current/max values? The underlying API
> doesn't seem to even use them (of course I'm just reading the docs - maybe it
> does). It doesn't make sense to me to have to provide the progress amounts when
> I pause or error.

Pause and error show a determinate progress, so the underlying API does need a value.

> 
> >+
> >+// Defined in dwmapi in a header that needs a higher numbered _WINNT #define
> >+#define DWM_SIT_DISPLAYFRAME 0x1
> 
> Copy & paste? This doesn't belong here.

Oops. Fixed.

> 
> A good chunk of the rest of the patch strikes me as being very similar to my
> Taskbar*Preview classes. I think we would save a good chunk of code if
> TaskbarProgress wasn't separate since it's essentially parallel to the
> WindowPreviewClass in terms of data structure (weak ptr from nsWindow) and most
> of the behavior. I'm not sure that we need a new interface either -
> nsITaskbarProgress could be merged into nsITaskbarWindowPreview.

Would we call the combined interface something different then?

Comment 74

[Jim Mathies [:jimm]]

(In reply to comment #73)

> > 
> > A good chunk of the rest of the patch strikes me as being very similar to my
> > Taskbar*Preview classes. I think we would save a good chunk of code if
> > TaskbarProgress wasn't separate since it's essentially parallel to the
> > WindowPreviewClass in terms of data structure (weak ptr from nsWindow) and most
> > of the behavior. I'm not sure that we need a new interface either -
> > nsITaskbarProgress could be merged into nsITaskbarWindowPreview.
> 
> Would we call the combined interface something different then?

nsITaskbarWindow?

Comment 75

[Rob Arnold [:robarnold]]

I thought about that but nsITaskbarWindowPreview still works - showing download progress on the taskbar is a preview of the window contents.

Comment 76

[Rimas Kudelis [use needinfo to ping]]

Wouldn't nsITaskbarPreview (without the Window part) do just as well?

Comment 77

[Rob Arnold [:robarnold]]

No, there are also tab previews (nsITaskbarTabPreview) on Windows 7. See bug 501490. Tab previews cannot have progress shown on them (taskbar ui choice).

Comment 78

[Jim Mathies [:jimm]]

(In reply to comment #75)
> I thought about that but nsITaskbarWindowPreview still works - showing download
> progress on the taskbar is a preview of the window contents.

Could we still avoid extra code chunks and have it inherit?

nsITaskbarPreview
 - nsITaskbarTabPreview
 - nsITaskbarWindowPreview
 - nsITaskbarProgress

I think the progress is app global, so associating it with a particular window might not make sense.

Comment 79

[Rob Arnold [:robarnold]]

The actual API is per window. What you see in the taskbar actually depends on your taskbar settings. If you have the old one-taskbar-item-per-window setting, then you can see a different progress bar on each window. The default setting (one icon per app) combines them via some formula I think.

We could have TaskbarWindowPreview implement a separate interface, sure. I'm not sure if that's valuable unless we think that some other platform might have an implementation (presumably w/o the other nsITaskbarWindowPreview bits).

Comment 80

[Rain [:rain1] (inactive)]

Attachment: Part 2, v4.0

Addressed most of sdwilsh's comments except for /// on non-function properties, since they're parse-able by doxygen.

Comment 81

[Rain [:rain1] (inactive)]

(In reply to comment #79)
> I'm
> not sure if that's valuable unless we think that some other platform might have
> an implementation

There was talk about a mac implementation earlier in the thread, comment 14. I think I like Jim's idea of the same component implementing multiple interfaces.

Comment 82

[Shawn Wilsher :sdwilsh]

Comment on attachment 399988 [details] [diff] [review]
Part 2, v4.0

http://reviews.visophyte.org/r/399988/

on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 158
>    *        The window to set as active

global nit: period at end of sentence please.


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 225
>     if (this._activeWindowIsDownloadWindow || (this._taskbarState ==
>         Ci.nsITaskbarProgress.kNormal))
>       this._activeTaskbarProgress.setProgressState(this._taskbarState,
>                                                    this._totalTransferred,
>                                                    this._totalSize);
>     // Clear any state otherwise
>     else
>       this._clearTaskbar();

please use braces here and have the second condition of the or on its own line
please


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 243
>     if (this._activeTaskbarProgress)
>       this._activeTaskbarProgress.setProgressState(
>         Ci.nsITaskbarProgress.kNoProgress);

Please use braces here too. In general, even if it's only one "line" of code
but spans multiple numbered lines, please use braces.


on file: toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm line 344
>   /* nsIObserver */

nit: like this please
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManagerUI.js#58

r=sdwilsh

Comment 83

[Rain [:rain1] (inactive)]

Rob's going to look a bit closer at the window preview implementation by the end of this week -- specifically, the part about controllers.

Comment 84

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

But is this bug basically done with that patch, or is there more to do? Wondering if we can get an ETA on resolution of this bug so I can build a beta schedule.

Comment 85

[Rain [:rain1] (inactive)]

(In reply to comment #84)
> But is this bug basically done with that patch, or is there more to do?
> Wondering if we can get an ETA on resolution of this bug so I can build a beta
> schedule.

Yes, it's done once part 1 gets reviews. Rob or Jim, could you make a call here? 

Looking at the code, I think that while there is a bit of duplication, the entire component is rather small to begin with, and that the benefit of not having to clutter up the taskbar preview code outweighs the cost of duplication.

Comment 86

[Jim Mathies [:jimm]]

(In reply to comment #85)
> (In reply to comment #84)
> > But is this bug basically done with that patch, or is there more to do?
> > Wondering if we can get an ETA on resolution of this bug so I can build a beta
> > schedule.
> 
> Yes, it's done once part 1 gets reviews. Rob or Jim, could you make a call
> here? 

The idea was to blend this into the nsITaskbarWindowPreview class since we have code duplicated between rob's preview class and the progress class. The remaining issue was the controller. rob was suggesting in #gfx that a solution would be to implement a default dummy controller. We need to sort that problem out still and look at getting this into nsITaskbarWindowPreview.

Comment 87

[Shawn Wilsher :sdwilsh]

(In reply to comment #86)
> The idea was to blend this into the nsITaskbarWindowPreview class since we have
> code duplicated between rob's preview class and the progress class. The
> remaining issue was the controller. rob was suggesting in #gfx that a solution
> would be to implement a default dummy controller. We need to sort that problem
> out still and look at getting this into nsITaskbarWindowPreview.
Can we do that refactoring post-1.9.2?

Comment 88

[Jim Mathies [:jimm]]

(In reply to comment #87)
> (In reply to comment #86)
> > The idea was to blend this into the nsITaskbarWindowPreview class since we have
> > code duplicated between rob's preview class and the progress class. The
> > remaining issue was the controller. rob was suggesting in #gfx that a solution
> > would be to implement a default dummy controller. We need to sort that problem
> > out still and look at getting this into nsITaskbarWindowPreview.
> Can we do that refactoring post-1.9.2?

Anything is possible. Changes though would require changes to client code - anyone who relied on this interface (fx, songbird, thunderbird, etc.) would have to rework their code in the next release. Honestly this isn't a call I can make. Rob thinks another patch for his taskbar stuff will be out in a few days, from there I'm not sure how long it would take sid to update his code. (Although some of the work could be done now based on the current patch in bug 501490).

Comment 89

[Shawn Wilsher :sdwilsh]

We are working against a beta freeze - this has to make beta if we want it in for 3.6.

Comment 90

[Rain [:rain1] (inactive)]

(In reply to comment #88)
> Changes though would require changes to client code -
> anyone who relied on this interface (fx, songbird, thunderbird, etc.) would
> have to rework their code in the next release.

Not if we keep the interfaces separate but combine the components into one.

Comment 91

[Dão Gottwald [:dao]]

Comment on attachment 399988 [details] [diff] [review]
Part 2, v4.0

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+#ifdef XP_WIN
>+    // On Windows, initialize download taskbar progress
>+    Cu.import("resource://gre/modules/DownloadTaskbarProgress.jsm");
>+    if (DownloadTaskbarProgress)
>+      DownloadTaskbarProgress.onBrowserWindowLoad(window);
>+#endif

Make this local, DownloadTaskbarProgress doesn't need to be global:

let tempScope = {};
Cu.import("resource://gre/modules/DownloadTaskbarProgress.jsm", tempScope);
tempScope.DownloadTaskbarProgress.onBrowserWindowLoad(window);

What's the point of 'if (DownloadTaskbarProgress)'?

Comment 92

[Rain [:rain1] (inactive)]

(In reply to comment #91)
> What's the point of 'if (DownloadTaskbarProgress)'?

It's for builds using older SDKs, which won't have the taskbar component. DownloadTaskbarProgress is nulled out in _init.

Comment 93

[Dão Gottwald [:dao]]

Making onBrowserWindowLoad and onDownloadWindowLoad no-ops in that case seems more consumer-friendly to me, and would be consistent with the other win7 modules.

Comment 94

[Rain [:rain1] (inactive)]

(In reply to comment #93)
> Making onBrowserWindowLoad and onDownloadWindowLoad no-ops in that case seems
> more consumer-friendly to me, and would be consistent with the other win7
> modules.

OK, that should be easy enough.

Comment 95

[Dão Gottwald [:dao]]

Comment on attachment 398953 [details] [diff] [review]
Part 1, now on top of the taskbar preview patch

>+interface nsITaskbarProgress : nsISupports
>+{
>+  /**
>+   * Stop displaying progress on the taskbar button. This should be used when
>+   * the operation is complete or cancelled.
>+   */
>+  const nsTaskbarProgressState kNoProgress    = 0;
>+
>+  /**
>+   * Display a cycling, indeterminate progress bar.
>+   */
>+  const nsTaskbarProgressState kIndeterminate = 1;
>+
>+  /**
>+   * Display a determinate, normal progress bar.
>+   */
>+  const nsTaskbarProgressState kNormal        = 2;
>+
>+  /**
>+   * Display a determinate, error progress bar.
>+   */
>+  const nsTaskbarProgressState kError         = 3;
>+
>+  /**
>+   * Display a determinate progress bar indicating that the operation has
>+   * paused.
>+   */
>+  const nsTaskbarProgressState kPaused        = 4;

The style guide dictates NO_PROGRESS, INDETERMINATE, etc.
A STATE_ prefix would probably make sense too.

Comment 96

[Jim Mathies [:jimm]]

Sid, any chance for an updated Part 1 based on rob's latest?

Comment 97

[Rain [:rain1] (inactive)]

Attachment: part 1, now combined with TaskbarWindowPreview

Comment 98

[Rain [:rain1] (inactive)]

Attachment: Part 2, v5.0

Incorporating fixes to Dao's nits. Thanks!

Comment 99

[Rob Arnold [:robarnold]]

Attachment: Part 1, revised

Fixed several race conditions, unbitrotted patch and added some comments.

Comment 100

[Jim Mathies [:jimm]]

Comment on attachment 404765 [details] [diff] [review]
Part 1, revised

no tests!

Comment 101

[Rob Arnold [:robarnold]]

Attachment: backend tests and minor bug fix

Here are some backend tests and a fix for a bug discovered while writing them.

Comment 102

[Rob Arnold [:robarnold]]

Comment on attachment 404734 [details] [diff] [review]
Part 2, v5.0

This patch is out of date. In particular:
There is a new framework for windows 7 features in browser.js that we should use (added in bug 474056).
An observer is no longer needed for the taskbar interface.

Also this patch needs tests for the frontend behavior before it can be checked in.

Comment 103

[Rain [:rain1] (inactive)]

Attachment: part 1, backend and tests combined

Comment 104

[Rain [:rain1] (inactive)]

Attachment: [checked-in] Part 1

Comment 105

[Rain [:rain1] (inactive)]

Comment on attachment 404782 [details] [diff] [review]
[checked-in] Part 1

r+ for tests, conditional on the fact that the tests (both this one and taskbar preview) need to be fixed for older SDKs.

Comment 106

[Johnathan Nightingale [:johnath]]

This is a P1 blocker and today's code freeze - can we get a status update? Looks like the whiteboard might be out of date, too?

Comment 107

[Rain [:rain1] (inactive)]

Attachment: part 2, v6.0

Comment 108

[Jim Mathies [:jimm]]

Part 1 (widget) - http://hg.mozilla.org/mozilla-central/rev/77d0e4a45bcd

Comment 109

[Rob Arnold [:robarnold]]

Comment on attachment 404875 [details] [diff] [review]
part 2, v6.0

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -136,22 +136,29 @@ XPCOMUtils.defineLazyGetter(this, "Win7F
>   const WINTASKBAR_CONTRACTID = "@mozilla.org/windows-taskbar;1";
>   let taskbar = Cc[WINTASKBAR_CONTRACTID].getService(Ci.nsIWinTaskbar);
>   if (taskbar.available) {
>     Cu.import("resource://gre/modules/wintaskbar/preview-per-tab.jsm");
>     return {
>       onOpenWindow: function () {
>         AeroPeek.onOpenWindow(window);
>       },
>+      onDownloadManagerInit: function () {
>+        let tempScope = {};
>+        Cu.import("resource://gre/modules/wintaskbar/DownloadTaskbarProgress.jsm",
>+                  tempScope);
>+        tempScope.DownloadTaskbarProgress.onBrowserWindowLoad(window);
>+      },

Why not let tempScop = this; ?

>+#ifdef XP_WIN
>+#ifndef WINCE
>+  let tempScope = {};
>+  Cu.import("resource://gre/modules/wintaskbar/DownloadTaskbarProgress.jsm",
>+            tempScope);
>+  tempScope.DownloadTaskbarProgress.onDownloadWindowLoad(window);
>+#endif
>+#endif
> }

Actually I'm not sure why you use a temp scope.

A browser peer should review this as well.

Comment 110

[Rain [:rain1] (inactive)]

> Actually I'm not sure why you use a temp scope.

See comment 91.

Comment 111

[Jim Mathies [:jimm]]

Attachment: [checked-in 1.9.2] Part 1

Part 1, 1.9.2, pushed - http://hg.mozilla.org/releases/mozilla-1.9.2/rev/611acfead9ec

Comment 112

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 404875 [details] [diff] [review]
part 2, v6.0

Couldn't this be implemented as a service with a DOMWindowOpened listener that checked window URIs rather than hooking into the various window onloads?

>diff --git a/toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm b/toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm

>+  _init: function DTPU_init()

>+    this._updateStatus();

Why is this needed?

>+  _updateStatus: function DTPU_updateStatus()

Seems like most of this code should actually live in the download manager, which would probably also make it easier to keep a running total to the places that need it without having them loop through all downloads every time they want a status update. The download status bar in browser windows seems like another potential consumer. Followup bug? 

>+  onProgressChange: function DTPU_onProgressChange()
>+  {
>+    this._updateStatus();
>+    this._updateTaskbar();

>+  onDownloadStateChange: function DTPU_onDownloadStateChange()
>+  {
>+    this._updateStatus();
>+    this._updateTaskbar();

Can't you avoid calling updateStatus if updateTaskbar won't actually display it? Seems like updateStatus() should only be called from _updateTaskBar() when it calls setProgressState?

Comment 113

[Jim Mathies [:jimm]]

Comment on attachment 404782 [details] [diff] [review]
[checked-in] Part 1

didn't mean to obsolete this.

Comment 114

[Rain [:rain1] (inactive)]

(In reply to comment #112)
> (From update of attachment 404875 [details] [diff] [review])
> Couldn't this be implemented as a service with a DOMWindowOpened listener that
> checked window URIs rather than hooking into the various window onloads?

That's how I used to do it, but I changed it because of comment 33. Although the original reasoning for comment 33 is no longer valid, it still is quite convenient to use window onload, especially for browser windows (since the module needs to be imported after we know for sure that the download manager has been initialized).

> 
> >diff --git a/toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm b/toolkit/mozapps/downloads/src/DownloadTaskbarProgress.jsm
> 
> >+  _init: function DTPU_init()
> 
> >+    this._updateStatus();
> 
> Why is this needed?

To update the status, since _setActiveWindow doesn't call _updateStatus.

> 
> >+  _updateStatus: function DTPU_updateStatus()
> 
> Seems like most of this code should actually live in the download manager,
> which would probably also make it easier to keep a running total to the places
> that need it without having them loop through all downloads every time they
> want a status update. The download status bar in browser windows seems like
> another potential consumer. Followup bug?

There was already talk of doing this in comment 37 and comment 42. Maybe, yeah.

> 
> >+  onProgressChange: function DTPU_onProgressChange()
> >+  {
> >+    this._updateStatus();
> >+    this._updateTaskbar();
> 
> >+  onDownloadStateChange: function DTPU_onDownloadStateChange()
> >+  {
> >+    this._updateStatus();
> >+    this._updateTaskbar();
> 
> Can't you avoid calling updateStatus if updateTaskbar won't actually display
> it? Seems like updateStatus() should only be called from _updateTaskBar() when
> it calls setProgressState?

For the state where no download window is open, _updateStatus determines whether we're going to call setProgressState :)

Comment 115

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #114)
> module needs to be imported after we know for sure that the download manager
> has been initialized).

Ok, I guess this makes sense.

> > >+  _init: function DTPU_init()
> > 
> > >+    this._updateStatus();
> > 
> > Why is this needed?
> 
> To update the status, since _setActiveWindow doesn't call _updateStatus.

It seemed weird to do this before _setActiveWindow is called, but I suppose it doesn't matter too much, since _setActiveWindow is always called right after the module is imported anyways, and you don't want to updateStatus() each time it's called.

> There was already talk of doing this in comment 37 and comment 42. Maybe,
> yeah.

Sorry, I hadn't read through the entire bug. Seems worth doing in a followup, for sure!

Comment 116

[Mike Connor [:mconnor]]

Comment on attachment 404875 [details] [diff] [review]
part 2, v6.0

sdwilsh says his prior review carries over.

Comment 117

[Rob Arnold [:robarnold]]

Comment on attachment 404875 [details] [diff] [review]
part 2, v6.0

This should have tests. Shawn and I will discuss the details tomorrow.

Comment 118

[Joe Drew (not getting mail)]

This doesn't block the release.

Comment 119

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

To be clear, it doesn't block the release, but remains wanted. I think Rob's right, it's predicated on having appropriate tests before we can land it on 1.9.2, though.

If completed before Firefox 3.6 RC1, we should consider landing it.

Comment 120

[Johnathan Nightingale [:johnath]]

Setting flags to match comments.

Comment 121

[Dão Gottwald [:dao]]

Comment on attachment 404875 [details] [diff] [review]
part 2, v6.0

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -136,22 +136,29 @@ XPCOMUtils.defineLazyGetter(this, "Win7F
>   const WINTASKBAR_CONTRACTID = "@mozilla.org/windows-taskbar;1";
>   let taskbar = Cc[WINTASKBAR_CONTRACTID].getService(Ci.nsIWinTaskbar);
>   if (taskbar.available) {
>     Cu.import("resource://gre/modules/wintaskbar/preview-per-tab.jsm");
>     return {
>       onOpenWindow: function () {
>         AeroPeek.onOpenWindow(window);
>       },
>+      onDownloadManagerInit: function () {
>+        let tempScope = {};
>+        Cu.import("resource://gre/modules/wintaskbar/DownloadTaskbarProgress.jsm",
>+                  tempScope);
>+        tempScope.DownloadTaskbarProgress.onBrowserWindowLoad(window);
>+      },
>       onCloseWindow: function () {
>         AeroPeek.onCloseWindow(window);
>       }
>     };
>   } else {
>-    return { onOpenWindow: function () {}, onCloseWindow: function () {} };
>+    return { onOpenWindow: function () {}, onDownloadManagerInit: function () {},
>+             onCloseWindow: function () {} };
>   }
> });
> #endif
> #endif

>   // downloads will start right away, and getting the service again won't hurt.
>   setTimeout(function() {
>     gDownloadMgr = Cc["@mozilla.org/download-manager;1"].
>                    getService(Ci.nsIDownloadManager);
> 
>     // Initialize the downloads monitor panel listener
>     DownloadMonitorPanel.init();
>+
>+#ifdef WIN7_FEATURES
>+    Win7Features.onDownloadManagerInit();
>+#endif
>   }, 10000);

I'm trying to fix this pattern in bug 521188. When that's done, you should do something like this instead:

>      if (Win7Features) {
>        let tempScope = {};
>        Cu.import("resource://gre/modules/wintaskbar/DownloadTaskbarProgress.jsm",
>                  tempScope);
>        tempScope.DownloadTaskbarProgress.onBrowserWindowLoad(window);
>      }

Comment 122

[Rain [:rain1] (inactive)]

Attachment: part 2, v6.01

Unbitrotted to trunk.

Comment 123

[Rain [:rain1] (inactive)]

Attachment: part 2, v6.02

oops, forgot to fix the module paths

Comment 124

[Dão Gottwald [:dao]]

Comment on attachment 406445 [details] [diff] [review]
part 2, v6.02

>+      onDownloadManagerInit: function () {
>+        let tempScope = {};
>+        Cu.import("resource://gre/modules/DownloadTaskbarProgress.jsm",
>+                  tempScope);
>+        tempScope.DownloadTaskbarProgress.onBrowserWindowLoad(window);
>+      },

>+    if (Win7Features)
>+      Win7Features.onDownloadManagerInit();

Any reason why you're adding a onDownloadManagerInit method to Win7Features rather than executing that code right away?

Comment 125

[Rain [:rain1] (inactive)]

(In reply to comment #124)
> (From update of attachment 406445 [details] [diff] [review])
> >+      onDownloadManagerInit: function () {
> >+        let tempScope = {};
> >+        Cu.import("resource://gre/modules/DownloadTaskbarProgress.jsm",
> >+                  tempScope);
> >+        tempScope.DownloadTaskbarProgress.onBrowserWindowLoad(window);
> >+      },
> 
> >+    if (Win7Features)
> >+      Win7Features.onDownloadManagerInit();
> 
> Any reason why you're adding a onDownloadManagerInit method to Win7Features
> rather than executing that code right away?

I think Rob said to do so over IRC, in the interest of centralizing all the Win7 stuff in one place. I don't really have an opinion one way or the other.

Comment 126

[Dão Gottwald [:dao]]

I don't think this is beneficial. In fact, keeping the download manger init code together makes more sense to me.

Comment 128

[Shawn Wilsher :sdwilsh]

(In reply to comment #117)
> (From update of attachment 404875 [details] [diff] [review])
> This should have tests. Shawn and I will discuss the details tomorrow.
I don't recall the details of our conversation.  Rob, do you?

Comment 129

[Rob Arnold [:robarnold]]

(In reply to comment #128)
> (In reply to comment #117)
> > (From update of attachment 404875 [details] [diff] [review] [details])
> > This should have tests. Shawn and I will discuss the details tomorrow.
> I don't recall the details of our conversation.  Rob, do you?

I don't recall much. We definitely did not reach a consensus on where the code should live. I also believed that it was possible to write tests for the front end (I believe this is still possible) since the behavior of the progress bar is rather minimal and well defined (versus the tab previews). In particular, there's no user interaction with the progress so we just need to verify that we are setting the progress correctly.

Comment 130

[Darren Kalck [:D-Kalck]]

Is it supposed to be displayed when downloading a firefox update ?

Comment 131

[Rob Arnold [:robarnold]]

(In reply to comment #130)
> Is it supposed to be displayed when downloading a firefox update ?

Huh, I never thought about that. I think if you have the update download dialog open, it should. I don't think the background download should have progress unless it was user-initiated since that could confuse the user. Perhaps the extension manager could use this too updating addons. I'm not sure if the frontend is written in such a way as to facilitate that.

Comment 132

[Rain [:rain1] (inactive)]

(In reply to comment #131)
> I'm not sure if the
> frontend is written in such a way as to facilitate that.

I don't think it is -- I think that since each window has its own UI requirements, the right design is for each window to update its download progress separately. This will also mean that the right progress indicator is displayed for each window in case taskbar buttons aren't combined.

Comment 133

[Francis Gagné]

(In reply to comment #132)
> (In reply to comment #131)
> > I'm not sure if the
> > frontend is written in such a way as to facilitate that.
> 
> I don't think it is -- I think that since each window has its own UI
> requirements, the right design is for each window to update its download
> progress separately. This will also mean that the right progress indicator is
> displayed for each window in case taskbar buttons aren't combined.

I can see a problem with this solution: if a window is actually opened as a tab, what will happen? The main window will get the progress, overwriting any previous status? Nothing happens, because the "window" isn't a top-level window? Some extensions, like All-in-One Sidebar, even put the add-ons manager in the sidebar.

Comment 134

[Rain [:rain1] (inactive)]

I hadn't considered that. I guess the right solution to this would be to always make sure that such "special" tabs are displayed in the taskbar in the Aero Peek code, and for the taskbar progress code to be aware of this. The current backend code is definitely not smart enough.

Comment 135

[:Felipe Gomes (needinfo for replies!)]

In the current implementation, opening windows inside tabs wouldn't be much of a problem, because if a window already exists, the new window (inside the tab) being opened will first verify that and won't try to take over the download progress (except if it is the Download Manager, but its progress is always the same as the other windows).

The other features that display download progress (such as add-on installation or software update) should be able to set their progress on their windows too. We could handle this at the service by using a parameter of the type of download ("regular download", "update", etc), and use that to decide when to set the window progress or not.

The trickiest part AFAICT is not setting the regular download progress in all of the windows (like they are in all of the status bars), because this would look just ugly in the never-group-windows taskbar setting, but the current code already handles that.

Comment 136

[:Felipe Gomes (needinfo for replies!)]

Attachment: part 2, v6.1 + tests

Alright, this is my first set of tests! 
I don't know if they need to be browser-chrome, but I think so, because I use the window object here.

The tests here test the assignment of the active taskbar window. It does as follows: 
- first check if services are available
- then checks if the taskbar display was assigned to the current and only window
- opens the download manager and check if the taskbar display was passed to it
- closes the download manager and check if it was returned to the original window
- opens a second window and check if it didn't change since it isn't supposed to change when a new window is created (only closed)

The only thing that it doesn't check is to close the original window and see if it changes (because it isn't possible to do this in this test), but this situation is no different than closing the Download Manager. The case is that we just pick the first window that is enumerated by the windowatcher, which happens to always be the original one.

I'm working on some mores tests to check the state of the progress indicator after pausing/resuming downloads. I'll post them soon, but I wanted to post these already to get some feedback on the tests. In particular, the flow of the test was a bit tricky to do due to waiting for windows to open/close, so I'm not sure if I did the right thing here (went with a combination of observers and timeouts)

Comment 137

[Shawn Wilsher :sdwilsh]

(In reply to comment #136)
> The only thing that it doesn't check is to close the original window and see if
> it changes (because it isn't possible to do this in this test), but this
> situation is no different than closing the Download Manager. The case is that
> we just pick the first window that is enumerated by the windowatcher, which
> happens to always be the original one.
You could do that if you first open a new browser window then start your test with that window, right?

Comment 138

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #137)
> You could do that if you first open a new browser window then start your test
> with that window, right?

I could just because the download monitor only kicks in after 10sec, so it's possible to force the active taskbar to start in the second window before the original one takes it. This would go against the natural behavior though, so I didn't know if it should be done or not.

Are windows preserved between tests? If we don't start with a brand new one (younger than 10sec) the test would fail.

Comment 139

[Rimas Kudelis [use needinfo to ping]]

This is a very much unrelated question, but does a similar bug exists for OS X? I saw Photoshop showing progress bar in Dock when opening large files (I can even provide a screenshot). I think it would make sense to generalize this feature of Windows, OS X (and perhaps some other platforms in future) in Toolkit, no?

Comment 140

[Shawn Wilsher :sdwilsh]

(In reply to comment #138)
> Are windows preserved between tests? If we don't start with a brand new one
> (younger than 10sec) the test would fail.
The main browser window is, so you should open a new one anyway.

Comment 141

[Dave Vasilevsky]

(In reply to comment #139)
> This is a very much unrelated question, but does a similar bug exists for OS X?

Rimas, I have an extension that does something pretty similar: https://addons.mozilla.org/en-US/firefox/addon/10485

There's definitely an impedance mismatch between this patch (with one progress bar per window) and the way we'd want it to work on OS X, which has just one dock icon for multiple windows. I don't know how we'd want that resolved: Multiple progress bars in the dock icon? Just one coalesced progress bar?

Comment 142

[Rimas Kudelis [use needinfo to ping]]

Dan: in order not to spam this bug, I've spun the OS X issue off into Bug 548763.

Comment 143

[:Felipe Gomes (needinfo for replies!)]

Attachment: part 2, v6.2

More tests!  Now we also test the displayed state of the progress bar. The test adds two downloads to the Download Manager, pause each one, resume each one and let them finish, and in each of these steps it checks if the state displayed (nothing/downloading/paused) is correct.

Comment 144

[Dão Gottwald [:dao]]

Comment on attachment 429261 [details] [diff] [review]
part 2, v6.2

>+      onDownloadManagerInit: function () {
>+        let tempScope = {};
>+        Cu.import("resource://gre/modules/DownloadTaskbarProgress.jsm",
>+                  tempScope);
>+        tempScope.DownloadTaskbarProgress.onBrowserWindowLoad(window);
>+      },

...

>+    if (Win7Features)
>+      Win7Features.onDownloadManagerInit();

Don't add that method to Win7Features, just put the code there directly:

>if (Win7Features) {
>  let tempScope = {};
>  Cu.import("resource://gre/modules/DownloadTaskbarProgress.jsm",
            tempScope);
>  tempScope.DownloadTaskbarProgress.onBrowserWindowLoad(window);
>}

Comment 145

[:Felipe Gomes (needinfo for replies!)]

Attachment: part 2, v6.2.1

>Don't add that method to Win7Features, just put the code there directly.
Ok. I didn't know if it was decided on one way or the other, so I had left it as is. I've changed it now.

Also made some small clean-ups and used gavin's Services.jsm

Comment 146

[:Felipe Gomes (needinfo for replies!)]

An extra question about this feature: do we have any private browsing concerns about it?
As I understand it, private browsing is not about hiding on-screen what's going on the browser, so it doesn't apply here; but it's worth double checking.

Comment 147

[Shawn Wilsher :sdwilsh]

I don't think we have anything to worry about in this instance.

Comment 148

[(no longer active)]

(In reply to comment #146)
> An extra question about this feature: do we have any private browsing concerns
> about it?
> As I understand it, private browsing is not about hiding on-screen what's going
> on the browser, so it doesn't apply here; but it's worth double checking.

Nope, rest assured that there are no PB concerns here.

Comment 149

[Shawn Wilsher :sdwilsh]

Comment on attachment 429431 [details] [diff] [review]
part 2, v6.2.1

For more detailed review context, please see http://reviews.visophyte.org/r/429431/.

on file: toolkit/mozapps/downloads/DownloadTaskbarProgress.jsm line 54
> var DownloadTaskbarProgress =

nit: const this please


on file: toolkit/mozapps/downloads/DownloadTaskbarProgress.jsm line 66
>     if (!DownloadTaskbarProgressUpdater)
>       return;

global-nit:  brace even one line ifs please (I feel terrible asking for this
so far along in the review process, but our style guide has changed for new
code and I'm trying to enforce it)


on file: toolkit/mozapps/downloads/DownloadTaskbarProgress.jsm line 83
>   /**
>    * Getters for internal DownloadTaskbarProgressUpdater values
>    */
> 
>   getActiveTaskbarProgress: function DTP_getActiveTaskbarProgress() {
>     if (!DownloadTaskbarProgressUpdater)
>       return null;
>     return DownloadTaskbarProgressUpdater._activeTaskbarProgress;
>   },
> 
>   getActiveWindowIsDownloadWindow: function DTP_getActiveWindowIsDownloadWindow() {
>     if (!DownloadTaskbarProgressUpdater)
>       return null;
>     return DownloadTaskbarProgressUpdater._activeWindowIsDownloadWindow;
>   },
> 
>   getTaskbarState: function DTP_getTaskbarState() {
>     if (!DownloadTaskbarProgressUpdater)
>       return null;
>     return DownloadTaskbarProgressUpdater._taskbarState;
>   },

nit: make these actual getters so we can just use it as a property access


on file: toolkit/mozapps/downloads/DownloadTaskbarProgress.jsm line 236
>       this._activeTaskbarProgress.setProgressState(
>         Ci.nsITaskbarProgress.STATE_NO_PROGRESS);

nit: trailing paren on new line with no indent please.


on file: toolkit/mozapps/downloads/tests/chrome/browser_taskbarprogress_service.js line 120
>   let DMWindow = wwatch.openWindow(parent,
>                                    DOWNLOAD_MANAGER_URL,
>                                    "Download:Manager",
>                                    "chrome,dialog=no,resizable", []);

This isn't how you want to open the UI - you want to use
nsIDownloadManagerUI::show


In general, I'd like to see a generator based test for async tests instead of callback one since it's easier to read and follow along.  See http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/whatwg/test_bug500328.html?force=1 for an example.

Comment 150

[:Felipe Gomes (needinfo for replies!)]

Attachment: part2, for check-in

Patch with comments addressed

(In reply to comment #149)
> > var DownloadTaskbarProgress =
> 
> nit: const this please
done

> >     if (!DownloadTaskbarProgressUpdater)
> >       return;
> 
> global-nit:  brace even one line ifs please (I feel terrible asking for this
> so far along in the review process, but our style guide has changed for new
> code and I'm trying to enforce it)

Okay, adjusted all instances on the patch. Just so I know, is this a general style rule for JS code in all modules?

> 
> nit: make these actual getters so we can just use it as a property access
> 
done
 
> on file: toolkit/mozapps/downloads/DownloadTaskbarProgress.jsm line 236
> >       this._activeTaskbarProgress.setProgressState(
> >         Ci.nsITaskbarProgress.STATE_NO_PROGRESS);
> 
> nit: trailing paren on new line with no indent please.
>
is this what you mean? 
this._activeTaskbarProgress.setProgressState(
  Ci.nsITaskbarProgress.STATE_NO_PROGRESS
);
if yes, done

> This isn't how you want to open the UI - you want to use
> nsIDownloadManagerUI::show
> 
Using it now.

> 
> In general, I'd like to see a generator based test for async tests instead of
> callback one since it's easier to read and follow along.  See
> http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/whatwg/test_bug500328.html?force=1
> for an example.

Did this on both tests!

Comment 151

[Shawn Wilsher :sdwilsh]

(In reply to comment #150)
> Okay, adjusted all instances on the patch. Just so I know, is this a general
> style rule for JS code in all modules?
For new code, yes.  Existing code it all depends on what the reviewer says (generally follow convention in the file, but this is one of those things were the reviewer may ask otherwise).

> is this what you mean? 
> this._activeTaskbarProgress.setProgressState(
>   Ci.nsITaskbarProgress.STATE_NO_PROGRESS
> );
> if yes, done
sure did mean that :)

Comment 152

[Marco Bonardo [:mak]]

(In reply to comment #151)
> (In reply to comment #150)
> > Okay, adjusted all instances on the patch. Just so I know, is this a general
> > style rule for JS code in all modules?
> For new code, yes.  Existing code it all depends on what the reviewer says
> (generally follow convention in the file, but this is one of those things were
> the reviewer may ask otherwise).

it depends on module yet, for example Places still allows no-braced ifs if all sides of it are one-line (i thought Storage was doing the same since its style code says that).
Btw platform rules say to always brace.

Comment 153

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a352d0413476

Comment 154

[:Felipe Gomes (needinfo for replies!)]

Attachment: Fix tests for non win7

Both tests were broken if not run on win7.  I think this should solve them.

Comment 155

[Dão Gottwald [:dao]]

Comment on attachment 433427 [details] [diff] [review]
Fix tests for non win7

>--- a/toolkit/mozapps/downloads/tests/chrome/test_taskbarprogress_downloadstates.xul
>+++ b/toolkit/mozapps/downloads/tests/chrome/test_taskbarprogress_downloadstates.xul
>@@ -190,17 +190,16 @@ function checkCorrectState() {
>     //No more downloads
>     ok(taskbarState == nsITP.STATE_NO_PROGRESS, "Correct finished downloads state");
>     testState.noDownloadsStateTested = true;
>   }
> 
> }
> 
> function test() {
>-  SimpleTest.waitForExplicitFinish();
>   testSetup();
> }
> 
> function testSetup()
> {
>   //Test setup
>   let dmui = getDMUI();
>   if (!dmui) {
>@@ -249,16 +248,17 @@ function testSetup()
> 
>   let testObs = {
>     observe: function(aSubject, aTopic, aData)
>     {
>       if (aTopic != DLMGR_UI_DONE) {
>         return;
>       }
>       os.removeObserver(testObs, DLMGR_UI_DONE);
>+      SimpleTest.waitForExplicitFinish();
>       continueTest();
>     }
>   };
> 
>   // Register with the observer service
>   os.addObserver(testObs, DLMGR_UI_DONE, false);
> 
>   // Show the Download Manager UI

This looks wrong. waitForExplicitFinish needs to be called before 'test' returns.

Comment 156

[:Felipe Gomes (needinfo for replies!)]

Attachment: Fix tests - 2nd try

Yeah, that's true... aaah, second try

Comment 157

[u88484]

All of my downloads crash Firefox when they complete.  I was using the 18th's nightly and then decided to try this feature out using an hourly build (cset a352d0413476) and now every time a download completes I crash.  New bug or should this be backed out?

Comment 158

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #157)
> All of my downloads crash Firefox when they complete.  I was using the 18th's
> nightly and then decided to try this feature out using an hourly build (cset
> a352d0413476) and now every time a download completes I crash.  New bug or
> should this be backed out?

We will back out so I can investigate it. Kurt, can you tell me which system are you using?

Comment 159

[Dão Gottwald [:dao]]

Backed out: http://hg.mozilla.org/mozilla-central/rev/c906699b14db

Comment 160

[u88484]

(In reply to comment #158)
> (In reply to comment #157)
> > All of my downloads crash Firefox when they complete.  I was using the 18th's
> > nightly and then decided to try this feature out using an hourly build (cset
> > a352d0413476) and now every time a download completes I crash.  New bug or
> > should this be backed out?
> 
> We will back out so I can investigate it. Kurt, can you tell me which system
> are you using?

Build ID	20100318124752
Branch	1.9.3
OS	Windows NT
OS Version	6.1.7600
CPU	x86
CPU Info	AuthenticAMD family 15 model 104 stepping 2

Comment 161

[ronin.achilles]

Why is this backed out? I just used the hourly build and downloaded a bunch of files across multiple sites. Works perfectly for me. No crashes here.

Comment 162

[ronin.achilles]

(In reply to comment #161)
> Why is this backed out? I just used the hourly build and downloaded a bunch of
> files across multiple sites. Works perfectly for me. No crashes here.

Spoke too soon. Crashes on exit!

Comment 163

[Marco Bonardo [:mak]]

Asking just in case, did you run browser chrome tests locally on a Win7 machine with yout patch? If possible i'd like to avoid bugs like bug 545228, since we don't have win7 boxes running b-c tests automatically they should be run manually to check for leaks (actually there should be no leak reported).
Since i see a b-c test in your push i suspect answer is positive, but the leak has been fixed after your last patch.

Comment 164

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

The bug for the 'leak' fix which was causing the crash-on-close has been backed out, and I've not seen any close crashes today so far. 

bug backed out: https://bugzilla.mozilla.org/show_bug.cgi?id=545228

Any chance this could re-land to exonerate it causing the crash-on-close ?

Comment 165

[Rob Arnold [:robarnold]]

(In reply to comment #164)
> The bug for the 'leak' fix which was causing the crash-on-close has been backed
> out, and I've not seen any close crashes today so far. 
> 
> bug backed out: https://bugzilla.mozilla.org/show_bug.cgi?id=545228
> 
> Any chance this could re-land to exonerate it causing the crash-on-close ?

I'm pretty sure this bug can help trigger the crash (it's a race condition related to the TaskbarPreview class I think).

Comment 166

[:Felipe Gomes (needinfo for replies!)]

Attachment: par 2, v6.3

Just posting an updated patch with the test fixes applied, and fixed a leak too. I ran the b-c tests as mak suggested (with the patch from bug 545228) and no leaks are reported now.

Comment 167

[:Felipe Gomes (needinfo for replies!)]

Attachment: part 2, for check-in

moved a browser-chrome test to mochitest-chrome

Comment 168

[ronin.achilles]

Are we waiting for some other Bug to close before landing this one?

Comment 169

[Kevin Brosnan]

Looks like the bug is waiting on Felipe to do more work on patch part 2 or to submit the patch part 2 for review.

Comment 170

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 437117 [details] [diff] [review]
part 2, for check-in

The patch is finished and reviewed. The latest update just changed the test type, but I already cleared that with shawn on IRC. Carrying forward r+ to make this more clear.

I didn't mark checkin-needed last week because bug 557557 appeared and I wanted to avoid having this patch related to that. It's fine to land now.

Comment 171

[Marco Bonardo [:mak]]

I'll import this patch to my repo, make a tryserver run and take care of pushing it today if possible. Let's try to drive it out.

Comment 172

[Marco Bonardo [:mak]]

Tryserver said GO, thus part 2 pushed:
http://hg.mozilla.org/mozilla-central/rev/b45e2fd7ce4a

Comment 173

[u88484]

Verifying fixed using Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100415 Minefield/3.7a5pre