871057 - Simplify session store initialization

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This is just a small refactoring. I think that the _initWindow code could be a little simpler. I worked through the different cases and I think this new version has the same behavior, keeping in mind that onLoad returns immediately if the window is already tracked.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Thanks for your patch, Bill. The startup path is unfortunately not well (or at all) tested and I'm not sure I understand all of it, yet. I'll need a little more time to review this but I'm sure it'll be worth to dive more into this part of the code.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK. I just wanted to point out that it should be possible to review this patch mostly in isolation by working through all the possible values of the different fields and checking that the same thing happens with and without the patch.

If aWindow is null, then both versions do this:
  if (this._loadState == STATE_STOPPED)
    this._loadState = STATE_RUNNING;
  return;

If aWindow is non-null and this._loadState is STATE_STOPPED, then both versions do this:
  this.onLoad(aWindow);

If aWindow is non-null and this._loadState is STATE_RUNNING, then the old version does this:
  if (!aWindow.__SSi || !this._windows[aWindow.__SSi])
    this.onLoad(aWindow);

...and the new version does this:
  this.onLoad(aWindow);

However, these are equivalent because the onLoad function immediately does then:
  if (aWindow && aWindow.__SSi && this._windows[aWindow.__SSi])
    return;

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 748312 [details] [diff] [review]
patch

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

Thanks for the quick walk-through, that was helpful!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +529,1 @@
>      this.onLoad(aWindow);

I think we can make this even clearer:

if (aWindow) {
  this.onLoad(aWindow);
} else if (this._loadState == STATE_STOPPED) {
  this._loadState = STATE_RUNNING;
}

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

I applied my feedback to your patch and landed it so that we don't forget it :)

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

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2dc1f3a6e404