397935 - Download Manager ("Downloads") window doesn't stay open when clicking on download-complete alert notification with auto-close pref ("close when done")

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P2)

Description

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

Summary: Download Manager ("Downloads") window doesn't stay open when clicking on alert-notification dialog with auto-close pref (my nomination for the most verbose summary!)

Steps to Reproduce:

1. Under Tools | Options | Main | Download, check "Close it when all downloads are finished"; click OK (close the window on Mac).
2. Start file download.
3. Wait for the download to complete.
4. Observe the "Downloads Complete" notification slider in the lower right of your screen.
5. Click on the "All files have finished downloading" link.

Expected Results:

The "Downloads" window should appear, be focused, and remain until minimized or closed.

Actual Results:

The "Downloads" window briefly appears, but just as quickly disappears.

Comment 1

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

Build ID: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007092804 Minefield/3.0a9pre

Also, this is an outcropping from a misunderstanding in bug 395708.

Comment 2

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

autoRemoveAndClose() is called from Startup(), and closes the DM if the pref is set and it hasn't detected "user action" and if the window's opener is nonexistent or has the same URI as the current window. I don't understand this code but it seems pretty likely that it's what's causing the DM window to close as soon as you open it.

Comment 3

[Peter van der Woude [:Peter6]]

in litmus for 2 cases:
1) with "show the DM when downloading files" checked
2) with "show the DM when downloading files" unchecked

Comment 4

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

(In reply to comment #3)
> in litmus for 2 cases:
> 1) with "show the DM when downloading files" checked

http://litmus.mozilla.org/show_test.cgi?id=4657

> 2) with "show the DM when downloading files" unchecked

http://litmus.mozilla.org/show_test.cgi?id=4710

Comment 5

[Mike Connor [:mconnor]]

Shouldn't be that hard to fix, but hardly a new bug afaict, so not blocking.

Comment 6

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

I can't reproduce this in 2.0, certainly seems like a new bug to me.

Comment 8

[Mike Connor [:mconnor]]

ok, interesting.

Still not a blocker, its definitely an odd edge case.

Comment 9

[Andreas]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112905 Minefield/3.0b2pre

I can reproduce this exactly as described in Comment #1 

Comment 11

[Jubal Kessler]

I hope this would be fixed before general release; it's the sort of thing every (Windows?) FF user will notice if they click on the popup alert, which by default always appears. I wouldn't call it an edge case.

Comment 12

[Shawn Wilsher :sdwilsh]

It's an edge case because the pref isn't set this way by default.  Most users will never hit this.

Comment 13

[Jubal Kessler]

