Recall Reporter
	Description • 16 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041015 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041015 Minefield/3.0pre

Unchecking "warn on close multiple tabs" breaks the option for "save tabs on exit" It used to work a few builds ago but not sure of regression range.

If you keep the option checked, then you still get the prompt to save tabs on exit. If it is unchecked then firefox does not give the option.

Reproducible: Always

Steps to Reproduce:

1.Uncheck "warn on close multiple tabs
2.Open a few tabs
3.Exit firefox
Actual Results:  
"Do you want to save tabs" is not given as an option.

Expected Results:  
Unchecking the option should still give the option to save tabs on exit.

This is a regression.

	Recall Reporter
	Comment 1 • 16 years ago

OK seems bug 419009 has caused this, is this intentional behavior then?

	Roman R.
	Comment 2 • 16 years ago

(In reply to comment #0)
> Unchecking the option should still give the option to save tabs on exit.

Should it? I think FF should save a session if the "show my windows and tabs from last time" is checked, and it should do it without asking.

	Recall Reporter
	Comment 3 • 16 years ago

I dont like getting bugged that I am closing multiple tabs. I do however like to be able to save my tabs on exit, but not always. See bug 416696 that was fixed in February for and idea of what i mean.

	Roman R.
	Comment 4 • 16 years ago

It seems like somebody should reevaluate this whole thing, get requirements from users and redesign this area, including the pref UI, of course, to avoid confusion.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 5 • 16 years ago

This was an intentional change from bug 419009. See bug 428742 comment 6 for the prefs you need to change to get the old behavior.

	Recall Reporter
	Comment 6 • 16 years ago

Gavin, thanks but this is really such a confusing setup. Can we not get a preference for "prompt to save tabs on restart" checkbox under tools>options>tabs this way you can choose to always have your tabs saved, or not as in my case. Should be a quick fix?

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 7 • 16 years ago

We're well past string freeze, we can't be adding checkboxes to the pref dialog for Firefox 3.

	Roman R.
	Comment 8 • 16 years ago

3.0.1 then?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 9 • 16 years ago

Roman: yes, we need to revisit the entire set of prefs around quit and restart, we meant to do it for Firefox 3 but ran out of time.

However, I do agree that the way this has been fixed has ended up putting us in a pretty bad state; we fixed bug 419009 in order to resolve an annoyance (not asking before closing multiple tabs when there was only one window and browser.startup.page = 3) in which there was no dataloss, only timeloss, and the result is we've created an entirely possible case in which there can now be dataloss.

I'm gonna think about this for a bit. I still do think that there are some advantages to the fix from bug 419009, namely that the one exposed user pref implies that users will or will not be warned before closing multiple tabs. It also allows people to reset the warnOnQuit pref, but that comes at the cost of not being warned when they close a window with multiple tabs.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 10 • 16 years ago

Spoke with mconnor about this, and we agreed that this doesn't block. The relevant advantage that swayed the decision here was that at least now there's UI for getting the "Save and Quit" dialog back (it's tied to "Warn me when closing multiple tabs") whereas before there wasn't.

We definitely want to clean this up in the future, though.

	Recall Reporter
	Comment 11 • 16 years ago

Mike, I really do not think this is the way to go. The UI is now more unusable than before and now get an extra annoyance for closing tabs. This has caused bug 428742 as well ( which has been marked wontfix! ) This seems to me a crazy decision is there no chance of backing out bug 419009?

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 12 • 16 years ago

(In reply to comment #10)
> Spoke with mconnor about this, and we agreed that this doesn't block. The
> relevant advantage that swayed the decision here was that at least now there's
> UI for getting the "Save and Quit" dialog back (it's tied to "Warn me when
> closing multiple tabs") whereas before there wasn't.

You think that that advantage outweighs the impact of this bug and the lack of a good workaround for bug 428742? If so, I disagree. I think that being unable to re-enable the Quit dialog is a minor issue that should be addressed by reworking preferences UI in the next release, whereas the inability to configure warnings such that you're warned if dataloss will occur (closing a window with multiple tabs), but aren't warned in the cases where your session will be restored (restarting, quitting with the "save my tabs" preference enabled) without toggling a hidden pref will be a much more annoying problem for most users. The Quit dialog is a useful feature, and tying it to the "close a window with multiple tabs" pref cripples it unnecessarily.

I'm not sure why you would mark this "wanted-firefox3", since there is no solution other than taking a different approach in bug 419009, and you've indicated that you don't think we're going to do that.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 13 • 16 years ago

In bug 428742 claimed that setting browser.startup.page=3 should be tied directly to showing or not showing the warning when quitting, and I stand by that; we don't always successfully restore state, so a warning should be shown to avoid potential dataloss, especially in the restart case.

I've reopened bug 428742, as I think it was marked incorrectly twice, alas.

The one thing I didn't consider in my blocking decision was that the way things are now, checking "don't show me this warning again" when quitting ends up also turning off the multiple tab warning, which isn't very intuitive.

Based on that, I think we need to back out bug 419009, ensure that browser.WarnOnQuit = (browser.startup.page==3), and then actually figure out how to solve bug 419009 properly, which is to say ensure that the multiple tab warning is given irrespective of browser.startup.page/browser.WarnOnQuit.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 14 • 16 years ago

(In reply to comment #13)
> In bug 428742 claimed that setting browser.startup.page=3 should be tied
> directly to showing or not showing the warning when quitting, and I stand by
> that; we don't always successfully restore state, so a warning should be shown
> to avoid potential dataloss, especially in the restart case.

Ick, this was half edited. Please ignore everything except:

I don't think that setting browser.startup.page=3 should effect the restart case.

	Simon Bünzli
	Comment 15 • 16 years ago

What about the following stopgap solution:
* the visible "warn me when closing..." UI controls both browser.tabs.warnOnClose _and_ browser.warnOnQuit (in the same way as the "new pages should open in..." UI controls two prefs);
* when one of both prefs is set to false, we remove the checkbox;
* toggling the checkbox toggles both prefs,
* however as long as the checkbox is untouched, the prefs can diverge, and
* removing the checkbox from the warning prompt only toggles one of both prefs.

Like this, there'd be a somewhat obvious way to disable/enable both prefs without us having to introduce new UI at this stage (at least people could be walked through without having to touch about:config)...

	Recall Reporter
	Comment 16 • 16 years ago

Simon, I like that idea if indeed it is too late to get a pref ui in. At least then by unchecking the warn on close tabs we will still keep the option for save on quit, unless "always do this" is checked for not saving. Get so close to final now is it possible to shoehorn this in before it ships?

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 17 • 16 years ago

(In reply to comment #15)
> What about the following stopgap solution:

I think we should do this. Can you prepare the patch?

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 19 • 16 years ago

Comment on attachment 318187 [details] [diff] [review]
fix per comment #15

>Index: browser/components/preferences/tabs.js

>+  writeWarnCloseTabs: function() {
>+    var warnOnClose = document.getElementById("warnCloseMultiple");
>+    document.getElementById("browser.warnOnQuit").value = warnOnClose.checked;
>+    return warnOnClose.value;

warnOnClose doesn't have a ".value", it's a checkbox. This works anyways because undefined maps to "default element value", though, so you can just change this to return undefined explicitly.

	Mike Connor [:mconnor]
	Comment 20 • 16 years ago

Comment on attachment 318187 [details] [diff] [review]
fix per comment #15

I don't like this.  It overloads the checkbox in an unintuitive way.  If one of the warnings is off, and you want to turn the other off in the prefwindow, you need to check and uncheck it.  That feels pretty non-deterministic to me, and I'm not willing to ship something that will sometimes look like its broken.  We should just fix this the right way in 3.1.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 21 • 16 years ago

Talked a bit with Beltzner, this is an alternative patch that addresses my issues with the current behavior.

The main changes are to never show the quit/restart prompts if the session will be restored, and to not disable the "close window with multiple tabs" warning when disabling the quit warning with session store enabled. Disabling the quit prompt without enabling session restore all the time will still disable the "close window with multiple tabs" warning, but since the user is opting in to no-warning dataloss (quitting with multiple tabs and session store disabled), the other dataloss case (closing a window with multiple tabs) shouldn't bother them too much (and they can still re-add the quit prompt in our UI since it still uses the warnOnClose pref).

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 23 • 16 years ago

Comment on attachment 318541 [details] [diff] [review]
alternative patch that works (-w)

uir=beltzner, thanks!

(I'll, uh, warn dria and phik)

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 24 • 16 years ago

(Still not blocking, but I'd approve the reviewed patch)

	Recall Reporter
	Comment 25 • 16 years ago

Is this patch gonna make it in?

	Mike Connor [:mconnor]
	Comment 26 • 16 years ago

Comment on attachment 318541 [details] [diff] [review]
alternative patch that works (-w)

code looks right.  I'm not completely convinced this is any better, but its probably more conservative.

If this regresses anything, it gets backed out and we just ship.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 27 • 16 years ago

mozilla/browser/components/nsBrowserGlue.js 	1.93 

	Soothsayer
	Comment 28 • 16 years ago

Following this check in, I now get the close multiple tabs prompt every time I close Firefox even when no tabs are open.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9pre) Gecko/2008050812 Minefield/3.0pre

	Recall Reporter
	Comment 29 • 16 years ago

I don't get the prompt here with no tabs open. In fact nothing has changed at all for me, the only way for me to be prompted on close to save session is to keep the annoying multiple tabs warning on. I don't even think an extension can fix this, unless it uses its own restore session.

Can we just get this reopened until a proper fix lands with a gui interface?

	Soothsayer
	Comment 30 • 16 years ago

If it helps reproduce / identify the cause, my pertinent preferences are:
browser.warnOnRestart = false
browser.tabs.warnOnClose = true

Also, overwriting my nsBrowserGlue.js with the one from yesterday's build fixes the problem.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 31 • 16 years ago

(In reply to comment #28)
> Following this check in, I now get the close multiple tabs prompt every time I
> close Firefox even when no tabs are open.

That's an expected change from this patch, to cover cases where you close the window accidentally and lose your session. Please file a new bug if you think we should revert it.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 32 • 16 years ago

(In reply to comment #29)
> I don't get the prompt here with no tabs open. In fact nothing has changed at
> all for me, the only way for me to be prompted on close to save session is to
> keep the annoying multiple tabs warning on.

You're right, this patch only affected the cases where you have "Always restore my tabs" enabled. To get the behavior you describe we'd need to decouple the prefs again, which we can't do for Firefox 3. Please file a new bug to cover that case.

	Soothsayer
	Comment 33 • 16 years ago

As suggested, I have created bug 433018 requesting this patch be backed out due to undesired behaviour and inaccurate pop up message.

	George Carstoiu
	Comment 34 • 14 years ago

Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre

Issue is still present and can cause data loss (session is lost) if "Warn me when closing multiple tabs" is unchecked.