Bug 674452 generated a lot of feedback from users about the behavior introduced there. Personally I think it's the right thing to do, but it clearly doesn't fit everyone's expectations. I propose to add a boolean pref (browser.sessionstore.restore_pinned_tabs?), default to true, that users can reset if they don't like the behavior introduced in bug 674452. Paul: what's your opinion on this? Daniel/Ahmed/Robert: does this fit your needs? Should I CC someone from UX?

This is definitely great and neat, and I appreciate your kind proposal =)

Depending on how users would know about it, I think it's a positive step. On a related note: It's been interesting how my browsing experience has evolved since "restore on demand" has become an option. I now habitually use extra tabs as sort of a "temporary bookmark" function in which I feel free to procrastinate (in a good way) getting back to tabs at my convenience over a day or even a week. Relaunching FF without reloading allows this beautifully -- making for a surprisingly pleasant new browsing style I wasn't expecting. Pinned tabs are essential to making it work though, so that it's always easy to find email, facebook, calendar, etc. Restoring all pinned tabs automatically, however, has caused me to limit my pinning of tabs from a peak of 5 with FF8 down to 2 tabs with FF9 (which still bogs down a bit). Keep in mind that I always use an older "test laptop" for regular browsing in order to match typical user rigs. In any case, my browsing style evolved automatically in ways I wasn't expecting once I discovered these options, and I think there's a marketing opportunity for FF if it can be tapped. Bookmark-bloat is such a common problem, and using tabs this way to get back to things more casually -- but without forgetting them -- it really allows me to be more relaxed while browsing, and more selective about what I actually end up bookmarking. This deserves to be spun in marketing.

Reuben: Your proposed preference would be great so I can turn it to false on myside.

Concur with Daniel. There's also bug 688962, where users are discussing wording in Preferences UI about whether it should reflect new behaviuor in 9.0.1, because current wording means _all_ tabs, and this includes pinned tabs, too. This is rather confusing. The behaviour in Firefox 8.0 is basically a killer feature compared to what Google Chrome does right now. So I'd like a separate UI feature or at least an about:config setting to set whether pinned tabs should or should not be restored alongside usual tabs. Separately, users should in the future be able to set which pinned tabs they want to be restored automatically when other tabs are not restored.

This is great news, 10x

My wishful thinking has it that I'd like to see it very soon at 9.0.2 (or 9.1).

this is great news.

While this is a very positive, I still believe a major opportunity is being squandered by keeping from the UI. By adding a single additional checkbox to the options menu -- to strategically and DELIBERATELY draw attention to app tabs -- this feature could be adopted by a much wider swath of users as a potent asset. As I noted in a post above, pinned tabs revolutionized my browsing habits in a very unexpected way. Since then I have shared this feature with numerous people and in every case their experiences have been similar and lasting. It's sticky. It's powerful. It's liberating. In my opinion it's time for pinned tabs to arise from obscurity to the forefront. The only problem is that loading them automatically all at once severely hampers their usefulness. Make them flexible so you can really showcase them in marketing, and you'll add a revolutionary step to the way many millions of people browse.

(In reply to Daniel from comment #11) I intend to file a follow up bug to add the option to Preferences UI. Meanwhile, let's focus on getting this landed. dev-apps-firefox [1] is a better place for discussing features, and will get the proper attention. Please create a topic there if you want to discuss this behavior.

(In reply to Reuben Morais [:reuben] from comment #12) > dev-apps-firefox [1] [1] https://lists.mozilla.org/listinfo/dev-apps-firefox Sorry for the noise.

removing all my pinned tabs since all of them are loaded on startup and they take a lot of time to load especially when i only have 64-128Kbps -sometimes burst to 256Kbps- bandwidth. hope indonesia will get better internet speed and bandwidth next year. *finger crossed* thanks to firefox 9 for this new pinned tabs behavior, now am trying to get used to chrome. they have these good things of apps such as offline gmail and tweetdeck apps. with those two apps (two offline gmail apps and one tweetdeck apps for facebook and twitter pinned), chrome needs lesser time and bandwidth to open all of them on startup compared to firefox. if there will be no light on getting back the firefox 8 pinned tabs behavior on future firefox release, i hope there will be apps like offline gmail and tweetdeck on firefox in the near future. keep on the good work firefox! merry xmas to you all..:)

Thank you for the patch.

thank you! :)

