Closed Bug 615394 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: iangilman, Assigned: zpao)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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?
Sounds great to me!
Attached patch Patch v0.1 (WIP) (obsolete) — Splinter Review
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.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #494235 - Flags: feedback?(dietrich)
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
Nominating for blocking, as it blocks two blockers.
Blocks: 594644
blocking2.0: --- → ?
blocking2.0: ? → betaN+
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)
Attachment #494235 - Flags: feedback?(dietrich) → feedback+
Blocks: 617511
No longer blocks: 594644
No longer blocks: 598795
What about "SSWindowRestoring" and "SSWindowRestored" for the name?
(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.
Attached patch Patch v0.3Splinter Review
Attachment #494235 - Attachment is obsolete: true
Attachment #497992 - Flags: review?(dietrich)
Attachment #497992 - Flags: review?(dietrich) → review+
Whiteboard: [has patch][has review][waiting for bug 618151]
Pushed http://hg.mozilla.org/mozilla-central/rev/92104acff2b3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][has review][waiting for bug 618151]
Target Milestone: --- → Firefox 4.0b9
Version: unspecified → Trunk
Is there a way I can verify this bug?
(In reply to comment #12)
> Is there a way I can verify this bug?

Tests pass &  panorama is using it.
Status: RESOLVED → VERIFIED
Blocks: 633722
You need to log in before you can comment on or make changes to this bug.