238324 - Implement JavaScript code-sharing module system

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Alex Fritze]

We haven't got any proper facilities for sharing JavaScript code - neither
between JavaScript-implemented XPCOM modules nor between XUL windows or XBL
bindings. I don't mean an include-facility (as implemented by
mozIJSSubScriptLoader) where the included scripts get evaluated in the master
script's context everytime they are included somewhere, but a library-facility
where shared scripts only get evaluated once and some of their symbols can be
dynamically imported into other scripts.

The idea is something like this:

---------------------------------------------------------
// myUtils.js
MOZ_EXPORTED_SYMBOLS = ['XXXUtils', 'FOOFunc', 'ABCConst'];

var XXXUtils = { ... };
function FOOFunc(...) {...};
const ABCConst = "@mozilla.org/foobar;1";
...
---------------------------------------------------------
// myComponent.js
importModule('resource:/jscodelib/myUtils.js');

function bar() { XXXUtils.xxx(); ... FOOFunc(); .... }
----------------------------------------------------------

Patch coming up.

Comment 1

[Alex Fritze]

Attachment: jscodelib - first attempt

This patch adds the jscodelib facility.

Basic Operation:

Components.classes["@mozilla.org/jscodelib;1"].getService(C.I.mozIJSCodeLib)
	  .importModule('resource:/jscodelib/utils.js');
installs all symbols in the array MOZ_EXPORTED_SYMBOLS in utils.js as
properties
on the caller's global object. If utils.js gets imported into more than 1 file,
its exported objects will be shared between all files; utils.js only gets
evaluated once.

For convenience, the JS component loader defines the JS function 
importModule(), which is functionally equivalent to the XPConnect version
above.

importModule() (both the xpconnect & js version) takes an object as an optional
2. argument. If given, the exported symbols will be installed on this object
instead of the global object.

Instead of using the jscodelib as a service where imported modules will be
shared by all clients, it can also (simultaneously) be used as a component,
enabling private code collections:
var collection =
Compts.classes["@mozilla.org/jscodelib;1"].createInstance(C.I.mozIJSCodeLib);


The patch also adds a file JSComponentUtils.js which can be used by
JS-implemented XPCOM modules to automatically generate the boilerplate
nsIModule and factory implementations, e.g.:

importModule('resource:/jscodelib/JSComponentUtils.js');
...
var NSGetModule = ComponentUtils.generateNSGetModule(
  [{
    className  : "FilePicker JS Component",
    cid        : FILEPICKER_CID,
    contractID : FILEPICKER_CONTRACTID,
    factory    : ComponentUtils.generateFactory(nsFilePicker)
  }]);

Comment 2

[Alex Fritze]

Brendan, Robert: Can I get you guys to give me some feedback on this?

Comment 3

[Alex Fritze]

Attachment: codelib - 2nd attempt

Adding updated patch. I accidentally put some unrelated stuff into the first
patch.

Comment 4

[Alex Fritze]

Attachment: codelib - 3rd attempt

Updated patch. I forgot to add js/src/xpconnect/loader/JSComponentUtils.js in
the previous patch.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 144554 [details] [diff] [review]
codelib - 3rd attempt

Shaver should definitely review.

Cool stuff -- a couple of nits below.

>+  JSContext *ctx = nsnull;
>+  cc->GetJSContext (&ctx);

Canonical name is cx, not ctx.

