Closed Bug 904529 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

Attachments

(1 file, 1 obsolete file)

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 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.
Oops. Will fix, thanks for catching that!
Attachment #789491 - Attachment is obsolete: true
Attachment #789491 - Flags: review?(smacleod)
Attachment #789594 - Flags: review?(smacleod)
Attachment #789594 - Flags: review?(smacleod) → review+
https://hg.mozilla.org/mozilla-central/rev/68f187b1e3e5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.