browser.download.manager.showAlertOnComplete seems to be set to _true_ by default, as I didn't explicitly set it in a user.js, about:config doesn't present the name/value in boldface (indicating it'd been changed from default), and I don't see anything in the GUI prefs to toggle this.

I'm not harping -- I am trying to reconcile your (sdwilsh) statement with my conflicting observation. My browser version is:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

Comment 14

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

There are two prefs, and I believe Shawn was referring to browser.download.manager.closeWhenDone, set in http://lxr.mozilla.org/seamonkey/source/browser/app/profile/firefox.js#216 to |false|.

Comment 15

[Jubal Kessler]

Thanks for the clarification. For me, browser.download.manager.closeWhenDone is also the default value, so my original post (comment #11) still obtains...

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> Thanks for the clarification. For me, browser.download.manager.closeWhenDone is
> also the default value, so my original post (comment #11) still obtains...
Then you are experiencing a different bug - please file a new one with steps to reproduce because this bug is dealing with browser.download.manager.closeWhenDone set to true.

Comment 17

[Jubal Kessler]

(In reply to comment #16)

Looks like it's an error on my part, very sorry -- I had to retrace my steps with the prefs. I apparently see the same bug as stephend, with browser.download.manager.closeWhenDone indeed set to true.

Comment 20

[Tim Potter]

I would just like to comment that the links name being in the title might help with dup's,  I had trouble finding it amongst all the bugs.

"All files have finished downloading"  instead of download-complete alert notification or additional to it would possibly help?

Comment 21

[Shawn Wilsher :sdwilsh]

note:  this may get fixed by bug 413093

Comment 23

[Franco Cassar Manghi]

So I've been testing Firefox 3 beta 4 today (29/02/2008 - testday) and I confirm that this bug still exists on b4pre.

So the target milestone has to be changed as well as it (ff3b3) has long been passed without this bug being solved.

Comment 25

[Grey Hodge (jX)]

This may be an "edge case", but I think it's a pretty big edge, and it's definitely a regression from 2 and was one of the first things I noticed when jumping to B3. It's a dumb little bug that has a lot of visibility potential. "Open DL Manager link doesn't!" I say dumb little bug because it's not catastrophic, not because I'm saying anyone here is dumb nor little. Not that there's anything wrong with that.

Comment 26

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

I'm re-requesting blocking.  Although dup-count is low, we get a lot of feedback about this being broken every testday (and, in #qa on other occasions); I expect the same feedback come ship day, because these are the pref values we ship by default.

Comment 27

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

making comment early in the am--

http://mxr.mozilla.org/seamonkey/source/browser/app/profile/firefox.js#231: pref("browser.download.manager.closeWhenDone", false);

It's actually false by default, but I think folks will discover the pref, change it to true, and be bitten (but I might be wrong, not sure if we know what most users do).

Comment 28

[Niraj Bhatt]

Previous version of Firefox will open the actual file that is downloaded when clicked on the link that appears in the notification box. In Firefox 3 beta 4 when you click this link the download manager window pops up for a second and then disappear.

Not the end of the world but for the people who are used to clicking this link after every download so they don't have to open the download manager window will be so annoyed.

Comment 30

[Ed Lee :Mardak]

The problem is caused by the alertclickcallback notification that triggers a dmui->Show(nsnull, 0) which has a null window, so it looks kinda like the auto-open.

I tried making the alert box give its window as the aSubject, but because we use growl on os x, we can't quite pass in the window... I think?

Alternatively, we can specially detect that Show(nsnull, 0) is being called and know that it's not an auto-opened show because nsDownloadProxy will call Show with the id of the download.

Comment 31

[Shawn Wilsher :sdwilsh]

We should probably just special case this.  I can't think of a better solution currently :/

Edward - will you have time for this?  I can't see myself having time to code and write a test for this for at least three weeks...

Comment 33

[Srirang G Doddihal (Brahmana)]

I went through the control flow related to this bug and have documented it here: http://wiki.mozilla.org/User:Brahmana/Downloads_Complete_Alert_Control_Flow

As stated in the document there the variable gUserInteracted is not being used at all. I think we can use this variable to solve this bug as we know for sure that the user has clicked the alert link. We can set the gUserInteracted to true before the nsIDMUI->show() is called. As this is a JS variable we cannot obviously set it DM code where the show() is called. 

My solution here is to change the nsIDownloadManagerUI interface, in 2 ways:

1. Change the signature of the Show() function to accept a third boolean argument telling whether this Show() is because of an user action. So in the DM we would call it as: dmui->show(nsnull, 0, PR_TRUE);

This might break any consumers of this interface.

2. Add another wrapper for the above function to the interface, say something like: ShowPersistent() or ShowOnUserAction() which has the same signature as Show(). It will just set the gUserInteracted to true before calling Show() and after that set it back to false. So we will use this function for this alertclickcallback case and wherever else required.

Comment 34

[Shawn Wilsher :sdwilsh]

Best bet is to add an optional aReason argument - let's not add any methods.

Comment 35

[Ed Lee :Mardak]

Attachment: testcase v1

This test passes without setting the close-when-done pref to true. I.e., it fails per this bug when the pref is set to false.

Comment 36

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

Work in progress.

Good analysis Brahmana.  It's a bit late in the game to be adding methods I think, and using reasons in method's is pretty normal in our codebase.

I haven't had a chance to run Edward's test for this (I have class!) - but maybe I will later today.  Someone else should feel free to try it if they want.  I don't think it will pass just yet.

Still TODO:
write an xpcom unit test ensuring that null elements are appended to nsIMutableArrays (per request of bsmedberg).
make downloads.js use the reason
get a test to test already open and calling show again with user interacted reason.

Comment 37

[Shawn Wilsher :sdwilsh]

Comment on attachment 312044 [details] [diff] [review]
testcase v1

>+    ok(dmui.visible, "Download Manager stays open on alert click");
change that to a todo, and land
r=sdwilsh

Thanks for the test!

Comment 38

[Ed Lee :Mardak]

Attachment: testcase v1.1

Commented out the ok line with TODO comment.

Comment 39

[Ed Lee :Mardak]

Kinda testcase+.. make sure to uncomment the TODO and leave the ok ;)

Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.21; previous revision: 1.20
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_397935.js,v
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_397935.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_397935.js,v  <--  browser_bug_397935.js
initial revision: 1.1
done

Comment 40

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This passes Edward's test when locally applied.  Requesting sr on this since there is an API change.

I've found a number of bugs while looking into this as well:
1) We never actually used any of the arguments passed into us in downloads.js.  We now use one, but the others are not used...
2) gUserInteracted is not set to true anywhere - probably should do that if any command is executed.

I think both of those should be fixed in follow-ups though since they aren't terribly serious.

Patch coming soon with xpcom unit test...

Comment 41

[Ed Lee :Mardak]

Comment on attachment 312089 [details] [diff] [review]
v1.0

>+++ b/toolkit/components/downloads/public/nsIDownloadManagerUI.idl
>  /**
>   * Shows the Download Manager's UI to the user.
>   *
>-  * @param aWindowContext
>+  * @param [optional] aWindowContext
>   *        The parent window context to show the UI.
>-  * @param aID
>+  * @param [optional] aID
>   *        The id of the download to be preselected upon opening.
>+  * @param [optional] aReason
>+  *        The reason to show the download manager's UI.  This defaults to
>+  *        REASON_USER_INTERACTION, and should be one of the previously listed
>+  *        constants.
>   */
nit: REASON_USER_INTERACTED not INTERACTION

>+++ b/toolkit/components/downloads/src/nsDownloadManagerUI.js
>+  show: function show(aWindowContext, aID, aReason)
>   {
Because the arguments are optional, we should make sure we assign some useful defaults. aWindowContext to null, aID to 0?, aReason to USER_INTERACTED
>     // First we see if it is already visible
>-    if (this.recentWindow) {
>-      this.recentWindow.focus();
>+    let window = this.recentWindow;
>+    if (window) {
>+      window.focus();
>+      
>+      // If we are being asked to show again, with a user interaction reason,
>+      // set the appropriate variable.
>+      if (aReason == Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED)
>+        window.gUserInteracted = true;
Either check aReason != NEW_DOWNLOAD or rely on setting the correct default if we don't get called with aReason.

Comment 42

[Shawn Wilsher :sdwilsh]

Attachment: xpcom test v.10

Just testing the one thing for now - someone else can come in with more extensive tests later.

Comment 43

[Ed Lee :Mardak]

(In reply to comment #41)
> Because the arguments are optional, we should make sure we assign some useful
> defaults. aWindowContext to null, aID to 0?, aReason to USER_INTERACTED
Actually.. perhaps nevermind. xpcommagic will correctly give 0/null for these things? Unless someone else from JSland is calling nsIDownloadManagerUI.show?

Comment 44

[Shawn Wilsher :sdwilsh]

(In reply to comment #43)
> (In reply to comment #41)
> > Because the arguments are optional, we should make sure we assign some useful
> > defaults. aWindowContext to null, aID to 0?, aReason to USER_INTERACTED
> Actually.. perhaps nevermind. xpcommagic will correctly give 0/null for these
> things? Unless someone else from JSland is calling nsIDownloadManagerUI.show?
Yup - xpconnect magic FTW.  That's actually how optional arguments are supposed to work, and that's why also why REASON_USER_INTERACTED is set to 0.

(In reply to comment #41)
> nit: REASON_USER_INTERACTED not INTERACTION
whoops - fixed locally.  Will attach a new patch after sr.

Comment 45

[Shawn Wilsher :sdwilsh]

Comment on attachment 312085 [details] [diff] [review]
testcase v1.1

>+    // TODO bug 397935 ok(dmui.visible, "Download Manager stays open on alert click");
I meant the todo function call instead of ok ;)

Comment 46

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Incorporates test update, review comments.

Comment 47

[Mike Connor [:mconnor]]

Comment on attachment 312112 [details] [diff] [review]
v1.1

sr=mconnor, I would probably explicitly check aReason in the impl, rather than relying on xpconnect to clean up optional input.

Comment 48

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

for checkin

Comment 49

[Shawn Wilsher :sdwilsh]

Checking in xpcom/tests/unit/test_nsIMutableArray.js;
initial revision: 1.1

Comment 50

[Shawn Wilsher :sdwilsh]

Checking in toolkit/components/downloads/public/nsIDownloadManagerUI.idl;
new revision: 1.2; previous revision: 1.1
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.181; previous revision: 1.180
Checking in toolkit/components/downloads/src/nsDownloadManagerUI.js;
new revision: 1.8; previous revision: 1.7
Checking in toolkit/components/downloads/src/nsDownloadProxy.h;
new revision: 1.24; previous revision: 1.23
Checking in toolkit/mozapps/downloads/content/downloads.js;
new revision: 1.142; previous revision: 1.141
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_397935.js;
new revision: 1.2; previous revision: 1.1

Comment 51

[Shawn Wilsher :sdwilsh]

mfinkle - not sure if you want to post about this like you have for other potentially breaking changes (although this one probably won't break anyone!)

Comment 53

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

Verified FIXED using:

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

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

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

Comment 54

[Eric Shepherd [:sheppy]]

documentation updated

Comment 55

[Matthias]

can confirm the bug is still in version 3.0b5

Comment 56

[Carsten Book [:Tomcat]]

(In reply to comment #55)
> can confirm the bug is still in version 3.0b5
> 

Hi Matthias, the patch was checked-in after Beta 5 was released, so this should be fixed in the coming RC1 Release. 

Comment 59

[Liam Crozier]

Can you please put this bug on the "known issues" section on the firefox 3 download page please.

http://www.mozilla.com/en-US/firefox/3.0b5/releasenotes/#issues

Comment 60

[Michael Lefevre]

Liam: I doubt there will be any changes to the beta 5 page at this point. The first release candidate will be out soon, and this bug has been fixed since beta 5.

Comment 61

[Liam Crozier]

is there a set date for the first RC? even better would be a link to a page display when RC1, RC2, etc.. will be released. Thanks

Comment 62

[Shawn Wilsher :sdwilsh]

(In reply to comment #61)
> is there a set date for the first RC? even better would be a link to a page
> display when RC1, RC2, etc.. will be released. Thanks
They will be released when they are ready - we don't know exact dates as of yet.