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

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[:Paolo Amadini]

Attachment: The patch

For the new Downloads panel, having sessionType available after the session
startup phase finished can simplify some of the session restore code.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 559631 [details] [diff] [review]
The patch

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

Looks good. Thanks!

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/40e4b0f6174d

Comment 3

[:Margaret Leibovic]

https://hg.mozilla.org/mozilla-central/rev/40e4b0f6174d

Comment 4

[[e-mail]]

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.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to [e-mail] 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

[[e-mail]]

(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 ;-)