>+    if (!JS_ConvertArguments(ctx, argc, argv, "*o", &targetObject)) {

Seems like overkill, compared to just calling JS_ValueToObject(cx, argv[1],
&targetObject).

>+    // Our targetObject is the caller's global object. Find it by
>+    // walking the calling object's parent chain.
>+    
>+    nsCOMPtr<nsIXPConnectWrappedNative> wn;
>+    cc->GetCalleeWrapper (getter_AddRefs(wn));
>+    if (!wn) {
>+      NS_ERROR("null callee wrapper");
>+      return NS_ERROR_FAILURE;    
>+    }
>+    wn->GetJSObject (&targetObject);
>+    if (!targetObject) {
>+      NS_ERROR("null calling object");
>+      return NS_ERROR_FAILURE;
>+    }
>+    
>+    JSObject *parent = nsnull;
>+    while ((parent = JS_GetParent(ctx, targetObject)))
>+      targetObject = parent;

Nice to see correct static scoping being observed!

>+  JSObject *moduleObj=nsnull;

Discordant style of = operator with no space on both sides.

>+  JSContext*ctx=nsnull;

Ditto, and cx is canonical name...

>+  cxstack->GetSafeJSContext(&ctx);

...especially in light of cxstack name here!

>+  instream->Read(*buf, content_length, &bytesRead);
>+  if (bytesRead!=content_length) {

Ouch, spaces are nicer around operators.  Etc. below.

I'll give an sr when shaver has stamped r=.

/be

Comment 6

[Alex Fritze]

Attachment: updated patch

Thanks for the comments, Brendan. Here's an updated patch.

Comment 7

[Alex Fritze]

Attachment: updated patch

Updated patch for the nsIScriptObjectPrincipal deCOMtamination changes (bug
240745).

Comment 8

[Brendan Eich [:brendan]]

Alex, do you want shaver and me to review the updated patch?  If so, go ahead
and request review.  Thanks,

/be

Comment 9

[Alex Fritze]

Comment on attachment 146412 [details] [diff] [review]
updated patch

Yes please! :-)

Comment 10

[Alex Fritze]

Just as an aside...
I'm experiencing some xpconnect NewResolve() weirdness with the BackstagePass
global object. The symptom is that in debug builds, if I do 'dump(this)', I get
"[BackstagePass]". If, on the other hand, I do an implicit cast, as in 'dump("
"+this)', I get "[BackstagePass @ XXXXX]".
In the first case mozBackstagePass::NewResolve() is called to locate "toString"
(which isn't found). In the second case mozBackstagePass::NewResolve() is *not*
called and we somehow rightly end up in XPCWrappedNative::ToString(). What is
the right way of getting the first case to resolve to
XPCWrappedNative::ToString() as well? It seems to work for all the DOM classes.
I stepped through this for an hour or so, but I don't fully understand what is
going on. 
 

Comment 11

[Brendan Eich [:brendan]]

Maybe the thing to do is grab shaver or me on IRC when you've got things in the
debugger....

/be

Comment 12

[Alex Fritze]

Attachment: new patch

Apologies if you have started reviewing...
I've updated the patch with 2 additions:

- mozIJSCodeLib now has a new method 'probeModule(url)' which loads the given
module if it isn't loaded yet and returns its global object.

- jsloader implements a new interface 'mozIJSComponentLib' with a method
'probeComponent(location)', which similarly loads a js xpcom component and
returns its global object.

The rationale for having these methods is so that we can dynamically modify js
code modules & components while Mozilla is running. E.g. JSSh's xemacs script
now honours the variable 'moz-jssh-buffer-globalobj'. If this is given, e.g. as
a file variable, as in 

-*- moz-jssh-buffer-globalobj: 
"Components.classes['@mozilla.org/jscodelib;1']
  .getService(Components.interfaces.mozIJSCodeLib)
  .probeModule('resource:/jscodelib/JSComponentUtils.js')" -*-,

a call to moz-jssh-eval-buffer (C-c m e) will execute the buffer's code in the
given global object.

Comment 13

[Darin Fisher]

Comment on attachment 147317 [details] [diff] [review]
new patch

>+mozJSCodeLib::LoadURL(char **buf, const nsACString &url)

>+  channel->Open(getter_AddRefs(instream));
>+  if (!instream) {
>+    NS_WARNING("could not open stream");
>+    goto failure;
>+  }
>+
>+  if (NS_FAILED(channel->GetContentLength(&content_length))) {
>+    NS_WARNING("could not get content length");
>+    goto failure;
>+  }
>+  
>+  *buf = new char[content_length+1];
>+  if (!*buf) {
>+    NS_WARNING("could not alloc buffer");
>+    goto failure;
>+  }
>+
>+  instream->Read(*buf, content_length, &bytesRead);
>+  if (bytesRead != content_length) {
>+    NS_WARNING("stream read error");
>+    goto failure;
>+  }
>+
>+  return content_length;

better not depend on the return value of GetContentLength.  the API
allows the nsIChannel to return -1 !!

you should instead use the value returned from instream->Available().
and you will need to write a loop.  something like this:

  nsCString buf;
  PRUint32 len, n;
  nsresult rv;

  for (;;) {
    rv = instream->Available(&len);
    if (NS_FAILED(rv) || len == 0)
      break;

    buf.SetLength(buf.Length() + len);
    char *p = buf.EndWriting() - len;

    rv = instream->Read(p, len, &n);
    if (NS_FAILED(rv) || n != len)
      break;
  }

i'd make LoadURL take a |nsCString &buf| out param.

also, beware that if you call nsIChannel::open on a http:// URI or
some other remote resource, the Open, Available, and Read calls may
block the calling thread (and hence the browser UI) for lengthy
periods of time.  moreover, calls to these methods may lead to 
PLEvents being dispatched, which could cause your code to be
re-entered.  that may or may not be a concern.	if you are loading
a local URI (e.g., file://), then this is not a problem.

Comment 14

[Alex Fritze]

Attachment: updated patch

Patch updated to trunk & incorporating Darin's feedback.

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 187113 [details] [diff] [review]
updated patch

I'll take a look at this this weekend.

Alex: does this incorporate the single-load/shared-object stuff we talked about
at XTech?

Comment 16

[Alex Fritze]

(In reply to comment #15)
> Alex: does this incorporate the single-load/shared-object stuff we talked about
> at XTech?

Shaver:
It works much like the js component loader.
Code modules are loaded and evaluated with a new BackstagePass the first time
they are asked for (by 'importModule()') and a reference to the global object is
stored in a hash table. All symbols listed in MOZ_EXPORTED_SYMBOLS are then set
as properties on the caller's global object. 
Next time the module is requested the script won't be re-evaluated; the
MOZ_EXPORTED_SYMBOLS will be taken straight from the hashed global object.

Comment 17

[Alex Fritze]

Attachment: patch v8

Patch updated for trunk changes.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 188027 [details] [diff] [review]
patch v8

I understand better where this is headed now, thanks.

I'd like to see it hung off of Components.util, where it a) doesn't pollute the
global namespace for components, and b) is also accessible to chrome.  We've
had many painful experiences in calendar/OMX land recently in which we wanted
to share utility code between components and chrome.
We don't need or want, IMO, the configure switch.  Honestly, I'd rather see us
lose the jsloader config switch too.  There are too many configuration knobs,
and it just weakens our platform to not know what's set where.	I'd also rather
see us not add another shared/ directory.   Put the ContextHelper def'n in
xpcprivate.h, rename and de-static the sandbox's dump/debug from
xpccomponents.cpp to use in all 3 contexts (sandbox, components, code-module),
and lose both JSFunctions.h and JSFunctions.cpp.  (That way I don't have to
tell you to rename them to avoid using the sacred "JS" prefix, too. =) )

In terms of how the importing works, I think I'd like to see us support some
way to import into an existing object, such that, for example, your
ComponentUtils functions could be loaded right into
Components.utils.generateFactory and .generateModule.  We don't have to do that
right now, but I wanted to write it down here somewhere, and we should make
sure that we like what happens in these scenarios:

1)
EXPORTED_SYMBOLS = ["Foo.Bar"];

2)
Components.util.importModule("exports-Whatever.js");
Whatever.newPropertyFoo = "blip";
Components.util.importModule("exports-Whatever.js");

I am really not in love with the mozI prefix, I have to say.  I know, I started
it back in the day, but I'd rather see the remaining interface be
xpcICodeLoader or something.  In fact, I think I'm going to want some other
interface points on the component loader for something else I'm playing with;
what do you think of just calling it xpcIJSComponentLoader?

"probeComponent" doesn't seem like a great name to me: can we use importModule
in both places?

Thanks for sticking with this; I think it'll be a real boon for us.

Comment 19

[Alex Fritze]

(In reply to comment #18)
[...]
> In terms of how the importing works, I think I'd like to see us support some
> way to import into an existing object, such that, for example, your
> ComponentUtils functions could be loaded right into
> Components.utils.generateFactory and .generateModule.  We don't have to do that
> right now, but I wanted to write it down here somewhere, 

Note that importModule() takes an optional 'targetObject' arg, so you can do:

var a = {};
importModule("foo.js", a);

Here, all exported symbols from foo.js would be installed as properties on 'a'
instead of the global object.

> and we should make
> sure that we like what happens in these scenarios:
> 
> 1)
> EXPORTED_SYMBOLS = ["Foo.Bar"];
Hmm, do you think this would be of much use in practice? At the moment only
top-level properties can be exported.
 
> 2)
> Components.util.importModule("exports-Whatever.js");
> Whatever.newPropertyFoo = "blip";
> Components.util.importModule("exports-Whatever.js");
This should work fine. The second importModule() will have no effect. Whatever
will have newPropertyFoo=='blip' everywhere it is being imported to (or has been
imported to).

Comment 20

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #19)
> Note that importModule() takes an optional 'targetObject' arg, so you can do:
> 
> var a = {};
> importModule("foo.js", a);

Yeah, or the code could just mutate Components.util in its top-level script (or
from an EXPORTED_SYMBOLS getter, woo) and omit those from the list it returns.

> > 1)
> > EXPORTED_SYMBOLS = ["Foo.Bar"];
> Hmm, do you think this would be of much use in practice? At the moment only
> top-level properties can be exported.

I'm not sure, but I bet that someone writing that code expects to be setting

 global.Foo.Bar

and not

 global["Foo.Bar"]

Want to toss up a message via reportError if there's a . in the symbol name, and
omit it from the list?

