Closed
Bug 1107673
Opened 10 years ago
Closed 10 years ago
[JSAPI] add WrappedJSToDictionary that doesn't require a JSContext
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla37
Tracking | Status | |
---|---|---|
b2g-v2.2 | --- | fixed |
People
(Reporter: huseby, Assigned: huseby)
References
Details
Attachments
(1 file, 3 obsolete files)
2.04 KB,
patch
|
Details | Diff | Splinter Review |
I talked with :waldo about getting the right way to use WrappedJSToDictionary with nsISupports but without an existing context. The solution is to get the native context from the wrapped JSObject and then create the AutoEntryScript used for rooting the object and building the dictionary.
Assignee | ||
Comment 1•10 years ago
|
||
This bug tracks the patch to create a cx-less WrappedJSToDictionary.
Assignee | ||
Comment 2•10 years ago
|
||
This adds a new WrappedJSToDictionary that doesn't require the caller to pass in a JS context.
Attachment #8532267 -
Flags: review?(bobbyholley)
Comment 3•10 years ago
|
||
Comment on attachment 8532267 [details] [diff] [review] Bug_1107673.patch Review of attachment 8532267 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley with those fixes. Thanks for doing this! ::: dom/bindings/BindingUtils.h @@ +3083,5 @@ > +inline bool > +WrappedJSToDictionary(nsISupports* aObject, T& aDictionary) > +{ > + nsCOMPtr<nsIXPConnectWrappedJS> wrappedObj = do_QueryInterface(aObject); > + NS_ENSURE_TRUE(wrappedObj && wrappedObj->GetJSObject(), false); Thinking about this again, I think this would be a little nicer as: NS_ENSURE_TRUE(wrappedObj, false); JS::Rooted<JSObject*> obj(CycleCollectedJSRuntime::get(), wrappedObj->GetJSObject()); NS_ENSURE_TRUE(obj, false); And then substituting "obj" into the uses below. @@ +3087,5 @@ > + NS_ENSURE_TRUE(wrappedObj && wrappedObj->GetJSObject(), false); > + > + nsIGlobalObject* global = xpc::NativeGlobal(wrappedObj->GetJSObject()); > + NS_ENSURE_TRUE(global, false); > + AutoEntryScript aes(global); Please add aes.TakeOwnershipOfErrorReporting() here, so that we correctly handle any exceptions that are thrown when parsing the dictionary. Also, please add a comment saying that we need an AutoEntryScript because the spec requires us to execute getters when parsing a dictionary.
Attachment #8532267 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 4•10 years ago
|
||
I'll update the patch soon. Thanks for the review. One question, where are the unit tests for this code? I'd like to write some tests to go along with this patch. Which test framework is used for the dom bindings?
Flags: needinfo?(bobbyholley)
Comment 5•10 years ago
|
||
There is nothing that tests the individual BindingUtils.h helpers that you're changing (and doing so would be pretty hard and not very valuable). You should test the consumers (in this case, presumably the code you're writing, or one of the other callers of WrappedJSToDictionary that you change).
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 6•10 years ago
|
||
updated patch with bholley changes to get r+
Attachment #8532267 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
try: http://mzl.la/1yXfKG8
Assignee | ||
Comment 8•10 years ago
|
||
doh! something seriously wrong with that patch...fixing it now.
Assignee | ||
Comment 9•10 years ago
|
||
fixed a dumb compile error.
Attachment #8535315 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
try: http://mzl.la/1BCdE0E
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8535376 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
try pass: http://mzl.la/1wBLtun it looks clean.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/436b67d8493a
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/436b67d8493a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•9 years ago
|
status-b2g-v2.2:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•