Closed Bug 956455 Opened 6 years ago Closed 6 years ago

de-holder WrapNative and friends

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(3 files, 2 obsolete files)

In the modern handle-ified world, holders are often redundant. One particularly ugly instance of this is nsContentUtils::WrapNative.  A similar de-holdering can be done for the WrapNative methods in nsDOMClassInfo.  With those converted, nsXPConnect::WrapNativeToJSVal can be de-holdered, assuming the comment "Passing a holder here inhibits slim wrappers under WrapNativeToJSVal" is bogus.

By hoisting holder creation into nsXPConnect::WrapNative, maybe I could remove the holders from XPCConvert::NativeInterface2JSObject, but that's more than I want to face at the moment.
Depends on: 956423
Comment on attachment 8355715 [details] [diff] [review]
part 1 - WrapNative's holder argument is unnecessary.

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

::: content/base/src/nsFrameMessageManager.cpp
@@ +925,5 @@
>        NS_ENSURE_TRUE(param, NS_ERROR_OUT_OF_MEMORY);
>  
>        JS::Rooted<JS::Value> targetv(ctx);
>        JS::Rooted<JSObject*> global(ctx, JS_GetGlobalForObject(ctx, object));
> +      nsContentUtils::WrapNative(ctx, global, aTarget, &targetv, true);

Hrm, it's probably no good that we ignore the return value here
> Hrm, it's probably no good that we ignore the return value here
Good catch.

try run is green: https://tbpl.mozilla.org/?tree=Try&rev=5cfa8f3f1177
> Hrm, it's probably no good that we ignore the return value here
Filed bug 956569 for that.
Attachment #8355715 - Flags: review?(Ms2ger)
Attachment #8355716 - Flags: review?(Ms2ger)
Attachment #8355717 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8355717 [details] [diff] [review]
part 3 - Remove the holder argument to nsXPConnect::WrapNativeToJSVal.

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

Delegating to gabor.
Attachment #8355717 - Flags: review?(bobbyholley+bmo) → review?(gkrizsanits)
Comment on attachment 8355717 [details] [diff] [review]
part 3 - Remove the holder argument to nsXPConnect::WrapNativeToJSVal.

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

/**
      * Same as wrapNative, but also returns the JSObject in aVal. C++ callers
-     * can pass in null for the aHolder argument, but in that case they must
-     * ensure that aVal is rooted.
+     * must ensure that aVal is rooted.

I think you can get rid of the 'also' as well here. Otherwise it looks all good!
Attachment #8355717 - Flags: review?(gkrizsanits) → review+
Attachment #8355715 - Attachment is obsolete: true
Attachment #8355715 - Flags: review?(Ms2ger)
Attachment #8356163 - Flags: review?(bugs)
Attachment #8355716 - Attachment is obsolete: true
Attachment #8355716 - Flags: review?(Ms2ger)
Attachment #8356164 - Flags: review?(bugs)
Attachment #8356163 - Flags: review?(bugs) → review+
Attachment #8356164 - Flags: review?(bugs) → review+
Blocks: 958644
You need to log in before you can comment on or make changes to this bug.