> > 2)
> > Components.util.importModule("exports-Whatever.js");
> > Whatever.newPropertyFoo = "blip";
> > Components.util.importModule("exports-Whatever.js");
> This should work fine. The second importModule() will have no effect. Whatever
> will have newPropertyFoo=='blip' everywhere it is being imported to (or has been
> imported to).

Quite right (and righteous).

I'm not sure how I feel about MOZ_EXPORTED_SYMBOLS vs. EXPORTED_SYMBOLS. 
Probably fine, and I'm just knee-jerking against MOZ_ification.

Also: do you want to set __LOCATION__ here like I do for JS components?  Might
help libraries find their sub-libraries/dependencies.  (And in the nearish
future, I'd like to finish off bug 291374, which would want to find this, I
_think_.)  It'd also let people parameterize their library imports, through
query strings, though that's getting a little arcane.

I thought about the global/this stuff a bit, and I think it's OK: though a
library implementor has to write a bit of code to find the global for the
current call, the common case (calling functions in the same library, including
its own importModule calls) is what people expect.  I like that those imports
are "private" too, by default.

I have hopes that JS2 or JS1.9 or JS3000 will give us additional language
features to help package, version, and import code in pieces like this, but
until then -- and perhaps beyond! -- this looks great.

Comment 21

[Alex Fritze]

