Requesting ui-review too from Ian Neal, as his on mac as i remember. From parent bug: 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.

Comment on attachment 601204 [details] [diff] [review] patch v1 >-pref("browser.sessionstore.max_concurrent_tabs", 3); I'm not convinced that we should remove this pref. >+ <hbox align="center"> [Don't think this is necessary...] >+ <checkbox id="restoreOnDemand" >+ label="&restoreOnDemand.label;" >+ accesskey="&restoreOnDemand.accesskey;" >+ class="indent" >+ preference="browser.sessionstore.restore_on_demand"/> This pref affects undo close window as well, does it not? In which case I feel it should have its own section. >+<!ENTITY restoreOnDemand.label "Donât load tabs until selected"> Nit: no smart quotes please.

>>-pref("browser.sessionstore.max_concurrent_tabs", 3); > I'm not convinced that we should remove this pref. Me neither. For one thing it would upset therube ;)

(In reply to neil@parkwaycc.co.uk from comment #1) > >-pref("browser.sessionstore.max_concurrent_tabs", 3); > I'm not convinced that we should remove this pref. Looks reasonable, shouldn't we expose it in UI too ?

Ignore previous comment, I'm just stupid.

Comment on attachment 601547 [details] [diff] [review] patch v2 (includes Neil's mockup and keeps old pref too) >+++ b/suite/common/pref/pref-navigator.xul > <preferences id="navigator_preferences"> > <preference id="browser.startup.page" > name="browser.startup.page" > type="int"/> >+ <preference id="browser.sessionstore.restore_on_demand" >+ name="browser.sessionstore.restore_on_demand" >+ type="bool"/> This isn't actually used in the pref window, whereas browser.sessionstore.max_concurrent_tabs is. >+ <!-- session restore background tabs --> >+ <groupbox> >+ <caption label="When restoring sessions and windows"/> >+ <radiogroup align="start" >+ preference="browser.sessionstore.max_concurrent_tabs"> >+ <radio value="-1" label="Restore all tabs immediately"/> >+ <hbox align="center"> >+ <radio selected="true" label="Restore"/> >+ <textbox type="number" size="2" max="99" min="-1" value="3" >+ preference="browser.sessionstore.max_concurrent_tabs"/> >+ <label value="tabs at a time"/> >+ </hbox> >+ <radio value="0" label="Only restore tabs when I need them"/> >+ </radiogroup> >+ </groupbox> Shouldn't there be some code that updates "browser.sessionstore.restore_on_demand" when this is set to "0" or not? >+++ b/suite/locales/en-US/chrome/common/pref/pref-navigator.dtd >+<!ENTITY restoreOnDemand.label "Do not load tabs until selected"> >+<!ENTITY restoreOnDemand.accesskey "d"> Nit: this should be "D"

Comment on attachment 601244 [details] [diff] [review] Mockup Seems fine but see comments on the other patch.

Comment on attachment 601547 [details] [diff] [review] patch v2 (includes Neil's mockup and keeps old pref too) When changing the value in the second option, dropping it to 0 correctly selected the 3rd option and to -1 to the 1st option, but when increasing to above 0 the 2nd option doesn't get selected.

Also fixes issues with the wrong radiobutton getting selected, also focuses the input as necessary, also with l10n and accesskeys. I had to remove the home page groupbox to get everything to fit though.

Comment on attachment 602775 [details] [diff] [review] Now with string changes The only problem with this is that we're not even adding the new preference, but we're adding the equivalent functionality, so r=me unless you can think of a way of doing both.

If I get everything right ...

Comment on attachment 604852 [details] [diff] [review] patch with Neil UI part >+pref("browser.sessionstore.restore_on_demand", true); Sorry, but I don't want this to default to true. >+ <caption label="&restoreSessionIntro.label;"/> My patch has string changes so it's best landed separately, which means that we can land your changes on branches if we need to. >diff --git a/suite/common/tests/browser/browser_586068-cascaded_restore.js b/suite/common/tests/browser/browser_586068-cascaded_restore.js > // Reset the pref > try { > Services.prefs.clearUserPref("browser.sessionstore.max_concurrent_tabs"); >+ Services.prefs.clearUserPref("browser.sessionstore.restore_on_demand"); > } catch (e) {} Some questions: As you never set max_concurrent_tabs, why do you reset it? With these changes, do we still end up testing an automatic restore? Did the existing tests test different numbers of concurrent tabs? If so, is there any way of keeping that test? >+// Some tests here assume that all restored tabs are loaded without waiting for >+// the user to bring them to the foreground. We ensure this by resetting the >+// related preference (see the "browser-prefs.js" defaults file for details). Well, in the light of my first comment, this is no longer true ;-)

(In reply to neil@parkwaycc.co.uk from comment #13) > >+pref("browser.sessionstore.restore_on_demand", true); > Sorry, but I don't want this to default to true. > Please reconsider, it greatly reduces memory usage and startup times. Will be big win for ordinary users. > >+ <caption label="&restoreSessionIntro.label;"/> > My patch has string changes so it's best landed separately, which means that > we can land your changes on branches if we need to. > OK, I'll make another patch complementing Your's. > >diff --git a/suite/common/tests/browser/browser_586068-cascaded_restore.js b/suite/common/tests/browser/browser_586068-cascaded_restore.js > > // Reset the pref > > try { > > Services.prefs.clearUserPref("browser.sessionstore.max_concurrent_tabs"); > >+ Services.prefs.clearUserPref("browser.sessionstore.restore_on_demand"); > > } catch (e) {} > Some questions: > As you never set max_concurrent_tabs, why do you reset it? > With these changes, do we still end up testing an automatic restore? > Did the existing tests test different numbers of concurrent tabs? > If so, is there any way of keeping that test? > As I understood, new numbers just reflect new defaults, no functionality loss in tests. > >+// Some tests here assume that all restored tabs are loaded without waiting for > >+// the user to bring them to the foreground. We ensure this by resetting the > >+// related preference (see the "browser-prefs.js" defaults file for details). > Well, in the light of my first comment, this is no longer true ;-) Please reconsider :)

Comment on attachment 602775 [details] [diff] [review] Now with string changes Pushed changeset 4c05177d7f2a to comm-central.

Comment on attachment 602775 [details] [diff] [review] Now with string changes Review of attachment 602775 [details] [diff] [review]: ----------------------------------------------------------------- Note: Irrespective of the default, I think the pref should by synced. Since I think you'll need to push a fix for my nit anyway, please add the following to browser-prefs.js while you're there (rs=me as Sync owner): pref("services.sync.prefs.sync.browser.sessionstore.max_concurrent_tabs", true); Thanks. ::: suite/locales/en-US/chrome/common/pref/pref-navigator.dtd @@ +23,5 @@ > + using (restoreTabs.label) and a number (restoreTabsAtATime.label). --> > +<!ENTITY restoreTabs.label "Restore"> > +<!ENTITY restoreTabs.accesskey "s"> > +<!ENTITY restoreTabsAtATime.label "tab(s) at a time"> > +<!ENTITY restoreDeferred.label "Only restore tabs when I need then"> ITYM s/then/them/ (if yes, fix before uplift, if not, please explain!).

BTW, with this change, the bottom of the Browser pane is now cut off by default on Windows 7 (only the top pixels of the the lowest button, "Restore Default", are visible). :-( Maybe it would have been better to align the four lower buttons horizontally and let the Home pages text box adjust its height accordingly using a flex.

More issues with what landed: 1. If I select the third option, close Preferences and reopen it, the second option is selected (with value 0). [The first option however sticks.] I could imagine there's a problem with properly detecting 0 vs "0" (didn't actually check, though). 2. I don't see which effect option 3 has anyway. There are some scenarios where the previous session is not restored (e.g. with default settings, choosing Quit instead of Save and Quit), but in those, the same is true for option 1 or 2 (with value != 0). OTOH, if the previous session is saved (Save and Quit), it is restored upon next startup even with option 3. :-/ I would understand that behavior for Restore Previous Session (Display on / Browser Startup), but not so much for the rest. [We need to understand the logic here before we can finish Help bug 734994.]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #16) > pref("services.sync.prefs.sync.browser.sessionstore.max_concurrent_tabs", > true); > (...) > > +<!ENTITY restoreDeferred.label "Only restore tabs when I need then"> > > ITYM s/then/them/ (if yes, fix before uplift, if not, please explain!). Got r=Neil over IRC for both: http://hg.mozilla.org/comm-central/rev/1c3e03827e86

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #17) > BTW, with this change, the bottom of the Browser pane is now cut off by > default on Windows 7 (only the top pixels of the the lowest button, "Restore > Default", are visible). :-( Sorry, I only tried this in Modern and Windows Classic. (In reply to Jens Hatlak (:InvisibleSmiley) from comment #18) > 1. If I select the third option, close Preferences and reopen it, the second > option is selected (with value 0). [The first option however sticks.] Oops, I thought I'd checked that :-( > 2. I don't see which effect option 3 has anyway. Option 1 restores all the tabs immediately. Option 2 starts restoring N tabs at a time in the background. Option 3 only restores a tab if you switch to it or reload it.

>>>+pref("browser.sessionstore.restore_on_demand", true); >> Sorry, but I don't want this to default to true. I agree with Neil, this is nice to have but shouldn't be turned on by default. Changing summary to fit.

(In reply to neil@parkwaycc.co.uk from comment #15) > Comment on attachment 602775 [details] [diff] [review] > Now with string changes > > Pushed changeset 4c05177d7f2a to comm-central. ITYM pushed comm-central changeset 4c05177d7f2a http://hg.mozilla.org/comm-central/rev/4c05177d7f2a

...shouldn't this bug be "RESOLVED FIXED"?

Yes, the pref is working. If I miss something, please feel free to undo it.

Comment on attachment 606157 [details] [diff] [review] backend only (In reply to OstGote! from comment #25) > Yes, the pref is working. If I miss something, please feel free to undo it. Err, we have a patch with a pending review here - what's the story on that?

The patch that landed dealt with "browser.sessionstore.max_concurrent_tabs" Misak's patch in attachment 606157 [details] [diff] [review] deals with "browser.sessionstore.restore_on_demand" is waiting on r? I think all the remaining issues should be moved to a new bug. 1. Misak wants the default setting to be true, Neil wants it to be false. 2. There are some unanswered questions in Comment 13. 3. The tests are probably bit-rotted. Going forward, someone should rebase the patch on comm-central tip, flip the pref to false and submit it to Neil for review. To avoid answering the questions on the tests, you could split the tests off into another patch.

I'm not actually sure what this new preference gains us, now that we have the "Only restore tabs when I need them" option in Preferences.