873771 - TabRestoreQueue should keep track of and evaluate restore_on_demand prefs

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

The refactoring from bug 865127 should go even further and read and check the restore_on_demand prefs. This way the main sessionstore module only needs to ask for a tab and gets one if there's more to be restored automatically.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: TabRestoreQueue should keep track of and evaluate restore_on_demand prefs

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 751371 [details] [diff] [review]
TabRestoreQueue should keep track of and evaluate restore_on_demand prefs

Review of attachment 751371 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3237,1 @@
>        return;

I don't understand why you can simplify the condition that much.
Have you moved all the logics to shift()? If so, what's the rationale?

@@ +4456,5 @@
>    // The separate buckets used to store tabs.
>    tabs: {priority: [], visible: [], hidden: []},
>  
> +  // Lazy getter that returns whether tabs are restored on demand.
> +  get restoreOnDemand() {

This should probably be renamed and/or namespaced so that we know at first glance it's a preference.
What about moving this to TabRestoreQueue.Prefs.restoreOnDemand?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +3237,1 @@
> >        return;
> 
> I don't understand why you can simplify the condition that much.
> Have you moved all the logics to shift()? If so, what's the rationale?

Indeed, it's all in shift() now. I figured that it's a lot easier to read the main sessionstore module if it doesn't have to care about the restore_on_demand prefs and whether we have pinned tabs that would be restored right now. If shift() returns a tab we can restore it, if not we're not really interested in finding out why and just accept that.

> @@ +4456,5 @@
> >    // The separate buckets used to store tabs.
> >    tabs: {priority: [], visible: [], hidden: []},
> >  
> > +  // Lazy getter that returns whether tabs are restored on demand.
> > +  get restoreOnDemand() {
> 
> This should probably be renamed and/or namespaced so that we know at first
> glance it's a preference.
> What about moving this to TabRestoreQueue.Prefs.restoreOnDemand?

Yeah... so how about having a small Prefs object/module inside of sessionstore that simply holds live values of all the preferences we need? We can do that in a follow-up soon? This would remove a lot of boilerplate code.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> What about moving this to TabRestoreQueue.Prefs.restoreOnDemand?

Will do exactly this for now.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/817df00cc0ff

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/817df00cc0ff

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Backed out:

https://hg.mozilla.org/integration/fx-team/rev/2ea5465ee011

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Oh, hm. I just realized that this isn't one of the bugs that needs to be put behind a kill switch. Its regression (bug 875266) was quite "bad" anyway so let's fix this before re-landing.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Re-landed with a fix for a tiny, stupid mistake that was the cause of bug 875266.

https://hg.mozilla.org/integration/fx-team/rev/0ac101760944

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/0ac101760944