Closed Bug 409686 Opened 17 years ago Closed 11 years ago

remove off-by-default restart prompt code

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: flod, Assigned: jaws)

References

Details

(Keywords: polish, Whiteboard: [see comment 29 for updated STR])

Attachments

(1 file, 6 obsolete files)

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.
OS: Mac OS X → All
Hardware: PC → All
Component: Extension/Theme Manager → Session Restore
QA Contact: extension.manager → session.restore
Assignee: nobody → ehsan.akhgari
Keywords: polish
Target Milestone: --- → Firefox 3 M11
Attached patch Patch (v1) (obsolete) — Splinter Review
Simple patch to add the neverPrompt string for the case where messageRestart is used.
Attachment #294756 - Flags: review?(mano)
Comment on attachment 294756 [details] [diff] [review]
Patch (v1)

please ask beltzner for ui-r first.
Attachment #294756 - Flags: review?(mano)
Attachment #294756 - Flags: ui-review?(beltzner)
Status: NEW → ASSIGNED
Attached patch Patch (v1) (strings only) (obsolete) — Splinter Review
String changes to land before the l10n freeze...
Attachment #298779 - Flags: ui-review?(beltzner)
Attachment #298779 - Flags: review?(beltzner)
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!
Flags: blocking-firefox3?
Do these checkboxes flip the same or different prefs? I totally can't remember.
(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 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.
Attachment #294756 - Flags: ui-review?(beltzner) → ui-review-
Attachment #298779 - Flags: ui-review?(beltzner)
Attachment #298779 - Flags: ui-review-
Attachment #298779 - Flags: review?(beltzner)
Attachment #298779 - Flags: review-
(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.
Summary: the quit prompt displays a statement but the checkbox label refers to a question → Remove the "Do not ask next time" checkbox from the quit prompt when restarting
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
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?
This does not block the final release of Firefox 3.
Flags: blocking-firefox3? → blocking-firefox3-
Attached patch Patch (v2) (obsolete) — Splinter Review
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.
Attachment #294756 - Attachment is obsolete: true
Attachment #298779 - Attachment is obsolete: true
Attachment #302083 - Flags: review?(mano)
mano: ping...
Whiteboard: [has patch] [needs review Mano]
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 on attachment 302083 [details] [diff] [review]
Patch (v2)

r=mano code-wise, need ui-r though, see previous comment.
Attachment #302083 - Flags: review?(mano) → review+
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.
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
Component: Session Restore → General
QA Contact: session.restore → general
Assignee: nobody → ehsan.akhgari
Whiteboard: [has patch] [needs review Mano] → [has patch]
Target Milestone: Firefox 3 beta4 → ---
Attachment #302083 - Flags: ui-review?(beltzner)
Whiteboard: [has patch] → [has patch][needs ui-review beltzner]
Flags: wanted-firefox3.6?
Flags: wanted-firefox3.6?
Updating to reality: I won't have the time to work on this for the foreseeable future!
Assignee: ehsan.akhgari → nobody
Whiteboard: [has patch][needs ui-review beltzner] → [has patch]
Attachment #302083 - Flags: ui-review?(beltzner) → ui-review+
Someone on the front-end team should own this is it's worth driving through.
Attached patch Rebased Ehsan's patch (obsolete) — Splinter Review
I rebased Ehsan's patch and tested it out locally. Carrying forward r+ from mano and ui-r+ from beltzner.
Attachment #302083 - Attachment is obsolete: true
Attachment #632178 - Flags: ui-review+
Attachment #632178 - Flags: review+
(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 :)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Keywords: checkin-needed
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.
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.
Attachment #632458 - Flags: review?(gavin.sharp)
Whiteboard: [has patch]
Attachment #632458 - Flags: review?(felipc)
Attachment #632178 - Attachment is obsolete: true
Attachment #632178 - Flags: ui-review+
Attachment #632178 - Flags: review+
Attachment #632178 - Flags: feedback-
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
Attachment #632458 - Flags: review?(felipc)
Review ping? I just ran into this again today on my beta installation :(
Summary: Remove the "Do not ask next time" checkbox from the quit prompt when restarting → Don't prompt on restart ("Firefox will try to restore your tabs upon restart [do not ask next time]" dialog removes trust and is unnecessary)
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).
Attachment #632458 - Flags: review?(gavin.sharp) → feedback+
Attached patch Patch v2.1 (obsolete) — Splinter Review
Moving the check for aQuitType=="restart" up earlier allowed me to remove the showPrompt variable too \o/
Attachment #632458 - Attachment is obsolete: true
Attachment #695858 - Flags: review?(gavin.sharp)
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...
Attachment #695858 - Flags: review?(gavin.sharp)
Attached patch Patch v2.2Splinter Review
Fixed the logic within _onQuitRestart that was broken with the v2.1 patch.
Attachment #695858 - Attachment is obsolete: true
Attachment #695911 - Flags: review?(gavin.sharp)
Summary: Don't prompt on restart ("Firefox will try to restore your tabs upon restart [do not ask next time]" dialog removes trust and is unnecessary) → remove off-by-default restart prompt code
Attachment #695911 - Flags: review?(gavin.sharp) → review+
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
Whiteboard: [see comment 29 for updated STR]
https://hg.mozilla.org/mozilla-central/rev/7066060a3384
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: