1369456 - nsSessionStartup.js is loaded too early during startup

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement)

Description

[Florian Quèze [:florian]]

nsSessionStartup.js is currently loaded during app-startup (before a profile is selected) only to add a final-ui-startup observer.

It seems we should instantiate this component from nsBrowserGlue.js instead. I'm not sure we could move this to after first paint though, so we may not win much.

Comment 1

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> It seems we should instantiate this component from nsBrowserGlue.js instead.
> I'm not sure we could move this to after first paint though, so we may not
> win much.

We need to start reading the session file as soon as possible, right?

Comment 2

[Florian Quèze [:florian]]

This is now becoming slightly more important in the context of bug 1336227, as anything loaded before nsBrowserGlue.js delays the early blank window.

Comment 3

[brunoais]

Why did you remove P2?

Comment 4

[:Paolo Amadini]

Because we were incorrectly using the priority field to indicate in which bucket the bug was for the front-end performance team, instead of using a whiteboard tag like we are doing now. By using the whiteboard tag, we keep the priority field available for the separate triage process of the bug component. Hope this helps!

Comment 5

[brunoais]

It helps. Thank you.

Comment 6

[Dão Gottwald [:dao]]

Attachment: Bug 1369456 - Replace nsSessionStartup.js with SessionStartup.jsm.

Review commit: https://reviewboard.mozilla.org/r/261338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261338/

Comment 7

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5dee00231705fbe3287a6061bb5a52491750d2b

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8997649 [details]
Bug 1369456 - Replace nsSessionStartup.js with SessionStartup.jsm.

https://reviewboard.mozilla.org/r/261338/#review268496

Nice cleanup, thanks! :-)

::: browser/components/sessionstore/SessionStartup.jsm
(Diff revision 1)
> -    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");

Can you explain why it's fine to remove these 2 lines?

Comment 9

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] from comment #8)
> Comment on attachment 8997649 [details]
> Bug 1369456 - Replace nsSessionStartup.js with SessionStartup.jsm.
> 
> https://reviewboard.mozilla.org/r/261338/#review268496
> 
> Nice cleanup, thanks! :-)
> 
> ::: browser/components/sessionstore/SessionStartup.jsm
> (Diff revision 1)
> > -    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");
> 
> Can you explain why it's fine to remove these 2 lines?

Because the observer service is using a weak reference.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/409ecda4588d
Replace nsSessionStartup.js with SessionStartup.jsm. r=florian

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/409ecda4588d