99838 - xpconnect should not require DOM

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P2)

Description

[John Bandhauer]

Folks are trying to clean up the rat's nest of inter-module #includes. 

xpconnect relies on DOM headers and perhaps it should not need to.

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/makefile.win#40
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcprivate.h#97

#include "nsIScriptContext.h"  // used to notify: ScriptEvaluated

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappedjsclass.c
pp#80

Maybe we could use one of the more generic signaling mechanisms (or interfaces) 
to send this ScriptEvaluated message. Then we could remove the dependency from 
this lower level xpconnect module on the higher level DOM module. I expect we 
don't want the overhead of using the observer service. But we could maybe use a 
more generic interface to receive the message - or declare such an interface in 
xpconnect - to keep the REQUIRES relationship one-way.

Are there other such dependencies in xpconnect on DOM?

Comment 1

[Alec Flett]

Adding it to the dependency list, because we're doing all kinds of wierd depend
fixing

Comment 2

[David Bradley]

Try this patch on for size. jst, are the DOM changes ok with you?

This is a simple solution, I'm wondering should we supply a more extensible
notification system. Are there other things users might want to get notified about?

Comment 3

[David Bradley]

Attachment: Removes DOM dependency

Comment 4

[David Bradley]

Meant to mention I compiled this under Linux as well. I'll also need some
assistence when it comes to Mac because of the new IDL file.

Comment 5

[John Bandhauer]

dbradley:

I'm OK with the general scheme. A few things...

You can remove the #ifndef XPCONNECT_STANDALONE conditionals around all this 
stuff in xpconnect - it was only there to limit the DOM dependency in the 
non-standalone build.

My inclination with *some* things was to just put multiple interface 
declarations in existing idl files to avoid the build change. But, I suppose 
this time it probably makes sense to seperate this.

I'm a *little* iffy with matching the sig for the ScriptEvaluated method in the 
other interface and then having the implmentation do "double duty". This 
certainly simplifies things, but it is kind of a hidden oddity. jst ought to 
comment on his thoughts on the changes from the DOM perspective.

Comment 6

[David Bradley]

Yes, the ScriptEvaluated method caused me to wince. I error'ed on the side of
least impact to DOM. But I would feel better to have it as a separate method as
well. I'll defer to jst on this issue.

I considered adding the interface to an existing IDL file to save me the pain of
build changes. I just didn't see anything that jumped out at me, other than
maybe nsIXPConnect.idl. Given what I saw jst go through this morning with adding
an IDL I'm not unwilling to rethink that decision ;-)

#ifdef's will be zapped.

Comment 7

[Johnny Stenback (:jst)]

Yeah, piggybacking on the current ScriptEvaluated() method seems iffy, I was
staring at the patch for a while wondering how this can even compile before I
realized :-)

I'd vote for renaming the method, but I can't think of a better name, maybe
something like JSExecuted(), but that gets funny with the method name
capitalization we typically do, hmm. Or maybe ScriptExecuted()?

Other than that I'm ok with the changes.

Comment 8

[David Bradley]

My initial thought was naming it just Evaluated, since the interface would give
the context. The name would also appear in the class where the context may not
be as clear and possibly confusing with ScriptEvaluated. So I guess I fine with
your suggestion of ScriptExecuted.

So ScriptExecuted would just forward to the existing ScriptEvaluated correct?
And one last question, is there any reason to have a boolean parameter to
indicate termination? It's currently always set to true when called from
XPConnect. I don't want to close any doors unecessarily.

Comment 9

[David Bradley]

This patch elimates the #ifndef XPCONNECT_STANDALONE in the CPP, they were still
needed in the header, as it causes the define of XPC_USE_SECURITY_CHECKED_COMPONENT

Method name was changed to ScriptExecuted and it takes no parameters and I added
the new implementation to the nsJSContext

I added some comments.

Lastly I noticed the include of nsIXPCScriptNotify.h is only used by the one
CPP. My first thought was to yank it into the CPP. But after looking around the
convention seems to be put everything in xpcprivate.h so I left it. Wasn't sure
if this was just a convention or if there was another reason for this.

