904529 - Remove SessionStore._initialState and pass it as an argument to .onLoad()

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Currently, SessionStore.initSession() parses the state read from disk and saves it to ._initialState. This field is then used by the onLoad() call for the first window. This is all handled by .init():

> gSessionStartup.onceInitialized.then(() => {
>   this.initSession();
>
>   if (!aWindow.closed) {
>     this.onLoad(aWindow);
>   }
>
>   this._deferredInitialized.resolve();
> });

I think it would be a lot better if we don't confuse the reader for what _initialState might be used as well but rather just return the parsed state from initSession() and pass it to onLoad().

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Remove SessionStore._initialState and pass it as an argument to .onLoad()

Comment 2

[Steven MacLeod [:smacleod]]

Comment on attachment 789491 [details] [diff] [review]
Remove SessionStore._initialState and pass it as an argument to .onLoad()

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

Just one thing, otherwise looks good.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ -420,5 @@
>            // with app tabs to restore.
> -          if (iniState.windows.length)
> -            this._initialState = iniState;
> -          else
> -            this._initialState = null;

Why was this removed? It seems to me the truthiness of |state| will be different now if this code path is followed.

Before we'd have |if (state && !iniState.windows.length) { state = null; }| but now it would be |if (state && !iniState.windows.length) { state = state; }|. I'm unsure if this is important, but it seems like a change in the logic to me that could modify behavior.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Oops. Will fix, thanks for catching that!

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Remove SessionStore._initialState and pass it as an argument to .onLoad(), v2

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68f187b1e3e5