60117 - use generic startup mechanism in mozilla

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P1)

Description

[Alec Flett]

Some components need to be run at the startup of mozilla, to start new services,
etc.

We need a generic way to start up arbitrary components without requiring them to
hack into main1() as we do for things like:
- wallet
- cache prefs
- command line handlers
- appshell components

My proposal is that we have a category called "apprunner-startup" which
components register themselves in. When mozilla starts, we can automatically
enumerate these startup components, notify them that startup has begun, and
release them.

Patches forthcoming. I'll be using the nsIObserver interface that XPCOM has, to
notify components in the "apprunner-startup" category on the "apprunner-startup"
topic.

Comment 1

[Alec Flett]

Attachment: proposed notification mechanism

Comment 2

[Alec Flett]

Attachment: transition wallet to this new mechanism

Comment 3

[Alec Flett]

I've used wallet as a first example, because with this mechanism we no longer
have to instantiate the Wallet Service in main1()

Comment 4

[Daniel Bratell]

Is it possible to add some sort of priority order to the startup components so 
that component constructors can control the order in which extension components 
are started? 

I think we will se more and more extension components with time and there will 
probably be dependencies among them.

Comment 5

[Bruce Mitchener]

+    nsXPIDLCString previous;
+
+    return categoryManager->AddCategoryEntry("apprunner-startup", "wallet",
+                                             NS_WALLETSERVICE_CONTRACTID,
+                                             PR_TRUE, PR_TRUE,
+                                             getter_Copies(previous));

Since you don't care about the value of 'previous' you might as well just pass 
NULL instead.  It appears to me that nsCategoryManager::AddCategoryEntry() 
handles that correctly.

Comment 6

[Brendan Eich [:brendan]]

Nit: common "apprunner-startup" into a const char APPRUNNER_STARTUP[].

The idea of a priority can lead to pigeon hole problems, although it can scale
for a good while (if you use base 1000, e.g.).  Could we not use an exact
representation of the dependencies among apprunner components?  Something like
MSCOM category UUIDs and a < operator that partially orders their components?

/be

Comment 7

[Matthew Tuck [:CodeMachine]]

Is this mechanism loading the whole shebang at startup?  I know there's a bug
somewhere the lazily instantiate wallet, I'm not sure about all the other
components ...

Comment 8

[Alec Flett]

add morse to the CC to get his review of the wallet stuff.

Comment 9

[Stephen P. Morse]

The bug about lazy initialize of wallet is bug 14889.

Cookies should probably also be on your list.

r=morse for the wallet changes.

Comment 10

[Bill Law]

>My proposal is that we have a category called "apprunner-startup" which
>components register themselves in. When mozilla starts, we can automatically
>enumerate these startup components, notify them that startup has begun, and
>release them.

Of course, this is precisely what nsIAppShellComponent (along with
nsAppShellService::EnumerateAndInitializeComponents) was intended to accomplish.
 It predated nsIObserver and categories so the implementation differs.  But the
intent is the same.

There's also overlap with nsICommandLineHandler.  Maybe these should all be
merged so there's only 1 way to get a component called at startup.

One thing I'd like to see preserved: the factoring out of this logic from
main/main1.  I.e., move it to
nsAppShellService::EnumerateAndInitializeComponents (or equivalent).  Mostly,
that's because I have an aversion to large, monolithic functions (versus
smaller-scale, overrideable object methods).

Actually, if you just implemented a new nsIAppShellComponent and implement
Initialize with the category enumeration code and nsIObserver calls, you
wouldn't have to further hack main1 at all :-).

Comment 11

[Alec Flett]

yeah, my hope was to eventually roll appshell components into this mechanism... 
hopefully main1().. and you're right about the commandline handler stuff - it 
can probably be rolled in down the road.

Comment 12

[Matthew Tuck [:CodeMachine]]

What I was saying was that if most of these components should be lazily
instantiated and if lazy instantiation doesn't fit in with this, perhaps it's
not as necessary.

Comment 13

[Alec Flett]

