Closed Bug 1107673 Opened 7 years ago Closed 7 years ago

[JSAPI] add WrappedJSToDictionary that doesn't require a JSContext

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
b2g-v2.2 --- fixed

People

(Reporter: huseby, Assigned: huseby)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
This bug tracks the patch to create a cx-less WrappedJSToDictionary.
Blocks: 1093688
Blocks: 1107681
Attached patch Bug_1107673.patch (obsolete) — Splinter Review
This adds a new WrappedJSToDictionary that doesn't require the caller to pass in a JS context.
Attachment #8532267 - Flags: review?(bobbyholley)
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+
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)
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)
Attached patch Bug_1107673.patch (obsolete) — Splinter Review
updated patch with bholley changes to get r+
Attachment #8532267 - Attachment is obsolete: true
doh! something seriously wrong with that patch...fixing it now.
Attached patch Bug_1107673.patch (obsolete) — Splinter Review
fixed a dumb compile error.
Attachment #8535315 - Attachment is obsolete: true
Attachment #8535376 - Attachment is obsolete: true
try pass: http://mzl.la/1wBLtun

it looks clean.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/436b67d8493a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Blocks: 1114667
You need to log in before you can comment on or make changes to this bug.