Export helpers should be available on Components.utils

RESOLVED FIXED in mozilla28

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: Gabor Krizsanits (INACTIVE))

Tracking

unspecified
mozilla28
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
I just realized they aren't. Chrome can technically just create a sandbox to do this stuff, but that's super lame. We should expose these on Cu directly, which should be a trivial patch to write.

Gabor, can you take this?
(Assignee)

Comment 1

4 years ago
Sure... This API was designed for Add-ons and JSMs, both are running in sandboxes... Since I didn't know about any use case where a chrome window would need these I wouldn't really bothered adding them to Cu.
(Assignee)

Comment 2

4 years ago
ehh... s/wouldn't/didn't
(Reporter)

Comment 3

4 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> Sure... This API was designed for Add-ons and JSMs, both are running in
> sandboxes... Since I didn't know about any use case where a chrome window
> would need these I wouldn't really bothered adding them to Cu.

JSMs don't run in sandboxes. They used wrapped BSPs for the global. That's why we need it on Cu.
(Assignee)

Comment 4

4 years ago
Whoops, sorry... my bad I'll fix this soon.
(Assignee)

Comment 5

4 years ago
Created attachment 824052 [details] [diff] [review]
part1: options object for Cu.createObjectIn. v1
Attachment #824052 - Flags: feedback?(bobbyholley+bmo)
(Assignee)

Comment 6

4 years ago
Created attachment 824053 [details] [diff] [review]
part2: exportFunction for Cu. v1
Attachment #824053 - Flags: feedback?(bobbyholley+bmo)
(Assignee)

Comment 7

4 years ago
Created attachment 824054 [details] [diff] [review]
part3: evalInWindow for Cu. v1
Attachment #824054 - Flags: feedback?(bobbyholley+bmo)
(Assignee)

Comment 8

4 years ago
Tests will come soon...
(Reporter)

Comment 9

4 years ago
Comment on attachment 824052 [details] [diff] [review]
part1: options object for Cu.createObjectIn. v1

Review of attachment 824052 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCComponents.cpp
@@ +3055,5 @@
> +                                                       : nullptr);
> +    CreateObjectInOptions options(cx, optionsObject);
> +    if (voptions.isObject() &&
> +        !options.Parse())
> +        return NS_ERROR_FAILURE;

{}
Attachment #824052 - Flags: feedback?(bobbyholley+bmo) → review+
(Reporter)

Comment 10

4 years ago
Comment on attachment 824053 [details] [diff] [review]
part2: exportFunction for Cu. v1

Review of attachment 824053 [details] [diff] [review]:
-----------------------------------------------------------------

You can just flag for review directly for stuff like this. Only do feedback if there's likely to be a lot of discussion and reworking.

::: js/xpconnect/idl/xpccomponents.idl
@@ +321,5 @@
> +     * the caller's compartment.
> +     * The 3rd argument is the name of the property that will
> +     * be set on the target scope, with the forwarder function as
> +     * the value.
> +     * The principal of the caller must subsume that of the target.

I'd leave the last part out, since we can assert system principal for Cu.
Attachment #824053 - Flags: feedback?(bobbyholley+bmo) → review+
(Reporter)

Comment 11

4 years ago
Comment on attachment 824054 [details] [diff] [review]
part3: evalInWindow for Cu. v1

Review of attachment 824054 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/Sandbox.cpp
@@ +547,5 @@
> +    RootedString srcString(cx, args[0].toString());
> +    RootedObject targetScope(cx, &args[1].toObject());
> +
> +    nsDependentJSString srcDepString;
> +    srcDepString.init(cx, srcString);

need to check the return value here.
Attachment #824054 - Flags: feedback?(bobbyholley+bmo) → feedback+
(Assignee)

Comment 12

4 years ago
Created attachment 825224 [details] [diff] [review]
part3: evalInWindow for Cu. v2

I don't really see a reason to test these methods for anything else than availability, since they share the implementation with the sandbox version. What do you think?
Attachment #824054 - Attachment is obsolete: true
Attachment #825224 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 13

4 years ago
Oops... swapped the two arguments in the Cu.evalInWindow test accidentally...
(Reporter)

Comment 14

4 years ago
Comment on attachment 825224 [details] [diff] [review]
part3: evalInWindow for Cu. v2

Review of attachment 825224 [details] [diff] [review]:
-----------------------------------------------------------------

In the future, please try to avoid obsoleting patches that have already been r+ed unless you really need to. Interdiffing is a pain, and in this case the tests could have just been a separate patch.

::: js/xpconnect/tests/unit/test_exportFunction.js
@@ +85,5 @@
>    Cu.evalInSandbox("(" + function() {
>      checkIfCalled();
>    }.toSource() + ")()", epsb);
> +
> +  // expotrFunction and createObject in should be available from Cu too.

exportFunction

and

createObjectIn
Attachment #825224 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 15

4 years ago
(In reply to Bobby Holley (:bholley) from comment #14)
> In the future, please try to avoid obsoleting patches that have already been
> r+ed unless you really need to. Interdiffing is a pain, and in this case the
> tests could have just been a separate patch.

It was only f+-ed so I thought you want to read it again for some reason...
https://hg.mozilla.org/mozilla-central/rev/29c40458efff
https://hg.mozilla.org/mozilla-central/rev/b854d05169de
https://hg.mozilla.org/mozilla-central/rev/27ffaa0d3c03
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Comment 18

4 years ago
https://hg.mozilla.org/mozilla-central/rev/27ffaa0d3c03
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.