Comment 10

[David Bradley]

Attachment: Revised patch to remove DOM dependancy from XPConnect

Comment 11

[Johnny Stenback (:jst)]

Use 2 spaces for indentation in nsJSEnvironment and not 4:

+NS_IMETHODIMP
+nsJSContext::ScriptExecuted()
+{
+    return ScriptEvaluated(PR_FALSE);
 }

Other than that, r/sr=jst

Comment 12

[John Bandhauer]

nits...

+  NS_IMETHOD ScriptExecuted(); // From nsIXPCScriptNotify

I encourage use of the generated declaration macro - even if there's only one
method. jst might prefer otherwise in his zone.

     // If this is a DOM JSContext, then notify nsIScriptContext of script
     // completion so that it can reset its infinite loop detection mechanism.

Please fix the comment.

-nsIXPCScriptable.idl
\ No newline at end of file
+nsIXPCScriptable.idl
+nsIXPCScriptNotify.idl

Your version has the missing eol, right?

And, the Mac project needs to be modified too, right?

Keep an eye on the license - if you don't beat the relicensing tree whackage 
then you'll have to fix this one by hand. (note you copied 1998 here - I do that 
too!)

Otherwise, r=jband

Comment 13

[David Bradley]

Attachment: Removes xpconnect's dependancy on DOM

Comment 14

[David Bradley]

I updated the license, comment, etc.

Check the comment, as I reworded it somewhat to be more generic.

This patch to MANIFEST does not have the eol at the end. I was keeping it the
same as the original. I meant to do that in the previous patches but it slipped
past me. I assume it's absence isn't a problem since it was originally missing.

Comment 15

[John Bandhauer]

> This patch to MANIFEST does not have the eol at the end.

I was asking to verify that you'd added the new line, not suggesting you don't 
add it.

Please fix these missing eols when you find them. Our build might work either 
way, but some text processing tools will ignore such lines and who knows who'll 
get burned by that in the future.

My point about the Mac build project is that you still need to add the idl file 
to the project - not just the MANIFEST). That would be 
mozilla/js/macbuild/XPConnectIDL.mcp

It looks like http://camelot is working these days.
jst: Do you use camelot? Or do it my hand on a Mac?

Otherwise r/sr=jband (no need to post another patch for the eol nit)

Comment 16

[David Bradley]

Oops sorry about the EOL, I misread your comment. I'll keep a look out in the
future for missing EOL's at EOF.

I can get to Camelot. So can I do my regular commit and then go in there and add
the file and it will automatically update the CodeWarrior project in CVS?

Comment 17

[John Bandhauer]

> I can get to Camelot. So can I do my regular commit and then go in there and 
> add the file and it will automatically update the CodeWarrior project in CVS?

That's what is supposed to happen. It is good to have a backup plan - someone 
with a current build to help you via #mozilla is good.

In the past I have sometimes added a placeholder source file ahead of time and 
then updated the makefiles and Mac project. Then you can see in the tinderbox 
build logs that it really got pulled and built. After that you can check in the 
real files. This is not necessary, but it avoids possible build bustage. You do 
have to be careful in making/adding a placeholder and then modifying it locally 
with your real file and commiting that. In this case you could just put in the 
real idl file and xpconnect build changes and then add the changes to the h/cpp 
files that depend on it afterwards. Or you could just jam it all in and trust 
camelot to do what you expect when you expect it.

Comment 18

[Johnny Stenback (:jst)]

I've kinda given up on camelot since every time when I need it it doesn't
respond, so after trying to use it for numerous changes I gave up even trying,
maybe camelot is working better now, who knows. I usually ask
[e-mail] or [e-mail] or someone else who happens to have a
mac to help out with project file changes.

Comment 19

[David Bradley]

Patch applied.

Camelot appears to have worked. One thing I noticed is that the last file in the
list needs to have a linefeed. I'm pretty sure the first time I tried it, I
didn't put one in and it failed. I added one the second time and it worked.

Comment 20

[Phil Schwartau]

Marking Verified -