615394 - Session Restore should notify when it is beginning and ending a restore

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Ian Gilman [:iangilman]]

This should cover setWindowState, as well as other restore triggers. This is necessary for Panorama to properly handle session restore. We need this sooner than later, as it blocks a number of Panorama bugs.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So we really want to make it clear when we're adding/replacing state.

This can be done via restoreLastSession, setBrowserState, setWindowState, setTabState, duplicateTab, undoCloseTab, undoCloseWindow.

Of those, restoreLastSession & setBrowserState will send sessionstore-browser-state-restored when finished.

setWindowState will fire SSTabRestoring on each tab it's restoring. setTabState, undoCloseTab, duplicateTab should also be firing a single SSTabRestoring.

undoCloseWindow will open a new window so doesn't actually matter to Panorama right now.

So what I'm thinking right now is that we fire an event on the window when we start modifying state then fire another event indicating that we're ready for that state to be inspected/modified. Something like SSWindowStateBusy and SSWindowStateReady?

That might be a bit too much of a wide angle approach, but I think it achieves what we want and is enough for Panorama to know not to just start doing things on TabOpen.

Ian, do you think something like that would be enough or would a more fine-grained system of events be better?

Comment 2

[Ian Gilman [:iangilman]]

Sounds great to me!

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1 (WIP)

I haven't tested this at all (I think I can dispatch the event from the window?), but this is what I would do.

SSWindowStateBusy is fired before any tabs are opened/closed/modified in the window. SSWindowStateReady is fired at the end of restoreHistoryPrecursor, at which point all tabs are opened & get/setTabValue are ready for use (through the early access path). That should be late enough, even though technically we're still "busy". The alternative is to fire after restoreHistory (that might actually be better...)

Tests will come soon.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I traced through and (before testing) I think this is covered just via code paths - the following call _sendWindowStateBusy (with many calls coming via restoreWindow)

setTabState
duplicateTab
undoCloseTab
restoreWindow
  via restoreWindow
    via onLoad
      via _openWindowWithState
        via restoreLastSession
        via setBrowserState
        via undoCloseWindow
  via setBrowserState
  via setWindowState
  via restoreLastSession

Comment 5

[Ian Gilman [:iangilman]]

Nominating for blocking, as it blocks two blockers.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 494235 [details] [diff] [review]
Patch v0.1 (WIP)


>+    // At this point we're essentially ready for consumers to read/write data
>+    // via the sessionstore API so we'll send the SSWindowStateReady event.
>+    this._sendWindowStateReady(aWindow);

not actually ready if the consumer is trolling tab history, so as you mentioned in the comments, i'd be more conservative here and wait until restoreHistory has completed.

>+  _sendWindowStateBusy: function sss__sendWindowStateBusy(aWindow) {
>+    let event = aWindow.document.createEvent("Events");
>+    event.initEvent("SSWindowStateBusy", true, false);
>+    aWindow.dispatchEvent(event);
>+  },
>+
>+  _sendWindowStateReady: function sss__sendWindowStateReady(aWindow) {
>+    let event = aWindow.document.createEvent("Events");
>+    event.initEvent("SSWindowStateReady", true, false);
>+    aWindow.dispatchEvent(event);
>+  },

could compact this into a single sendWindowStateEvent(aWindow, aEvent)

Comment 7

[ithinc]

What about "SSWindowRestoring" and "SSWindowRestored" for the name?

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #7)
> What about "SSWindowRestoring" and "SSWindowRestored" for the name?

So the impression I get from those names is that it can only happen once. Once a window is "restored" we wouldn't get a "restoring" event from it again.

Since this can happen via several different paths (overwriting, adding state) I figured just a busy vs ready denotation would be best.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.3

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/92104acff2b3

Comment 12

[u279076]

Is there a way I can verify this bug?

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #12)
> Is there a way I can verify this bug?

Tests pass &  panorama is using it.