Last Comment Bug 544356 - Exit when only the download manager window is open and there are no downloads
: Exit when only the download manager window is open and there are no downloads
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: Firefox 3.7a5
Assigned To: :Felipe Gomes (needinfo me!)
:
Mentors:
Depends on: 591747 652982
Blocks: SmTestFail cuts-control
  Show dependency treegraph
 
Reported: 2010-02-04 13:47 PST by Robert Strong [:rstrong] (use needinfo to contact me)
Modified: 2011-11-25 14:44 PST (History)
25 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.7-fixed
-
.12-fixed


Attachments
v1 - Close if last window and no downloads (8.18 KB, patch)
2010-03-25 17:20 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
v2 - Close if last window and no downloads (10.80 KB, patch)
2010-03-26 12:28 PDT, :Felipe Gomes (needinfo me!)
sdwilsh: review-
Details | Diff | Splinter Review
v3 - Close if last window and no downloads (9.09 KB, patch)
2010-04-01 21:53 PDT, :Felipe Gomes (needinfo me!)
sdwilsh: review+
Details | Diff | Splinter Review
For check-in (8.87 KB, patch)
2010-04-06 16:32 PDT, :Felipe Gomes (needinfo me!)
felipc: review+
dveditz: approval1.9.2.7+
Details | Diff | Splinter Review
Fix test (1.36 KB, patch)
2010-04-22 12:54 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Patch for 1.9.2 (9.21 KB, patch)
2010-05-14 18:38 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Use getDMUI (2.17 KB, patch)
2010-06-18 14:02 PDT, :Felipe Gomes (needinfo me!)
neil: review+
Details | Diff | Splinter Review
Use getDMUI - for m-c (2.23 KB, patch)
2010-06-21 15:39 PDT, :Felipe Gomes (needinfo me!)
felipc: review+
Details | Diff | Splinter Review
Pathc for 1.9.2 (3.03 KB, patch)
2010-06-21 15:41 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Patch for 1.9.2 (9.27 KB, patch)
2010-06-21 15:44 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Patch for 1.9.1 (10.58 KB, patch)
2010-07-22 22:24 PDT, :Felipe Gomes (needinfo me!)
gavin.sharp: review+
dveditz: approval1.9.1.12+
Details | Diff | Splinter Review
1.9.1 updated (10.60 KB, patch)
2010-08-11 10:14 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review

Description Robert Strong [:rstrong] (use needinfo to contact me) 2010-02-04 13:47:32 PST
Not sure what the best behavior is for this. We have seen many reports via the Kampyle survey for the installer that indicate that the user has closed Firefox via the upper right close button (e.g. X) and believe Firefox has closed but the Download Mgr is still open. Perhaps prompt the user so they are aware that they haven't exited Firefox?

Related bugs:
Bug 240696
Bug 510299
Bug 448846
Comment 1 Johnathan Nightingale [:johnath] 2010-02-04 14:36:44 PST
I know you don't like prompting any more than anyone else - but we also don't want to upset people who were doing this on purpose. The chief reason I can think of to close firefox but not the download manager *deliberately* is when there are downloads in progress. Would it be enough, in terms of reducing our zombie-ing (and confusion!) of users, to just implement behaviour that causes us to exit (unprompted) when the only window left is the dlmgr and there are no active downloads?

I'm sure this ground has been trod before, and I'll even believe that there are people who want the current behaviour - but I feel like it's unlikely they represent the dominant case, here.

Would that reduced version get us most of the win we want, here? Would it bring any significant pain down upon us and our userbase?
Comment 2 Robert Strong [:rstrong] (use needinfo to contact me) 2010-02-04 14:38:38 PST
from my point of view that sounds like a great tradeoff.
Comment 3 Marat Tanalin | tanalin.com 2010-02-09 16:21:35 PST
It makes sense to close download manager automatically (and by default) when closing Firefox. If there are downloads in progress then Firefox could just prompt user to confirm closing download manager. Such confirmation window may contain following message and two buttons:

There are downloads in progress. Are you sure you want to abort these downloads and close Download manager window?

