Last Comment Bug 670033 - Persist deferred session across a browser restart
: Persist deferred session across a browser restart
Status: RESOLVED FIXED
[inbound]
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Dave Townsend [:mossop]
:
: Mike de Boer [:mikedeboer]
Mentors:
Depends on: 670040
Blocks: 476430
  Show dependency treegraph
 
Reported: 2011-07-07 15:48 PDT by Dave Townsend [:mossop]
Modified: 2013-10-28 11:29 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (13.59 KB, patch)
2011-07-07 15:57 PDT, Dave Townsend [:mossop]
no flags Details | Diff | Splinter Review
WIP (3.88 KB, patch)
2011-07-07 16:41 PDT, Dave Townsend [:mossop]
no flags Details | Diff | Splinter Review
patch rev 1 (4.96 KB, patch)
2011-07-27 12:52 PDT, Dave Townsend [:mossop]
paul: review+
Details | Diff | Splinter Review

Description Dave Townsend [:mossop] 2011-07-07 15:48:02 PDT
In bug 476430 we're going to be directing users to restart almost straight after starting their browser. This will currently lose any deferred session, throwing away whatever they were last viewing.
Comment 1 Dave Townsend [:mossop] 2011-07-07 15:57:47 PDT
Created attachment 544643 [details] [diff] [review]
WIP

This is a WIP that basically seems to work. It works by saving any existing deferred session along with the current session. On startup the previous previous session is thrown away unless we're doing a one-time restore in which case it is loaded into _lastSessionState ready for restoring at the users request.

Don't know whether to pursue this or hold off till the app tabs stuff changes
Comment 2 Dave Townsend [:mossop] 2011-07-07 16:41:45 PDT
Created attachment 544654 [details] [diff] [review]
WIP
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-07-18 16:27:41 PDT
Comment on attachment 544654 [details] [diff] [review]
WIP

So this _works_ for your case, but I'm not 100% sure it's not setting us up for more confusion. It definitely makes this a possibility...

0. user has enabled quit dialog
1. quit normally (don't "save and quit")
2. startup (restore previous session is enabled)
3. decide you don't need that session, just do normal browsing
4. quit, but set resume_session_once (via quit dialog, or maybe an extension)
5. session is restored, but restore previous session is also enabled, which is actually the previous previous session.

I don't want to do this as is, but apart from new topics like we talked about, there's no great way to do this. feedback:ehh?

Poking Dietrich to get another set of eyes on this
Comment 4 Dave Townsend [:mossop] 2011-07-27 12:52:19 PDT
Created attachment 548874 [details] [diff] [review]
patch rev 1

This should work better, clears the deferred session on shutdowns that aren't restarts. This makes the restore-next-time button in the quit dialog behave better.
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-03 15:40:36 PDT
Comment on attachment 548874 [details] [diff] [review]
patch rev 1

Review of attachment 548874 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. My only nit is that you add _lastSessionState to the SessionStoreService prototype along with a comment.
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-04 13:10:48 PDT
(In reply to comment #5)

> Looks good. My only nit is that you add _lastSessionState to the
> SessionStoreService prototype along with a comment.

D'oh. That's there already. But do add a comment that it's persisted sometimes (primarily) to make the addon restart case better.
Comment 8 Marco Bonardo [::mak] 2011-08-05 09:03:13 PDT
http://hg.mozilla.org/mozilla-central/rev/086328df641d

Note You need to log in before you can comment on or make changes to this bug.