Closed
Bug 335864
Opened 19 years ago
Closed 19 years ago
Hookup Session Restore to installs view and blocklist restart app capability
Categories
(Toolkit :: Add-ons Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.8.1beta1
People
(Reporter: robert.strong.bugs, Assigned: zeniko)
References
Details
(Keywords: fixed1.8.1)
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mconnor
:
review+
mconnor
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Once Session Restore lands we need to hookup the installs view and blocklist restart app buttons to Session Restore.
Comment 1•19 years ago
|
||
Looks like the Add-ons manager needs to set browser.sessionstore.resume_session_once to true before closing Firefox:
http://lxr.mozilla.org/seamonkey/source/browser/components/sessionstore/src/nsSessionStore.js#58
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/content/extensions.js&rev=1.91&mark=1320-1327#1320
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/content/list.js&rev=1.2.2.4&mark=202-208#202
Comment 2•19 years ago
|
||
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?
Assignee | ||
Comment 3•19 years ago
|
||
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
Comment 4•19 years ago
|
||
> 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.
Reporter | ||
Comment 5•19 years ago
|
||
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).
Assignee | ||
Comment 6•19 years ago
|
||
This shouldn't really be too complicated to fix. Taking.
Assignee: nobody → zeniko
Status: ASSIGNED → NEW
Assignee | ||
Comment 7•19 years ago
|
||
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)
Assignee | ||
Comment 8•19 years ago
|
||
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
Reporter | ||
Comment 9•19 years ago
|
||
Comment on attachment 221627 [details] [diff] [review]
add exit reason to quit-application
bsmedberg for nsAppStartup review.
Attachment #221627 -
Flags: review?(mconnor) → review?(benjamin)
Comment 10•19 years ago
|
||
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 11•19 years ago
|
||
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+
Assignee | ||
Comment 12•19 years ago
|
||
(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.
Reporter | ||
Comment 13•19 years ago
|
||
Simon, you want a checkin?
Assignee | ||
Comment 14•19 years ago
|
||
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.
Updated•19 years ago
|
Whiteboard: [checkin needed] - see comment 14
Comment 15•19 years ago
|
||
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
Updated•19 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed] - see comment 14
Version: Trunk → 2.0 Branch
Reporter | ||
Updated•19 years ago
|
Attachment #221627 -
Flags: approval-branch-1.8.1?(benjamin)
Updated•19 years ago
|
Attachment #221627 -
Flags: approval-branch-1.8.1?(benjamin) → approval-branch-1.8.1+
Comment 16•19 years ago
|
||
robert asked me to back-port the second patch to the 1.8 branch for FF 2.0
Comment 17•19 years ago
|
||
Please don't put this on branch yet. I think it broke session restoration, and am looking further into it.
Comment 18•19 years ago
|
||
ok, I will wait for the "all clear" signal before I attempt to port it to the branch.
Comment 19•19 years ago
|
||
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)
Reporter | ||
Comment 20•19 years ago
|
||
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. ;)
Assignee | ||
Comment 21•19 years ago
|
||
(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.
Assignee | ||
Comment 22•19 years ago
|
||
Attachment #224126 -
Flags: review?(mconnor)
Comment 23•19 years ago
|
||
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)
Updated•19 years ago
|
Flags: blocking-firefox2? → blocking-firefox2+
Updated•19 years ago
|
Attachment #224126 -
Flags: review?(mconnor)
Attachment #224126 -
Flags: review+
Attachment #224126 -
Flags: approval-branch-1.8.1+
Assignee | ||
Updated•19 years ago
|
Whiteboard: [checkin needed]
Updated•19 years ago
|
Attachment #221627 -
Attachment is obsolete: true
Comment 24•19 years ago
|
||
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]
Comment 25•19 years ago
|
||
Setting target milestone to b1 since this is needed for bug 335864 as I understand it, which is targetted at b1.
Assignee | ||
Comment 26•19 years ago
|
||
(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.
Comment 27•19 years ago
|
||
(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 :)
Assignee | ||
Updated•19 years ago
|
Whiteboard: [checkin needed: 1.8 branch]
Comment 28•19 years ago
|
||
Comment 29•19 years ago
|
||
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]
Comment 30•19 years ago
|
||
(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.
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•