I'd like us to expose the on-demand tab loading (currently done via the browser.sessionstore.max_concurrent_tabs=0 preference) in the prefs under session restore. It would look like this when selected: When Firefox starts: [Show my windows and tabs from last time ▼] [✓] restore tabs on-demand If we can't resize the window to accommodate this extra line that only shows when restoring tabs is selected, it's OK to leave a blank line here for the other options. It should be unchecked by default, so the current behavior doesn't change unless you opt in to it. Some additional notes: 1. I'm sure the wording can be improved, suggestions welcome. 2. Based on discussions, we probably want to add a "restore tabs on demand" preference that short-circuits the session restore concurrency setting instead of overwriting the number with 0. 3. There might be a better place or way to expose this setting, suggestions welcome. Wanted to get a bug in, so Paul can take a look if we want to try landing this for the upcoming release — since the code is already in Firefox 4.

Do it. Do it now.

I know I said I would mentor somebody through this, but I just went ahead and did it. Oops? This adds a new pref browser.sessionstore.restore_on_demand, which when true overrides the behavior of max_concurrent_tabs and restore_hidden_tabs. I figured having a checkbox that actually controls an int pref was awkward, but then again, adding a new bool pref (which just mimics max_concurrent_tabs == 0) is just more to maintain. I went with what felt natural to me, but I'm not opposed to just having the checkbox set max_concurrent_tabs to 0 and restoring default when unchecked. As far as the pref pane UX goes, the checkbox is disabled unless restore session is selected. That's a bit awkward too since the pref controls the behavior of "restore previous session" as well. But it seemed weird to enable changing the pref when home/blank page was selected.

Comment on attachment 549165 [details] [diff] [review] Patch v0.1 I think I would prefer keeping the underlying backend pref situation simple, even if it means additional complexity for the prefs UI. But that is a gut-feeling and I reserve the right to change my mind should you tell me that I'm crazy!

Is browser.sessionstore.max_concurrent_tabs still needed at all?

