409686 - remove off-by-default restart prompt code

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Francesco Lodolo [:flod]]

The string with key NeverAsk (quitDialog.properties) is "Do not ask next time".

This checkbox is used in combination with (at least) two other strings:
    * "messageNoWindows" when you try to close a window 
    * "messageRestart" when you press the button Restart in the
      Add-ons Manager (see https://bugzilla.mozilla.org/show_bug.cgi?id=385425 )

While the first message is a question ("Do you want %S to save your tabs and
windows for the next time it starts?"), the second message 
is a statement ("%S will try to restore your tabs and windows when it restarts.
"), so "ask" doesn't fit.

We should have a second string to use with a statement like
messageRestart.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Simple patch to add the neverPrompt string for the case where messageRestart is used.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 294756 [details] [diff] [review]
Patch (v1)

please ask beltzner for ui-r first.

Comment 3

[(no longer active)]

Attachment: Patch (v1) (strings only)

String changes to land before the l10n freeze...

Comment 4

[(no longer active)]

This is a discrepancy in the UI which should be fixed, I guess.

Beltzner: can you do a review on the string changes (attachment 298779 [details] [diff] [review]) so that this has a change to land in Firefox 3?  Thanks!

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Do these checkboxes flip the same or different prefs? I totally can't remember.

Comment 6

[(no longer active)]

(In reply to comment #5)
> Do these checkboxes flip the same or different prefs? I totally can't remember.

Yeah: browser.warnOnQuit:

<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/nsBrowserGlue.js&rev=1.49&mark=301-304#296>

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 294756 [details] [diff] [review]
Patch (v1)

I think a better solution is to just remove the checkbox from the restart dialog entirely.

I'm not comfortable, really, with having a box which you can check when you restart also end up affecting the question about saving on quit, which will be more frequently seen. Especially when there's nowhere to flip that pref back.

Comment 8

[(no longer active)]

(In reply to comment #7)
> (From update of attachment 294756 [details] [diff] [review])
> I think a better solution is to just remove the checkbox from the restart
> dialog entirely.
> 
> I'm not comfortable, really, with having a box which you can check when you
> restart also end up affecting the question about saving on quit, which will be
> more frequently seen. Especially when there's nowhere to flip that pref back.

You're right.  I'm morphing the bug accordingly.  Since this won't need a string change, I'll post a patch after the l10n freeze.

Comment 9

[(no longer active)]

Beltzner: Now that I think of it, wouldn't we want a similar browser.warnOnRestart pref which is effective for restarts?  I think just hiding that checkbox on restart confirmation dialogs might be a mistake, because users may be annoyed by being prompted before restarting every time, so there should be a way to disable it.  What do you think?

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

This does not block the final release of Firefox 3.

Comment 11

[(no longer active)]

Attachment: Patch (v2)

Implementing Beltzner's suggestion in comment 7.  I was not sure if it's OK to implement my suggestion in comment 9 at this point in the release cycle, so I ignored it for now.

This patch only removes the check box from the restart dialog.  This should be quick to review.

Comment 12

[(no longer active)]

mano: ping...

Comment 13

[:deanis74]

If we're removing the option to not show this message the next time, then do we need this prompt at all?  When installing an update, we already have a Restart Now/Don't Restart step.  Without the checkbox, this second prompt seems redundant.

Comment 14

[Asaf Romano (gone)]

Comment on attachment 302083 [details] [diff] [review]
Patch (v2)

r=mano code-wise, need ui-r though, see previous comment.

Comment 15

[Simon Bünzli]

The Quit prompt isn't displayed by Session Restore but rather by nsBrowserGlue. Those bugs seem to live in Firefox:General for lack of a better fit.

Comment 16

[(no longer active)]

Updating to reality: I won't have the time to work on this for the foreseeable future!

Comment 18

[(no longer active)]

Someone on the front-end team should own this is it's worth driving through.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Rebased Ehsan's patch

I rebased Ehsan's patch and tested it out locally. Carrying forward r+ from mano and ui-r+ from beltzner.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ehsan Akhgari [:ehsan] from comment #18)
> Someone on the front-end team should own this is it's worth driving through.

I'll take care of this. Thanks for fixing this Ehsan :)

Comment 21

[:Gavin Sharp [email: [e-mail]]]

I'm having a hard time following the reasoning for why we want to remove this checkbox. Prompting all of the time is annoying - I'd rather we go in the opposite direction and just never prompt in the restart case.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2 - Removing the restart dialog

Yeah, I agree that the restart dialog doesn't add any benefit. All it does is reduce user confidence in our restart/session-restore implementation.

This patch removes the restart dialog altogether.

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 632458 [details] [diff] [review]
Patch v2 - Removing the restart dialog

I told Jared that I'll leave this review for Gavin as all these quit modes codepaths are quite involved and might exist for reasons that I don't know about

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Review ping? I just ran into this again today on my beta installation :(

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 632458 [details] [diff] [review]
Patch v2 - Removing the restart dialog

Sorry I didn't look into this one sooner - I didn't realize that this was removing code that was effectively dead (by default) since bug 592822 (2.0).

The removal looks good, but you can further simplify the code in _onQuitRequest by returning much earlier if aQuitType=="restart" (because it's now impossible for showPrompt to be true in that case, AFAICT).

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.1

Moving the check for aQuitType=="restart" up earlier allowed me to remove the showPrompt variable too \o/

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 695858 [details] [diff] [review]
Patch v2.1

Hmm, typing up that comment made me think about it again. I need to make sure that the logic is correct there...

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.2

Fixed the logic within _onQuitRestart that was broken with the v2.1 patch.

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Update STR:
Open about:config and set browser.warnOnRestart=true
Make sure there are at least three tabs open
Open the Developer Toolbar (Shift+F2 on Windows)
Type "restart" in the Developer Toolbar and hit Enter

Pre-patch:
The Restart dialog would show up.

Post-patch:
The Restart dialog should not show up.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7066060a3384

Comment 30

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/7066060a3384