Closed Bug 460334 Opened 11 years ago Closed 11 years ago

sessionstore-windows-restored fires too early

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.1b2

People

(Reporter: Dolske, Assigned: mossop)

References

Details

(Keywords: fixed1.9.0.5)

Attachments

(2 files, 1 obsolete file)

When implementing a first-run page for Labs' Geode extension, I had a hard time figuring out how to open a tab. Session restore was reusing the tab for a restored page (which seems like another bug, but I digress).

I ended up using the sessionstore-windows-restored notification, but found that it fires too early... It's send when session restores *starts* restoring the last window of a saved session. It should really wait until it's *done* restoring the last window of a saved session.

I ended up working around this by waiting for that notification and then calling setTimeout(addMyTab, 0), but that's a hack.
This should be very easy to fix - we'd just have to move the notification to the end of sss_restoreWindow (i.e. after the tabs used by SessionStore have been determined). You'll see this fixed very soon if you volunteer that patch...
When porting sessionstore to SeaMonkey (bug 36810), i faced this problem and introduced one more variable - RestoreInprogress, which is true when browser restoring windows.

This needed to workaround SeaMonkey implementation of navigator.js, which uses extra variables when opening new window.

My solution is not perfect, but potential bug fixer can use it to write more general (not Firefox dependant) sessionstore, taking in account that Zenico has plans to move sessionstore to toolkit.
Attached patch patch rev 1 (obsolete) — Splinter Review
I wanted to be positive that the notification was always dispatched so rather than just move it later I broke it out into its own method called at each exit point from restoreWindow. I ran this with some logging in browser glue and watched it receive the notification just once for a single window restore, a multi-window restore and with restoration turned off.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #345174 - Flags: review?(zeniko)
Comment on attachment 345174 [details] [diff] [review]
patch rev 1

Looks good, but please name the function something slightly more obvious (e.g. notifyIfAllWindowsRestored) and move the new function below to the Auxiliary Functions part of the file. With that, r+=me and thanks.
Attachment #345174 - Flags: review?(zeniko) → review+
Attached patch patch rev 2Splinter Review
Updated with comments
Attachment #345174 - Attachment is obsolete: true
Attachment #345252 - Flags: review+
Landed: http://hg.mozilla.org/mozilla-central/rev/97497ea32242
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
Want this as part of landing bug 456439 on branch.
Attachment #348056 - Flags: review?(zeniko)
Blocks: 456439
Comment on attachment 348056 [details] [diff] [review]
Branch Patch, v.1

+  },
+
+
 /* ........ Storage API .............. */

Please remove the second blank line on check-in.
Attachment #348056 - Flags: review?(zeniko) → review+
Attachment #348056 - Flags: approval1.9.0.5?
Comment on attachment 348056 [details] [diff] [review]
Branch Patch, v.1

Requesting approval for FF3.0.5... This moves when the existing sessionstore-windows-restored notification fires, so that it fires after it's actually restored the tabs in the last window. Should be low risk, it seems unlikely that any extensions would be relying on the specific timing (when restoring multiple windows, the tabs in other windows would already be restored, only the last window would look different).
Attachment #348056 - Flags: approval1.9.0.5? → approval1.9.0.5+
Comment on attachment 348056 [details] [diff] [review]
Branch Patch, v.1

Approved for 1.9.0.5, a=dveditz for release-drivers
Checked in on branch.
Keywords: fixed1.9.0.5
You need to log in before you can comment on or make changes to this bug.