631796 - Port |Bug 474060 - Show download progress in app icon in Windows 7 taskbar| and |Bug 524811 - add glowing Firefox icon in the taskbar when downloads are completed| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

Seeing the download progress in the Windows taskbar surely is a plus. I thought about adding it to other windows like MailNews, but I think it would be confusing there (the association there would probably be with messages rather than downloads) so I limited it to the browser and Download Manager. Also I introduced a new file for it because preprocessing is so frowned upon here.

If anyone wants to try this: There's chances you'll need the Win7 SDK (I have it installed, so I didn't check further).

Comment 1

[[e-mail]]

Comment on attachment 510013 [details] [diff] [review]
patch

>+  Components.utils.import("resource://gre/modules/DownloadTaskbarProgress.jsm",+                          tempScope);
Typo?

>+  tempScope.DownloadTaskbarProgress.onDownloadWindowLoad(window);
window is not defined in a .jsm!

Also, onDownloadWindowLoad is only suitable for a download window, as it keeps tracking the progress even in cases where another window wouldn't be interested.

If you close the download manager while a trackable download is in progress, the toolkit module will unfortunately pick any old window as its tracking window, which is inconvenient...

And I don't have Windows 7, let alone the SDK, so I can't actually test this.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

Sorry about the oversights. Yes there are two different methods for browser and download manager, and the FF implementation uses both (copy/paste mistake by me).

While I was at it I thought I should better turn this into a proper JSM (extension and contents). The idea is to not do anything if there is no support, but if you like we can throw an exception if the window type is not supported or something like that.

The behavior of the current implementation is as follows (TBE = taskbar entry):
a) No downloads in progress or paused: No effect
b) Paused download: Browser TBE shows nothing, DM TBE shows amber progress bar
c) Active download: Browser TBE shows green progress bar if DM is closed, DM TBE shows green progress bar (irrespective of whether a browser window is open).

Note: I didn't expect you to have Win7, I just want a code review. Requesting feedback from mcsmurf and/or Ratty (anyone else with Win7 who can test this, actually).

Comment 3

[[e-mail]]

(In reply to comment #2)
> c) Active download: Browser TBE shows green progress bar if DM is closed
Ah, but what if you open MailNews and then close all the other windows?

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> (In reply to comment #2)
> > c) Active download: Browser TBE shows green progress bar if DM is closed
> Ah, but what if you open MailNews and then close all the other windows?

Good point. Actually what happens is this (active download case only): If the Download Manager is opened and then another SM window remains (browser, MailNews, Address Book, Composer, ChatZilla, DOMI, you name it), the one that was first opened will show the download progress. Browser windows are only special (wrt. the current implementation) in that the download progress is also shown in one of them if the DM has not been opened during the current session. AFAIC that's good enough, but if you want I can try and extend support to other windows (e.g. MailNews, or add it to tasksOverlay.js and make it available to all that have the DM entry under Tools).

Comment 5

[Philip Chee]

Comment on attachment 510035 [details] [diff] [review]
patch v2

> While I was at it I thought I should better turn this into a proper JSM
> (extension and contents). The idea is to not do anything if there is no
> support, but if you like we can throw an exception if the window type is not
> supported or something like that.
> 
> The behavior of the current implementation is as follows (TBE = taskbar entry):
> a) No downloads in progress or paused: No effect
OK.

> b) Paused download: Browser TBE shows nothing, DM TBE shows amber progress bar
OK. (But more like a sickly yellow rather than amber)

> c) Active download: Browser TBE shows green progress bar if DM is closed,
OK.

> DM TBE shows green progress bar (irrespective of whether a browser window is
> open).
OK.

