68720 - nsEmbedAPI.cpp should use generic startup observers

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Judson Valeski]

Rather than inserting everything needed for app running at startup in our init 
routine, we should just notify startup observers instead.

Comment 1

[Chak Nanga]

I'm just lost following the depends/blocks chain associated with this bug.

You're talking about changing the way we do things now in NS_InitEmbedding()? 

Comment 2

[Judson Valeski]

yea, some of the current startup services in initembedding would use this new 
service, but the impetus for the change is the charset obsever service which 
this bug blocks. that bug suggests a new initembedding insertion which would be 
the tip of the iceberg, so subsequently we want to provide a generic mechanism.

Comment 3

[Chak Nanga]

Attachment: Patches to makefiles etc. to build the AppstartupNotifier component

Comment 4

[Chak Nanga]

Attachment: AppStartupNotifier component - unzip to embedding\components\appstartup dir

Comment 5

[Chak Nanga]

Attachment: Patch to NS_InitEmbedding() to notify startup observers

Comment 6

[Chak Nanga]

Conrad : Could you please verify these patches on Mac when you get a chance?
(I have'nt made any changes to the mcp files)

Can someone please verify this on UNIX for me?

Thanks

Comment 7

[Alec Flett]

so I'm wondering if it's worth having a seperate interface just for this. How
about just using nsIObserver?

you could say 
nsCOMPtr<nsIObserver> mStartupNotifier =
do_CreateInstance(NS_APPSTARTUPNOTIFIER_CONTRACTID, &rv);

mStartupNotifier->Observe(NS_LITERAL_STRING("gobabygo!"));

or something to that effect

Comment 8

[Judson Valeski]

leveraging nsIObserver makes sense to me.

Comment 9

[Chak Nanga]

Attachment: New src files + changes to makefiles etc. to build appstartup component

Comment 10

[Chak Nanga]

Attachment: Changes to NS_InitEmbedding()

Comment 11

[Chak Nanga]

Two new patches (id=26290/26291)submitted to leverage nsIObserver instead of a 
new interface per Alec and Jud's comments.

Patches dated 2/22/01 are invalid now...

Comment 12

[Alec Flett]

CC'ing law because he wrote the original appshell startup component idea, and
will be interested in this, as I explain below)

I just had a (hopefully brilliant) idea - what if the Topic of the observe that
the startup observer sees was actually the name of the category that gets
enumerated? 

That way if we did eventually want to have various startup levels (a la sysvinit
on unix) then we could do it that way.

The reason I bring this up is that we (in navigator-land) are trying to figure
out a way to pre-load mozilla.exe at windows startup - and we're trying to start
as many "essential" services as possible, in the background. These wouldn't
necessarily be the "app-startup" components, but rather would be things that we
(as the navigator) wanted to pre-load, like bookmarks, history, etc.. it would
be great if we could leverage that.

(also, diff -u is the preferred diff mechanism, you can combine it with -N)

Comment 13

[Chak Nanga]

This seems to have taken (yet another) life of it's own. 

Setting target milestone 0.9 - hopefully we can get this in.

Comment 14

[Alec Flett]

regarding jud's concerns about startup ordering - I don't think we should worry
about that here.

my comments were meant only to say that if you changed this just slightly, then
we could use it in another context... 

the only change I'm suggesting is forwarding the original Observe() topic as the
category name, and then forwarding this same topic to the observers that get
notified... 

Comment 15

[Chak Nanga]

Set of patches to match Alec's comments above
are forthcoming....

Comment 16

[Chak Nanga]

Attachment: New src files + changes to makefiles etc. to build appstartup component

Comment 17

[Chak Nanga]

Attachment: Changes to NS_InitEmbedding()

Comment 18

[Alec Flett]

looks great
only two more comments:
- no tabs allowed in source code :)
- you can't just cast PRUnichar* to char* - char* is an single-byte array of
arbitrary encoding, and PRUnichar is double-byte array of unicode. Try using
some combination NS_ConvertASCIItoUCS2/etc

Comment 19

[Chak Nanga]

I knew the casting i was doing was bogus, but, thought i'll try :-)


The string class seems to be too confusing..

What's the function to convert from UCS2 to Ascii? 

Comment 20

[Chak Nanga]

(Yet) another try...