(In reply to comment #4) > Is browser.sessionstore.max_concurrent_tabs still needed at all? Well, `max_concurrent_tabs` is a dial whereas `restore_on_demand` is an on/off switch. Since the dial can go to zero and assume the 'off' position of the switch, I don't really understand why we need the switch at all. I guess that's just a fancy way of saying that I agree with Gavin.

What other dial positions do we care about at this point? "[ ] Don’t load tabs until selected" could of course set max_concurrent_tabs to 0 and 3, respectively, but this seems wrong if we assume that someone would want to set it to something other than 0 or 3.

(In reply to comment #3) > Comment on attachment 549165 [details] [diff] [review] [diff] [details] [review] > Patch v0.1 > > I think I would prefer keeping the underlying backend pref situation simple, > even if it means additional complexity for the prefs UI. But that is a > gut-feeling and I reserve the right to change my mind should you tell me > that I'm crazy! The UI can stay a simple checkbox and just lie about the pref value. That should be easy. (In reply to comment #6) > What other dial positions do we care about at this point? > > "[ ] Don’t load tabs until selected" could of course set max_concurrent_tabs > to 0 and 3, respectively, but this seems wrong if we assume that someone > would want to set it to something other than 0 or 3. That's why I made a new pref. It could well be the case that nobody ever changes it to anything other than 0 or restores default to 3. There's also a value of -1 where we don't have a max and we just restore everything at once like the old days. I don't really have strong feelings either way, it just didn't make sense to me to use an int as a bool. But if we don't change the default (as Alex said to start), then the box will be unchecked by default and leave people's custom values in there. If they happened to put 0 in there, then the checkbox is magically checked.

(In reply to comment #7) > (In reply to comment #6) > > What other dial positions do we care about at this point? > > > > "[ ] Don’t load tabs until selected" could of course set max_concurrent_tabs > > to 0 and 3, respectively, but this seems wrong if we assume that someone > > would want to set it to something other than 0 or 3. > > That's why I made a new pref. Right, so I think we should either take your patch... > It could well be the case that nobody ever > changes it to anything other than 0 or restores default to 3. ... or add restore_on_demand but remove max_concurrent_tabs (have a MAX_CONCURRENT_TAB_RESTORES const in nsSessionStore.js instead).

(In reply to comment #8) > ... or add restore_on_demand but remove max_concurrent_tabs (have a > MAX_CONCURRENT_TAB_RESTORES const in nsSessionStore.js instead). This sounds like a good option to me. I don't think we need to support arbitrary values as a pref, and I don't think there are many valid use cases for using -1 to restore the previous all-at-once behavior.

Is this going to happen for Firefox 8?

(In reply to Dão Gottwald [:dao] from comment #10) > Is this going to happen for Firefox 8? That's the hope. New patch shortly.

This gets rid of max_concurrent_tabs, but I'm also going to need to fix several session restore tests which were using different values for that pref (metering to 1 tab at a time was really handy)

Comment on attachment 552171 [details] [diff] [review] Patch v0.2 >diff --git a/browser/components/preferences/main.js b/browser/components/preferences/main.js >+ readStartupPagePref: function () >+ { >+ this.startupPagePrefChanged(); >+ return undefined; This can just happen onload rather than in an onsyncfrompreference, right? >diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js >+ _migratePrefs: function sss__migratePrefs() { >+ // Added For Firefox 8 >+ // max_concurrent_tabs is going away. We're going to hard code a max value >+ // (MAX_CONCURRENT_TAB_RESTORES) and start using a boolean pref restore_on_demand. >+ if (this._prefBranch.prefHasUserValue("sessionstore.max_concurrent_tabs") && >+ !this._prefBranch.prefHasUserValue("sessionstore.restore_on_demand")) { >+ let maxConcurrentTabs = >+ this._prefBranch.getIntPref("sessionstore.max_concurrent_tabs"); >+ this._prefBranch.setBoolPref("sessionstore.restore_on_demand", maxConcurrentTabs == 0); >+ this._prefBranch.clearUserPref("sessionstore.max_concurrent_tabs"); >+ } >+ }, This could be annoying for people going back and forward between builds where this lands. I guess that isn't a huge deal, but you could also just avoid clearing the user pref and use another pref to store whether the migration occurred. >+ case "sessionstore.restore_on_demand": >+ // When turning restore on demand off, we should kick off restores to make >+ // sure any remaining tabs get restored. >+ if (!this._restoreOnDemand) >+ for (let i = 0; i < MAX_CONCURRENT_TAB_RESTORES; this.restoreNextTab(), i++); Empty for loop with side effects is gross! :) Does this preference really need to be this "live" (or live at all?). What are the odds that someone will flip the pref in the middle of a restore? > restoreNextTab: function sss_restoreNextTab() { > // If it's not possible to restore anything, then just bail out. >- if (this._maxConcurrentTabRestores >= 0 && >- this._tabsRestoringCount >= this._maxConcurrentTabRestores) >+ if (this._restoreOnDemand || >+ this._tabsRestoringCount >= MAX_CONCURRENT_TAB_RESTORES) Doesn't this still need to be && rather than ||?

(In reply to Gavin Sharp from comment #13) > >+ readStartupPagePref: function () > >+ { > >+ this.startupPagePrefChanged(); > >+ return undefined; > > This can just happen onload rather than in an onsyncfrompreference, right? Truth. I'd based this on the download checkbox, which uses onsyncfrompreference. > >+ _migratePrefs: function sss__migratePrefs() { > >+ // Added For Firefox 8 > >+ // max_concurrent_tabs is going away. We're going to hard code a max value > >+ // (MAX_CONCURRENT_TAB_RESTORES) and start using a boolean pref restore_on_demand. > >+ if (this._prefBranch.prefHasUserValue("sessionstore.max_concurrent_tabs") && > >+ !this._prefBranch.prefHasUserValue("sessionstore.restore_on_demand")) { > >+ let maxConcurrentTabs = > >+ this._prefBranch.getIntPref("sessionstore.max_concurrent_tabs"); > >+ this._prefBranch.setBoolPref("sessionstore.restore_on_demand", maxConcurrentTabs == 0); > >+ this._prefBranch.clearUserPref("sessionstore.max_concurrent_tabs"); > >+ } > >+ }, > > This could be annoying for people going back and forward between builds > where this lands. I guess that isn't a huge deal, but you could also just > avoid clearing the user pref and use another pref to store whether the > migration occurred. True. I'm not too worried about those people (such is the life of a nightly tester), but I'll just add a migrated prefbranch instead of clearing. > >+ case "sessionstore.restore_on_demand": > > >+ // When turning restore on demand off, we should kick off restores to make > >+ // sure any remaining tabs get restored. > >+ if (!this._restoreOnDemand) > >+ for (let i = 0; i < MAX_CONCURRENT_TAB_RESTORES; this.restoreNextTab(), i++); > > Empty for loop with side effects is gross! :) Does this preference really > need to be this "live" (or live at all?). What are the odds that someone > will flip the pref in the middle of a restore? I guess it doesn't need to be live. It wasn't live in this way for max_concurrent_tabs but if's helpful for the "I just started Firefox and I'm about to get on a plane so I want to load all of my tabs" case. Though like I said the 0 => 3 case wasn't supported before either. > > restoreNextTab: function sss_restoreNextTab() { > > > // If it's not possible to restore anything, then just bail out. > >- if (this._maxConcurrentTabRestores >= 0 && > >- this._tabsRestoringCount >= this._maxConcurrentTabRestores) > >+ if (this._restoreOnDemand || > >+ this._tabsRestoringCount >= MAX_CONCURRENT_TAB_RESTORES) > > Doesn't this still need to be && rather than ||? No. The previous code was supporting the _maxConcurrentTabs == -1 case where that should never return early. Now _restoreOnDemand for sure overrides so that being true is enough to return early.

Updated per Gavin's comments. Also now includes changes to all tests that were using max_concurrent_tabs (Tim, can you make sure the change to the Panorama test is ok - it should be since those were defaults). Tree was mostly green (a couple leaks which didn't seem related): http://tbpl.mozilla.org/?tree=Try&rev=78106aa33d91

Comment on attachment 552773 [details] [diff] [review] Patch v0.3 Review of attachment 552773 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, and the language is clear and simple.

Comment on attachment 552773 [details] [diff] [review] Patch v0.3 >+ _migratePrefs: function sss__migratePrefs() { >+ // Added For Firefox 8 >+ // max_concurrent_tabs is going away. We're going to hard code a max value >+ // (MAX_CONCURRENT_TAB_RESTORES) and start using a boolean pref restore_on_demand. >+ if (!(this._prefBranch.prefHasUserValue("sessionstore.max_concurrent_tabs.migrated") && >+ this._prefBranch.getBoolPref("sessionstore.max_concurrent_tabs.migrated")) { Pretend there isn't a missing ) here so this actually runs.

Comment on attachment 552773 [details] [diff] [review] Patch v0.3 >+ _migratePrefs: function sss__migratePrefs() { >+ // Added For Firefox 8 >+ // max_concurrent_tabs is going away. We're going to hard code a max value >+ // (MAX_CONCURRENT_TAB_RESTORES) and start using a boolean pref restore_on_demand. >+ if (!(this._prefBranch.prefHasUserValue("sessionstore.max_concurrent_tabs.migrated") && >+ this._prefBranch.getBoolPref("sessionstore.max_concurrent_tabs.migrated")) { >+ // We'll default to MAX_CONCURRENT_TAB_RESTORES in case getting the old value throws >+ let maxConcurrentTabs = MAX_CONCURRENT_TAB_RESTORES; >+ try { >+ maxConcurrentTabs = this._prefBranch.getIntPref("sessionstore.max_concurrent_tabs"); >+ } >+ catch (ex) { } >+ this._prefBranch.setBoolPref("sessionstore.restore_on_demand", maxConcurrentTabs == 0); >+ this._prefBranch.setBoolPref("sessionstore.max_concurrent_tabs.migrated", true); You don't need to set sessionstore.max_concurrent_tabs.migrated for half a billion users. This is just junk that clutters up about:config and about:support unless you take extra steps to hide it (except that about:support seems to miss all sessionstore prefs -- that's probably a bug). At least, you can exclude those users who hadn't touched sessionstore.max_concurrent_tabs.

(In reply to Dão Gottwald [:dao] from comment #18) > You don't need to set sessionstore.max_concurrent_tabs.migrated for half a > billion users. This is just junk that clutters up about:config and > about:support unless you take extra steps to hide it (except that > about:support seems to miss all sessionstore prefs -- that's probably a > bug). At least, you can exclude those users who hadn't touched > sessionstore.max_concurrent_tabs. I agree that it's junk. I was really trying to do a 1 time migration but I haven't seen any standard for that. I would have been ok just killing it entirely. I've changed it again now to this which avoids the migrated pref but leads to possibly changing restore_on_demand multiple times... Thoughts? >if (!this._prefBranch.prefHasUserValue("sessionstore.restore_on_demand")) { > // We'll default to MAX_CONCURRENT_TAB_RESTORES in case getting the old value throws > let maxConcurrentTabs = MAX_CONCURRENT_TAB_RESTORES; > try { > maxConcurrentTabs = this._prefBranch.getIntPref("sessionstore.max_concurrent_tabs"); > } > catch (ex) { } > this._prefBranch.setBoolPref("sessionstore.restore_on_demand", maxConcurrentTabs == 0); >}

Comment on attachment 552773 [details] [diff] [review] Patch v0.3 Review of attachment 552773 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the switch back to delete-old-pref approach, unless y'all think that sounds too unfriendly to upgrade->downgrade->upgrade users. ::: browser/app/profile/firefox.js @@ +779,5 @@ > // (this pref has no effect if more than 6 hours have passed since the last crash) > pref("browser.sessionstore.max_resumed_crashes", 1); > +// restore_on_demand overrides MAX_CONCURRENT_TAB_RESTORES (sessionstore constant) > +// and restore_hidden_tabs. When true, tabs will not be restored until they are > +// focused and tabs that aren't visible won't be restored. When false, the values "tabs that aren't visible won't be restored" - not restored at all or not restored until focused? ::: browser/components/sessionstore/src/nsSessionStore.js @@ +460,5 @@ > + } > + catch (ex) { } > + this._prefBranch.setBoolPref("sessionstore.restore_on_demand", maxConcurrentTabs == 0); > + this._prefBranch.setBoolPref("sessionstore.max_concurrent_tabs.migrated", true); > + } i think delete the old pref sounds better. it'll reset to default if they downgrade, but that's the only downside. and we're not stuck with one-off migration prefs *forever*.

Back to deleting the old pref. Also adjusted the comment to make that more clear.

Comment on attachment 553368 [details] [diff] [review] Patch v0.4 The prefpane changes look fine to me (but use != instead of !(==) in startupPagePrefChanged).

(In reply to Gavin Sharp from comment #22) > Comment on attachment 553368 [details] [diff] [review] > Patch v0.4 > > The prefpane changes look fine to me (but use != instead of !(==) in > startupPagePrefChanged). I'm not sure why I would ever write that... but fixed. http://hg.mozilla.org/integration/mozilla-inbound/rev/32d45b1d060d

"d" looks like a bad choice for a shortcut in the Main panel ("t" should be fine) <!ENTITY showWhenDownloading.accesskey "D"> <!ENTITY restoreOnDemand.accesskey "d"> Personally I found this string quite focusing, I would have found easier to understand the opposite pref (e.g. "Load automatically only the active tab"), but that's probably because I didn't know about browser.sessionstore.max_concurrent_tabs

I get consistent hangs with this build (8.0a1 (2011-08-15)) on one of my machines with a lot of tabs (100+). Build 2011-08-14 was ok. There are two points of hanging: - lookup or loading a page - change the group in panorama (which is probably related to point one) Nightly freezes with hogging exactly one core. Hangs in safe mode and with new profile. Seems to work with a heavily reduced number of tabs (about 50%).

(In reply to Volkmar Kostka from comment #26) > I get consistent hangs with this build (8.0a1 (2011-08-15)) Your build doesn't have this patch.

(In reply to flod (Francesco Lodolo) from comment #25) > "d" looks like a bad choice for a shortcut in the Main panel ("t" should be > fine) Quite right. I changed it to "l", since "t" would underline the t in "Don't". http://hg.mozilla.org/mozilla-central/rev/2e4ddf750415

Where is this UI in Firefox?

(In reply to Asa Dotzler [:asa] from comment #29) > Where is this UI in Firefox? Options/Preferences > General > Startup

(In reply to Dão Gottwald [:dao] from comment #30) > (In reply to Asa Dotzler [:asa] from comment #29) > > Where is this UI in Firefox? > > Options/Preferences > General > Startup Thanks, Dão.

It might be nice if app tabs actually loaded as per normal, since they are often expected to do things like provide calendar alerts or email notifications.

(In reply to Brian Crowder from comment #32) > It might be nice if app tabs actually loaded as per normal, since they are > often expected to do things like provide calendar alerts or email > notifications. Yea, I wanted to have that in before this landed, but it didn't make it. I'm working on it now. See bug 674452.

I had browser.sessionstore.max_concurrent_tabs set to 1 :( I liked that value more than 3 because it gave me greater reponsiveness for loading new tabs that I opened manually, while still loading all the saved tabs eventually.

(In reply to Nicholas Nethercote [:njn] from comment #34) > I had browser.sessionstore.max_concurrent_tabs set to 1 :( I liked that > value more than 3 because it gave me greater reponsiveness for loading new > tabs that I opened manually, while still loading all the saved tabs > eventually. +1. This is like a regression, a feature working since Firefox 4 now compromised.

(In reply to Dão Gottwald [:dao] from comment #6) > What other dial positions do we care about at this point? > > "[ ] Don’t load tabs until selected" could of course set max_concurrent_tabs > to 0 and 3, respectively, but this seems wrong if we assume that someone > would want to set it to something other than 0 or 3. I need this feature to be set to "unlimited". Why was the seemingly-arbitrary "3" selected? I currently have browser.sessionstore.max_concurrent_tabs set to "50". For my research I will often have over 30 tabs open. My computer has enough memory and processing power for this, and Tree Style Tabs with the tabs on the left make it easy to work with. I will occasionally need to shut down Firefox or the computer for whatever reason, and I have no problem waiting 10 or 60 seconds while the tabs load at the beginning. But having to wait for them to load while I am in the middle of work is ridiculous. Please have the setting "0" or "unlimited", do not set it to merely "3". Thanks.

Tab contents don't get updated after deferred reload. Steps to reproduce (Firefox 8.0a2): 1. Enable 'Don't load tabs until selected' 2. Go to any site X with frequently updated content or some load-time timestamp (e.g. www.rbc.ru which has a client-side live clock widget.) 3. Create a new tab and switch to it. 4. Reload Firefox. The last newly created tab is loaded. 5. Optional - wait a little to be sure site X was updated indeed. 6. Switch back to tab with site X. Content has not changed! This problem was present with browser.sessionstore.max_concurrent_tabs=0 too, but I didn't post a bug since that pref was of dubious status. It's really annoying in use and also inconsistent with the default regular all-tab-reloading behaviour of Firefox. Maybe I should post a new bug for this?

(In reply to Dimitri Saltanov from comment #37) I like and want the behaviour you find annoying. The session restore feature is named session restore, no ? It is not named session refresh. I want the session restore feature to restore the session *as it was*. So, as much as possible, with the same content, not refreshed. Then, if I want to refresh, I click Refresh. Often, the session restore feature comes in as a nice workaround of computer mishaps. Like quitting to free memory, or to update Firefox, or to install a plugin, or do maintenance of the computer - reboot, various cleaning or repairing operations, various software updates, hardware part change... -. Like crash of Firefox. Like crash of the computer. In these cases, I want to get my session restored *as it was*, as if the mishap never happened. See in my request https://bugzilla.mozilla.org/show_bug.cgi?id=680605#c0 the problem I encountered with form data. The new form data saving feature is great, but refreshing broke it. The session restore feature comes in after a clean quitting, which comes more or less from my will, or after a crash, which definitely does not come from my will. Let's suppose I am filling in a big paper form. Time to go to bed, so I fold the papers and I put them in my drawer. Tomorrow, I take the paper form, and I find it exactly as when I had stopped. The session restore feature achieves this for a Web form. If there is no refreshing, of course. If a refreshing comes in, this is broken and I get an empty form instead - too bad ! -.

(In reply to Dimitri Saltanov from comment #37) > Tab contents don't get updated after deferred reload. This is actually how session restore has always worked. We will load from the cache if available (I believe the webpage can set headers to not allow caching or set cache expiry and we will respect that). Regardless, nothing about restoring on demand has changed that behavior.

Verified the new pref on all OSs on Firefox 8 beta 6 (build1): - the new interface exists in the proper place in the Preferences/Options window - option is accessible from the keyboard - that the preference sets the proper behavior when on and off Marking as VERIFIED.