Closed Bug 381693 Opened 13 years ago Closed 13 years ago

Allow null targetObj arg to xpcIJSModuleLoader::import()

Categories

(Core :: XPConnect, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: alex, Assigned: alex)

References

Details

Attachments

(3 files)

xpcIJSModuleLoader::import() was intended to have 3 different usages:

1. Import symbols from the EXPORTED_SYMBOLS array to the caller's global object.
2. Import symbols from EXPORTED_SYMBOLS to a targetObj passed into import().
3. Gain access to a module's global object.

The 3rd functionality is helpful for debugging or for importing XPCOM JS modules that don't define EXPORTED_SYMBOLS into other JS code. To support this functionality, all we have to do is to allow a null targetObj argument, so such that 

  var foo = Components.utils.import("gre:myModule.js", null);

returns the global object of myModule.js into foo, but doesn't try to install any symbols on foo's global object.

Patch coming up.
Attached patch Proposed patchSplinter Review
Attachment #265780 - Flags: review?(sayrer)
Please also fix the IDL docs: http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/idl/xpccomponents.idl#172
(as well as the duplicated comment)
Attachment #265787 - Flags: review?(sayrer)
Can you write a couple tests for this functionality?  There are examples of how to do this in the original bug; testing it shouldn't be much work.
Good catch. I can never keep this straight, because

js> typeof(null)
object

Do we want/need to deal with |undefined| as well?
Attached patch testcasesSplinter Review
Attachment #266884 - Flags: review?(sayrer)
(In reply to comment #5)
> Do we want/need to deal with |undefined| as well?

I think we should let |undefined| throw an exception like it does now.



Comment on attachment 265780 [details] [diff] [review]
Proposed patch

looks ok to me.
Attachment #265780 - Flags: superreview?(brendan)
Attachment #265780 - Flags: review?(sayrer)
Attachment #265780 - Flags: review+
Attachment #265787 - Flags: review?(sayrer) → review+
Attachment #266884 - Flags: review?(sayrer) → review+
Comment on attachment 265780 [details] [diff] [review]
Proposed patch

>Index: mozJSComponentLoader.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v
>retrieving revision 1.133
>diff -u -p -r1.133 mozJSComponentLoader.cpp
>--- mozJSComponentLoader.cpp	15 May 2007 23:27:40 -0000	1.133
>+++ mozJSComponentLoader.cpp	23 May 2007 08:40:04 -0000
>@@ -1338,7 +1338,7 @@ mozJSComponentLoader::Import(const nsACS
>         jsval *argv = nsnull;
>         rv = cc->GetArgvPtr(&argv);
>         NS_ENSURE_SUCCESS(rv, rv);
>-        if (JSVAL_IS_PRIMITIVE(argv[1]) ||
>+        if ((JSVAL_IS_PRIMITIVE(argv[1]) && !JSVAL_IS_NULL(argv[1])) ||

See jsapi.h:

#define JSVAL_IS_PRIMITIVE(v)   (!JSVAL_IS_OBJECT(v) || JSVAL_IS_NULL(v))

So all you need here is 'if (!JSVAL_IS_OBJECT(v) ||'.

>             !JS_ValueToObject(cx, argv[1], &targetObject)) {
>             return ReportOnCaller(cc, ERROR_SCOPE_OBJ,
>                                   PromiseFlatCString(registryLocation).get());

Given that, you don't need JS_ValueToObject -- just set targetObject = JSVAL_TO_OBJECT(argv[1]).

sr=me with that.

/be
Attachment #265780 - Flags: superreview?(brendan) → superreview+
Blocks: 317491
Thanks for the reviews.

Checked in 2007-07-15 11:21.

xpcIJSModuleLoader.idl 1.5
xpccomponents.idl 1.36
mozJSComponentLoader.cpp 1.142
test_bogus_files.js 1.4
test_import.js 1.5
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.