544356 - Exit when only the download manager window is open and there are no downloads

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Robert Strong (they/them - no direct email)]

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]]

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 (they/them - no direct email)]

from my point of view that sounds like a great tradeoff.

Comment 3

[Marat Tanalin]

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]

(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

[Jaquez]

(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]]

(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 (they/them - no direct email)]

(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 for replies!)]

Attachment: 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]]

(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 for replies!)]

Attachment: 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]

(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]

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 for replies!)]

Attachment: 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]

(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 for replies!)]

(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]

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 for replies!)]

(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 for replies!)]

Attachment: 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]

(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 Apr 25 - May 5)]

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

Comment 21

[Johnathan Nightingale [:johnath]]

(w00t!)

Comment 22

[Robert Strong (they/them - no direct email)]

Thanks Felipe!

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Attachment: 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 24

[[e-mail]]

Comment on attachment 440850 [details] [diff] [review]
Fix test

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

Comment 25

[ken kovash]

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

[LegNeato]

Comment on attachment 437447 [details] [diff] [review]
For check-in

a=LegNeato for 1.9.2.5

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Attachment: 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

[[e-mail]]

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 for replies!)]

Attachment: Use getDMUI

Comment 30

[[e-mail]]

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 for replies!)]

Attachment: Use getDMUI - for m-c

Updated patch reviewed by Neil with the todo(false,...).  Ready to land

Comment 32

[:Felipe Gomes (needinfo for replies!)]

Attachment: 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 for replies!)]

Attachment: Patch for 1.9.2

forgot to `hg add` the test

Comment 34

[Justin Wood (:Callek)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2d7cbaf44b2b

Comment 35

[Justin Wood (:Callek)]

(In reply to comment #31)
> Created an attachment (id=452880) [details]

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

Comment 36

[ken kovash]

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]

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 for replies!)]

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]

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 for replies!)]

Attachment: 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]]

Not blocking 1.9.1, but we can approve it.

Comment 42

[Daniel Veditz [:dveditz]]

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 for replies!)]

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]

(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 for replies!)]

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 for replies!)]

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: [e-mail]]]

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: [e-mail]]]

(I also filed bug 581878 for a separate issue discovered while reviewing this.)

Comment 49

[:Felipe Gomes (needinfo for replies!)]

(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: [e-mail]]]

Felipe: this still needs landing on 1.9.1, right?

Comment 51

[:Felipe Gomes (needinfo for replies!)]

Attachment: 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]]

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/4b76384e608e

Comment 53

[[e-mail]]

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]

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]

Please be aware of Bug 590104

Comment 56

[(no longer active)]

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 for replies!)]

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

[(no longer active)]

(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 for replies!)]

(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

[(no longer active)]

(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]

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.)