411207 - Add notification of session restore completing restore of windows

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Dave Townsend [:mossop]]

Attachment: patch rev 1

There are certain tasks that need to be performed after Firefox has started, in particular for bug 408115 I need to open the Add-ons UI on top of any initial browser windows which means opening it after they have all opened. Gavin also had a similar need in bug 311605.

This patch adds an observer notification to the session restore process "sessionstore-restore-complete" to indicate that session restore has completed. It is sent out in the event that there is no session to restore, that session restore is disabled, or after all windows from the initial session have opened. It is only set out once per Firefox session.

For the disabled case I couldn't see much option other than to add a new loadState which basically means session store has attempted to init once and stopped because it was disabled.

When there is an initial session we keep a count of the number of windows still to complete loading. Once that hits 0 we notify.

Comment 1

[Dave Townsend [:mossop]]

Comment on attachment 295843 [details] [diff] [review]
patch rev 1

Dietrich is overloaded and suggested Simon as a reviewer

Comment 2

[Simon Bünzli]

Comment on attachment 295843 [details] [diff] [review]
patch rev 1

>+    if (this._loadState == STATE_DISABLED)
>+      return;

Disabled is disabled, so you can move these two lines really to the top of the function.

>+        this._restoreCount = this._initialState.windows.length;

So far, we've been quite liberal in what we've accepted. Please write |this._initialState.windows ? this._initialState.windows.length : 0| instead.

>     if (aTabs.length == 0) {
>+      if (this._restoreCount) {
>+        this._restoreCount--;
>+        if (this._restoreCount == 0) {

I won't guarantee that you'll reach this point exactly _restoreCount times, as theoretically one of the additional windows could be closed while we're still in the _restoreHistoryPrecursor wait-for-all-browsers loop (e.g. by an extension). Besides this will "break" the other way round when a user reopens a recently closed tab before all other windows have been completely restored (because undoCloseTab uses the same path). Both conditions might be neglectable, though.

OTOH, why not move this part into restoreWindow? That would at least guard against the second issue.

Finally: Please make sure that dev-doc includes the note that this new notification won't mean that all restored tabs have completed loading their content. You'll quite surely still get SSTabRestored events after this notification.

Anyway: r+ with the first two nits fixed, if you're convinced that this is what you need.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Taken on all the review comments including moving the notification to restoreWindow, so this fires once all windows have opened but often before tabs themselves have been restored. As such I've changed the name of the notification to "sessionstore-windows-restored"

Carrying over review and requesting approval, this is a low risk change that allows us to behave appropriately when showing notification windows on startup and is required for a blocking bug.

Comment 4

[Dave Townsend [:mossop]]

Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.92; previous revision: 1.91
done

Comment 5

[Dave Townsend [:mossop]]

Added description to http://developer.mozilla.org/en/docs/Observer_Notifications#Application_startup