Hookup Session Restore to installs view and blocklist restart app capability

RESOLVED FIXED in mozilla1.8.1beta1

Status

()

defect
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: rstrong, Assigned: zeniko)

Tracking

({fixed1.8.1})

1.8 Branch
mozilla1.8.1beta1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Once Session Restore lands we need to hookup the installs view and blocklist restart app buttons to Session Restore.
Shouldn't session store default to preserving sessions when restarting using nsIAppStartup? IMO it would be better than having to fix all of nsIAppStartup consumers (that includes some extensions, like RestartFirefox).

Do any nsIAppStartup consumers want to bypass automatic session restoring? Is it a good idea to make all consumers, that want to take advantage of session store, set a SS-specific pref?
Flags: blocking-firefox2?
I hope the idea wouldn't be to let all consumers set a specific preference but to offer a simple API (nsISessionStore.restart) which would take care of the implementation details.

It might be worth though to think about nsAppStartup sending a notification "restart-application" so that SessionStore and other extensions can react appropriately. Would you mind to file a bug for that?
Status: NEW → ASSIGNED
> I hope the idea wouldn't be to let all consumers set a specific preference but
> to offer a simple API (nsISessionStore.restart) which would take care of the
> implementation details.
> 
This makes those components dependent on session store, when all they *really* want to do is restart the app.

> It might be worth though to think about nsAppStartup sending a notification
> "restart-application" so that SessionStore and other extensions can react
> appropriately. Would you mind to file a bug for that?
> 
I hope it can happen in this bug. nsAppStartup already sends out quit-application with no additional information. Perhaps it can also inform observers whether it's a normal quit or a quit-and-restart situation.
With other work I have to finish up I won't be taking on anything outside of what this bug is specifically for (e.g. using the method provided by session restore).
This shouldn't really be too complicated to fix. Taking.
Assignee: nobody → zeniko
Status: ASSIGNED → NEW
The good news is that the fix is indeed quite simple (mostly just adding a shutdown reason to the quit-application notification). The bad news is that this isn't enough. SessionStore still needs the quit-application-requested notification to know when the last chance for updating its internal state is and the quit-application-granted notification to know from what point on to ignore closing windows. So you'll at least have to call canQuitApplication before restarting.

In the end, extensions will still have to make sure to be good citizens, but there's no new API to know (note that all extension authors whose extensions I've found to do this wrong have already been notified - because they break my Session Manager/Crash Recovery extensions which rely on these notifications as well).

The alternative to this fix would be to call canQuitApplication's code from nsAppStartup::Quit when it's first called. Who's to decide on this?
Attachment #221627 - Flags: review?(mconnor)
An alternative to the above would be to introduce goRestartApplication in globalOverlay.js (which does the same as goQuitApplication with the subtle difference of adding the eRestart flag). Though this bug probably isn't the place for that decision...
Status: NEW → ASSIGNED
Comment on attachment 221627 [details] [diff] [review]
add exit reason to quit-application

bsmedberg for nsAppStartup review.
Attachment #221627 - Flags: review?(mconnor) → review?(benjamin)
So, this is a better approach, since nsISessionRestore is in browser, not toolkit, so that's a bad dependency.  The patch looks ok to me, but I think we can wait on adding the right method to nsISessionStore instead of setting the pref directly.
Comment on attachment 221627 [details] [diff] [review]
add exit reason to quit-application

