After removing the quit warning dialog, people are losing data since they've come to depend on that dialog. We should show the "closing multiple tabs" warning in the case of closing the last window. Hopefully this along with adding the "restore previous session" button on the home page will make the people happy while still giving the streamlining of "I'm intentionally quitting via file|exit or the cmd/ctrl-Q" case.

This is could be duped to bug 384907 but that's awfully noisy, and I'd rather just fix it then here a bunch of "finally" comments. Then we can dupe that here.

is the dialog also intended to show when firefox is set to "show my windows and tabs from last time"? this would seem a bit redundant.

(In reply to comment #2) > is the dialog also intended to show when firefox is set to "show my windows and > tabs from last time"? this would seem a bit redundant. No. The change as I understand it is just for the last window when your session would not be restored.

Some of the confusion arising from bug 592822 that I have seen from forum posts and bug reports is people think the UI option in the tabs pref pane to warn on closing multiple tabs is now no longer working. When it is explained that that pref (b.t.warnOnClose) is, and always was, only for secondary windows not the final window it doesn't make much sense to them. Would a relatively simple solution be for that UI pref pane checkbox to control both browser.warnOnQuit and browser.tabs.warnOnClose? It would still maintain separate prefs for those that choose to always restore a previous session (thus allowing a prompt when closing secondary windows with tabs but no prompt on quitting), but (un)checking a single UI option would give consistent behaviour for the default setting. Bug 628080 maybe related to this in as much as it talks about changing the default setting and possibly the behaviour of browser.tabs.warnOnClose. Just some thoughts.

If this is indeed a hardblocker, can we triage it out of the nom queue? Should it be BetaN or Final?

Please don't set hardblocker/softblocker on bugs that don't actually block.

(In reply to comment #5) > If this is indeed a hardblocker, can we triage it out of the nom queue? Should > it be BetaN or Final? I would say betaN. (In reply to comment #6) > Please don't set hardblocker/softblocker on bugs that don't actually block. I had hear that if we nom something we should also put our thoughts on hard vs soft. Marked with [hardblocker?] to denote that.

(In reply to comment #7) > (In reply to comment #5) > > If this is indeed a hardblocker, can we triage it out of the nom queue? Should > > it be BetaN or Final? > > I would say betaN. > > (In reply to comment #6) > > Please don't set hardblocker/softblocker on bugs that don't actually block. > > I had hear that if we nom something we should also put our thoughts on hard vs > soft. Marked with [hardblocker?] to denote that. Still shows up on https://bugzilla.mozilla.org/buglist.cgi?quicksearch=sw%3Ahardblocker But it's not just inconvenient. There was a nice little story on slashdot linking to such a bug list. Then again, it caught even more false positives, as just mentioning "hardblocker" in a comment would put a bug on that list... http://it.slashdot.org/story/11/01/19/1320229/Firefox-4-A-Huge-Pile-of-Bugs

This is essentially just what I had done in an earlier version of my patch in bug 592822. I think it's doing what we want here but I haven't actually tested it yet. Gavin - I know we wanted to avoid changing logic in here, but it seems pretty unavoidable.

Comment on attachment 508463 [details] [diff] [review] Patch v0.1 I think this looks mostly fine, except for the part that loses the "browser.warnOnQuit=false means we will never prompt in any case" characteristic, which I think is necessary to keep (with this patch as-is, we would start prompting in the restart case if warnOnQuit=false but warnOnRestart=true). I would refactor the logic such that showPrompt is defined as: var showPrompt = Services.prefs.getBoolPref("browser.warnOnQuit") && !(aQuitType == "restart" && !Services.prefs.getBoolPref("browser.warnOnRestart")); and have that after the aQuitType=="lastwindow" check.

(In reply to comment #10) > Comment on attachment 508463 [details] [diff] [review] > Patch v0.1 > > I think this looks mostly fine, except for the part that loses the > "browser.warnOnQuit=false means we will never prompt in any case" > characteristic, which I think is necessary to keep (with this patch as-is, we > would start prompting in the restart case if warnOnQuit=false but > warnOnRestart=true). I know we talked about this before, but I still *really* hate that warnOnQuit is supposed to override in that case. We have different dialogs. I would expect warnOnRestart to warn me before restarting regardless of the value of warnOnQuit. warnOnQuit is no longer a global override with this bug anyway. We're changing logic in there now anyway, so if we're ever going to fix it, now seems like the most appropriate time. Even though this bug is specifically about showing the close window dialog, I still think we should do it - in a new bug if we have to.

First off... I talked with Gavin on IRC. We're going to keep warnOnQuit as a global override and introduce a new pref for the quit dialog. This is going to mean switching warnOnQuit back to true and having the new pref be false. I'm going to do that work here even though it's not directly about the window closing dialog (I'll morph the bug a bit later). Secondly... Remember when we all thought browser-lastwindow-close-requested was notified when the last window was closing? Yea that was fun. That was my mistake. browser-lastwindow-close-requested is only notified when the last window is closed but not actually quitting (other windows open). When closing the last window quits, we just follow the normal quit-application-requested path. So what I did here is not going to work in the case we want. I have an idea though, but so far it requires modifying globalOverlay. Patch tomorrow.

The change to globalOverlay.js here sends "lastwindow" as the data with the quit-application-requested notification, which allows us to distinguish quitting via the X on the window vs file > exit/quit & cmd/ctrl+q We then use that in nsBrowserGlue to make sure that we use a quit type of "lastwindow" to determine which dialog to show (if any). I think the comments I added in there should make it pretty clear the process we take to determine which dialog to show.

Comment on attachment 509508 [details] [diff] [review] Patch v0.2 [Checked in: See comment 16] >diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js > _onQuitRequest: function BG__onQuitRequest(aCancelQuit, aQuitType) { >+ // If the session is going to be restored, then we will not show any dialogs >+ // regardless of other conditions. browser.warnOnQuit == false also overrides >+ // other possibilites of the prompt showing. Otherwise these are the conditions >+ // that will result in a dialog being shown: We should probably list the cases we explicitly don't show *any* dialog: - session will be restored (resume_session_once or browser.startup.page=3) - there is only one tab open - browser.warnOnQuit is false - we're in private browsing mode >+ // 1. aQuitType == "lastwindow" or "quit" and browser.showQuitWarning == true >+ // - The quit dialog will be shown >+ // 2. aQuitType == "restart" && browser.warnOnRestart == true >+ // - The restart dialog will be shown >+ // 3. aQuitType == "lastwindow" && browser.tabs.warnOnClose == true >+ // - The "closing multiple tabs" dialog will be shown This seems to be overloading "lastwindow" - it's now sent both for "last window is closing, but we're not quitting" and also "we're quitting (via closing last window)"... I guess that's not a problem, probably worth a comment? >+ var showPrompt = false; >+ var mostRecentBrowserWindow; > try { I think this try/catch is unneeded, can you just remove it? >+ if (aQuitType == "lastwindow") { >+ mostRecentBrowserWindow = Services.wm.getMostRecentWindow("navigator:browser"); >+ aCancelQuit.data = !mostRecentBrowserWindow.gBrowser.warnAboutClosingTabs(true); Do we want to do this in private browsing mode? I think not. Should just move the PB mode check to earlier in this function, I think. I think the logic would be clearer as: if (aQuitType != "restart" && showQuitWarning) showPrompt = true; else if (aQuitType == "restart" && warnOnRestart) showPrompt = true; else if (aQuitType == "lastwindow") { ... } r=me with these addressed. We should probably get a couple of followup bugs on file: - remove the lastwindow-close-requested stuff, since I don't think we need to support showing a dialog in that case, and it would simplify these code paths significantly. - remove support for showing the quit dialog entirely, and simplify the shutdown path even further.

That all makes sense. Thanks for the review. I know exactly how painful all of this dialog stuff is and I look forward to those followups.

Pushed http://hg.mozilla.org/mozilla-central/rev/84921e24be9c with the changes requested. I filed bug 632270 and bug 632271 for those followups.

I just lost all my tabs due to this change. I got the single-tab page on startup, i.e. the new so-call Home page, and clicked 'restore'. That worked fine, but the bug about opening the session in a 'new window' instead of the current one with only one tab rose up to bite me. When I closed the browser again I forgot, and also got no warning I was closing 'multiple' windows, thus when I closed the one with all my tabs, there remained the stupid 'home page' window. Closed that.. and BAM lost all my tabs.

With browser.showQuitWarning set to true and browser.warnOnQuit set to true I'm not getting any warning box... I'm closing the application like probably 95% of windows usere with the X in the upper right corner, if that matters.

(In reply to comment #19) > With browser.showQuitWarning set to true and > browser.warnOnQuit set to true > > I'm not getting any warning box... I'm closing the application like probably > 95% of windows usere with the X in the upper right corner, if that matters. Yes, it DOES matter - using File->Exit does produce the dialog box. IMO, the X should do the same thing.

the is bug in _onQuitRequest var quitDialogTitle = quitBundle.formatStringFromName(aQuitType + "DialogTitle", [appName], 1); chrome://browser/locale/quitDialog.properties don't have an entry for lastwindowDialogTitle so when we close last window with the X aQuitType = lastwindow and this function faile to show the dialog !

Do you have browser.showQuitWarning set to true?

please read http://blog.zpao.com/post/3174360617/about-that-quit-dialog before commenting.

(In reply to comment #21) > the is bug in _onQuitRequest > > var quitDialogTitle = quitBundle.formatStringFromName(aQuitType + > "DialogTitle", [appName], 1); > > chrome://browser/locale/quitDialog.properties don't have an entry for > lastwindowDialogTitle > > so when we close last window with the X aQuitType = lastwindow and this > function faile to show the dialog ! Well shit. Thanks for that. I had taken something out between the patch here and what I pushed because I thought that was covered. Patch in a minute.

Since I messed this up, here's the patch I checked in for posterity's sake.

And here's the part that should fix the problem.

On IRC, Gavin said he would prefer this, so bam.

Pushed the followup: http://hg.mozilla.org/mozilla-central/rev/39959be42c6f

(In reply to comment #29) > *** Bug 632797 has been marked as a duplicate of this bug. *** This is not the case, I reported the bug in the latest Beta Version. In the "real" release version, I am prompted for closure. I thought I was submitting the bug report against the latest beta "4" version. Anyone verify it has been fixed in the latest Beta Version?

(In reply to comment #30) > (In reply to comment #29) > > *** Bug 632797 has been marked as a duplicate of this bug. *** > > This is not the case, I reported the bug in the latest Beta Version. In the > "real" release version, I am prompted for closure. I thought I was submitting > the bug report against the latest beta "4" version. Anyone verify it has been > fixed in the latest Beta Version? No it wasn't fixed for Beta 11, but it will be for Beta 12. That bug is a duplicate to this since we fixed the bug here (even if you don't have the fix yet, the bug still counts as fixed)

Here is how I see a user's thought process going when they upgrade from 3.6 to 4.0. The user has default settings apart from their homepage (I believe this to be a very common case): 1. "I have lots of tabs, but I need to close the browser and log out now. That's OK, Firefox usually asks me if I want to save my tabs." 2. Click X. 3. See "You are about to close 6 tabs. Are you sure you want to continue? [Cancel/Close Tabs]" 4. "Erm, I don't know. I usually click 'Save and Quit'. I don't want to Cancel (title says 'Confirm close') because I need to close the browser and log out now, but I don't want to Close Tabs because I want them to be there when I open up Firefox again. 5A. Click Cancel and tediously save tabs manually by bookmarking them or something. Or, 5B. Click Close Tabs and hope for the best. "Maybe they automatically save and restore these days? I guess that could make sense." 6. Reopen Firefox. See homepage and no tabs. Assume Firefox has eaten your tabs. Swear profusely and wonder what was wrong with the "Quit/Save on Quit/Cancel" dialog. Please tell me if you think this is an unrealistic or unlikely scenario and explain why, because I think this will occur often. How is a user who is used to the 3.6 method of saving tabs going to know that they can click "Close Tabs" safe in the knowledge that they can get their tabs back? And how to get those tabs back?

Hello: All things considered, Firefox is still way better than IE. And I promote Firefox. What I have a hard time understanding is why "they" can not release a sub-letter of the beta version and fix* the problem "temporarily." *copy the code from production version and "slip it in." People aren't asking for a change in how this function works, just put it back in. I know as reported here - it will be "restored" in the next beta version, however I am beseeched by another problem, I don't know how I will be notified when the next beta will be out, and this beta does not offer me an opportunity to check for updates in the help menu. Currently I work in a position where people do things and fix them later. I am sorry to report, we "Americans" are losing the ability to think and do things right the first time. The just do it mentality is good, but just do it right.

We are not going to put out a beta 11.5 or anything of the sort. The fix will be in beta 12 when that goes out, hopefully in the next week. They are called betas because they are unfinished. If you're unhappy waiting, use a nightly version. Check for updates was moved into the about dialog. I think the betas get notified of newer betas when they are released.

Well - Surprise, surprise.... I accidently discovered that if two firefox windows are opened at the SAME time; firefox prompts you to save open tabs, or quit - - - YOU must have two windows opened. Thought I just provide this little tidbit; just in case the next beta works in reverse; multiple windows opened and Firefox just closes the window without a prompt.

(In reply to comment #39) > I accidently discovered that if two firefox windows are opened at the SAME > time; firefox prompts you to save open tabs, or quit - - - YOU must have two > windows opened. That shouldn't be the case. By default we shouldn't be showing any dialogs when "quitting" (closing the last window is slightly different). Care to expand upon your situation (version, OS, value of some prefs - browser.warnOnQuit, browser.showQuitWarning, browser.tabs.warnOnClose)

Windows 7 64bit Home Premium Version is being used. I have not accessed the "settings window/tab", but have selected the option warn if closing a window with multiple tabs. I am not at the window 7 computer right now. Will post values when I get to that computer (by the way, that computer is the only one running the latest beta version.

Windows 7 64bit Home Premium Version is being used. I have not accessed the "settings window/tab", but have selected the option warn if closing a window with multiple tabs. I am not at the window 7 computer right now. Will post values when I get to that computer (by the way, that computer is the only one running the latest beta version.

This is just farcical. You're basically saying "People are crying out for the quit dialog back so I'll give them the close multiple tabs one instead"! You're deliberately confusing users and deliberately not telling them that their session is safe and how to get it back. The number of users not on about:home could be HUGELY greater than the initial 13% estimate of 52m - see here: https://groups.google.com/group/mozilla.dev.apps.firefox/msg/b10164f573606818?hl=en As if that wasn't enough - and it sure as hell is - you're also planning to move to completely removing the ability to prompt on exit, even by tweaking the settings in about:config: Bug 632271 Words fail me. Twitter prompts the user before retweeting, and you refuse to do it when closing 50 tabs!

The 'You are about to close X tabs' confirmation now comes up when closing the last window in Fx4.0b12, but not if selecting File > Exit. I understand that this is by design. Well, the item in Options > Tabs does not say "Warn me when closing multiple tabs some of the time". What if they were trying for File > Work Offline and mis-clicked? What if they forgot they had something important open in a tab in another window? Yes, it's two clicks instead of one, but this doesn't make it foolproof, and the user has ASKED to be warned when closing multiple tabs (a warning that they can easily turn off right there on the dialog if they're going for streamlined).

Using beta 12 on Linux and I have been accidentally doing CTRL+Q when I meant to CTRL+Tab. The browser closes and I lose my tabs. It's not fun.

Pressing Ctrl+Q, or in my case the Apple key and Q, instead of Ctrl+Tab is something that happens quit frequently. I strongly urge the developers to quit arguing about details here and think about it from the perspective of the user! There needs to be a warning to prevent unintended termination of the application. I don't see how hard it is to understand that. I can also confirm that on Firefox 4 on Mac OS X pressing Apple key and Q quits Firefox without any warning when having several tabs open.

I wish someone would explain to me why this interface decision was made. I have lost a lot of time and state to this silly behavior. If I want the application to warn me before quitting with multiple tabs, I don't see why I should have to go fiddlefaffing around in the about:config entries or change my home page to make that happen. Really, what is the point of having a preferences checkbox that says "warn before closing multiple tabs" if you aren't going to respect the user's selection?

I just opened bug #1138054 before finding this one. Can someone confirm that Ctrl-Q is supposed to actually close all tabs without any warning? Is that the correct behaviour? It seems odd that there is a confirmation for the user in case they accidentally press Alt-F4 to close the window, but if they accidentally press Ctrl-Q (which is far more common, due to Ctrl-W being used to close tabs etc.) then there is no confirmation. Perhaps it would be easiest to just get rid of the Ctrl-Q default - Chrome uses Ctrl-Shift-Q which is more sensible.