Closed Bug 686065 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 10

People

(Reporter: Paolo, Assigned: Paolo)

References

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

Details

Attachments

(1 file)

Attached patch The patchSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/40e4b0f6174d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 10
Blocks: 698276
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?
(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 ;-)
Depends on: 748516
You need to log in before you can comment on or make changes to this bug.