So in general if code wants to restart the app it should be using nsIAppStartup.quit(eRestart)?
Attachment #221627 - Flags: review?(benjamin) → review+
(In reply to comment #11)
> So in general if code wants to restart the app it should be using
> nsIAppStartup.quit(eRestart)?

Something along these lines would be the obvious choice, wouldn't it? Although to get a somewhat nicer (XPCOM hiding) interface and prevent code from forgetting to call canQuitApplication, I'd add goRestartApplication as suggested in bug 338039.

(In reply to comment #10)
> I think we can wait on adding the right method to nsISessionStore
> instead of setting the pref directly.

If we go this way, we won't need any specific method to nsISessionStore at all. Setting the pref would then indeed be all that is required.
Simon, you want a checkin?
Sure. When doing so, please also remove the now obsolete comment line right below restartApp in extensions.js that I overlooked (though I hope to get rid of that method altogether as soon as bug 338039 is fixed). Thanks.
Whiteboard: [checkin needed] - see comment 14
mozilla/toolkit/components/startup/src/nsAppStartup.cpp 	1.13
mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.12
mozilla/toolkit/mozapps/extensions/content/extensions.js 	1.92
mozilla/toolkit/mozapps/extensions/content/list.js 	1.7
mozilla/toolkit/mozapps/extensions/content/list.xul 	1.4
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed] - see comment 14
Version: Trunk → 2.0 Branch
Attachment #221627 - Flags: approval-branch-1.8.1?(benjamin)
Attachment #221627 - Flags: approval-branch-1.8.1?(benjamin) → approval-branch-1.8.1+
robert asked me to back-port the second patch to the 1.8 branch for FF 2.0
Please don't put this on branch yet. I think it broke session restoration, and am looking further into it.
ok, I will wait for the "all clear" signal before I attempt to port it to the branch.
The patch as checked-in prevents any session data from being saved: Windows are closed after quit-application-granted, and before quit-application. The patch caused data to be saved after quit-application was received, at which time windows have already closed and sessionstore has already removed their state information.

This fix forces data to be saved at quit-application-granted, and no longer saves after receiving quit-application.
Attachment #224123 - Flags: review?(mconnor)
Comment on attachment 224123 [details] [diff] [review]
Fixes regression in previous patch

>Index: browser/components/sessionstore/src/nsSessionStore.js
...
>@@ -256,20 +256,18 @@ SessionStoreService.prototype = {
...
>+    // if not resuming, discard all session related data 
>+    if (!this._doResumeSession()) {
>       this._clearDisk();
>     }
I suspect the first thing mconnor is going to say is remove the brackets. ;)
(In reply to comment #19)
> Created an attachment (id=224123) [edit]

> This fix forces data to be saved at quit-application-granted, and no longer
> saves after receiving quit-application.

So far, quit-application is pretty much guaranteed to happen, quit-application-granted is not. Rather change onClose to not delete the window data when _loadState == STATE_QUITTING.
Attachment #224126 - Flags: review?(mconnor)
Comment on attachment 224123 [details] [diff] [review]
Fixes regression in previous patch

zeniko's patch fixes the problem better, obsoletes this
Attachment #224123 - Attachment is obsolete: true
Attachment #224123 - Flags: review?(mconnor)
Flags: blocking-firefox2? → blocking-firefox2+
Attachment #224126 - Flags: review?(mconnor)
Attachment #224126 - Flags: review+
Attachment #224126 - Flags: approval-branch-1.8.1+
Whiteboard: [checkin needed]
Attachment #221627 - Attachment is obsolete: true
Landed "additional trunk patch" on the trunk.
mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.13

Should I land both attachments on this bug on the branch?
Whiteboard: [checkin needed]
Setting target milestone to b1 since this is needed for bug 335864 as I understand it, which is targetted at b1.
Blocks: 311744, 328154
No longer depends on: 328154
Target Milestone: --- → Firefox 2 beta1
(In reply to comment #24)
> Should I land both attachments on this bug on the branch?

Any more objections, Dietrich? AFAICS, both patches should be landable "as is" on the branch.
(In reply to comment #26)
> (In reply to comment #24)
> > Should I land both attachments on this bug on the branch?
> 
> Any more objections, Dietrich? AFAICS, both patches should be landable "as is"
> on the branch.
> 

Looks fine to me :)
Whiteboard: [checkin needed: 1.8 branch]
Attachment 224882 [details] [diff] and attachment 224126 [details] [diff] [review] checked in on the branch. Simon/Dietrich, could you guys verify that everything is as it should be?
Keywords: fixed1.8.1
Whiteboard: [checkin needed: 1.8 branch]
(In reply to comment #29)
> Attachment 224882 [details] [diff] [edit] and attachment 224126 [details] [diff] [review] [edit] checked in on the branch.
> Simon/Dietrich, could you guys verify that everything is as it should be?
> 

Clobbered branch, rebuilt, tested. Seems fine, thanks Gavin.
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.