(In reply to comment #18)
> "probeComponent" doesn't seem like a great name to me: can we use importModule
> in both places?

Hmm, how about getComponent() instead of probeComponent() and getModule()
instead of probeModule()? 
These functions are different to Component.utils.importModule in that they
return the global object of the component or module instead of looking at
EXPORTED_SYMBOLS.

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #21)
> Hmm, how about getComponent() instead of probeComponent() and getModule()
> instead of probeModule()? 
> These functions are different to Component.utils.importModule in that they
> return the global object of the component or module instead of looking at
> EXPORTED_SYMBOLS.

What about making importModule return the global object, and losing probeModule
altogether?  If importModule is explicitly passed null, it doesn't load into the
caller's context, but just returns the global object.  importModuleToJSObject
would behave similarly.  We'd lose the force parameter, but I don't especially
like it anyway.

Also, I just noticed that you're using the safe context to compile the scripts.
 On the trunk, we now use mContext for all our compilation needs, to avoid
problems with context reuse.

Comment 23

[Alex Fritze]

Attachment: patch v9

Ok, how about this?
This patch should address all review comments apart from checking the exported
symbols for ".". 
I've also added some code to set the __LOCATION__ property if the module url
points to a local file.

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 188292 [details] [diff] [review]
patch v9

>+++ js/src/xpconnect/Makefile.in	4 Jul 2005 20:52:10 -0000	1.26.16.2
>@@ -46,6 +46,8 @@ include $(DEPTH)/config/autoconf.mk
> MODULE		= xpconnect
> DIRS		= public idl
> 
>+DIRS         += codelib
>+

Just stick that into the previous line.

>+     * The implementation maintains a hash of moduleURL->global
>+     * obj. Subsequent invocations of importModule with the same
>+     * 'moduleURL' will not cause the file at 'moduleURL' to be
>+     * re-evaluated.

..., but the symbols in EXPORTED_SYMBOLS will be exported into
the specified targetObj as above.

>Index: js/src/xpconnect/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/xpconnect/src/Makefile.in,v
>retrieving revision 1.81
>retrieving revision 1.80.2.3
>diff -u -p -r1.81 -r1.80.2.3
>--- js/src/xpconnect/src/Makefile.in	29 Jun 2005 14:22:52 -0000	1.81
>+++ js/src/xpconnect/src/Makefile.in	4 Jul 2005 20:52:12 -0000	1.80.2.3
>@@ -62,6 +62,7 @@ PACKAGE_FILE = xpconnect.pkg
> REQUIRES	= xpcom \
> 		  string \
> 		  js \
>+		  jsloader \

If we put the IDL in idl/, instead of the loader/ dir, then it
seems like we don't have to do nearly as much Makefile whacking here.  That
loader is a separate directory is a historical artifact, from when it used to
be its own component.

(See also below about just merging the component loader and
the code-lib loader.)

>Index: js/src/xpconnect/src/xpccomponents.cpp
>+JSBool JS_DLL_CALLBACK
>+JSDump(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+JSBool JS_DLL_CALLBACK
>+JSDebug(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)

We shouldn't use the "JS" prefix here; that belongs to the JS engine. 
XPCScriptDebug and XPCScriptDump?

> #ifdef XPC_USE_SECURITY_CHECKED_COMPONENT
> /* string canCreateWrapper (in nsIIDPtr iid); */
> NS_IMETHODIMP
>+NS_IMPL_THREADSAFE_ISUPPORTS2(mozJSComponentLoader,
>+                              nsIComponentLoader,
>+                              xpcIJSComponentLoader)

How do you feel about just making the JS component loader implement
xpcIJSCodeLoader?  It looks like it could share a decent amount of
bootstrapping code, as well as the compilation context, principal, etc.  (Maybe
even use the same hash table for components and modules?)

(That would also let us leave the contexthelper and errorreporterhelper in
mozJSComponentLoader.cpp, though that's not really an interesting motivation
either way.)

>+    jsval *retval = nsnull;
>+    cc->GetRetValPtr(&retval);
>+    if (*retval)
>+        *retval = global ? OBJECT_TO_JSVAL(global) : JSVAL_NULL;

This doesn't look right to me; shouldn't you be testing |retval|, rather than
|*retval|?  Also, it's legal to call
OBJECT_TO_JSVAL on null, so you don't need the alternation.

>Index: js/src/xpconnect/loader/JSComponentUtils.js

Let's name this "ComponentUtils.js" to match its exported symbol?

>+#ifdef DEBUG
>+    printf("] onto %s\n", PromiseFlatCString(moduleName).get());
>+#endif

"from" here rather than "onto"?  (Though I should really convert the JS
component loader and this to NSPR logging as well someday, that day is not
today.)

I think we're almost there!

Comment 25

[Alex Fritze]

(In reply to comment #24)
> 
> How do you feel about just making the JS component loader implement
> xpcIJSCodeLoader?  It looks like it could share a decent amount of
> bootstrapping code, as well as the compilation context, principal, etc.  (Maybe
> even use the same hash table for components and modules?)

I like the idea. Code modules could just go into the mGlobals hash. 
I'll try to make some time for this in the next few weeks.

Comment 26

[Alex Fritze]

Attachment: patch v10

This new, improved, slimmer patch implements the module loading code on the component loader as shaver suggested, and thus takes advantage of bryner's optimizations from bug#279839 :-)

This is what is does:

- The component loader implements a new interface xpcIJSModuleLoader.idl which implements two methods: importModule() & importModuleToJSObject(). The former is to be called from JS only, the latter is the [noscript] version.

- xpccomponents implements a shortcut to importModule: Components.utils.importModule().

- importModule() can be used in 3 ways:

1.
Components.utils.importModule('rel:foo.js', null);
returns the files global object. 

2.
Components.utils.importModule('rel:foo.js');
installs all properties found in the array EXPORTED_SYMBOLS (if any) in foo.js onto the caller's global object (and returns foo's global object).

3.
var bar = {};
Components.utils.importModule('rel:foo.js', bar);
installs all properties from EXPORTED_SYMBOLS onto 'bar' (and returns foo's global object).

I've found that all of these three uses are actually useful.

- The component loader maintains a cache of the global objects of files imported in this way, so a file will only be evaluated once per Mozilla session (which is  of course the whole point of this patch).

- Whether or not a JS implements nsIModule is irrelevant to the operation of importModule. (*) see also below

- In contrast to the earlier patches, importModule expects a registry location rather than a URI. I've found that often you want a js file to both implement an xpcom module (i.e. implement nsIModule) AND, when using it from script, be accessible directly via EXPORTED_SYMBOLS or by getting the global object, so locating all js modules (be they xpcom, non-xpcom or both) in bin/components seemed to make sense. 
One slight problem with this is that the loader will try to autoregister js files that don't implement nsIModule. While this is benign it could be a performance problem, so I have started giving non-xpcom js files the prefix *.jsm instead of *.js.

- The patch comes with ComponentUtils.jsm, which can be imported by js modules to implement the nsIModule boilerplate.

(*) The one thing I am not entirly happy about is the ambiguous naming introduced by this patch. 'JS Module' can now mean xpcom module or js module or both. Any thoughts welcome.

Comment 27

[Alex Fritze]

Comment on attachment 201132 [details] [diff] [review]
patch v10

This is now all broken because of 316416. *sigh*

Comment 28

[Alex Fritze]

Since #316416 is backed out, v10 of the patch is working again. 

I also have an updated version that works with the changes on #316416 (reverting back from a single hashtable to separate globals/modules hashtables).

Comment 29

[Alex Fritze]

Changing to more appropriate product/component.

Comment 30

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 201132 [details] [diff] [review]
patch v10

On scan, this looks great.  We should not let MOZ_JSLOADER continue to live, so don't bother with it here.

The DEBUG printfs should be to stderr, or more likely DEBUG_xpchacker or use PR_LOG.

Pinging brendan for input here, since he's designing package and import for JS1.9 now.  I'll get back to this tomorrow to stamp and nit-pick.

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 201132 [details] [diff] [review]
patch v10

>+#ifdef MOZ_JSLOADER
>+    nsCOMPtr<xpcIJSModuleLoader> moduleloader = do_GetService(MOZJSCOMPONENTLOADER_CONTRACTID);
>+    if (!moduleloader)
>+        return NS_ERROR_FAILURE;
>+    return moduleloader->ImportModule(registryLocation);
>+#else
>+    return NS_ERROR_NOT_AVAILABLE;
>+#endif

I boldly propose that we eliminate MOZ_JSLOADER ifdefs, but that's not
a job for this patch.

>+    }
>+    else {

We cuddle our elses in the JS loader!

>+        // Our targetObject is the caller's global object. Find it by
>+        // walking the calling object's parent chain.
>+    
>+        nsCOMPtr<nsIXPConnectWrappedNative> wn;
>+        rv = cc->GetCalleeWrapper(getter_AddRefs(wn));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        
>+        wn->GetJSObject (&targetObject);
>+        if (!targetObject) {
>+            NS_ERROR("null calling object");
>+            return NS_ERROR_FAILURE;
>+        }
>+        
>+        JSObject *parent = nsnull;
>+        while ((parent = JS_GetParent(cx, targetObject)))
>+            targetObject = parent;
>+    }

Is this going to run afoul of inner/outer stuff?  (I really have no 
idea, and will hopefully remember to cc: mrbkap after I finish this
review.)

>+    JSObject *globalObj = nsnull;
>+    rv = ImportModuleToJSObject(registryLocation, targetObject, &globalObj);
>+
>+    jsval *retval = nsnull;
>+    cc->GetRetValPtr(&retval);
>+    if (*retval)
>+        *retval = globalObj ? OBJECT_TO_JSVAL(globalObj) : JSVAL_NULL;

It's legal to use OBJECT_TO_JSVAL on null, so just

if (*retval)
  *retval = OBJECT_TO_JSVAL(globalObj);

>+    if (globalObj)
>+        *_retval = globalObj;
>+    else {
>+        *_retval = nsnull;
>+        return NS_ERROR_FAILURE;
>+    }

If else-block has braces, then-block must also.

>+        for (jsuint i=0; i<symbolCount; ++i) {

Spaces around = and <, please.

>+            if (!JS_GetProperty(mContext, globalObj, JS_GetStringBytes(symbolName), &val)) {
>+                NS_ERROR("Could not get symbol");
>+                return NS_ERROR_FAILURE;
>+            }
>+            
>+            if (!JS_SetProperty(mContext, targetObj, JS_GetStringBytes(symbolName), &val)) {
>+                NS_ERROR("Could not set property on target");
>+                return NS_ERROR_FAILURE;

Should those set ExceptionWasThrown to propagate errors more usefully? (With some source info, etc.)

>Index: js/src/xpconnect/loader/ComponentUtils.jsm

I'm leaving the utils thing unreviewed for now, though the patch would be very welcome in 61789, given the code sharing system.  (My 2004 reassignment looks positively prescient now, no?)

r+, please fix above stuffs, and accept my insufficient apologies for the delay.

Comment 32

[Georg Maaß]

Where is this interface available? Is it part of SpiderMonkey and therefor available everywhee including XULRunner 1.9 or is it part of SeaMonkey?

Comment 33

[Nickolay_Ponomarev]

This hasn't been checked in, so nowhere, unless you patch and build yourself.

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Alex, do you have a plan for this patch? Is this something that you plan to resurrect?

Comment 35

[Brendan Eich [:brendan]]

Sayrer may want to help get this landed. I'll promptly review an updated patch that addresses shaver's comments.

/be

Comment 36

[Robert Sayre]

working on it right now, figuring out what bug 316416 did to mozJSComponentLoader::GlobalForLocation

Comment 37

[Robert Sayre]

Attachment: (WIP) update to trunk

doesn't compile due to API changes in GlobalForLocation, just saving work

Comment 38

[Robert Sayre]

Attachment: (WIP) Components.utils.importModule

This works, but it needs refactoring, since it copies a bunch of code from LoadModule. Also haven't gotten to shaver's comments.

Given this file
--------------------------------------

EXPORTED_SYMBOLS = ["foo"]

function foo(a, b) {
  return "arg1: " + a + " arg2: " + b + "." + 
  " Called with " + arguments.length + " arguments.";
}

--------------------------------------

this is how you use it from the shell:

--------------------------------------

js>  Components.utils.importModule("/home/sayrer/test.js")
Installing symbols [ foo ] from /home/sayrer/test.js
[object BackstagePass @ 0x61e3f0 (native @ 0x71a308)]
js> foo("hmm", "yeah", "something else")
arg1: hmm arg2: yeah. Called with 3 arguments.
js> 

--------------------------------------

Comment 39

[Robert Sayre]

(In reply to comment #38)
> 
> js>  Components.utils.importModule("/home/sayrer/test.js")

Oh, this is wrong too, it needs to take chrome URLs.

Comment 40

[Robert Sayre]

Attachment: (WIP) load as before

This patch loads URLs in the same manner as the patch reviewed so long ago. It uses a method on nsIComponentManagerObsolete to get this done. Basically, the "rel:" scheme looks for files in dist/bin/components. This isn't as bad as it sounds, since the current ComponentManager still uses this, just doesn't expose it.

This crashes on shutdown in JS_ClearScope, but otherwise seems to work.

Comment 41

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 264440 [details] [diff] [review]
(WIP) load as before

>+    nsAutoPtr<ModuleEntry> entry(new ModuleEntry);

It doesn't look like entry is ever forgotten, so won't this stick a dangling pointer in the hashtable? Maybe that's the source of your crash?

Comment 42

[Robert Sayre]

(In reply to comment #41)
> 
> It doesn't look like entry is ever forgotten, so won't this stick a dangling
> pointer in the hashtable? Maybe that's the source of your crash?

Thanks, that's it. Good catch.

Comment 43

[Robert Sayre]

Attachment: Components.utils.import

Comment 44

[Robert Sayre]

(In reply to comment #31)
> 
> Is this going to run afoul of inner/outer stuff?  (I really have no 
> idea, and will hopefully remember to cc: mrbkap after I finish this
> review.)

I don't know the answer to this either.

> >Index: js/src/xpconnect/loader/ComponentUtils.jsm
> 
> I'm leaving the utils thing unreviewed for now

Should we keep this file? If we do, I propose we rename it XPCOMUtils.js, because it is confusing to write 

Components.utils.import("rel:ComponentUtils.js,")

Once we have a file we can keep, I will write an XPCShell test for it.

Comment 45

[Robert Sayre]

Attachment: Components.utils.import

... now including the added files

Comment 46

[Ben Bucksch (:BenB)]

> EXPORTED_SYMBOLS = ["foo"]
> Components.utils.import("rel:ComponentUtils.js,")
> Components.utils.import("rel:test.js");

Am I the only one who thinks that this is nice as a project internal framework,
but not a proper syntax for all Mozilla users? At the minimum, "foo" should not
be a string, but the (function) object foo, and  the "rel:" should not be
necessary.

IMHO, it should be more like:
definition:
  export function foo() { ... }
usage:
  import "test.js";
  foo();

Comment 47

[Ben Bucksch (:BenB)]

(And, preferably, "all Mozilla users" should include webpages. That would relieve AJAX and co libs from having everything in one file.)

Comment 48

[Robert Sayre]

Attachment: Components.utils.import

I forgot to mention two important details. 

Shaver and I changed the name to "Components.utils.import" because the name module makes it hard to read the source file, since we actually don't want our entry objects to have a |module| member.

The second is that we need two hashtables to guard against a file being imported as both a component and a js library. If it's loaded using Components.utils.import first, it will trigger the "bad hashtable entry" error check in LoadModule.

(updated patch drops a mistakenly included diff, sorry for the bugspam)

Comment 49

[Alex Vincent [:WeirdAl]]

Comment on attachment 264476 [details] [diff] [review]
Components.utils.import

Peanut gallery comments, coming to this fairly late:

I realize the original patch didn't have a xpcshell testcase.  Could we have a supplementary patch to do so?

>+  generateFactory: function(ctor, interfaces) {

Venkman users will thank you if you name your functions:
generateFactory: function generateFactory(ctor, interfaces) {}

Comment 50

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #44)
> Should we keep this file? If we do, I propose we rename it XPCOMUtils.js,
> because it is confusing to write 

I would vote for keeping it... That's boilerplate code that is annoying to write over and over again in JS components.

From comment 26, though, I think you mean XPCOMUtils.jsm, right? That also helps to make sure you're importing files that are designed to be imported.

Comment 51

[Brendan Eich [:brendan]]

Comment on attachment 264476 [details] [diff] [review]
Components.utils.import


>+  /**
>+   * Imports the JS module at 'registryLocation' to the JS object
>+   * 'targetObj' (if != null) as described for importModule() and
>+   * returns the module's global object.
>+   */
>+  [noscript] JSObjectPtr importToJSObject(in AUTF8String registryLocation,
>+				          in JSObjectPtr targetObj);

Nit: importToObject? importInto? The "JS" seems unnecessary, and perhaps the ending word could be more specific, or left out with the right preposition?

>+/**
>+ * Utilities for JavaScript components loaded by the JS component
>+ * loader.
>+ *
>+ * Import into a JS component using
>+ * 'importModule("res:/jscodelib/ComponentUtils.js");'

This is out of date now, right?

[righteous use of closures deleted ;-)]

>+    if (argc > 1) {
>+        // The caller passed in the optional second argument. Get it.
>+        jsval *argv = nsnull;
>+        rv = cc->GetArgvPtr(&argv);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        if (!JS_ValueToObject(cx, argv[1], &targetObject)) {
>+            cc->SetExceptionWasThrown(JS_TRUE);

This is right, IIRC.

>+            return NS_OK;

But shouldn't this be NS_ERROR_FAILURE or the like?

>+        }
>+    }
>+    else {
>+        // Our targetObject is the caller's global object. Find it by
>+        // walking the calling object's parent chain.
>+
>+        nsCOMPtr<nsIXPConnectWrappedNative> wn;
>+        rv = cc->GetCalleeWrapper(getter_AddRefs(wn));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        
>+        wn->GetJSObject (&targetObject);
>+        if (!targetObject) {
>+            NS_ERROR("null calling object");
>+            return NS_ERROR_FAILURE;
>+        }
>+        
>+        JSObject *parent = nsnull;

No reason to initialize parent when the very next line sets it:

>+        while ((parent = JS_GetParent(cx, targetObject)))
>+            targetObject = parent;
>+    }
>+    nsAutoPtr<ModuleEntry> entry(new ModuleEntry);
>+    if (!entry)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+
>+

Nit: doubled empty line.

>+    rv = GlobalForLocation(componentFile, &entry->global, &entry->location);
>+    if (NS_FAILED(rv)) {
>+        *_retval = nsnull;
>+        return NS_ERROR_FAILURE;
>+    } 
>+
>+    *_retval = entry->global;
>+
>+    jsval symbols;
>+    nsCAutoString err;
>+    if (targetObj &&
>+        JS_GetProperty(mContext, entry->global,
>+                       "EXPORTED_SYMBOLS", &symbols)) {

If JS_GetProperty returns false, you want to note the pending exception and fail here.

>+
>+        JSObject *symbolsObj = nsnull;
>+        if (!JSVAL_IS_OBJECT(symbols) ||

Use JSVAL_IS_PRIMITIVE instead of !JSVAL_IS_OBJECT here to handle null as well.

>+        if (!JS_GetArrayLength(mContext, symbolsObj, &symbolCount)) {
>+            err += aLocation;
>+            err.AppendLiteral(" - Error getting array length of ");
>+            err.AppendLiteral("EXPORTED_SYMBOLS.");
>+            NS_ERROR(err.get());
>+            return NS_ERROR_FAILURE;

Need to note pending exception here? Or is this different from the case above (in a different function) where you have to tell xpconnect about that?

>+        }
>+#ifdef DEBUG
>+        printf("Installing symbols [ ");
>+#endif
>+    
>+        for (jsuint i = 0; i < symbolCount; ++i) {
>+            jsval val;
>+            JSString *symbolName;
>+            if (!JS_GetElement(mContext, symbolsObj, i, &val) ||

!JS_GetElement => pending exception on mContext, tell xpconnect about it.

>+                !JSVAL_IS_STRING(val) ||

No pending exception in this case.

>+                !(symbolName = JSVAL_TO_STRING(val))) {

Or this one.

>+                err += aLocation;
>+                err.AppendLiteral(" - EXPORTED_SYMBOLS array element ");
>+                err.AppendLiteral("is not a string.");
>+                NS_ERROR(err.get());
>+                return NS_ERROR_FAILURE;
>+            }
>+#ifdef DEBUG
>+            printf("%s ", JS_GetStringBytes(symbolName));
>+#endif
>+    
>+            if (!JS_GetProperty(mContext, entry->global,
>+                                JS_GetStringBytes(symbolName), &val)) {

Ditto.

>+                err += aLocation;
>+                err.AppendLiteral(" - Could not get symbol '");
>+                err.Append(nsDependentCString(JS_GetStringBytes(symbolName)));
>+                err.AppendLiteral("'");
>+                NS_ERROR(err.get());
>+                return NS_ERROR_FAILURE;
>+            }
>+            
>+            if (!JS_SetProperty(mContext, targetObj,
>+                                JS_GetStringBytes(symbolName), &val)) {

Ditto.

>+                err += aLocation;
>+                err.AppendLiteral(" - Could not set property '");
>+                err.Append(nsDependentCString(JS_GetStringBytes(symbolName)));
>+                NS_ERROR("' on target object.");
>+                return NS_ERROR_FAILURE;
>+            }
>+        }
>+#ifdef DEBUG
>+        printf("] from %s\n", PromiseFlatCString(aLocation).get());
>+#endif
>+    }

The comment at the top of Import about "must be called only from JS" -- you could #ifdef DEBUG some code to test and essentially assert that fact.

Looks good otherwise, minusing pro-forma to get comments addressed.

/be

Comment 52

[Robert Sayre]

Attachment: updated with brendan's comments

still need to add unit tests

Comment 53

[Brendan Eich [:brendan]]

Comment on attachment 264694 [details] [diff] [review]
updated with brendan's comments

Incomplete?

$ lsdiff ~/tmp/sayrer-A
js/src/xpconnect/idl/Makefile.in
js/src/xpconnect/idl/xpcIJSModuleLoader.idl
js/src/xpconnect/idl/xpccomponents.idl
js/src/xpconnect/loader/ComponentUtils.jsm
js/src/xpconnect/loader/Makefile.in
js/src/xpconnect/loader/mozJSComponentLoader.cpp
js/src/xpconnect/loader/mozJSComponentLoader.h
js/src/xpconnect/src/xpccomponents.cpp
$ lsdiff ~/tmp/sayrer-B
js/src/xpconnect/idl/Makefile.in
js/src/xpconnect/idl/xpccomponents.idl
js/src/xpconnect/loader/Makefile.in
js/src/xpconnect/loader/mozJSComponentLoader.cpp
js/src/xpconnect/loader/mozJSComponentLoader.h
js/src/xpconnect/src/xpccomponents.cpp

Ultra-nit: canonical name for JSBool API success/fail result var is "ok", not "success".

/be

Comment 54

[Robert Sayre]

Attachment: Components.utils.import v2

now with unit tests and added files. The tests caught a pretty bad bug. Turns out that you still need to install the symbols, even if you find the module in the cache :)

Comment 55

[Robert Sayre]

Attachment: Components.utils.import v2.1

Comment 56

[Robert Sayre]

Attachment: Components.utils.import v2.2 (fix ultra-nits)

Comment 57

[Brendan Eich [:brendan]]

Comment on attachment 264755 [details] [diff] [review]
Components.utils.import v2.2 (fix ultra-nits)

>+    jsval symbols;
>+    nsCAutoString err;
>+    if (targetObj) {
>+        if (!JS_GetProperty(mContext, mod->global,
>+                            "EXPORTED_SYMBOLS", &symbols)) {
>+            if (cc)
>+                cc->SetExceptionWasThrown(JS_TRUE);
>+            err += aLocation;
>+            err.AppendLiteral(" - EXPORTED_SYMBOLS is not present.");
>+            NS_ERROR(err.get());

As noted, NS_ERROR compiles away in release builds, so the err mutations should be #ifdef NS_DEBUG I guess -- also err's declaration.

>+            return NS_ERROR_FAILURE;
>+        }
>+
>+        JSObject *symbolsObj = nsnull;
>+        if (JSVAL_IS_PRIMITIVE(symbols) ||
>+            !(symbolsObj = JSVAL_TO_OBJECT(symbols)) ||

Sorry, I misread things here -- if you test !JSVAL_IS_OBJECT(symbols) and keep the explicit !(symbolsObj = ...) test, you're better off. This way duplicates the null test.

>+            !JS_IsArrayObject(mContext, symbolsObj)) {
>+            err += aLocation;
>+            err.AppendLiteral(" - EXPORTED_SYMBOLS is not an array.");
>+            NS_ERROR(err.get());

Ditto the NS_DEBUG comment.

>+            return NS_ERROR_FAILURE;
>+        }
>+
>+        // Iterate over symbols array, installing symbols on targetObj:
>+
>+        jsuint symbolCount = 0;
>+        if (!JS_GetArrayLength(mContext, symbolsObj, &symbolCount)) {
>+            if (cc)
>+                cc->SetExceptionWasThrown(JS_TRUE);
>+            err += aLocation;
>+            err.AppendLiteral(" - Error getting array length of ");
>+            err.AppendLiteral("EXPORTED_SYMBOLS.");
>+            NS_ERROR(err.get());

Ditto.

>+            return NS_ERROR_FAILURE;
>+        }
>+#ifdef DEBUG
>+        printf("Installing symbols [ ");
>+#endif
>+    
>+        for (jsuint i = 0; i < symbolCount; ++i) {
>+            jsval val;
>+            JSString *symbolName;
>+            JSBool ok;
>+            if (!(ok = JS_GetElement(mContext, symbolsObj, i, &val))) {

Uber-ultra-nit: I find it clearer to initialize the ok decl and not test if (!(...)).

>+                if (cc)
>+                    cc->SetExceptionWasThrown(JS_TRUE);
>+            }
>+
>+            if (!ok || !JSVAL_IS_STRING(val) ||
>+                !(symbolName = JSVAL_TO_STRING(val))) {

There's no need to null-test JSVAL_TO_STRING(val) if JSVAL_IS_STRING(val).

>+                err += aLocation;
>+                err.AppendLiteral(" - EXPORTED_SYMBOLS array element ");
>+                err.AppendLiteral("is not a string.");
>+                NS_ERROR(err.get());

Ditto NS_DEBUG (or is DEBUG the thing to test? See below, and sigh).

>+                return NS_ERROR_FAILURE;
>+            }
>+#ifdef DEBUG
>+            printf("%s ", JS_GetStringBytes(symbolName));
>+#endif
>+    
>+            if (!JS_GetProperty(mContext, mod->global,
>+                                JS_GetStringBytes(symbolName), &val)) {
>+                if (cc)
>+                    cc->SetExceptionWasThrown(JS_TRUE);
>+                err += aLocation;
>+                err.AppendLiteral(" - Could not get symbol '");
>+                err.Append(nsDependentCString(JS_GetStringBytes(symbolName)));
>+                err.AppendLiteral("'");
>+                NS_ERROR(err.get());

Ditto.

>+                return NS_ERROR_FAILURE;
>+            }
>+            
>+            if (!JS_SetProperty(mContext, targetObj,
>+                                JS_GetStringBytes(symbolName), &val)) {
>+                if (cc)
>+                    cc->SetExceptionWasThrown(JS_TRUE);
>+                err += aLocation;
>+                err.AppendLiteral(" - Could not set property '");
>+                err.Append(nsDependentCString(JS_GetStringBytes(symbolName)));
>+                NS_ERROR("' on target object.");

Ditto.

Maybe the right thing to do is build up the error detail string and attach it to the exception object that should be pending in mContext. Sounds like work, though. So question: is this debug chatter likely to be very helpful to developers using release builds?

/be

Comment 58

[Robert Sayre]

(In reply to comment #57)
>
> Maybe the right thing to do is build up the error detail string and attach it
> to the exception object that should be pending in mContext. Sounds like work,
> though. So question: is this debug chatter likely to be very helpful to
> developers using release builds?
> 

The stuff that is #ifdef DEBUG right now should stay that way, because it prints for successful imports. I think developers using release builds could use the error cases, otherwise the failures can be pretty hard to diagnose unless you understand exactly how this function works. Will fix.

Comment 59

[Robert Sayre]

Attachment: Components.utils.import v3

> >+        if (!JS_ValueToObject(cx, argv[1], &targetObject)) {
> >+            cc->SetExceptionWasThrown(JS_TRUE);
> 
> This is right, IIRC.
> 
> >+            return NS_OK;
> 
> But shouldn't this be NS_ERROR_FAILURE or the like?

I think NS_OK is what we want, because otherwise XPConnect seems to overwrite our error messages with COM stuff. If returning a failure code is required for some reason, we should audit the tree, because I found other callers doing this. This also means we don't need separate handling for situations that set an exception on mContext, I think.

Comment 60

[Brendan Eich [:brendan]]

Comment on attachment 264916 [details] [diff] [review]
Components.utils.import v3

Nice -- right you are about NS_OK with JS exception set (confusing; also we no doubt do have bugs where we overwrite a JS exception; we greatly fear hidden JS exceptions, because they'll bite subsequent attempts to interpret on the tainted cx).

Imputing r+jst, he should re-review if the code changes warrant but I think this is ready. Thanks for doing it.

/be

Comment 61

[Alex Vincent [:WeirdAl]]

Attachment: additional testcase (loading a component)

The testcases which were checked in didn't actually createInstance() any components.  This patch adds support for that test case too, and hopefully demonstrates the Right Way.

I do wonder why the generated factory's createInstance method calls on "return ctor().QueryInterface(iid);" instead of "return new ctor().QueryInterface(iid)".  Support for services, maybe?

Comment 62

[Robert Sayre]

Comment on attachment 264944 [details] [diff] [review]
additional testcase (loading a component)

nice, thanks.

Comment 63

[Robert Sayre]

Checking in idl/Makefile.in;
/cvsroot/mozilla/js/src/xpconnect/idl/Makefile.in,v  <--  Makefile.in
new revision: 1.24; previous revision: 1.23
done
Checking in idl/xpcIJSModuleLoader.idl;
/cvsroot/mozilla/js/src/xpconnect/idl/xpcIJSModuleLoader.idl,v  <--  xpcIJSModuleLoader.idl
new revision: 1.2; previous revision: 1.1
done
Checking in idl/xpccomponents.idl;
/cvsroot/mozilla/js/src/xpconnect/idl/xpccomponents.idl,v  <--  xpccomponents.idl
new revision: 1.30; previous revision: 1.29
done
Checking in loader/Makefile.in;
/cvsroot/mozilla/js/src/xpconnect/loader/Makefile.in,v  <--  Makefile.in
new revision: 1.20; previous revision: 1.19
done
RCS file: /cvsroot/mozilla/js/src/xpconnect/loader/XPCOMUtils.jsm,v
done
Checking in loader/XPCOMUtils.jsm;
/cvsroot/mozilla/js/src/xpconnect/loader/XPCOMUtils.jsm,v  <--  XPCOMUtils.jsm
initial revision: 1.1
done
Checking in loader/mozJSComponentLoader.cpp;
/cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v  <--  mozJSComponentLoader.cpp
new revision: 1.133; previous revision: 1.132
done
Checking in loader/mozJSComponentLoader.h;
/cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.h,v  <--  mozJSComponentLoader.h
new revision: 1.28; previous revision: 1.27
done
Checking in src/xpccomponents.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpccomponents.cpp,v  <--  xpccomponents.cpp
new revision: 1.109; previous revision: 1.108
done
Checking in tests/Makefile.in;
/cvsroot/mozilla/js/src/xpconnect/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.32; previous revision: 1.31
done
RCS file: /cvsroot/mozilla/js/src/xpconnect/tests/unit/bogus_element_type.jsm,v
done
Checking in tests/unit/bogus_element_type.jsm;
/cvsroot/mozilla/js/src/xpconnect/tests/unit/bogus_element_type.jsm,v  <--  bogus_element_type.jsm
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/src/xpconnect/tests/unit/bogus_exports_type.jsm,v
done
Checking in tests/unit/bogus_exports_type.jsm;
/cvsroot/mozilla/js/src/xpconnect/tests/unit/bogus_exports_type.jsm,v  <--  bogus_exports_type.jsm
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/src/xpconnect/tests/unit/test_bogus_files.js,v
done
Checking in tests/unit/test_bogus_files.js;
/cvsroot/mozilla/js/src/xpconnect/tests/unit/test_bogus_files.js,v  <--  test_bogus_files.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/src/xpconnect/tests/unit/test_import.js,v
done
Checking in tests/unit/test_import.js;
/cvsroot/mozilla/js/src/xpconnect/tests/unit/test_import.js,v  <--  test_import.js
initial revision: 1.1
done

Comment 64

[Alex Vincent [:WeirdAl]]

Comment on attachment 264944 [details] [diff] [review]
additional testcase (loading a component)

I'm actually withdrawing this patch, to switch to the "new ctor()" code I suggested in comment 61.  This will require a trivial sr for the one- or two-line change to XPCOMUtils.

Comment 65

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks, sayrer!

Comment 66

[Alex Vincent [:WeirdAl]]

Attachment: additional testcase (loading a component)

shaver: aside from the change from "ctor()" to "new ctor()", no in-process code gets changed.  This is the only reason I'm asking for sr:  everything else is either comments or test code, and would normally not require sr.

sayrer: this patch is identical to what you saw earlier, except for what we discussed via IRC.  So the review should be pretty easy.

Comment 67

[Alex Vincent [:WeirdAl]]

Comment on attachment 264965 [details] [diff] [review]
additional testcase (loading a component)

>Index: js/src/xpconnect/loader/XPCOMUtils.jsm
>+ * 'Components.utils.import("rel:XPCOMUtils.js");'

Oops.  I still didn't get it right.  That should be "rel:XPCOMUtils.jsm".

Comment 68

[Alex Vincent [:WeirdAl]]

I just noticed something else.  XPCOMUtils does not alleviate the need for QueryInterface if your component implements nsIClassInfo and you want to show all your interfaces at once.  We might need a follow-up bug for that.

Comment 69

[Robert Sayre]

Comment on attachment 264965 [details] [diff] [review]
additional testcase (loading a component)

I checked this in. Please create new bugs for additional follow-up patches.

Comment 70

[Alex Fritze]

Robert (and everyone else who got this landed): A big thank you!

Comment 71

[Alex Fritze]

(In reply to comment #61)
> I do wonder why the generated factory's createInstance method calls on "return
> ctor().QueryInterface(iid);" instead of "return new
> ctor().QueryInterface(iid)".  Support for services, maybe?
> 

Alex: 
There are a couple of reasons why I think the version without 'new' is preferable:
- Support for singleton objects
- 'Complex' constructors; such as a function that returns an object out of some pool, or an object that is constructed differently than by calling 'new'. E.g. in zap (and in joost), we use a JS class framework where objects are constructed using 'myClass.instantiate()'.

Comment 72

[Alex Vincent [:WeirdAl]]

(In reply to comment #71)

So let me see if I understand this.  Under the ctor() model (as opposed to new ctor()), services would be returned like this:

Foo = {
  // nsISupports
  QueryInterface: function QI(aIID) {
    // ...
  }
}

FooFactory = XPCOMUtils.generateFactory(function getFoo() {
  return Foo;
});

That's how I'd prefer to implement services too, but I think it's not very correct.  That's one advantage of nsIClassInfo's flags; it allows a component t tell xpconnect that it is a service, and thus only one instance should be created.

Complex constructors would look like:

FooClass = {
  instantiate: function instantiate() {
    var rv = new Object();
    rv.QueryInterface = function QueryInterface(aIID) { /* ... */};
    rv._count = this._count++;
    return rv;
  }
}

FooFactory = XPCOMUtils.generateFactory(function getFoo() {
  return FooClass.instantiate();
});

My main beef with this is that I've never seen a JS-based factory written to call a method to return a component.  The above code could be done just as easily with |return new Foo()|.

If instead you wanted to pass in arguments to the instantiate() method, I have two arguments against that.  One, factories generally don't get additional arguments (although according to xpconnect, I think they could - shaver will probably correct me on that).  So you'd be stuck with what you already have as properties on the factory and in the factory's scope.  Two, even if additional arguments were permissible or even desired, why wouldn't an initialize() method on the component be appropriate for the component's interface?  It's quite common to have one, and we do have errors for NOT_INITIALIZED and ALREADY_INITIALIZED.  Further, Components.Constructor allows you to hide the need for initialize(), just like it would hide the need for the instantiate() method.

I am perfectly willing to accept that I'm wrong here, but I think you might win me (and more importantly, the reviewers!) over with test components demonstrating what you have in mind, loaded via xpcshell-simple's do_load_module().  My primary argument here is that your proposal bends the convention of XPCOM factories we'd been writing for years, and may not be worth the extra couple of lines for the vast majority of JS-based components we already have.

Comment 73

[Alex Fritze]

(In reply to comment #72)
> [...]
> My primary argument here is that your proposal bends the
> convention of XPCOM factories we'd been writing for years, and may not be worth
> the extra couple of lines for the vast majority of JS-based components we
> already have.

Right, I agree. The zap/joost usage of complex constructors is not very common, so best to stick with 'new ctor()' in XPCOMUtils.jsm. I'll stick a more generic version of generateFactory in a zap/joost-specific zapXPCOMUtils.jsm.

Comment 74

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #71)
> (In reply to comment #61)
> > I do wonder why the generated factory's createInstance method calls on "return
> > ctor().QueryInterface(iid);" instead of "return new
> > ctor().QueryInterface(iid)".  Support for services, maybe?
> > 
> 
> Alex: 
> There are a couple of reasons why I think the version without 'new' is
> preferable:
> - Support for singleton objects
> - 'Complex' constructors; such as a function that returns an object out of some
> pool, or an object that is constructed differently than by calling 'new'. E.g.
> in zap (and in joost), we use a JS class framework where objects are
> constructed using 'myClass.instantiate()'.

You can still call |new| and get those behaviours, because an object returned by a constructor function is taken as the result of the expression:

Edited lightly:

js> a = [1,2,3];
js> function ctor() { return a; }
js> a1 = new ctor();
js> a2 = new ctor();
js> a1 === a2
true

Comment 75

[Alex Fritze]

(In reply to comment #74)
> [...] an object returned by a constructor function is taken as 
> the result of the expression

Ah, nice, didn't know that. That simplifies things.

Comment 76

[Eric Shepherd [:sheppy]]

We have documentation for this now.

Comment 77

[Bill Wood]

Please see bug 447728

Comment 78

[David Rees]

Isn't import a reserved word? In strict mode now, but in not-strict in JS 1.9 FF 5 according to https://developer.mozilla.org/en/JavaScript/Reference/Reserved_Words.

Comment 79

[Brendan Eich [:brendan]]

See ES5.1 clause 7.6 -- import and other reserved identifiers are fine to use as property names (after . in member expressions, before : in object initialisers).

/be

Comment 80

[David Rees]

Ah, interesting. Looking at http://es5.github.com/#A.1,  the difference seems to be IdentifierName which can be used as you said and Identifier (which is an IdentifierName without the Reserved Words) which what is used for FunctionDeclaration and FunctionExpression.

So it seems that https://developer.mozilla.org/en/JavaScript/Reference/Reserved_Words needs to be updated?

Still, its an edge case, and import would be illegal in a FunctionDeclaration, so some might suggest that using some other name might have been a better idea. Water under the bridge I guess, but some JS lint tools are not happy and will have to be updated.