— Close anyway (or OK)
— Do not close (or Cancel)
Comment 4 Alex Limi (:limi) — Firefox UX Team 2010-02-09 16:22:25 PST
(In reply to comment #1)
> Would it be enough, in terms of reducing our
> zombie-ing (and confusion!) of users, to just implement behaviour that causes
> us to exit (unprompted) when the only window left is the dlmgr and there are no
> active downloads?

+1 from the UX Team to this. :)
Comment 5 Christopher Robert Jaquez 2010-02-11 05:57:47 PST
(In reply to comment #3)
> 
> There are downloads in progress. Are you sure you want to abort these downloads
> and close Download manager window?
> 

Correct me if I'm wrong but couldn't we offer to pause the downloads for resume upon restart rather than aborting them?  Or both?

— Pause active downloads and close
- Abort active downloads and close
— Continue downloading and do not close
Comment 6 Johnathan Nightingale [:johnath] 2010-02-11 06:08:34 PST
(In reply to comment #5)
> (In reply to comment #3)
> > There are downloads in progress. Are you sure you want to abort these downloads
> > and close Download manager window?
> 
> Correct me if I'm wrong but couldn't we offer to pause the downloads for resume
> upon restart rather than aborting them?  Or both?
> 
> — Pause active downloads and close
> - Abort active downloads and close
> — Continue downloading and do not close

I think that if we do introduce a prompt, we don't need the three cases here. Users can always cancel downloads anyhow - I think a prompt that says "You have active downloads, are you sure you want to exit?" can just, when the user says "Yes", implicitly pause the downloads and attempt to resume them later. We couldn't guarantee that paused downloads would be resumable in any event, so I think we shouldn't set a false expectation, there.  I also recognize that we can concoct scenarios where that isn't what you want ("What if I'm closing halfway through an Ubuntu ISO download, and the next time I open my laptop, I'm on tethered $1/MB internet and the download resumes?") but overall I think the simple prompt & choice is more likely to help users.

I also wonder if we should split this into two bugs, Rob? One that just says "if dlmgr is the last window open and there are no active downloads, just quit" and a second one that deals with prompting. I ask, because the former of those is UI-neutral and *maybe* even appropriate for backporting so that our 3.6 users face less of this strife moving to .Next.
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2010-02-11 08:20:40 PST
(In reply to comment #6)
>...
> I also wonder if we should split this into two bugs, Rob? One that just says
> "if dlmgr is the last window open and there are no active downloads, just quit"
> and a second one that deals with prompting. I ask, because the former of those
> is UI-neutral and *maybe* even appropriate for backporting so that our 3.6
> users face less of this strife moving to .Next.
That sounds like a good idea
Comment 8 :Felipe Gomes (needinfo me!) 2010-03-25 17:20:47 PDT
Created attachment 435058 [details] [diff] [review]
v1 - Close if last window and no downloads

This patch implements the behavior of closing the download manager when the last browser window closes and there are no active downloads (except for Mac, where the current behavior is kept).

Some things to decide:
  - Active downloads are defined in the DM as a file being downloaded *or* paused. So the patch in the current form doesn't exit if there are paused downloads.

  - Do we want a pref to disable this? I used one but didn't add it to the list of prefs.

  - The patch doesn't force Firefox to exit, it simply closes the Download Manager window when the last browser window is closed, and then Firefox exits itself.  What do we want when there are other open windows (like Bookmarks)?  In the current form the DM is closed anyhow, and the other windows are kept.
Comment 9 Johnathan Nightingale [:johnath] 2010-03-26 07:42:51 PDT
(In reply to comment #8)
> Created an attachment (id=435058) [details]
> v1 - Close if last window and no downloads

Thanks for picking this up, Felipe!

>   - Active downloads are defined in the DM as a file being downloaded *or*
> paused. So the patch in the current form doesn't exit if there are paused
> downloads.
> 
>   - The patch doesn't force Firefox to exit, it simply closes the Download
> Manager window when the last browser window is closed, and then Firefox exits
> itself.  What do we want when there are other open windows (like Bookmarks)? 
> In the current form the DM is closed anyhow, and the other windows are kept.

The motivation behind this change is that the download manager can be opened without the user explicitly asking for it (that is, it is opened as a side effect of downloading something) and thus they might forget that it's there. I'm fine, especially as a first step, with leaving all other windows (prefs, page info, library, &c) alone since they all require some explicit user action to open. I'm likewise fine with leaving the dl mgr open if there are paused downloads - I think that's a sign that the user was actively interacting with the window instead of just having it opened on their behalf. In this bug, per the summary, we're dealing with the "unaware" user - so I'm fine with leaving things as-is at the first sign of "awareness." :) 


>   - Do we want a pref to disable this? I used one but didn't add it to the list
> of prefs.

Guarding the "close DM if there's nothing active" logic with a pref doesn't introduce significant divergent code paths that we'll have to maintain, just an extra if statement, and probably makes someone somewhere happy, so I'm personally okay with it, but I'll happily defer to your reviewer, here.
Comment 10 :Felipe Gomes (needinfo me!) 2010-03-26 12:28:10 PDT
Created attachment 435241 [details] [diff] [review]
v2 - Close if last window and no downloads

Updated the test because I forgot to cover one case on the previous one.
Comment 11 Shawn Wilsher :sdwilsh 2010-03-31 14:05:09 PDT
(In reply to comment #8)
>   - Active downloads are defined in the DM as a file being downloaded *or*
> paused. So the patch in the current form doesn't exit if there are paused
> downloads.
Note that any download that can be paused can be resumed at application startup.  You probably don't need to worry about that in this bug, however.

(In reply to comment #9)
> Guarding the "close DM if there's nothing active" logic with a pref doesn't
> introduce significant divergent code paths that we'll have to maintain, just an
> extra if statement, and probably makes someone somewhere happy, so I'm
> personally okay with it, but I'll happily defer to your reviewer, here.
Except we also need to maintain tests for it, so I'd prefer to not add a preference.


We should also make sure that session restore will restore the download manager window in this case (follow-up is fine).
Comment 12 Shawn Wilsher :sdwilsh 2010-03-31 14:16:01 PDT
Comment on attachment 435241 [details] [diff] [review]
v2 - Close if last window and no downloads

http://reviews.visophyte.org/r/435241/
on file: toolkit/mozapps/downloads/content/downloads.js line 50
> const PREF_BDM_CLOSEONLASTWINDOWIDLE = "browser.download.manager.closeOnLastWindowIdle";

per discussion in bug, I'd prefer this to be removed.


on file: toolkit/mozapps/downloads/tests/chrome/Makefile.in line 85
> _BROWSER_FILES = \
>   browser_close_on_last_window.js \
>   $(NULL)
> 

Really want to keep all download manager tests as chrome tests.  It means
downstream consumers of the download manager can also run its tests (like
SeaMonkey).  As a toolkit application, this is important.


on file: toolkit/mozapps/downloads/tests/chrome/browser_close_on_last_window.js line 1
> const Cc = Components.classes;
> const Ci = Components.interfaces;

Please add the proper license (creative commons public domain per
http://www.mozilla.org/MPL/license-policy.html).


on file: toolkit/mozapps/downloads/tests/chrome/browser_close_on_last_window.js line 54
>   // Wait for a moment and make sure the Download Manager didn't close,
>   // because there's still one browser window
>   setTimeout(continueTest, 100); yield;
>   ok(dmui.visible, "Download Manager wasn't closed");

this moment likely isn't log enough to catch it not closing.


on file: toolkit/mozapps/downloads/tests/chrome/browser_close_on_last_window.js line 140
>   Services.prefs.setBoolPref(DOWNLOAD_MANAGER_PREF, oldPrefValue);

for future reference, you'd want to call clearUserPref here.


on file: toolkit/mozapps/downloads/tests/chrome/browser_close_on_last_window.js line 190
> function addDownload() {
>   function createURI(aObj) {
>     var ios = Cc["@mozilla.org/network/io-service;1"].
>               getService(Ci.nsIIOService);
>     return (aObj instanceof Ci.nsIFile) ? ios.newFileURI(aObj) :
>                                           ios.newURI(aObj, null, null);
>   }
> 
>   const nsIWBP = Ci.nsIWebBrowserPersist;
>   var persist = Cc["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"]
>                 .createInstance(Ci.nsIWebBrowserPersist);
>   persist.persistFlags = nsIWBP.PERSIST_FLAGS_REPLACE_EXISTING_FILES |
>                          nsIWBP.PERSIST_FLAGS_BYPASS_CACHE |
>                          nsIWBP.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION;
>   var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
>                getService(Ci.nsIProperties);
>   var dmFile = dirSvc.get("TmpD", Ci.nsIFile);
>   dmFile.append("dm-test-file-closedm");
>   if (dmFile.exists())
>     throw "Download file already exists";
> 
>   var dl = dm.addDownload(Ci.nsIDownloadManager.DOWNLOAD_TYPE_DOWNLOAD,
>                           createURI("http://example.com/httpd.js"),
>                           createURI(dmFile), null, null,
>                           Math.round(Date.now() * 1000), null, persist);
> 
>   persist.progressListener = dl.QueryInterface(Ci.nsIWebProgressListener);
>   persist.saveURI(dl.source, null, null, null, null, dl.targetFile);
> 
>   return dl;
> }

All of this is in a helper file that other chrome tests use that you can also
use when you move this to a chrome test.
Comment 13 :Felipe Gomes (needinfo me!) 2010-04-01 21:53:22 PDT
Created attachment 436643 [details] [diff] [review]
v3 - Close if last window and no downloads

Patch with comments addressed

> >   // Wait for a moment and make sure the Download Manager didn't close,
> >   // because there's still one browser window
> >   setTimeout(continueTest, 100); yield;
> >   ok(dmui.visible, "Download Manager wasn't closed");
> 
> this moment likely isn't log enough to catch it not closing.
> 
I increased these timeouts to 500ms, and added an extra ok(dmui.visible) check towards the end of the test that should also catch if the DM was improperly closed.
Comment 14 Shawn Wilsher :sdwilsh 2010-04-01 22:43:42 PDT
(In reply to comment #13)
> I increased these timeouts to 500ms, and added an extra ok(dmui.visible) check
> towards the end of the test that should also catch if the DM was improperly
> closed.
Probably need to fix bug 437063 too so we don't introduce more orange.
Comment 15 :Felipe Gomes (needinfo me!) 2010-04-01 23:15:56 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > I increased these timeouts to 500ms, and added an extra ok(dmui.visible) check
> > towards the end of the test that should also catch if the DM was improperly
> > closed.
> Probably need to fix bug 437063 too so we don't introduce more orange.

Actually, if dmui.visible change its current meaning it'd be better to then check if the window is open.

Note that there's no risk of a random orange here. These wait periods are to make sure that nothing happened (as expected), and the timeout should just be enough to catch a real failure (window closing when it shouldn't).
Comment 16 Shawn Wilsher :sdwilsh 2010-04-06 15:32:36 PDT
Comment on attachment 436643 [details] [diff] [review]
v3 - Close if last window and no downloads

For more context on comments, please see http://reviews.visophyte.org/r/436643/.

on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul line 35
> function test() {

nit: opening brace on newline please


on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul line 46
> function doTest() {

nit: opening brace on newline please


on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul line 47
>   let browserWindow1 = openBrowserWindow(continueTest);
>   let browserWindow2 = openBrowserWindow(continueTest);
>   let DMWindow = openDownloadManager(continueTest);
>   yield; yield; yield;

I think a comment explaining why we yield three times here would be useful. 
That, or maybe just doing a yield after each call to open.  In fact, I think
doing that would be best.


on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul line 99
> function continueTest() {

nit: opening brace on newline


on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul line 105
> function testSetup() {

nit: opening brace on newline


on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul line 148
> function openBrowserWindow(callback) {

nit: opening brace on newline


on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul line 163
> function openDownloadManager(callback) {

nit: opening brace on newline


on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul line 178
>   return Services.ww.openWindow(parent,
>                                 DOWNLOAD_MANAGER_URL,
>                                 "Download:Manager",
>                                 "chrome,dialog=no,resizable", []);

We should really be using nsIDownloadManagerUI::show here


on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul line 185
> function addCloseListener(aWins) {

nit: opening brace on newline


r=sdwilsh
Comment 17 :Felipe Gomes (needinfo me!) 2010-04-06 15:46:33 PDT
(In reply to comment #16)
> nit: opening brace on newline
right, will change these

> on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul
> line 47
> >   let browserWindow1 = openBrowserWindow(continueTest);
> >   let browserWindow2 = openBrowserWindow(continueTest);
> >   let DMWindow = openDownloadManager(continueTest);
> >   yield; yield; yield;
> 
> I think a comment explaining why we yield three times here would be useful. 
> That, or maybe just doing a yield after each call to open.  In fact, I think
> doing that would be best.

Ok. The 3 yields together is maybe a small win in loading time, but I think the change is better for clarity rather than a comment explaining, so I will go with the latter.

> on file: toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul
> line 178
> >   return Services.ww.openWindow(parent,
> >                                 DOWNLOAD_MANAGER_URL,
> >                                 "Download:Manager",
> >                                 "chrome,dialog=no,resizable", []);
> 
> We should really be using nsIDownloadManagerUI::show here
> 
I didn't use nsIDownloadManagerUI::show here because it doesn't return the window created, so I'd have to follow with a getMostRecentWindow after the yield when the window is sure to be loaded. Whichever method you prefer.
Comment 18 :Felipe Gomes (needinfo me!) 2010-04-06 16:32:06 PDT
Created attachment 437447 [details] [diff] [review]
For check-in

for check-in, carrying forward r+.

fixed nits, changed to dmui.show() and placed one yield after each open call.
Comment 19 Shawn Wilsher :sdwilsh 2010-04-06 17:30:09 PDT
(In reply to comment #17)
> I didn't use nsIDownloadManagerUI::show here because it doesn't return the
> window created, so I'd have to follow with a getMostRecentWindow after the
> yield when the window is sure to be loaded. Whichever method you prefer.
Would prefer nsIDownloadManagerUI::show.  People look at our tests for sample code, and we want them to always use nsIDownloadManagerUI::show.  If you want, we can add a getInterface method to nsDownloadManagerUI, and for nsIChromeWindow, return the window so you know you are getting the right one (and have the logic encapsulated).
Comment 20 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-04-22 06:09:32 PDT
http://hg.mozilla.org/mozilla-central/rev/a72996c0441f
Comment 21 Johnathan Nightingale [:johnath] 2010-04-22 06:17:05 PDT
(w00t!)
Comment 22 Robert Strong [:rstrong] (use needinfo to contact me) 2010-04-22 08:50:30 PDT
Thanks Felipe!
Comment 23 :Felipe Gomes (needinfo me!) 2010-04-22 12:54:35 PDT
Created attachment 440850 [details] [diff] [review]
Fix test

oops, obvious test error on Mac.  Since on Mac the DM won't be closed automatically, the test timed out waiting for it to be closed (although it later knew that on Mac the DM should still be there!).  This should fix it.
Comment 25 ken kovash 2010-05-13 14:18:38 PDT
I'd like to nominate this fix for branch (for Fx 3.6.5).  Based on past analysis*, this fix has the potential to positively impact a few million Firefox users.  In addition, we'd like to re-run our survey and analysis at a future date to estimate the actual impact... and in order to do so, we need users on non current versions of Firefox to have this fix.

* http://blog.mozilla.com/metrics/2010/02/09/an-improved-experience-for-new-users-of-firefox/
Comment 26 christian 2010-05-14 13:32:47 PDT
Comment on attachment 437447 [details] [diff] [review]
For check-in

a=LegNeato for 1.9.2.5
Comment 27 :Felipe Gomes (needinfo me!) 2010-05-14 18:38:08 PDT
Created attachment 445503 [details] [diff] [review]
Patch for 1.9.2

Patch for 1.9.2

Core code is exactly the same and applies cleanly.  On this patch I merged the test fix and updated the test to not use Services.jsm since it's trunk-only.
Comment 28 neil@parkwaycc.co.uk 2010-06-13 07:07:39 PDT
Comment on attachment 437447 [details] [diff] [review]
For check-in

>+      case "browser-lastwindow-close-granted":
>+#ifndef XP_MACOSX
[I didn't think Mac sent this notification anyway.]

>+  dmui = Cc["@mozilla.org/download-manager-ui;1"].
>+         getService(Ci.nsIDownloadManagerUI);
This needs to use getDMUI(). See bug 483781.
Comment 29 :Felipe Gomes (needinfo me!) 2010-06-18 14:02:26 PDT
Created attachment 452336 [details] [diff] [review]
Use getDMUI
Comment 30 neil@parkwaycc.co.uk 2010-06-18 16:24:35 PDT
Comment on attachment 452336 [details] [diff] [review]
Use getDMUI

>+  dmui = getDMUI();
>+  if (!dmui)
>+    return false;
Nit: could do with a
todo(false, "skip test for toolkit download manager UI");
Otherwise you get a warning from the test harness.
Comment 31 :Felipe Gomes (needinfo me!) 2010-06-21 15:39:43 PDT
Created attachment 452880 [details] [diff] [review]
Use getDMUI - for m-c

Updated patch reviewed by Neil with the todo(false,...).  Ready to land
Comment 32 :Felipe Gomes (needinfo me!) 2010-06-21 15:41:25 PDT
Created attachment 452882 [details] [diff] [review]
Pathc for 1.9.2

Patch for 1.9.2 with updated getDMUI. Same comment #27 applies: code is the same as m-c, tests differ by not using Services.jsm
Comment 33 :Felipe Gomes (needinfo me!) 2010-06-21 15:44:03 PDT
Created attachment 452883 [details] [diff] [review]
Patch for 1.9.2

forgot to `hg add` the test
Comment 34 Justin Wood (:Callek) 2010-06-21 22:07:31 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2d7cbaf44b2b
Comment 35 Justin Wood (:Callek) 2010-06-22 01:42:48 PDT
(In reply to comment #31)
> Created an attachment (id=452880) [details]

http://hg.mozilla.org/mozilla-central/rev/ce18b4287791
Comment 36 ken kovash 2010-07-13 16:21:06 PDT
Building on my comment #25, it would also be great to get this fix backported to Fx3.5 (i.e., I'd like to nominate it for the 3.5.x branch).  My apologies for not noting this in my earlier comment... I'm a rookie when it comes to nominating things for branch.

To see the size and scope of the impact of this fix and its ultimate impact on existing users trying to do a fresh download/install of Fx*, it'd make the Metrics team's job a lot easier if 3.5 users also have this fix.

* http://blog.mozilla.com/metrics/2010/02/09/an-improved-experience-for-new-users-of-firefox/
Comment 37 ai.bloodx 2010-07-22 16:47:19 PDT
How do I undo this fix? This has completely ruined everything for me. I like being able to keep the download manager open while closing the main firefox window to save resources and space. I also use the download manager to manage my "to watch list" of media and slowly remove media from the list as I watch. I cannot do this anymore because I have to leave the main firefox window open now which is annoying.
Comment 38 :Felipe Gomes (needinfo me!) 2010-07-22 18:04:32 PDT
We opted to not add an option to disable this feature, so there's no direct way to undo this. However, there's a little trick that you can do, perhaps you'll find it useful. The download manager only attempts to close itself when the last window closes, and it won't close if there are downloads in progress or paused. So you can just start downloading anything, pause it and close the main firefox window. You can cancel the download afterwards.
Comment 39 ai.bloodx 2010-07-22 18:46:48 PDT
This trick is a little tedious but a solution is a solution. Thanks for the help and the swift reply. Maybe in the future they could add in a feature to enable/disable, since I know there are quite a few others who would like that. Thanks again.
Comment 40 :Felipe Gomes (needinfo me!) 2010-07-22 22:24:42 PDT
Created attachment 459723 [details] [diff] [review]
Patch for 1.9.1

I already wrote the patch for 1.9.1 in case we want to take this.
The diffs for the test file and downloads.js are the same. I had to add the "browser-lastwindow-close-granted" event notification in browser.js as it was not present on 1.9.1.
I based it on the patch that added them on the tree but just brought the minimal code necessary to raise that notification only.
Comment 41 Daniel Veditz [:dveditz] 2010-07-23 10:27:29 PDT
Not blocking 1.9.1, but we can approve it.
Comment 42 Daniel Veditz [:dveditz] 2010-07-23 10:27:47 PDT
Comment on attachment 459723 [details] [diff] [review]
Patch for 1.9.1

Approved for 1.9.1.12, a=dveditz for release-drivers
Comment 43 :Felipe Gomes (needinfo me!) 2010-07-23 11:29:57 PDT
Comment on attachment 459723 [details] [diff] [review]
Patch for 1.9.1

Shawn, can you take a look at the new part in browser.js here? (this is for 1.9.1). It adds the browser-lastwindow-close-granted notification.
Comment 44 Shawn Wilsher :sdwilsh 2010-07-25 10:01:13 PDT
(In reply to comment #43)
> Shawn, can you take a look at the new part in browser.js here? (this is for
> 1.9.1). It adds the browser-lastwindow-close-granted notification.
You probably want someone like gavin or Mossop to look at that.  I'm not familiar enough with that code to feel comfortable reviewing it for 1.9.1.
Comment 45 :Felipe Gomes (needinfo me!) 2010-07-25 23:59:20 PDT
Comment on attachment 459723 [details] [diff] [review]
Patch for 1.9.1

Thanks Shawn. 

Mossop, could you take a look at the browser.js changes in here (this patch is for 1.9.1)?  (the other parts are already reviewed)

It adds the browser-lastwindow-close-granted notification that is present on 1.9.2-onwards and that is used by the other parts of this patch.
Comment 46 :Felipe Gomes (needinfo me!) 2010-07-26 00:04:04 PDT
Forgot to mention that the implementation here is basically the same as the one present on m-c's browser.js, but it was added on a much bigger patch that wasn't backported to 1.9.1.
Comment 47 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-07-26 01:07:19 PDT
Comment on attachment 459723 [details] [diff] [review]
Patch for 1.9.1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  if (reallyClose) {
>+    // Figure out if there's at least one other browser window around.
>+    let foundOtherBrowserWindow = false;
>+    let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
>+               .getService(Ci.nsIWindowMediator);
>+    let e = wm.getEnumerator("navigator:browser");
>+    while (e.hasMoreElements() && !foundOtherBrowserWindow) {
>+      let win = e.getNext();
>+      if (win != window && win.toolbar && win.toolbar.visible)

I don't think win.toolbar can reasonably be null, but I suppose it doesn't really hurt to check.

The trunk code that fires this notification doesn't do it if the current window is a popup. I think that's required to avoid notifying twice in the case where you have both a popup and a normal window, and then close the normal window followed by the popup. You can address that easily enough by adding (&& window.toolbar.visible) to the if (reallyClose) check...

... but that brings up the fact that is's kind of strange that we ignore popup windows when determining whether to fire this notification. AFAICT "browser-lastwindow-close-granted" does not actually mean "we're quitting" on non-Mac, since a popup window can keep us running. I think the sessionstore and browserglue users may be OK with that (though I'm not sure), but for this specific case (closing the download manager), don't we actually care about the last browser window, regardless of whether it's a popup? This problem applies equally to trunk/1.9.2, so I suppose it shouldn't necessarily block landing this on 1.9.1, but we should probably get a followup filed on it.

The test looks like it's using Services.*, which doesn't exist on 1.9.1, so it's going to need to be rewritten.

r=me on the browser.js portion, with these comments addressed.
Comment 48 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-07-26 01:08:09 PDT
(I also filed bug 581878 for a separate issue discovered while reviewing this.)
Comment 49 :Felipe Gomes (needinfo me!) 2010-07-26 11:38:20 PDT
(In reply to comment #47)
> Comment on attachment 459723 [details] [diff] [review]
> Patch for 1.9.1
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >+  if (reallyClose) {
> >+    // Figure out if there's at least one other browser window around.
> >+    let foundOtherBrowserWindow = false;
> >+    let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> >+               .getService(Ci.nsIWindowMediator);
> >+    let e = wm.getEnumerator("navigator:browser");
> >+    while (e.hasMoreElements() && !foundOtherBrowserWindow) {
> >+      let win = e.getNext();
> >+      if (win != window && win.toolbar && win.toolbar.visible)
> 
> I don't think win.toolbar can reasonably be null, but I suppose it doesn't
> really hurt to check.
> 
Right, I didn't know if that would be possible or not, so I didnt want this code to possibly throw an exception here. I'll keep it just in case.

> The trunk code that fires this notification doesn't do it if the current window
> is a popup. I think that's required to avoid notifying twice in the case where
> you have both a popup and a normal window, and then close the normal window
> followed by the popup. You can address that easily enough by adding (&&
> window.toolbar.visible) to the if (reallyClose) check...
> 
Ok, will add && window.toolbar.visible

> ... but that brings up the fact that is's kind of strange that we ignore popup
> windows when determining whether to fire this notification. AFAICT
> "browser-lastwindow-close-granted" does not actually mean "we're quitting" on
> non-Mac, since a popup window can keep us running. I think the sessionstore and
> browserglue users may be OK with that (though I'm not sure), but for this
> specific case (closing the download manager), don't we actually care about the
> last browser window, regardless of whether it's a popup? This problem applies
> equally to trunk/1.9.2, so I suppose it shouldn't necessarily block landing
> this on 1.9.1, but we should probably get a followup filed on it.
> 
That makes sense, we should care about the popup windows. I filed Bug 582021 for that. (This would require a change in approach of this patch, or a change in behavior of this notification, but I'll leave this discussion for that bug)

> The test looks like it's using Services.*, which doesn't exist on 1.9.1, so
> it's going to need to be rewritten.
> 

The tests are ok. When I backported to 1.9.2, instead of changing all Services.* usage on the test and poluting it with Cc[].Ci, I added this at the beginning of the test:

let Services = {
  wm: Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator),
  obs: Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService)
}
Comment 50 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-08-11 03:06:09 PDT
Felipe: this still needs landing on 1.9.1, right?
Comment 51 :Felipe Gomes (needinfo me!) 2010-08-11 10:14:05 PDT
Created attachment 464857 [details] [diff] [review]
1.9.1 updated

That's right, it does. Thanks Gavin, the "resolved fixed" confused me. Updated the patch with your comment:

Interdiff: 
-if (reallyClose) {
+if (reallyClose && window.toolbar.visible) {
Comment 52 Daniel Veditz [:dveditz] 2010-08-16 18:45:16 PDT
https://hg.mozilla.org/releases/mozilla-1.9.1/rev/4b76384e608e
Comment 53 neil@parkwaycc.co.uk 2010-08-29 16:02:00 PDT
http://hg.mozilla.org/mozilla-central/rev/58d7aaf82913 was a bustage fix that Callek landed to make the test bail out before messing with the windowtype (which breaks subsequent tests). Do we need any approvals to port the bustage fix to the 1.9.x branches?
Comment 54 BSaito 2010-08-31 19:18:07 PDT
I'm currently using Felipe's workaround but wanted to make sure you were all aware that here are others* besides ai.bloodx who are less than happy with new behavior.

* http://support.mozilla.com/en-US/questions/718101
Comment 55 makitk 2011-02-09 18:19:29 PST
Please be aware of Bug 590104
Comment 56 :Ehsan Akhgari 2011-04-17 10:00:28 PDT
Felipe, this test uses three 500ms timeouts (like this one: <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul#74>).  I'm trying to eliminate these types of timeouts in bug 649012.

I tried to figure out what you're really waiting on in that 500ms period, but it was not immediately obvious to me.  Could you please explain what is the reason that you're waiting 500ms like that?

Thanks!
Comment 57 :Felipe Gomes (needinfo me!) 2011-04-19 09:56:50 PDT
Ehsan, the timeouts exist here because they're testing that * nothing happened *: i.e. after the last browser window is closed and the last-window-closed listener had a chance to do its thing, we test that the Download window wasn't closed.

So the timeout isn't actually flaky, it will never cause a random orange. it could only miss to catch if this really breaks, so that's why it has a high value (500ms).

The alternative would be to listen to the close event, wait a few event cycles (2 executeSoon at least to guarantee that the order that the observers were registered won't matter) and then test that the download window wasn't closed. But this seemed too mirrored to the actual implementation to be useful ("testing that it works as implemented"), so that's the reason I chose the timeout instead.
Comment 58 :Ehsan Akhgari 2011-04-20 15:41:43 PDT
(In reply to comment #57)
> Ehsan, the timeouts exist here because they're testing that * nothing happened
> *: i.e. after the last browser window is closed and the last-window-closed
> listener had a chance to do its thing, we test that the Download window wasn't
> closed.
> 
> So the timeout isn't actually flaky, it will never cause a random orange. it
> could only miss to catch if this really breaks, so that's why it has a high
> value (500ms).
> 
> The alternative would be to listen to the close event, wait a few event cycles
> (2 executeSoon at least to guarantee that the order that the observers were
> registered won't matter) and then test that the download window wasn't closed.
> But this seemed too mirrored to the actual implementation to be useful
> ("testing that it works as implemented"), so that's the reason I chose the
> timeout instead.

OK, sold!

Are you willing to write a patch to add the appropriate requestFlakyTimeout annotation to this test?  If yes, let me know and I'll file the bug.  Thanks!
Comment 59 :Felipe Gomes (needinfo me!) 2011-04-22 08:46:55 PDT
(In reply to comment #58)
> OK, sold!
> 
> Are you willing to write a patch to add the appropriate requestFlakyTimeout
> annotation to this test?  If yes, let me know and I'll file the bug.  Thanks!

sure thing!
Comment 60 :Ehsan Akhgari 2011-04-26 15:17:47 PDT
(In reply to comment #59)
> (In reply to comment #58)
> > OK, sold!
> > 
> > Are you willing to write a patch to add the appropriate requestFlakyTimeout
> > annotation to this test?  If yes, let me know and I'll file the bug.  Thanks!
> 
> sure thing!

Thanks, filed bug 652982.  :-)
Comment 61 Sean Barrett 2011-11-25 14:44:15 PST
I understand that the powers that be chose not to offer a config option to bring back the old behavior, but would it be possible/reasonable for this to behave differently if the user explicitly opened the download manager window? I find it incredibly counter-intuitive to have a window up, open the download manager, close the window, and have the download manager go away. (I guess because I've developed a workflow that I've been using for five years that involves doing this.)

Note You need to log in before you can comment on or make changes to this bug.