Last Comment Bug 708585 - Add preference to control how app tabs are loaded when restore_on_demand is set.
: Add preference to control how app tabs are loaded when restore_on_demand is set.
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal with 8 votes (vote)
: Firefox 12
Assigned To: Reuben Morais [:reuben]
:
: Mike de Boer [:mikedeboer]
Mentors:
: 696568 713190 718677 (view as bug list)
Depends on:
Blocks: 752476
  Show dependency treegraph
 
Reported: 2011-12-08 04:30 PST by Reuben Morais [:reuben]
Modified: 2013-01-10 06:27 PST (History)
24 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (7.77 KB, patch)
2011-12-23 13:50 PST, Reuben Morais [:reuben]
paul: review-
Details | Diff | Splinter Review
Patch v2 (8.52 KB, patch)
2011-12-30 03:19 PST, Reuben Morais [:reuben]
no flags Details | Diff | Splinter Review
Patch v3 (8.69 KB, patch)
2011-12-30 06:39 PST, Reuben Morais [:reuben]
paul: review-
Details | Diff | Splinter Review
Patch v4 (9.51 KB, patch)
2012-01-10 19:16 PST, Reuben Morais [:reuben]
paul: review+
Details | Diff | Splinter Review
Patch v5 (9.51 KB, patch)
2012-01-19 14:16 PST, Reuben Morais [:reuben]
reuben.bmo: review+
Details | Diff | Splinter Review
Patch v6 (9.52 KB, patch)
2012-01-19 19:20 PST, Reuben Morais [:reuben]
reuben.bmo: review+
Details | Diff | Splinter Review

Description Reuben Morais [:reuben] 2011-12-08 04:30:48 PST
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?
Comment 1 Robert Ciang 2011-12-08 06:26:09 PST
This is definitely great and neat, and I appreciate your kind proposal =)
Comment 2 Daniel 2011-12-08 12:54:54 PST
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.
Comment 3 Ahmed 2011-12-08 19:30:11 PST
Reuben: Your proposed preference would be great so I can turn it to false on myside.
Comment 4 Mart Rootamm 2011-12-21 23:12:37 PST
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.
Comment 5 Alexander L. Slovesnik 2011-12-22 02:26:06 PST
*** Bug 696568 has been marked as a duplicate of this bug. ***
Comment 6 Reuben Morais [:reuben] 2011-12-23 13:50:35 PST
Created attachment 584121 [details] [diff] [review]
Patch
Comment 7 np2k8 2011-12-23 14:41:18 PST
This is great news, 10x
Comment 8 Mart Rootamm 2011-12-23 16:00:58 PST
My wishful thinking has it that I'd like to see it very soon at 9.0.2 (or 9.1).
Comment 9 chrone 2011-12-23 16:55:04 PST
this is great news.
Comment 10 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-12-23 16:57:24 PST
*** Bug 713190 has been marked as a duplicate of this bug. ***
Comment 11 Daniel 2011-12-23 17:51:25 PST
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.
Comment 12 Reuben Morais [:reuben] 2011-12-23 19:31:05 PST
(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.
Comment 13 Reuben Morais [:reuben] 2011-12-23 19:31:45 PST
(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.
Comment 14 chrone 2011-12-25 03:12:15 PST
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..:)
Comment 15 Constantine 2011-12-28 07:25:42 PST
Thank you for the patch.
Comment 16 chrone 2011-12-28 09:44:55 PST
thank you! :)
Comment 17 Ahmed 2011-12-28 10:06:31 PST
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?
Comment 18 Reuben Morais [:reuben] 2011-12-28 10:39:31 PST
(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 19 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-12-28 13:50:30 PST
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)
Comment 20 Reuben Morais [:reuben] 2011-12-28 14:00:08 PST
(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 :-)
Comment 21 Daniel 2011-12-28 22:07:57 PST
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?
Comment 22 Mart Rootamm 2011-12-29 01:36:02 PST
@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?
Comment 23 Daniel 2011-12-29 12:33:17 PST
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.
Comment 24 Reuben Morais [:reuben] 2011-12-30 03:19:43 PST
Created attachment 584938 [details] [diff] [review]
Patch v2

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 25 Dão Gottwald [:dao] 2011-12-30 06:07:24 PST
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)?
Comment 26 Reuben Morais [:reuben] 2011-12-30 06:39:48 PST
Created attachment 584959 [details] [diff] [review]
Patch v3

(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 27 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-10 12:07:17 PST
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)
Comment 28 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-10 12:08:41 PST
(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! ;)
Comment 29 Reuben Morais [:reuben] 2012-01-10 18:32:14 PST
(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.
Comment 30 Reuben Morais [:reuben] 2012-01-10 19:16:12 PST
Created attachment 587574 [details] [diff] [review]
Patch v4
Comment 31 Tim (fmdeveloper) 2012-01-17 16:11:10 PST
*** Bug 718677 has been marked as a duplicate of this bug. ***
Comment 32 rob64rock 2012-01-18 07:48:06 PST
Is there a try-server or tinderbox build with patch v4 available for testing?
Comment 33 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-18 11:23:44 PST
(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 34 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-18 11:26:27 PST
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 :)
Comment 35 Reuben Morais [:reuben] 2012-01-19 14:16:21 PST
Created attachment 589988 [details] [diff] [review]
Patch v5

Carrying r=zpao.

Sent to try at https://tbpl.mozilla.org/?tree=Try&rev=9b3e7f28948b
Comment 36 Reuben Morais [:reuben] 2012-01-19 19:20:06 PST
Created attachment 590084 [details] [diff] [review]
Patch v6

Just fixing the message in the test from "test_restore_apptabs:" to "test_restore_apptabs_ondemand:".
Comment 37 Mozilla RelEng Bot 2012-01-19 20:30:36 PST
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.
Comment 38 Reuben Morais [:reuben] 2012-01-20 03:03:35 PST
Results appear to be fine, the warnings were all from unrelated random oranges.
Comment 40 Sean Newman 2012-01-20 05:15:31 PST
Fockyeah, common sense wins against Asa again.
Comment 41 Shaun 2012-01-21 05:02:19 PST
@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).
Comment 42 Siddhartha Dugar [:sdrocking] 2012-01-21 05:10:36 PST
(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.
Comment 43 Mart Rootamm 2012-01-21 05:16:54 PST
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.
Comment 44 Ed Morley [:emorley] 2012-01-21 06:54:08 PST
https://hg.mozilla.org/mozilla-central/rev/2cabdf75c971
Comment 45 Alice0775 White 2012-12-06 06:40:16 PST
*** Bug 818863 has been marked as a duplicate of this bug. ***
Comment 46 Mihai Morar, (:MihaiMorar) 2013-01-10 06:27:36 PST
Verified Fixed on FF 19b1 and on Latest Nightly (2013/01/08)

Note You need to log in before you can comment on or make changes to this bug.