this is for components that cannot be lazily instantiated.... what you're 
talking about is beyond the scope of this bug.

Comment 14

[Matthew Tuck [:CodeMachine]]

Then my statement essentially boils down to that someone should _really_ make
sure that what's suggested here can't be lazily instatiated.

At least wallet can be apparently, so others might be as well.  "Cache prefs"
sounds suspicious to me, since I wouldn't expect anything to do with the cache
to be loaded if I have a purely mailnews session that never accesses embedded
URLs.

And if it does turn out that nearly everything should be lazily instantiated, it
might be worth just changing everything to being lazy rather than doing this.

If someone who knows more about these components than me tells me everything
other than wallet must be instantiated at startup, or that there are in fact
enough examples that absolutely need to be startup instantiated, I'll be
perfectly happy.  I'm just making sure this isn't for something that won't
really be needed once everything that can be is lazy.

Comment 15

[Alec Flett]

Here's an example: The splash screen cannot be lazily instantiated.

Comment 16

[Alec Flett]

add valeski.. I hope this hasn't bit-rotted but it probably has.

Comment 17

[Judson Valeski]

note: the meta-charset service wedges itself into main() as well.

Comment 18

[Conrad Carlen (not reading bugmail)]

While I agree with the overall plan here, the initializing of cache prefs in
particular is a matter not just of lazy initialization or ordering - it's one of
control. Seamonkey (in nsAppRunner), initializes the disk cache location to be
within the current profile dir. That also means that there is a distinct cache
dir for each profile. Some application might not want to do this - it may
instead wish to have a global cache dir shared by all profiles. If this behavior
is rolled into a hidden, automatic startup action, how is an embedding app
supposed to control this behavior? 

Comment 19

[Alec Flett]

I think that's something that the cache implementors are going to have to fix.

Comment 20

[Paul Chen]

This hasn't been nominated nsbeta1, I think this is too big of a task to be done 
in mozilla0.9. Bumping off mozilla0.9 train.

Comment 21

[Alec Flett]

cc'ing chak and putting back on 0.9 milestone - we've got a basic fix in hand,
it just doesn't cover all components

Comment 22

[Judson Valeski]

chak's stuff is in, let's get this hooked up in mozilla.

Comment 23

[Alec Flett]

yup, raising priority.... I'll get to this asap
adding heikki because he's got some xml extension stuff that depends on this
(updating summary slightly)

Comment 24

[Alec Flett]

Attachment: use new system just like embedding

Comment 25

[Alec Flett]

I need a super-reviewer and reviewer here. blizzard/jst? jag?
adding jst because heikki needs this, and figured jst would be sympathetic to
his cause :)

Comment 26

[Alec Flett]

sorry, that patch also includes the removal of a bunch of totally unnecessary
casts that I had in my tree.. just makes the code more readable

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Just a minor optimization opportunity, replace:

nsCOMPtr<nsIObserver> startupNotifier =
   do_CreateInstance(NS_APPSTARTUPNOTIFIER_CONTRACTID, &rv);

with:

nsCOMPtr<nsIObserver>
   mStartupNotifier(do_CreateInstance(NS_APPSTARTUPNOTIFIER_CONTRACTID, &rv));

This saves us from operator=() call.

Otherwise, r=heikki.

Comment 28

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Crap, messed up with the variable names... Anyway, you get the principle: use
the constructor directly when you can, use operator= only when you must.

Comment 29

[Simon Fraser [no longer active]]

Heikki: the direct construction form is only slightly more efficient, and only 
then on some compilers. I wouldn't go around changing assign forms to this if not 
necessary.

Comment 30

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Simon: yes, I know, and I am not saying we should go through the whole tree to
change them all. But I see no reason why we shouldn't write new code this way.

Comment 31

[Christopher Blizzard (:blizzard)]

scc did some testing and the assignment form vs. the constructor form didn't
make any difference when the compiler was doing optimization.

sr=blizzard

Comment 32

[Alec Flett]

fix is in, as is, per blizzard's comments