416696 - Restart and quit alerts should use independent "don't warn again" prefs

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Recall]

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

Currently Firefox 3 nightly have the option to "always do this" when you restart the application. However, I would like the option to be able to have independent choices for add-ons and a normal restart. For example, I always want my tabs restored when I install and add-on, but I would like to keep the choice of whether to restore when I do a normal exit. Currently if you tick "always do this" then firefox will always restore my tabs, whether its an extension or normal exit.

Reproducible: Always

Steps to Reproduce:
1.Install and add-on, or do a standard exit and save tabs
2.Tick always restore my tabs
3.Tabs are restored on restart for both add-ons and normal exit and save.

Actual Results:  
Tabs are always restored on either and add-on or standard exit.

Expected Results:  
Independent choices, so I can tick always restore my tabs when I install an add-on, while still preserving the choice on whether to restore my tabs on a normal exit and save.

Comment 1

[Oakwine]

I like the suggestion.  We should also add "installing a software update" to the list of reasons for restart, but it would probably be the same logic as "installing an add-on".

Comment 2

[Dão Gottwald [:dao]]

Under which circumstances would you not want to have your tabs restored after a restart due to installing a software update or an add-on? Doesn't make sense.

Comment 3

[Recall]

I always want my tabs restored with an add-on and software update. However I want to choose if I wish to restore on a normal exit and restart. Currently the always do the same is global.

For example I install and add-on and tick always restore tabs as for me its a given I am going to want to restore my tabs. Now when I go out to the pub, I might want to save my session for later, which is fine with the global option. 

However, sometimes I may go out and would rather not save my session. Due to the fact I have globally chose to always restore tabs, there is no ui available to change this behavior.

This could be seen as a security issue, as with always do this ticked you may forget your tabs are going to be restored on the next restart. This allows prying eyes to see what you were last looking at. Giving the ui option would prevent this from happening.

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> For example I install and add-on and tick always restore tabs as for me its a
> given I am going to want to restore my tabs.

"always restore my tabs" (browser.startup.page = 3) is entirely unrelated to installing an add-on, where the tabs are restored automatically (browser.sessionstore.resume_session_once).

Comment 5

[Recall]

Ok maybe I am not being clear enough.

I just tested again to make sure I am not going crazy.

1: I uninstalled add-on
2: Ticked restore my tabs and do not ask again
3: When I exit normally, the ui for choosing if I want to restore tabs is missing.

Expected Behavior

I choose always restore for add-ons, while preserving the option on exit to save or restore my tabs.

Comment 6

[Dão Gottwald [:dao]]

So the bug is that the "not ask me again" checkbox in "session will be restored once" alert is incorrectly mapped to the browser.startup.page pref?

Comment 7

[Recall]

Yep, that is correct! Sorry if I was not clear before.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch

Comment 9

[Simon Bünzli]

Comment on attachment 302456 [details] [diff] [review]
patch

This isn't really SessionStore related - which is why your pref is wrongly named: There isn't anything to ask about resume_session_once, there is however about warning before a restart...

So please name the pref "browser.warnOnRestart" and replace it for browser.warnOnQuit when |aQuitType == "restart"|.

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v2

yes, makes sense

> This isn't really SessionStore related

Which means that your review won't be enough, will it?

Comment 11

[Simon Bünzli]

Comment on attachment 302466 [details] [diff] [review]
patch v2

(In reply to comment #10)
> Which means that your review won't be enough, will it?
That's what I meant. :-)

Comment 12

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

This will need ui-review.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 302466 [details] [diff] [review]
patch v2

>+          else
>+            // could also set browser.warnOnQuit to false here,
>+            // but not setting it is a little safer.
>+            prefBranch.setIntPref("browser.startup.page", 3);
>+        }

please add braces around this block.

r=mano otherwise.

Comment 14

[Recall]

Dan, any chance you can get a new patch so it works as requested?

Comment 15

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

Comment on attachment 302466 [details] [diff] [review]
patch v2

yeah, these prefs should be seperate; we probably need to think about how we also allow people to re-enable these prefs, but ugh.

Comment 16

[Damon Sicore (:damons)]

Comment on attachment 302466 [details] [diff] [review]
patch v2

a1.9+=damons

Comment 17

[Reed Loden [:reed]]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.282; previous revision: 1.281
done
Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.56; previous revision: 1.55
done

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #13)
> (From update of attachment 302466 [details] [diff] [review])
> >+          else
> >+            // could also set browser.warnOnQuit to false here,
> >+            // but not setting it is a little safer.
> >+            prefBranch.setIntPref("browser.startup.page", 3);
> >+        }
> 
> please add braces around this block.

reed, could you please add the braces?

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> reed, could you please add the braces?

I did that when I landed it. ;)

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=/mozilla/browser/components&command=DIFF&root=/cvsroot&file=nsBrowserGlue.js&rev1=1.55&rev2=1.56#1

Comment 20

[Recall]

Hi guys, just want to thank you for fixing this ( my first ever submitted and fixed bug! )It works just as expected, I now feel all the bug reporting and beta testing is well worth it :)

Comment 21

[(no longer active)]

Shouldn't this pref be set to false for machines which run automated tests?  I'm not sure if any automated test includes a restart, but last time I checked, the warnOnQuit pref was set to false on those machines...