Closed
Bug 1032457
Opened 10 years ago
Closed 10 years ago
Allow passing callbacks to exported functions
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 3 obsolete files)
2.10 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
8.37 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
15.50 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
See bug 1031336 comment 7. This seems like a legitimate use-case. I think we should make a default-false option to exportFunction that allows argument cloning to operate with |cloneFunctions| = true.
Assignee | ||
Comment 1•10 years ago
|
||
This is going to need dev documentation, probably from Will. https://tbpl.mozilla.org/?tree=Try&rev=3ec50ddc02a8
Assignee: nobody → bobbyholley
Keywords: dev-doc-needed
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8449047 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 3•10 years ago
|
||
We're going to add functionality to the cloning version, and the non-cloning version is going away.
Attachment #8449048 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8449049 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8449050 -
Flags: review?(gkrizsanits)
Updated•10 years ago
|
Attachment #8449047 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8449048 -
Flags: review?(gkrizsanits) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8449049 [details] [diff] [review] Part 3 - Stash the clone options for function forwarders on the forwarder itself. v1 Review of attachment 8449049 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/ExportHelpers.cpp @@ +273,5 @@ > + if (!optionsObj) > + return false; > + js::SetFunctionNativeReserved(funObj, 1, ObjectValue(*optionsObj)); > + > + extra newline ::: js/xpconnect/src/xpcprivate.h @@ +3435,5 @@ > + JS::RootedValue val(cx); > + unsigned attrs = JSPROP_READONLY | JSPROP_PERMANENT; > + val = JS::BooleanValue(wrapReflectors); > + if (!JS_DefineProperty(cx, obj, "wrapReflectors", val, attrs)) > + return nullptr;; double ;;
Attachment #8449049 -
Flags: review?(gkrizsanits) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8449050 [details] [diff] [review] Part 4 - Implement the |allowCallbacks| parameter to exportFunction. v1 Review of attachment 8449050 [details] [diff] [review]: ----------------------------------------------------------------- I leave r? on me until we discuss this, because I have some concerns about this. ::: js/xpconnect/src/xpcprivate.h @@ +3420,3 @@ > > JS::RootedId defineAs; > + bool allowCallbacks; We talked about this feature a few times and I'm really glad that we'll have it finally. But it's not immediately clear to me why do you call it allowCallbacks. While the intention of this bug is clearly to enable callback args, this patch seem to clone every methods as well... Which actually might not be a good idea, like what's the point of a cloned toString method with a |this| = null call? So, am I overlooking something or do we clone methods too if the flag is on? Is it intentional if so? ::: js/xpconnect/tests/unit/test_exportFunction.js @@ +54,5 @@ > mixed = { xrayed: xrayed, xrayed2: xrayed2 }; > tobecloned = { cloned: "cloned" }; > + invokedCallback = false; > + callback = function() { invokedCallback = true; }; > + imported(42,tobecloned, native, mixed, callback); whoops, it's my fault but could you please add a space after "42," while here?
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8449050 [details] [diff] [review] Part 4 - Implement the |allowCallbacks| parameter to exportFunction. v1 (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7) > We talked about this feature a few times and I'm really glad that we'll have > it finally. But it's not immediately clear to me why do you call it > allowCallbacks. While the intention of this bug is clearly to enable > callback args, this patch seem to clone every methods as well... Which > actually might not be a good idea, like what's the point of a cloned > toString method with a |this| = null call? That is a very excellent point. I will do this a different way.
Attachment #8449050 -
Attachment is obsolete: true
Attachment #8449050 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8449644 -
Flags: review?(gkrizsanits)
Comment 10•10 years ago
|
||
Comment on attachment 8449644 [details] [diff] [review] Part 4 - Implement the |allowCallbacks| parameter to exportFunction. v2 Review of attachment 8449644 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/ExportHelpers.cpp @@ +215,1 @@ > if (!options.Parse()) Could you assert !cloneFunctions here?
Attachment #8449644 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 11•10 years ago
|
||
At this point, I don't think it really makes sense anymore to have FunctionForwarderOptions inherit StackScopedCloneOptions. I'm combining Part 3 and 4 into a new patch.
Attachment #8449049 -
Attachment is obsolete: true
Attachment #8449644 -
Attachment is obsolete: true
Attachment #8450331 -
Flags: review?(gkrizsanits)
Comment 12•10 years ago
|
||
Comment on attachment 8450331 [details] [diff] [review] Part 3 - Implement the |allowCallbacks| parameter to exportFunction. v3 Review of attachment 8450331 [details] [diff] [review]: ----------------------------------------------------------------- Yes, I was considering asking you to change that part actually... so I like this version a lot better, thanks :)
Attachment #8450331 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 13•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5ec2d1435d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/704be3e8cd10 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2afd5db295ee
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cd5ec2d1435d https://hg.mozilla.org/mozilla-central/rev/704be3e8cd10 https://hg.mozilla.org/mozilla-central/rev/2afd5db295ee
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 15•10 years ago
|
||
I've updated https://developer.mozilla.org/en-US/docs/Components.utils.exportFunction to include allowCallbacks. Let me know if this works for you.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Will Bamberg [:wbamberg] from comment #15) > I've updated > https://developer.mozilla.org/en-US/docs/Components.utils.exportFunction to > include allowCallbacks. Let me know if this works for you. Looks great! Thanks.
Flags: needinfo?(bobbyholley)
Updated•10 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•