364304 - add notification to the end of delayedStartup() in browser.js for extensions

Status: RESOLVED DUPLICATE of bug 529922

(Firefox :: General, enhancement)

Description

[(not reading, please use [e-mail] instead)]

[rfe] add notification to the end of delayedStartup() in browser.js for extensions

this problem comes from patrick hunt, the primary author of the del.icio.us extension from yahoo! (https://addons.mozilla.org/firefox/3615/)

patrick writes:  "In general we've seen problems where firefox services are not available during extension "onload" handler being called. I discussed this with Seth a few weeks ago. It would be great if mozilla could clarify what is available during code load, onload handler being called, etc...  Perhaps if extensions could register observers that get called when all mozilla services have been loaded this might be a good way to handle. For example: the rdf localstore doesn't seen to be available during onload handler, you need to set a timeout to run code after onload in order to gain access to localstore."

for firefox 2, for bookmarks, the bookmark service is not loaded until delayedStartup().  I know that patrick ran into this issue, and in his del.icio.us extension had to  use the setTimeout() hack.

trading emails with him, having a new notification at the end of delayedStartup() would solve the setTimeout() problem.

I'm not sure about all the specific localstore that he mentions, however.

Mark, would this be something up your alley?

Comment 1

[Robert Strong (they/them - no direct email)]

Having the notification during startup was a temporary solution for 2.0. I would prefer to fix bug 347585 and avoid this altogether.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

There does not appear to be any notifications fired in "delayedStartup", which is part of the problem. I am assuming that bug 347585 is more about a UI for users to be aware that a new extension has been installed and less about notifying the extension itself that all Firefox and toolkit services are ready.

In BrowserStartup, we fire "browser-window-before-show", then proceed to setTimeout(delayedStartup, 0)

We could fire a new notification ("browser-window-after-show") at the end of delayedStartup.

Comment 3

[Robert Strong (they/them - no direct email)]

(In reply to comment #2)
> I am assuming that bug 347585 is more about a UI for
> users to be aware that a new extension has been installed and less about
> notifying the extension itself that all Firefox and toolkit services are ready.
It is about moving the notification to after the app has completed startup and the notifications are already fired though the notification may not be as simple as desired due to the requirement to restart to load chrome, components, etc.

By moving the notification to after startup / during normal operations it should resolve this bug? True?

Comment 4

[Robert Strong (they/them - no direct email)]

btw: moving this to delayedStartup will most likely create bugs with session restore since we have to perform a restart after the extension installs... there are probably others as well. I think this bug should be wontfix and bug 347585 should instead be fixed.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

I am all for fixing this with bug 347585, if it's the better way. However, I do want to clarify, no notifications are being moved to delayedStartup. We are considering adding a *new* notification to the end of delayedStartup.

Perhaps, bug 347585 could be split into 2 parts:
1. establishing a spot in the code to fire an "application and services are started" notification.
2. create any UI parts of the current bug 347585.

Currently, extensions have no good notification that tells them that Firefox and toolkit services are started and initialized. That's the focus of this bug. Firing a notification at the end of delayedStartup seems like the "best place" to consider Firefox and toolkit ready to go.

Comment 6

[Robert Strong (they/them - no direct email)]

(In reply to comment #5)
> Currently, extensions have no good notification that tells them that Firefox
> and toolkit services are started and initialized. That's the focus of this bug.
> Firing a notification at the end of delayedStartup seems like the "best place"
> to consider Firefox and toolkit ready to go.
I believe my confusion about this bug has to do with the use of the word extensions along with my having the flu... this applies to app provided components, etc. (e.g. not just extensions) and this is to just provide that notification. Sorry about that.

Comment 7

[(not reading, please use [e-mail] instead)]

mark / robert:  thanks for hashing this out.

So something like:

var obs = Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService);
obs.notifyObservers(null, "browser-window-after-show", "");

at the end of delayedStartup()?

I didn't see browser-window-before-show documented anywhere.  Perhaps we could also start a doc on that notifcation and the proposed browser-window-after-show notification?

(kick me, this notification could have made it into Firefox 2.0, if I had been more on the ball.)

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: add 'notifyObservers' to end of delayedStartup

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 249150 [details] [diff] [review]
add 'notifyObservers' to end of delayedStartup

r=sspitzer

let's get a browser peer review from gavin sharp.

Comment 10

[(not reading, please use [e-mail] instead)]

mark, can you double check if that comment line is greater than 80 chars?

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: wrapped long comment line

the comment was way longer than 80 chars. fixed.

Comment 12

[(not reading, please use [e-mail] instead)]

Comment on attachment 249155 [details] [diff] [review]
wrapped long comment line

thanks for fixing the comment length.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

How would this be used by extension authors, given that they don't have a way to associate a given notification with a particular window? This notification would be sent out every time a new window opens, so this could potentially be hard to use correctly (multiple observers being called for different windows, etc). Should we send a reference to the window being loaded as the aSubject parameter? Should this be a window-specific DOM event instead?

I'm not exactly sure why extensions need to depend on delayedStartup to begin with - what does "the rdf localstore doesn't seen to be available during onload handler" mean? Is this extension just trying to share globally defined variables in browser.js's scope that don't exist until after delayedStartup, or does delayedStartup actually initialize something that isn't otherwise obtainable before it fires? I'd like to better understand the problems that extensions are having better before making changes like this, especially since this could potentially make things even more confusing for extension authors.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Extension developers don't want to depend on delayedStartup, they just need to. Several services are not initialized until the end of delayedStartup. Since extensions cannot hook 'delayedStartup' itself, they typically put a 'setTimeout(doMyThing, 0)' inside a window onload event listener, hoping that browser.js delayedStartup gets called and completes before their 'doMyThing' gets called. Inside 'doMyThing' the extension can (hopefully) safely access browser initialized services (bookmarks and microsummaries for instance).

As near as I can tell, there is no prefect spot to fire a 'firefox is completely finished doing all startup tasks' event/notification. The end of 'delayedStartup' is a good candidate, but you can't call the notification anything but 'browser-window-after-show' (if that is even a true name).

Passing the window reference sounds like a good idea in both 'browser-window-before-show' and 'browser-window-after-show'. What an extension or extension helper library can do is watch for the first 'browser-window-after-show' and use that as indication that service can be safely accessed. Any subsequent 'browser-window-after-show' would be ignored.

It is not ideal by any stretch, but it should provide for a workable solution.

Should I submit a patch that passes in a window reference for the aSubject of the notification?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #14)
> Extension developers don't want to depend on delayedStartup, they just need
> to. Several services are not initialized until the end of delayedStartup.

Why do they need to? What services, specifically, can they not use until delayedStartup is fired? For bookmarks, it would be much simpler for extensions to just get a reference to the bookmarks service and call ReadBookmarks themselves, rather than relying on delayedStartup doing it. Same goes for the microsummary service. Both are services reachable via XPCOM that can be instantiated regardless of whether the browser window already has. I don't think it's a good idea to encourage people to depend on delayedStartup when they don't have to, especially via a rather complicated nsIObserver. delayedStartup exists only as a hack to try and reduce "time until onload fires", which is what the Ts test uses to measure "startup time", and I'd like to avoid introducing dependencies on it's existence with the hopes of eventually removing it.

> Passing the window reference sounds like a good idea in both
> 'browser-window-before-show' and 'browser-window-after-show'. What an
> extension or extension helper library can do is watch for the first
> 'browser-window-after-show' and use that as indication that service can be
> safely accessed. Any subsequent 'browser-window-after-show' would be ignored.

If the extensions only care about the first window, why are they running their code in all windows (by overlaying browser.xul)? It sounds like they probably want to be using a JS component instead.

I'm not trying to be difficult, I just want to make sure that we know exactly what problems we're solving by adding observer topics and telling extension authors to use them. If there *are* specific cases of extensions needing to access something that's not available until after delayedStartup, it's probably easier to move those things out of delayedStartup than it is to make extensions use this notification.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

I am not sure it would be easier to move things out of delayedStartup (see bug 336833 - microsummaries were moved in), but I do agree that we should be cautious about added new nottifications.

I will spend some time looking at a few representative extensions to see what kind of hacks they currently have in place and why.

Comment 17

[Eric Shepherd [:sheppy]]

Perhaps we need to do some serious looking at the notifications we provide and what extensions do (or would like to do) and see what changes should be made, since Firefox 3 is going to offer an opportunity to bust some stuff anyway.

It might be worthwhile to survey extension authors and see what hoops they're sick of jumping through.

Comment 18

[Nickolay_Ponomarev]

> I just want to make sure that we know exactly what problems we're solving
> by adding observer topics and telling extension authors to use them.

One problem is bookmarks as you guessed:
http://groups-beta.google.com/group/mozilla.dev.extensions/browse_thread/thread/735f80b111e15d7b/f41dcdeffdfae2c1?lnk=st&q=Event+for+after+Firefox+is+finished+booting+up%2C+other+then+window+%22load%22&rnum=1#f41dcdeffdfae2c1

Do we tell people to call readBookmarks in their onload handler for now?

That kind of defeats the purpose of doing it in delayedStartup though. I think we should decide if we use delayedStartup just to game Ts, in which case we should just remove it, or it benefits the user, in which case we should let extensions use it too. (Certainly not via a global notification.)

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 249155 [details] [diff] [review]
wrapped long comment line

Clearing review request for now, until we figure out how we want to proceed here.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Nickolay: your point is well taken.  Is delayedStartup just around to maximize Ts, or does it actually improve startup time (or the appearance thereof) for users?  FWIW, I suspect that the answer will be some of both and that there's a place for it in our code.

Gavin: As Mark notes in comment 16, the microsummary service could have used this notification to initialize itself.  In early patches for bug 336833, I added a "bookmarks-service-initialized" notification to the bookmarks service itself, but then, per conversations with Ben, I reverted that in favor of initializing the microsummary service at the end of delayedStartup, although it's unclear now what effect that had on the Ts regression that prompted the change.

Seth: perhaps you can point Patrick at Gavin's comment 15 and see if Gavin's suggested alternative would work for his situation?  If not, perhaps Patrick could explain in more detail what he's trying to do that requires delayedStartup to have finished.

Comment 21

[Dietrich Ayala (:dietrich)]

From reading the comments on this bug, it seems that the issue boils down to:

1. Does delayedStartup serve a purpose other than to game Ts?

2. If it doesn't (or if gaming Ts is OK), what's the best way to notify internal services and extensions that it's ok to initialize, or start doing work.

alternate 2. If it does, someone needs to step forward and reset expectations for browser startup time ;)

My $.02: I think that Ts is already meaningless, as it takes the *best* score out of 10 - a non-realistic metric for measuring the startup experience for the average user. However, I think that Ts is a *user test* - testing the user's perception of application startup time. I think that delayedStartup is a key factor in the perception of app responsiveness at startup time, in that it puts off non-essential startup items until the chrome window is visible. It mostly contains things which enhance chrome with features not essential to the act of browsing. The psychological effect of *something* happening rsn (even if it's empty browser chrome) is very important for percieved startup time. That said, I don't know how this affects content, if it all. Does content load on the same thread? Does it block on chrome loading?

I think we should:

1. calculate Ts by tossing out the outliers and average the rest.*

2. keep delayedStartup and add some form of notification to indicate that it's ok for non-startup-critical internal services to initialize work. Encourage extensions to use JS XPCOM services where necessary, instead of overlays.**

* Fixing this bug doesn't depend on fixing how Ts is calculated. That should be filed separately, if it isn't already.

** Read any Fx extension tutorial, and overlay is the single example of integration. Isn't there some kind of Component.utils.import() trick now, to make JS XPCOM services easier? If so, we should be evangelizing it instead of overlays for extension singletons.

Comment 22

[Shawn Wilsher :sdwilsh]

This would actually be useful (if possible) to do on a more global scale (ie toolkit).  The download manager will need to resume cross session downloads, and mconnor has already said that app-startup/final-ui-startup is too early to be doing that.  The DM is part of toolkit, which means any browser specific thing will be useless.

Comment 23

[Matthew N. [:MattN]]

Good news is that this was implemented in Bug 529922 as "browser-delayed-startup-finished".