Don't clear nsSessionStartup::sessionType after the session startup phase finished

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Session Restore
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 559631 [details] [diff] [review]
The patch

For the new Downloads panel, having sessionType available after the session
startup phase finished can simplify some of the session restore code.
Attachment #559631 - Flags: review?(paul)
Comment on attachment 559631 [details] [diff] [review]
The patch

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

Looks good. Thanks!
Attachment #559631 - Flags: review?(paul) → review+
Landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/40e4b0f6174d
Whiteboard: [fixed-in-fx-team]

Comment 3

6 years ago
https://hg.mozilla.org/mozilla-central/rev/40e4b0f6174d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 10

Updated

6 years ago
Blocks: 698276

Comment 4

6 years ago
Comment on attachment 559631 [details] [diff] [review]
The patch

>@@ -172,16 +172,19 @@ SessionStartup.prototype = {
[init: function sss_init() {]
>+    if (this._sessionType != Ci.nsISessionStartup.NO_SESSION)
>+      Services.obs.addObserver(this, "browser:purge-session-history", true);
So, we have init() adding the browser:purge-session-history observer.

>@@ -192,29 +195,35 @@ SessionStartup.prototype = {
[case "final-ui-startup":]
>       Services.obs.removeObserver(this, "final-ui-startup");
>       Services.obs.removeObserver(this, "quit-application");
>       this.init();
And we have final-ui-startup calling init() adding the observer, but also removing the quit-application observer.

>     case "quit-application":
>       // no reason for initializing at this point (cf. bug 409115)
>       Services.obs.removeObserver(this, "final-ui-startup");
>       Services.obs.removeObserver(this, "quit-application");
>+      if (this._sessionType != Ci.nsISessionStartup.NO_SESSION)
>+        Services.obs.removeObserver(this, "browser:purge-session-history");
So, if the quit-application observer gets hit, this means that the final-ui-startup hasn't run, and hasn't called init, and hasn't added the browser:purge-session-history observer, so there's nothing to remove. In fact you'll find that the sessionType can only ever be NO_SESSION at this point.
(In reply to neil@parkwaycc.co.uk from comment #4)
> Comment on attachment 559631 [details] [diff] [review] [diff] [details] [review]

True story. So the correct way to do this would be to just put back in what was taken out in bug 588506.

Neil, want to file a new bug/write the patch or are you just following up for the SM patch?

Comment 6

6 years ago
(In reply to Paul O'Shannessy from comment #5)
> So the correct way to do this would be to just put back in what
> was taken out in bug 588506.
> 
> Neil, want to file a new bug/write the patch or are you just following up
> for the SM patch?

I was just trying to understand Misak's port. I'm not actually sure what it is you're trying to say, so perhaps it would be better if I wait for the code ;-)

Updated

5 years ago
Depends on: 748516
You need to log in before you can comment on or make changes to this bug.