> +DlTaskbarIntegration.init = function(aWindow, aWindowType) {
> +#ifdef XP_WIN
> +#ifndef WINCE

Hmm. The makefile says.

> ifeq ($(OS_ARCH),WINNT)
> EXTRA_JS_MODULES += \
>   DownloadTaskbarProgress.jsm \
>   $(NULL)
> endif

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

I suggest something like:

var sysInfo = Components.classes["@mozilla.org/system-info;1"]
                        .getService(Components.interfaces.nsIPropertyBag2);
var osName = sysInfo.getProperty("name");
var version = sysInfo.getProperty("version");
var isWin7OrHigher = (parseFloat(version) >= 6.1);

const WINTASKBAR_CONTRACTID = "@mozilla.org/windows-taskbar;1";
Win7Features = (WINTASKBAR_CONTRACTID in Components.classes &&
                Components.classes[WINTASKBAR_CONTRACTID]
                          .getService(Components.interfaces.nsIWinTaskbar)
                          .available &&
                          osName == "Windows_NT" && isWin7OrHigher;
// Not sure which bits are redundant in the above...
....

DlTaskbarIntegration.init = function(aWindow, aWindowType) { 
  if (Win7Features)
  {
    ... blah blah blah...
  }

Or maybe:

  if (DlTaskbarIntegration.Win7Features)
    DlTaskbarIntegration.init(window, "foobar");


Another suggestion:

  DlTaskbarIntegration.init(window);

...
DlTaskbarIntegration.init = function(aWindow) {
  var type = aWindow.document.documentElement.getAttribute("type");
  switch (type) {
    case "navigator"browser":
      this.progress.onBrowserWindowLoad(aWindow);
      return;
    case "Download:Manager":
      this.progress.onDownloadWindowLoad(aWindow);
      return;
  }
}

How about the download progress dialog? (need to add a window type to this dialog).

Comment 6

[[e-mail]]

> const WINTASKBAR_CONTRACTID = "@mozilla.org/windows-taskbar;1";
> Win7Features = (WINTASKBAR_CONTRACTID in Components.classes &&
>                 Components.classes[WINTASKBAR_CONTRACTID]
>                           .getService(Components.interfaces.nsIWinTaskbar)
>                           .available &&
>                           osName == "Windows_NT" && isWin7OrHigher;
> // Not sure which bits are redundant in the above...
The in checks the os and the available checks the version.

>   DlTaskbarIntegration.init(window);
Sorry, I don't like this approach.

> How about the download progress dialog?
Presumably the progress dialog would only want to monitor its own download. (Won't Win7 group the icons anyway, or how many can we have before it starts?)

Comment 7

[[e-mail]]

If we didn't care about tracking download progress in the browser then we could just poke the progress value for download-type windows in onUpdateProgress / updateDownload / onProgressChange64 directly into a taskbarProgress instance.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #6)
> >   DlTaskbarIntegration.init(window);
> Sorry, I don't like this approach.

OK, so what should it be? I made it all that complicated in the first place only because you don't like preprocessing. Please give some advice, thanks.

> > How about the download progress dialog?
> Presumably the progress dialog would only want to monitor its own download.

Exactly. And since I have no intention to mess with the Toolkit JSM, I wanted to keep this bug limited. The single-download case could easily go to a follow-up AFAIC.

> (Won't Win7 group the icons anyway, or how many can we have before it starts?)

Don't know, that's about the first thing I disable when I install Windows.

(In reply to comment #7)
> If we didn't care about tracking download progress in the browser

FTR: I do care. The whole point is to have an indication of the overall download progress without the need to have a DM/download window open. If this part would be dropped from the implementation here, then I'd have to create an extension. In that case I could spare me a lot of the review troubles here, including availability checks, so please let me know where this is going rather sooner than later.

Comment 9

[Philip Chee]

I noticed this dependency (rather trivial): Bug 524811 add glowing Firefox icon in the taskbar when downloads are completed

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3

Thanks for the suggestions Philip!

Note: ifeq (what the Makefile does) does not work; I guess OS_ARCH is not defined in that context. But I think we can leave out the WINCE case for SM. ;-)

Comment 11

[Philip Chee]

Eeep!

> +XPCOMUtils.defineLazyGetter(DlTaskbarIntegration, "available", function() {
> +#ifdef XP_WIN

If you don't care about WINCE/WINMO you can avoid preprocessing.

Just use if (/^Win/.test(navigator.platform)

> +  const WINTASKBAR_CONTRACTID = "@mozilla.org/windows-taskbar;1";
> +  return WINTASKBAR_CONTRACTID in Components.classes &&
> +         Components.classes[WINTASKBAR_CONTRACTID]
> +                   .getService(Components.interfaces.nsIWinTaskbar)
> +                   .available;
> +#else
> +  return false;
> +#endif
> +});
> +
> +DlTaskbarIntegration.init = function(aWindow) {
> +#ifdef XP_WIN

if (this.available) {

> +  switch (aWindow.document.documentElement.getAttribute("windowtype")) {
> +    case "navigator:browser":
> +      this.progress.onBrowserWindowLoad(aWindow);
> +      return;
> +    case "Download:Manager":
> +      this.progress.onDownloadWindowLoad(aWindow);
> +      return;
> +  }
> +#endif
> +};

Comment 12

[[e-mail]]

(In reply to comment #8)
> (In reply to comment #6)
> > >   DlTaskbarIntegration.init(window);
> > Sorry, I don't like this approach.
> OK, so what should it be?
Wasn't I complaining about Ratty's code? I thought I preferred it when navigator and downloadmanager had separate entry points.

> Exactly. And since I have no intention to mess with the Toolkit JSM, I wanted
> to keep this bug limited.
Eww, I just thought, what happens if you have two downloads, one currently displayed in a progress window, what do you show in the browser?

> The single-download case could easily go to a follow-up AFAIC.
Sure, I was just throwing ideas around.

> > If we didn't care about tracking download progress in the browser
> FTR: I do care. The whole point is to have an indication of the overall
> download progress without the need to have a DM/download window open.
Thanks for clearing that up; I've not seen this feature in action, so I wasn't sure what problem it was trying to solve.

Comment 13

[[e-mail]]

Comment on attachment 510764 [details] [diff] [review]
patch v3

>+#ifdef XP_WIN
>+  const WINTASKBAR_CONTRACTID = "@mozilla.org/windows-taskbar;1";
>+  return WINTASKBAR_CONTRACTID in Components.classes &&
In theory you don't need to ifdef this because this component will only exist on Windows. (Similarly for the other ifdef.)

>+Components.utils.import("chrome://communicator/content/downloads/DlTaskbarIntegration.jsm");
[Looks weird to import a chrome URL]

>+      gDMUI.getAttention();
This flashes the download manager, which is probably the wrong thing to do in a progress window. If you want to flash the progress window, you could move the code out of nsSuiteDownloadManagerUI.js into downloadmanager.js and call it from the three places as necessary. (But don't call it getAttention because that already exists as a global function!)

Comment 14

[Philip Chee]

> In theory you don't need to ifdef this because this component will only exist
> on Windows. (Similarly for the other ifdef.)

IIRC I believe that you also need to check for os version >= 6.1 as Vista has a taskbar but doesn't support the progress thingys.

> >+      gDMUI.getAttention();
> This flashes the download manager, which is probably the wrong thing to do in a
> progress window. If you want to flash the progress window, you could move the
> code out of nsSuiteDownloadManagerUI.js into downloadmanager.js and call it
> from the three places as necessary. (But don't call it getAttention because
> that already exists as a global function!)

Err? Isn't he calling the global function here?

> Eww, I just thought, what happens if you have two downloads, one currently
> displayed in a progress window, what do you show in the browser?
I think we should show the slowest and only blink when the last download finishes.

Comment 15

[Philip Chee]

> The in checks the os and the available checks the version.
oops missed that.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v4

(In reply to comment #12)
> Wasn't I complaining about Ratty's code? I thought I preferred it when
> navigator and downloadmanager had separate entry points.

Hmm, that was the case with my approach, too, and I didn't really like passing in "random" strings, so I actually liked Philip's approach better. Maybe option three it is: Different functions.

> Eww, I just thought, what happens if you have two downloads, one currently
> displayed in a progress window, what do you show in the browser?

Whatever the Toolkit JSM returns (the average, I guess).

(In reply to comment #13)
> In theory you don't need to ifdef this because this component will only exist
> on Windows. (Similarly for the other ifdef.)

Well, the idea of using preprocessing was to not have to do any calculations at all on platforms for which we know upfront the result is FALSE. But with the JSM approach we're down to one check, once (the "in" one).

I tried the attached v4 on Linux (Kubuntu). No errors, and getAttention worked for KDE like for Windows (i.e. the Download Manager taskbar entry was flashed if it was in the background when the download finished).

> >+Components.utils.import("chrome://communicator/content/downloads/DlTaskbarIntegration.jsm");
> [Looks weird to import a chrome URL]

Can I do it differently (without too much overhead)? Maybe move it to suite/modules/, like Sanitizer.jsm?

> >+      gDMUI.getAttention();
> This flashes the download manager

OK, I have to admit I didn't check that. Thanks. Since I don't want to fiddle through the details and change the world I removed the single download window support, which can easily go to a (possibly post-2.1) follow-up if anyone cares.

(In reply to comment #14)
> > Eww, I just thought, what happens if you have two downloads, one currently
> > displayed in a progress window, what do you show in the browser?
> I think we should show the slowest and only blink when the last download
> finishes.

This doesn't sound easy to me. I'd like to restrict this bug to using that Toolkit provides, which is (AFAICT) a summary of all active/paused downloads, plus the simplest getAttention case (which is "some download finished"). Please file follow-ups for anything beyond that, or take over responsibility for this bug.

Comment 17

[Philip Chee]

OK Lets get something basic in first and polish up in follow-ups

Comment 18

[[e-mail]]

Comment on attachment 511231 [details] [diff] [review]
patch v4

Yeah, moving it to suite/modules would work. r=me with that change.

>+XPCOMUtils.defineLazyGetter(DlTaskbarIntegration, "progress", function () {
>+  let tempScope = {};
>+  Components.utils.import("resource://gre/modules/DownloadTaskbarProgress.jsm",
>+                          tempScope);
>+  return tempScope.DownloadTaskbarProgress;
>+});
>+
>+XPCOMUtils.defineLazyGetter(DlTaskbarIntegration, "available", function() {
>+  const WINTASKBAR_CONTRACTID = "@mozilla.org/windows-taskbar;1";
>+  return WINTASKBAR_CONTRACTID in Components.classes &&
>+         Components.classes[WINTASKBAR_CONTRACTID]
>+                   .getService(Components.interfaces.nsIWinTaskbar)
>+                   .available;
>+});
It seems odd to have a lazy getter for this when we always call it exactly once ;-) I don't know whether this will work:
> var DlTaskbarIntegration = {
>   onBrowserWindowLoad: function(aWindow) {},
>   onDownloadWindowLoad: function(aWindow) {}
> };
> 
> if (WINTASKBAR_CONTRACTID in Components.classes &&
>     Components.classes[WINTASKBAR_CONTRACTID]
>                .getService(Components.interfaces.nsIWinTaskbar)
>                .available) {
>   Components.utils.import("resource://gre/modules/DownloadTaskbarProgress.jsm",
>                           DlTaskbarIntegration);
> }

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #18)
> Comment on attachment 511231 [details] [diff] [review]
> patch v4
> 
> Yeah, moving it to suite/modules would work. r=me with that change.

Actually I changed my mind and would like to go with DownloadTaskbarIntegration instead (esp. since it'll now be in a completely different location, but also to be in line with similarly named files). OK?

> It seems odd to have a lazy getter for this when we always call it exactly
> once ;-)

Yes, now we do. I just thought it would be cleaner, since both are supposed to return always the same thing.

> I don't know whether this will work:
> > var DlTaskbarIntegration = {
> >   onBrowserWindowLoad: function(aWindow) {},
> >   onDownloadWindowLoad: function(aWindow) {}
> > };
> > 
> > if (WINTASKBAR_CONTRACTID in Components.classes &&
> >     Components.classes[WINTASKBAR_CONTRACTID]
> >                .getService(Components.interfaces.nsIWinTaskbar)
> >                .available) {
> >   Components.utils.import("resource://gre/modules/DownloadTaskbarProgress.jsm",
> >                           DlTaskbarIntegration);
> > }

That doesn't work because then DownloadTaskbarProgress is only available as a property of DlTaskbarIntegration. The now empty functions of DlTaskbarIntegration can be changed to call the respective function on this.DownloadTaskbarProgress of course but that call still won't succeed then; I guess DownloadTaskbarProgressUpdater is somehow not accessible anymore from inside DownloadTaskbarProgress functions (DownloadTaskbarProgressUpdater is not exported). Unfortunately I don't know how (or if) I can remedy that. Not having a working debugger surely doesn't help. So, do you want to continue investigation here?

Comment 20

[Jens Hatlak (:InvisibleSmiley)]

Attachment: alternate patch [Checkin: comment 23]

This is (mostly?) what Neil suggested. I'd like to have someone other than me confirm that it works. If that isn't you Philip, please hand over to mcsmurf.

Will obsolete the other patch if this one is accepted (and works).

Note: I'm under the impression that the old patch over at bug 614557 which I had applied kept the taskbar integration from working properly (Karsten's new one is much better!). Please test with an otherwise clean tree if you can.

Comment 21

[[e-mail]]

Comment on attachment 512948 [details] [diff] [review]
alternate patch [Checkin: comment 23]

Seems reasonable, assuming it works; I take it the only changes are related to the module.

> EXTRA_PP_JS_MODULES = \
>+  DownloadTaskbarIntegration.jsm \
This really belongs in its own EXTRA_JS_MODULES section.

Comment 22

[Frank Wein [:mcsmurf]]

Comment on attachment 512948 [details] [diff] [review]
alternate patch [Checkin: comment 23]

I have tested this patch with 7, works fine, download progress appears in the taskbar.

Comment 23

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 512948 [details] [diff] [review]
alternate patch [Checkin: comment 23]

http://hg.mozilla.org/comm-central/rev/90c77c2b4df8
with comment 21 addressed on checkin