Closed Bug 397935 Opened 14 years ago Closed 14 years ago

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

Categories

(Toolkit :: Downloads API, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: stephend, Assigned: sdwilsh)

References

Details

(Keywords: addon-compat, dev-doc-complete)

Attachments

(3 files, 4 obsolete files)

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.
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.
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.
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
in litmus for 2 cases:
1) with "show the DM when downloading files" checked
2) with "show the DM when downloading files" unchecked
Flags: in-litmus?
(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

Flags: in-litmus? → in-litmus+
OS: Windows Vista → All
Target Milestone: --- → Firefox 3 M11
Priority: -- → P5
Shouldn't be that hard to fix, but hardly a new bug afaict, so not blocking.
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Priority: P5 → P4
Target Milestone: Firefox 3 Mx → Firefox 3 M11
I can't reproduce this in 2.0, certainly seems like a new bug to me.
Flags: blocking-firefox3- → blocking-firefox3?
Summary: Download Manager ("Downloads") window doesn't stay open when clicking on alert-notification dialog with auto-close pref → Download Manager ("Downloads") window doesn't stay open when clicking on download-complete alert notification with auto-close pref ("close when done")
Duplicate of this bug: 405112
ok, interesting.

Still not a blocker, its definitely an odd edge case.
Flags: blocking-firefox3? → blocking-firefox3-
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 
Duplicate of this bug: 407905
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.
It's an edge case because the pref isn't set this way by default.  Most users will never hit this.
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
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|.

Thanks for the clarification. For me, browser.download.manager.closeWhenDone is also the default value, so my original post (comment #11) still obtains...
(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.
(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.
Duplicate of this bug: 419546
Duplicate of this bug: 420021
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?
note:  this may get fixed by bug 413093
Duplicate of this bug: 420169
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.
Duplicate of this bug: 420417
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.
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.
Flags: blocking-firefox3- → blocking-firefox3?
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).
Assignee: nobody → sdwilsh
Flags: blocking-firefox3? → blocking-firefox3+
Priority: P4 → P2
Whiteboard: [needs patch][swag 3hr]
Target Milestone: Firefox 3 beta3 → Firefox 3 beta5
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.
Duplicate of this bug: 423108
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.
Target Milestone: Firefox 3 beta5 → Firefox 3
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...
Duplicate of this bug: 358738
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.
Status: NEW → ASSIGNED
Best bet is to add an optional aReason argument - let's not add any methods.
Keywords: late-compat
Attached patch testcase v1 (obsolete) — Splinter Review
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.
Attached patch v0.1 (obsolete) — Splinter Review
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 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!
Attachment #312044 - Flags: review+
Attached patch testcase v1.1Splinter Review
Commented out the ok line with TODO comment.
Attachment #312044 - Attachment is obsolete: true
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
Flags: in-testsuite+
Hardware: PC → All
Attached patch v1.0 (obsolete) — Splinter Review
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...
Attachment #312049 - Attachment is obsolete: true
Attachment #312089 - Flags: superreview?(shaver)
Attachment #312089 - Flags: review?(edilee)
Attachment #312089 - Attachment is patch: true
Attachment #312089 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #312089 - Flags: review?(edilee) → review+
Attached patch xpcom test v.10Splinter Review
Just testing the one thing for now - someone else can come in with more extensive tests later.
Attachment #312093 - Flags: superreview?(benjamin)
Attachment #312093 - Flags: review?(benjamin)
(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?
(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 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 ;)
Whiteboard: [needs patch][swag 3hr] → [has patch][needs sr shaver]
Attached patch v1.1 (obsolete) — Splinter Review
Incorporates test update, review comments.
Attachment #312089 - Attachment is obsolete: true
Attachment #312112 - Flags: superreview?(shaver)
Attachment #312089 - Flags: superreview?(shaver)
Attachment #312093 - Flags: superreview?(benjamin)
Attachment #312093 - Flags: superreview+
Attachment #312093 - Flags: review?(benjamin)
Attachment #312093 - Flags: review+
Attachment #312112 - Flags: superreview?(shaver) → superreview?(mconnor)
Whiteboard: [has patch][needs sr shaver] → [has patch][needs sr 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.
Attachment #312112 - Flags: superreview?(mconnor) → superreview+
Attached patch v1.2Splinter Review
for checkin
Attachment #312112 - Attachment is obsolete: true
Whiteboard: [has patch][needs sr mconnor] → [has patch][has reviews][can land]
Checking in xpcom/tests/unit/test_nsIMutableArray.js;
initial revision: 1.1
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews][can land]
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!)
Duplicate of this bug: 426682
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
Status: RESOLVED → VERIFIED
documentation updated
can confirm the bug is still in version 3.0b5
(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. 
Duplicate of this bug: 431162
Duplicate of this bug: 431518
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
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.
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

(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.
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.