Not resubmitting changes to NS_InitEmbedding(). Previous patch with (id=26440) 
will still do the job.

Will submit the new appstartup files + changes...

Comment 21

[Chak Nanga]

Attachment: New src files + changes to makefiles etc. to build appstartup component

Comment 22

[Alec Flett]

sr=alecf
however....sorry, I missed this in the last patch... you don't need an Init()
method in you object if you're not doing anything there - you can use
NS_GENERIC_FACTORY_CONSTRUCTOR() (instead of NS_GENERIC_FACTORY_CONSTRUCTOR_INIT())

you don't need to submit a second patch for that, consider this sr=alecf
thanks for doing this work!

Comment 23

[Chak Nanga]

Removed the Init(). Thanks for the review...

Comment 24

[Conrad Carlen (not reading bugmail)]

> Conrad : Could you please verify these patches on Mac when you get a chance?
I didn't see this as I wasn't CC'd. Will look at it soon. 

Comment 25

[Conrad Carlen (not reading bugmail)]

+                // creating a single instance
+                // if you need a service, do a GetService() from the observer
+                // this allows the instance to be released and the DLL unloaded,
+                // if necessary
+                nsCOMPtr<nsIObserver> startupObserver =
+                                do_CreateInstance(contractId, &rv);

I'm not clear on this. Why are we doing a "do_CreateInstance" on the component
notifying it, only to have it destroyed when startupObserver goes out of scope?
Aren't most things which need to be notified of startup services? Why not just
do do_CreateService instead? If the component in its Observe() does a getService
on itself, as I think the comment suggests, (tell me if I'm wrong) two objects
end up getting constructed - one for the do_CreateInstance() in this patch and
another if the observer does a getService. 

Comment 26

[Alec Flett]

not all objects that need this are services - if the object in question wants to
be a service, then as you said, then it's up to that object to create the
service. the idea is just to avoid any lingering objects that are only needed at
startup.

Comment 27

[Conrad Carlen (not reading bugmail)]

Couldn't we encode into the cat man info whether the component wanted to be
created as a component or service? Wallet, for example, could have in its
RegisterStartupObserver(): AddCategoryEntry("app-startup", "service", ...) Then,
in nsAppStartupNotifier::Observe(), it could check this and if so, do a
doCreateService() instead. If the component did AddCategoryEntry("app-startup",
"component", ...), it gets doCreateInstance(). I'm concerned that the number of
components which fit the pattern of: "get created, get notified, get destroyed"
is limited. Also, the overhead of the component that wants to be a service
having to do a getService on itself could be bad news.

Comment 28

[Judson Valeski]

conrad's idea makes sense to me. if we don't want the string to be in the entry 
field of the category registration, it could be part of the value string.

Comment 29

[Alec Flett]

I don't think we should go changing the category manager - it's really just
meant to store name/value pairs (and that's how they are stored in the registry)

we could encode it into the "value" part of each, something like:
"service,@mozilla.org/foo/bar;1" or something

However, I still find this kind of hacky, I dunno.. I'm against it but not
enough that I'm going to put up a big fight :)

Comment 30

[Chak Nanga]

If everyone's in agreement with Alec's latest suggestion then i'll go ahead and 
make the following changes:

If i see the word "service" before the "," in a 
components contract_id such as "service,@mozilla.org/foo/bar;1"
then i'll do a do_CreateService()

If i do not see "service" before the comma then i'll do a do_CreateInstance()

Comment 31

[Judson Valeski]

I thought the idea was to have "service" or "component" in the entry or value 
attribute of the category item registration. I think we should avoid modifying 
the standard format of contract ids.

Comment 32

[Chak Nanga]

Upon more careful reading...

guess i misread and misquoted what Alec had said earlier. I'll work on it.

Comment 33

[Chak Nanga]

Jud : is this what you're saying?

1. Leave the components contract_id (in the idl/header file as is) i.e. do not 
add the "service" keyword to it

2. Assuming we're encoding the service info in the "value" part as Alec 
suggested...
In the components RegistrationProc it will add itself to a category and specify 
whether or not it's a service by doing something like:

           categoryManager->AddCategoryEntry(APPSTARTUP_CATEGORY, "testcomp",
                                             "service,@mozilla.org/foo/bar;1",
                                             PR_TRUE, PR_TRUE,
                                             getter_Copies(previous));

(By adding the "service" keyword above we're indirectly changing the format of 
contract_id since it's this value we use as the contract id below to create the 
component)

3. During component enumeration the app startup notifier would get the 
contractid of the component by doing:

            nsXPIDLCString contractId;
            categoryManager->GetCategoryEntry(strCategory.get(), 
                                    categoryEntry,
                                    getter_Copies(contractId));

4. The contractId var above will have the "service" keyword in it since that's 
how the component would've registered in step 2 above

5. do a createInstance() or a createService depending on whether the "service" 
keyword is present. 

Comment 34

[Conrad Carlen (not reading bugmail)]

> 1. Leave the components contract_id (in the idl/header file as is) i.e. do not
> add the "service" keyword to it

Yes, you for sure don't want to change the contract id of anything. In the case
of wallet, you could just say:

categoryManager->AddCategoryEntry(APPSTARTUP_CATEGORY, "testcomp",
                                  "service," NS_WALLETSERVICE_CONTRACTID,

Then its contract id stays the same and you just parse out the "service," or
"component," before creating the instance.

Comment 35

[Chak Nanga]

Attachment: Latest appstartup component changes...

Comment 36

[Chak Nanga]

Conrad : Could you please verify this patch on the Mac...Thanks

Comment 37

[Conrad Carlen (not reading bugmail)]

+                PRInt32 serviceIdx = cid.Find(NS_ConvertASCIItoUCS2("service,"));
+
+                nsCOMPtr<nsIObserver> startupObserver;
+                if (serviceIdx == 0)

You've got to skip over the "service," prefix in contractID here.

+                    startupObserver = do_GetService(contractId, &rv);
+                else
+                    startupObserver = do_CreateInstance(contractId, &rv);

Comment 38

[Bill Law]

Here's how I dealt with this problem in another context, for what it's worth:

Always use CreateInstance.  When the object in question's Observe function is
called, then it simply calls nsIServiceManager::RegisterService for itself, if
it wants to be a service.  If it doesn't want to be a service, then it doesn't
register itself.

There might be some subtleties due to the service manager itself not having
created the object, but I think it can deal with that.

Comment 39

[Chak Nanga]

Attachment: Changes to include Conrad's last review...

Comment 40

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

Adding myself and Peter Van der Beken to Cc. XML Extras and XSLT components
currently use appshell to get loaded at startup, which is not available in the
embedding world. There are probably others... Jud pointed me to this bug, and it
looks like this is just the solution we need to get our stuff working
everywhere. It would be great if this landed ASAP so we could convert our
components in time for 0.9.

Comment 41

[Chak Nanga]

Fix to this issue was checked in yesterday and it seems to be building fine on 
the  Mac.

However, there was a screwup with the makefile names (w.r.t. case sensitivity) 
for Windows/Unix during the checkin. I tried to fix it and it majorly screwed up 
the CVS server last night. So, the appstartup dir was disconnected from the 
build process on Unix/Windows so we can move along.

I'm still having some issues trying to get the correctly named makefiles back 
into CVS.

I need someone to help me to:
 - Fix the issue (of checking out/checking in) with the makefiles in appstartup 
dir

- Then i can re-add the changes which were backed out last night.

Any volunteers to help me?

Thanks

Comment 42

[Alec Flett]

the only people who can help are people in mozilla.org.. talk to
[e-mail] or [e-mail] and/or get on IRC

Comment 43

[Chak Nanga]

All the files pertaining to this bug are now checked in.

Marking fixed.

Comment 44

[Michael Dunn]

Correction: Changing QA contact for the Embed API bugs to David Epstein.

Comment 45

[David Epstein]

verifying:
* nsAppStartupNotifier component built in msModule.cpp and makefiles.
* in nsAppStartupNotifier.cpp, nsAppStartupNotifier::Observe() method, the 
startup observer object is of type  nsCOMPtr<nsIObserver>. It calls 
do_GetService() if the service index = 0, otherwise it creates an instance. Then 
startupObserver->Observe() is called (if rv is successful).
* In nsEmbedAPI.cpp, NS_InitEmbedding() method, the startup observer object is 
of type nsCOMPtr<nsIObserver>. Calls do_CreateInstance(). Then, calls 
mStartupNotifier->Observe()