463308 - honor browser.warnOnQuit even if browser.tabs.warnOnClose is false

Status: RESOLVED WONTFIX

(Firefox :: Settings UI, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Some users set browser.tabs.warnOnClose to false because they regularly close windows with multiple tabs intentionally and don't want to be prompted when that happens.  But they still want to be prompted when they quit the application while multiple tabs are open because they do that unintentionally a greater percentage of the time and want to be protected from accidentally losing their tabs (f.e. bug 189290, comment 36).

Currently, if browser.tabs.warnOnClose is set to false, browser.warnOnQuit is ignored.  browser.warnOnQuit should be independent of browser.tabs.warnOnClose, so it's possible to configure Firefox to prompt you when you quit the application but not when you close one of its windows.

Note: this bug is not about what the default values should be or what kind of UI there should be for setting these preferences.  It only requests the ability to set them independently (and for Firefox to respect the value of browser.warnOnQuit even if browser.tabs.warnOnClose is set to false), leaving existing UI intact.  Modifications to the default values and the UI (if any are deemed appropriate) should be handled in a different bug (like bug 384907, bug 433123, bug 419009, or bug 443396).

[cc:ing some folks who have been active in related bugs]

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Possible fix

Searching for "warnOnQuit" gives only one relevant hit. Looking at the code, it does expose the described problem. I fixed that, patch attached.
When testing, however, I still see no prompt. I see only two potential reasons for that: Either my recompilation/testing was wrong, or there's another code place for Quit where warnOnQuit is not checked, but should be.

Note that this may also break automated tests (which set the pref to false, because it's default true). The code comment suggests that this is a pref to *not* warn (suspend warnings), not a pref to warn (add warnings).

Comment 2

[Ben Bucksch (:BenB)]

Comment on attachment 346598 [details] [diff] [review]
Possible fix

Actually, the reason was an
    if (pagecount < 2)
      return;
earlier in the function. When I have more than one tab or window open, I see the prompt now. So, the fix works.

Comment 3

[Ben Bucksch (:BenB)]

Attachment: Possible fix, v2

I think the current pref is meant to mean "never warn on quit", while "warnOnQuit" has to be understood as "*always* warn on quit". Therefore, I think the pref is misworded and I introduced both "neverWarnOnQuit" (for automated tests) and "alwaysWarnOnQuit" (for users).

Both patches will show the prompt even when it's unproblematic (e.g. only one page open), which may not be the desired result.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Isn't this dup of bug 404081?

Comment 5

[Ben Bucksch (:BenB)]

No.

Comment 6

[Ben Bucksch (:BenB)]

(They are similar or even related, but this bug is about browser.warnOnQuit not being honored, while the other one is about browser.tabs.warnOnClose not being honored.)

Comment 7

[Ben Bucksch (:BenB)]

Comment on attachment 346601 [details] [diff] [review]
Possible fix, v2

I noticed gladly that I am no longer affected by this bug. I noticed only now that it's because of my own patch running in my tree.

Please let's get this into Firefox, this has saved me several times already in the last few months. Requesting review.

Comment 8

[Arthur]

Cool. I'm in no position to make a formal review, but there's one thing I'd like to comment on and there's one question I have:

* browser/components/nsBrowserGlue.js has the comment
// browser.warnOnQuit is a hidden global boolean to override all quit prompts
which should be changed to alwaysWarnOnQuit.

* How is the transition from warnOnQuit to alwaysWarnOnQuit handled? What happens to the old pref when upgrading?

Comment 9

[Ben Bucksch (:BenB)]

> How is the transition from warnOnQuit to alwaysWarnOnQuit handled?

It's not exposed in the UI.

If we're really worried about automated builds, we could write a pref migration, but I don't know where that code is.

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 346601 [details] [diff] [review]
Possible fix, v2

This patch is wrong for a number of reasons:

* Changing all of the existing Talos machines over is a pain.  Let's not change the pref here.
* This patch will result in always showing the prompt regardless of prefs, unless you change the alwaysWarnOnQuit patch.  It doesn't check "sessionWillBeSaved" at all now.
* Short of reading the code, how is it clear which of neverWarnOnQuit and alwaysWarnOnQuit wins?  Why would you use two boolean prefs like this?

Bad implementation aside, I don't want to add more hidden prefs until we have a more coherent idea of what configuration options we want to support for shutdown, and adding more crap into the mix is going to make that code more frustrating.  Let's just not do that for now.

Comment 11

[Ben Bucksch (:BenB)]

> This patch will result in always showing the prompt ...
> unless you change the alwaysWarnOnQuit patch.  It doesn't check
> "sessionWillBeSaved" at all now.

Yes, that's the point. I *want* to see the prompt always, regardless of whether you (think you) can save the session or not. A, it apparently doesn't work, and B, it's still destructive: all pages have to be reloaded, which may not be the old content anymore (generated POST, page gone etc.), Firefox can't restore windows for Xinerama, etc.pp.. Even if all that were implemented, the fact remains that I didn't want to close Firefox in the first place, I hit Ctrl-Q by accident (wanted Ctrl-W, which I use a lot), and quiting is a non-trivial thing which I almost *never* do intentionally and I *want* confirmed.

> Let's just not do that for now.

We need something now, because the state as-is is dataloss and *very* painful.
Unless you have a concrete suggestion and working patch which solves the issue, please don't reject patches which improve the situation simply because you hate me (which I can understand and know).

Comment 12

[Ben Bucksch (:BenB)]

> Bad implementation aside

Speaking of bad implementation: The current state is that "warnOnQuit" = true doesn't actually warn. *That* is bad implementation or definition, whatever you call it.

Comment 13

[Arthur]

The current implementation is broken UI wise and code wise. It even says explicitly in the code "browser.warnOnQuit is a hidden global boolean to override all quit prompts", but it doesn't override all quit prompts. So I'd say leave the name of the pref as it is and just fix its broken behaviour. People can always un-set the warnOnQuit pref if they want the non-warning behaviour.

Comment 14

[Arthur]

Attachment: Honor warnOnQuit.

This leaves all the prefs alone and simply honors the semantic of warnOnClose as described in the code comment and as it can be derived from the prefs name. The browser still will silently close if the session is going to be saved and the pref *is not* set.

The only thing still debatable is what to do if the pref "warnOnClose" is set, in other bugs it is argued that the warning dialog should be shown because there is an explicit GUI setting which is silently overridden. This patch doesn't touch this issue at all. It can be dealt with seperately.

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 369037 [details] [diff] [review]
Honor warnOnQuit.

This, in itself, is insufficient, since the pref defaults to true, and the "always do this" checkbox won't do anything now.

warnOnQuit is badly-named.  This is known, but we didn't change the name for the same reason that we didn't change it last time.

Everyone needs to slow down and stop attaching patches until there's a spec for what we actually want to support.  Patches don't get to win here.

Comment 16

[Mike Connor [:mconnor]]

(In reply to comment #11)
> > This patch will result in always showing the prompt ...
> > unless you change the alwaysWarnOnQuit patch.  It doesn't check
> > "sessionWillBeSaved" at all now.
> 
> Yes, that's the point. I *want* to see the prompt always, regardless of whether
> you (think you) can save the session or not. A, it apparently doesn't work, and
> B, it's still destructive: all pages have to be reloaded, which may not be the
> old content anymore (generated POST, page gone etc.), Firefox can't restore
> windows for Xinerama, etc.pp.. Even if all that were implemented, the fact
> remains that I didn't want to close Firefox in the first place, I hit Ctrl-Q by
> accident (wanted Ctrl-W, which I use a lot), and quiting is a non-trivial thing
> which I almost *never* do intentionally and I *want* confirmed.

What you're trying to do, I think, is revert the change in bug 419009.  I get that you don't like the behaviour, but we're not going back to the Fx3 betas behaviour we got rid of.  If you want to be warned when closing multiple tabs, set that pref.

> > Let's just not do that for now.
> 
> We need something now, because the state as-is is dataloss and *very* painful.
> Unless you have a concrete suggestion and working patch which solves the issue,
> please don't reject patches which improve the situation simply because you hate
> me (which I can understand and know).

Please keep your ego in check.  I don't care enough about you to hate you.  Nor am I going to r- a patch over personal issues.  This code is bikeshedland, and I keep an eye out for behavioural conflicts.

This reverts to a behaviour we rejected after trunk testing, makes the "don't ask me again" checkbox broken, and implements conflicting boolean prefs, instead of making it an int pref with three states (current behaviour, always warn on quit regardless of warnOnClose, and never warn on quit ever).

You're trying to jam something in because you think it's wrong.  I don't think it's wrong, and I'm not convinced there's any urgency at all.  A few people aren't happy, but it's a strange edge case I honestly don't think is very interesting or important.

Here's the deal:

* enumerate the behaviours you're trying to support, and design a pref setup that's sane.
* if you're arguing for forcing a dialog in more cases, come up with a _great_ argument for that.  We're trying to reduce dialogs, not add more.
* explain why we should be trying to support the "I don't care about losing a whole window of tabs completely, but it's unacceptable to lose that window if I hit Quit.

It seems almost like just killing the Quit keybinding would solve your problem better anyway...

Comment 17

[Ben Bucksch (:BenB)]

> What you're trying to do, I think, is revert the change in bug 419009.

No, that is about tabs.
I need a confirmation when I quit (because Ctrl-Q is too easy to hit), optionally.
Please see this bug's title (not created by me), these are different issues. I don't want to be warned when I close a window (even if it has many tabs), because I do that a dozen times a day and usually intentionally and a warning would just wear me down.
I almost never close the browser intentionally an almost always accidentally and I need a warning there.

Yes, the pref is misnamed. That's why I tried to either 1) fix it (last patch by Arthur) or 2) introduce a new pref for correct behavior. I think "neverWarnOnQuit" and "alwaysWarnOnQuit" (the pair, implemented by patch v2) should be clear enough. We can also leave the existing, misnamed pref for compat, if you want, and/or make it an new integer pref, with warnOnQuitMode: 0 = auto (default), 1 = never, 2 = always.
I hope *one* of these suggestions is acceptable to you.

You seem to be rejecting everything because you don't see the point. I hope the first paragraph explains it sufficiently for everybody that there is a real issue that causes problems.

Comment 18

[Arthur]

> * explain why we should be trying to support the "I don't care about losing a
> whole window of tabs completely, but it's unacceptable to lose that window if I
> hit Quit.
But that doesn't work. You can have warnOnClose and warnOnQuit enabled and it
still quits without warning when session saving is enabled, completely
disregarding the GUI setting which says "Warn me when closing multiple tabs" (which isn't the point of this bug, but which makes your workaround unusable).

Comment 19

[Mike Connor [:mconnor]]

(In reply to comment #18)
> But that doesn't work. You can have warnOnClose and warnOnQuit enabled and it
> still quits without warning when session saving is enabled, completely
> disregarding the GUI setting which says "Warn me when closing multiple tabs"
> (which isn't the point of this bug, but which makes your workaround unusable).

Yes, that was a decision we made in bug 419009.  I argued your side, I've since decided I was wrong.  This bug is scope creeping like crazy now.

To sum up, because this is become bikesheddish:

* Current default behaviour will not change for the time being.  We iterated on it a lot, and we're not seeing any significant user feedback that our final behaviour was wrong.
* The warnOnQuit pref is hidden (and yes, it is now badly named, it was originally respected like this bug asks for, and for the sake of automated testing we didn't change it when we changed behaviour, but that's like complaining about a registry key having a bad name), so I don't care a whole lot about the naming.  The pref only really exists for automated testing, so I don't feel like forcing changes on everyone for the sake of pedantry.
* This bug is a strange case.  Really, the problem is not that we want a warning here, is that for some people, Quit is a generally unwanted behaviour.  The fix there is to write an extension which nukes/changes the keybinding, and be done with it.  (Ctrl+Shift+Q would work to avoid Ben's issue, I think.)

As filed, I don't intend to change this behaviour unless we're making other changes to solve some bigger issues around shutdown (i.e. save session vs. clearing data).  We're not going to make warnOnQuit part of the UI based on current plans, so respecting its value in addition to warnOnClose isn't going to happen.  So I'm going to WONTFIX this, based on the bug that was filed, and recommend that people interested in related issues find the bugs on those issues.