865127 - Clean up priority queue keeping track of tabs to restore

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: clean up priority queue keeping track of tabs to restore

SessionStore internally uses a priority queue to determine which tab should be restored next. This is interweaved with the SessionStoreInternal object but could really be much more readable if it was a dedicated object with the usual PQ-like API.

Looking good on try:
https://tbpl.mozilla.org/?tree=Try&rev=a6fbe688dfa2

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: clean up priority queue keeping track of tabs to restore

Small fixes.

Comment 2

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

Comment on attachment 741238 [details] [diff] [review]
clean up priority queue keeping track of tabs to restore

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

Looks good, thanks.
Do I understand correctly that the sole purpose of this is to clean up code?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3132,5 @@
>      if (aRestoreImmediately || aWindow.gBrowser.selectedBrowser == browser) {
>        this.restoreTab(tab);
>      }
>      else {
> +      TabRestoreQueue.add(tab);

Are you sure it's |tab| and not |tabData|?

@@ +4425,5 @@
> +
> +  get restoreHiddenTabs() {
> +    let updateValue = () => {
> +      let value = Services.prefs.getBoolPref(PREF);
> +      let definition = {value: value, configurable: true};

Nit: I would personally inline all of this into
Object.defineProperty(this, "restoreHiddenTabs", {
  value: ...,
  configurable: true
});

Do as you prefer, though.

@@ +4435,5 @@
> +    updateValue();
> +  },
> +
> +  reset: function () {
> +    this.tabs = {priority: [], visible: [], hidden: []};

Would it be useful to use |Set|s instead of arrays?
Not necessarily in this bug, of course.

@@ +4467,5 @@
> +    if (index > -1) {
> +      set.splice(index, 1);
> +    }
> +  },
> +

You should document this method. And possibly call it |shift| or |pop|, as it has a side effect. Your call.

@@ +4482,5 @@
> +    }
> +
> +    return set && set.shift();
> +  },
> +

Again, please document hiddenToVisible/visibleToHidden.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Do I understand correctly that the sole purpose of this is to clean up code?

Yes, indeed. No bugs to be fixed here.

> >      else {
> > +      TabRestoreQueue.add(tab);
> 
> Are you sure it's |tab| and not |tabData|?

Yes, I don't know why the old code relied on tabData. The tab is in the right state already (pinned, hidden, whatever) and we can just use the tab itself. That change was intentional.

> > +  reset: function () {
> > +    this.tabs = {priority: [], visible: [], hidden: []};
> 
> Would it be useful to use |Set|s instead of arrays?

My first implementation did use Sets but then I realized that those separate queues are all FIFOs realized by Array.pop() and .shift(). The order in Sets is not guaranteed.

> > +    if (index > -1) {
> > +      set.splice(index, 1);
> > +    }
> > +  },
> > +
> 
> You should document this method. And possibly call it |shift| or |pop|, as
> it has a side effect. Your call.

Good point, will do.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/fb8531fe086c