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

VERIFIED FIXED in Firefox 4.0b9

Status

()

Firefox
Session Restore
VERIFIED FIXED
8 years ago
5 years ago

People

(Reporter: iangilman, Assigned: zpao)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 4.0b9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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?
(Reporter)

Comment 2

8 years ago
Sounds great to me!
Created attachment 494235 [details] [diff] [review]
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.
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
(Reporter)

Comment 5

8 years ago
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+
(Reporter)

Updated

8 years ago
Blocks: 617511
(Reporter)

Updated

8 years ago
No longer blocks: 594644
(Reporter)

Updated

8 years ago
No longer blocks: 598795

Comment 7

8 years ago
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.
Created attachment 497992 [details] [diff] [review]
Patch v0.3
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
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][has review][waiting for bug 618151]
Target Milestone: --- → Firefox 4.0b9
Version: unspecified → Trunk
Depends on: 623135
Duplicate of this bug: 605334
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

Updated

7 years ago
Blocks: 633722
Duplicate of this bug: 534669
Depends on: 865674
You need to log in before you can comment on or make changes to this bug.