340898 - Remove the pref .sessionstore.resume_session in favor of .startup.page

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Simon Bünzli]

... because .resume_session currently overrides .startup.page anyway, and less prefs is almost always better.

The question is rather whether we want to use .startup.page = 2 (formerly: last page visited, change requested in bug 159357) or a new value (= 3).

Comment 1

[Dietrich Ayala (:dietrich)]

I think that the function of startup.page=2 should not change. The behavior and intent of "restore last visited page" and "restore previous session" are quite different. We should add support for a new value (startup.page=3) for resuming the previous session. Adding this new value will remove ambiguity about which has precedence when startup.page=2 and resume_session=true.

Comment 2

[Nickolay_Ponomarev]

If I'm reading the code correctly, right now browser.startup.page == 2 has the same effect (at startup) as if resume_session was set.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/sessionstore/src/nsSessionStore.js&rev=1.29&mark=1808#1797

If it is indeed so, the fix for this bug is to remove the session_restore pref and change the value we're comparing browser.startup.page to from 2 to 3, right? I can make that patch if desired :)

Comment 3

[Dietrich Ayala (:dietrich)]

(In reply to comment #2)
> If I'm reading the code correctly, right now browser.startup.page == 2 has the
> same effect (at startup) as if resume_session was set.
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/sessionstore/src/nsSessionStore.js&rev=1.29&mark=1808#1797
> 
> If it is indeed so, the fix for this bug is to remove the session_restore pref
> and change the value we're comparing browser.startup.page to from 2 to 3,
> right? I can make that patch if desired :)
> 

Sure it's desired. That sounds about right. My only concern is that beta 1 might have wide enough distribution that changing the pref will cause much consternation among users of it. However, I guess that's the risk you take when using beta software.

Comment 4

[Nickolay_Ponomarev]

Attachment: patch

OK. I think this should do the trick.

The 'case 3' line was added to match the defaultArgs impl, which passes 'about:blank' in case of browser.startup.page==3 and no overrides. (Ideally the code shouldn't be duplicated between that function and sss_isCmdLineEmpty.)

Comment 5

[Simon Bünzli]

(In reply to comment #4)
> The 'case 3' line was added to match the defaultArgs impl, which passes
> 'about:blank' in case of browser.startup.page==3 and no overrides. (Ideally the
> code shouldn't be duplicated between that function and sss_isCmdLineEmpty.)

That part of the code should be removed by the patch to bug 342471 (which directly accesses defaultArgs).

Comment 6

[Dietrich Ayala (:dietrich)]

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

r=me

Thanks for cleaning up the documentation.

Comment 7

[Dietrich Ayala (:dietrich)]

Checked in on trunk.

Comment 8

[Dietrich Ayala (:dietrich)]

On D.A.F, littlemutt brought up a good point: This change makes it so that when users manually open a new window, their homepage is not loaded.

I think that this is actually a plus in most cases. The first thing I do after ctrl+n is hit the stop button, because I almost always load the new window for a specific task, not to see the homepage.

> Perhaps as suggested in this thread:
> http://forums.mozillazine.org/viewtopic.php?t=441394
> by Zeniko that maybe we could get  browser.windows.loadOnNewWindow back
> (which has the same values as browser.startup.page: 0 = blank, 1 =
> homepage(s), 2 = last visited page; BTW: there even was
> brower.tabs.loadOnNewTab once).

I think this solution would be appropriate, and would help to clarify whether these prefs apply to application start, or new windows. 

However, I don't think that this is critical enough of an issue to block on.

Comment 9

[Adam Guthrie]

I filed bug 345743 for the issue Dietrich brought up in comment 8.

Comment 10

[Mike Connor [:mconnor]]

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

a=mconnor on behalf of drivers

Dietrich, before checkin can you add a comment explaining this to firefox.js at http://lxr.mozilla.org/mozilla/source/browser/app/profile/firefox.js#181 and remove the xxxblake, since we're not removing this pref now.