Is there a way, we as end users can apply this patch directly to Firefox 9/10? Or do we need to wait for a new Firefox build? If so, any expectations when can we see that build?

(In reply to Ahmed from comment #17) > Is there a way, we as end users can apply this patch directly to Firefox > 9/10? Or do we need to wait for a new Firefox build? If so, any expectations > when can we see that build? Unless you, as end users, have the necessary tools, skills and time to download the Firefox 9/10 source, backport the patch and build it again, no. :-) I'm sure it won't take long for the patch to go through the review cycle and be checked-in, though. If it wasn't for the holidays that would probably be happening already.

Comment on attachment 584121 [details] [diff] [review] Patch Review of attachment 584121 [details] [diff] [review]: ----------------------------------------------------------------- I'm not wild about this change but it seems there are a handful of people who want it (badly enough to take complaining to Wikipedia :/ ). ::: browser/app/profile/firefox.js @@ +798,5 @@ > pref("browser.sessionstore.restore_on_demand", false); > // Whether to automatically restore hidden tabs (i.e., tabs in other tab groups) or not > pref("browser.sessionstore.restore_hidden_tabs", false); > +// Whether to automatically restore pinned tabs or not > +pref("browser.sessionstore.restore_pinned_tabs", true); 2 things here. First, my concern with this pref is that the override actually works in the opposite direction. restore_hidden_tabs is only looked at when restore_on_demand is false. restore_pinned_tabs would only be looked at when restore_on_demand is true. It makes codifying that a bit messy. I guess it's livable though. Secondly, make sure you detail the behavior (overrides included) above. ::: browser/components/sessionstore/src/nsSessionStore.js @@ +3069,5 @@ > this.restoreTab(tab); > } > else { > // Put the tab into the right bucket > + if (tabData.pinned && this._restorePinnedTabs) This shouldn't be done here. The logic in restoreNextTab will need to be fixed to handle the new pref. ::: browser/components/sessionstore/test/browser_586068-cascaded_restore.js @@ +56,5 @@ > test_setWindowStateNoOverwrite, test_setWindowStateOverwrite, > test_setBrowserStateInterrupted, test_reload, > /* test_reloadReload, */ test_reloadCascadeSetup, > + /* test_reloadCascade, */ test_apptabs_only, > + test_restore_pinned_tabs]; Nit: Please call the test something about "apptabs" (to mirror test_apptabs_only). And the test is actually supposed to be testing that we didn't restore pinned tabs. @@ +780,5 @@ > > +// This test ensures that app tabs are not restored when restore_pinned_tabs is false > +function test_restore_pinned_tabs() { > + Services.prefs.setBoolPref("browser.sessionstore.restore_on_demand", true); > + Services.prefs.setBoolPref("browser.sessionstore.restore_pinned_tabs", false); Please reset this pref in runNextTest, not in this test. @@ +817,5 @@ > + tab = window.gBrowser.tabs[i]; > + } > + > + ok(gBrowser.selectedTab == tab, > + "test_restore_pinned_tabs: load came from pinned or hidden tab"); I don't think it did come from a pinned or hidden tab. @@ +821,5 @@ > + "test_restore_pinned_tabs: load came from pinned or hidden tab"); > + > + // We should get only 1 load: the selected tab > + if (loadCount < 1) > + return; loadCount can never be < 1 here. But more importantly, this test will just pass after the first load, which doesn't really test what you want it to. This should pass even with current behavior (selected tab would be restored first). I think a way to make this better would be to something like... > if loadCount < 2 > set timeout to remove progress listener & run next test > if loadCount > 1 > delete the timeout that way if we get more loads, the test fails due to timing out. (I realize we should probably structure some other tests like this, at least test_apptabs_only. The power of hindsight)

(In reply to Paul O'Shannessy [:zpao] from comment #19) Thanks for the review! > (I realize we should probably structure some other tests like this, at least > test_apptabs_only. The power of hindsight) Not bad for the first test I copie… er, wrote! I'll spend a little more time on it this time :-)

Meanwhile, it appears that this functionality can be replicated with an extension called UnloadTab, which adds some additional features as well -- assuming that this add-on is truly reliable and doesn't generate any unwanted effects. https://addons.mozilla.org/en-US/firefox/addon/unloadtab/?src=api Given the rapid evolution of FF, I suspect that it won't manage memory as optimally as a native feature. Reuben and Paul, for users wanting a quick temporary fix, do you think this extension is worth bothering with?

@Daniel, et al: I vote for native functionality, as it was its removal in Firefox 9.0, which broke behaviour first established in Firefox 4.0 with an about:config option of browser.sessionstore.max_concurrent_tabs. A UI option in the form of an additional checkbox would be very forthcoming to people with limited bandwidth. (Until then) What could be the projected target milestone?

Ok, I just tried the UnloadTab extension. While it seems competent at managing the loading of pinned tabs, it introduces buggy behavior when clicking links to spawn multiple additional tabs from the same parent tab. I suspect other bugs may also exist. Probably best to continue pursuing a native solution regardless.

This version uses the set-timeout-to-run-next-test approach, and it won't even timeout, since the next (bogus) load would fail in the tab == selectedtab check. For some reason I had a lot of fun fixing this test (maybe that's just the usual feeling when it works :-), thanks again for the review! (In reply to Mart Rootamm from comment #22) > (Until then) What could be the projected target milestone? Firefox 12, unless something goes terribly wrong.

Comment on attachment 584938 [details] [diff] [review] Patch v2 >+pref("browser.sessionstore.restore_pinned_tabs", true); Can you rename this to something more sane, like force_restore_pinned_tabs or restore_pinned_tabs_on_demand (default false)?

(In reply to Dão Gottwald [:dao] from comment #25) > Can you rename this to something more sane, like force_restore_pinned_tabs > or restore_pinned_tabs_on_demand (default false)? restore_pinned_tabs_on_demand sounds good.

Comment on attachment 584959 [details] [diff] [review] Patch v3 Review of attachment 584959 [details] [diff] [review]: ----------------------------------------------------------------- r- (but only because I'm trying to do fewer "r+ with things followup on"). Just about there! ::: browser/components/sessionstore/src/nsSessionStore.js @@ +315,5 @@ > this._prefBranch.addObserver("sessionstore.restore_hidden_tabs", this, true); > + > + this._restorePinnedTabs = > + this._prefBranch.getBoolPref("sessionstore.restore_pinned_tabs_on_demand"); > + this._prefBranch.addObserver("sessionstore.restore_pinned_tabs_on_demand", this, true); This is now named poorly (when the pref is false, restorePinnedTabs is false which means we should restore pinned tabs... confusing!) Also, I missed this last time around, but please add the property to the prototype with a default of null, like _restoreHiddenTabs. @@ +3202,5 @@ > + // If restore_on_demand and restore_pinned_tabs_on_demand are set, > + // there's nothing else to restore. > + if (this._restoreOnDemand && this._restorePinnedTabs) > + return; > + This falls under the previous block's "not possible to restore anything", so if you want to combine the 2 blocks & keep it readable, then I'm game. It might help to clear up the rules of precedence (restoreOnDemand && restorePinnedTabsOnDemand overrides the priority.length check) ::: browser/components/sessionstore/test/browser_586068-cascaded_restore.js @@ +779,5 @@ > } > > > +// This test ensures that app tabs are not restored when restore_pinned_tabs_on_demand is set > +function test_restore_apptabs() { Nit: Let's go with test_restore_apptabs_ondemand @@ +818,5 @@ > + } > + > + // Check that the load only comes from the selected tab. > + ok(gBrowser.selectedTab == tab, > + "test_restore_apptabs: load came from pinned, hidden or visible tab"); Well, that's not true either... the success case is that load came from the selected tab. Also, small nit: you have an extra space at the beginning of that line (" should align with the g)

(In reply to Reuben Morais [:reuben] from comment #24) > For some reason I had a lot of fun fixing this test (maybe that's just the > usual feeling when it works :-), thanks again for the review! If you wanted to file a bug & write a patch to cleanup that test, I wouldn't say no! ;)

(In reply to Paul O'Shannessy [:zpao] from comment #27) > @@ +3202,5 @@ > > + // If restore_on_demand and restore_pinned_tabs_on_demand are set, > > + // there's nothing else to restore. > > + if (this._restoreOnDemand && this._restorePinnedTabs) > > + return; > > + > > This falls under the previous block's "not possible to restore anything", so > if you want to combine the 2 blocks & keep it readable, then I'm game. It > might help to clear up the rules of precedence (restoreOnDemand && > restorePinnedTabsOnDemand overrides the priority.length check) Making long conditions readable is a task I'm afraid I'll never learn how to do, but I'll give it a try. > @@ +818,5 @@ > > + } > > + > > + // Check that the load only comes from the selected tab. > > + ok(gBrowser.selectedTab == tab, > > + "test_restore_apptabs: load came from pinned, hidden or visible tab"); > > Well, that's not true either... the success case is that load came from the > selected tab. > Hm, this is rather counter-intuitive, the message is shown in both cases… I guess it's just more visible in the failure case, which is what made me change it. Would "load should only come from selected tab" be fine? That way it's valid in both the failure and pass cases.

Is there a try-server or tinderbox build with patch v4 available for testing?

(In reply to Reuben Morais [:reuben] from comment #29) > Hm, this is rather counter-intuitive, the message is shown in both cases… > I guess it's just more visible in the failure case, which is what made me > change it. The messages there are always meant to be a quick way of expressing the success condition. In the logs you see TEST-PASS: message or TEST-FAIL: message. is() is a bit more obvious than ok() in that the FAIL message says, "got A, expecting B". I hope that clears it up and you keep writing tests :)

Comment on attachment 587574 [details] [diff] [review] Patch v4 Review of attachment 587574 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay in the review! r+ with that nit fixed & a try server run just to make sure it goes green. ::: browser/components/sessionstore/src/nsSessionStore.js @@ +3203,1 @@ > this._tabsRestoringCount >= MAX_CONCURRENT_TAB_RESTORES) Nit: you lost a paren set. Order of operations makes that OK I think, but I'd rather be explicit - (A && (B || C)) || D Also, do I spy tab characters. Let's not do that :)

Carrying r=zpao. Sent to try at https://tbpl.mozilla.org/?tree=Try&rev=9b3e7f28948b

Just fixing the message in the test from "test_restore_apptabs:" to "test_restore_apptabs_ondemand:".

Try run for 9b3e7f28948b is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=9b3e7f28948b Results (out of 29 total builds): success: 23 warnings: 6 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/reuben.morais@gmail.com-9b3e7f28948b Timed out after 06 hours without completing.

Results appear to be fine, the warnings were all from unrelated random oranges.

Fockyeah, common sense wins against Asa again.

@Sean: seconded here. =) I have an idea. Why not take this one step further and let people choose individual App Tabs to load on startup or load on demand? Something like how Tab Mix Plus allows individual tabs to have reload timers? This finer control ought to kill even more of us (because killer feature).

(In reply to Shaun from comment #41) > I have an idea. Why not take this one step further and let people choose > individual App Tabs to load on startup or load on demand? Something like how > Tab Mix Plus allows individual tabs to have reload timers? This finer > control ought to kill even more of us (because killer feature). I guess we should leave that to addons, and not make things more complicated for naive users.

Concur with Shaun's idea in Comment 41. In a way, I had similar thoughts, given that if all this is planned for Firefox 12 anyway, which seems like far-far away in the first place for functionality that was very unduly removed in 9.0 (making this version less appealing in terms of its touted least resource usage), but then thought that if more functionality were added, it wouldn't perhaps make it into 12 on time. The idea was that pinned tabs should be categorized separatey as pinned tabs and app tabs, such as with tab shortcut commands titled "Pin tab" and "Pin as App tab", because sometimes I want to browse within pinned tabs and I hate it when a link unexpectedly opens in a new tab. That way, a normally pinned tab would just be a pinned tab, while a pinned app tab would work as it does in 9.0 (would also load automatically). Separately, if a tab is pinned, a user could choose to make it an app tab ("Set as app tab") or remove app tab status ("Set as regular tab"), or with a checkbox-like command, with a checkbox next to "App tab" command if it's an app tab and one missing when it's not an app tab. @Dugar: Addons aren't always that stable and some environments, like workplaces, don't allow installing add-ons.

Verified Fixed on FF 19b1 and on Latest Nightly (2013/01/08)