956455 - de-holder WrapNative and friends

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - WrapNative's holder argument is unnecessary.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Remove holder argument from WrapNativeToJSVal nsDOMClassInfo.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Remove the holder argument to nsXPConnect::WrapNativeToJSVal.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 5

[Andrew McCreight [:mccr8]]

> 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

Comment 6

[Andrew McCreight [:mccr8]]

> Hrm, it's probably no good that we ignore the return value here
Filed bug 956569 for that.

Comment 7

[Bobby Holley (:bholley)]

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.

Comment 8

[Gabor Krizsanits (INACTIVE)]

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!

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 1 - WrapNative's holder argument is unnecessary.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Remove holder argument from WrapNativeToJSVal nsDOMClassInfo.

Comment 11

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b139d0427aa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5d406909e132
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b997d2cce0eb

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b139d0427aa
https://hg.mozilla.org/mozilla-central/rev/5d406909e132
https://hg.mozilla.org/mozilla-central/rev/b997d2cce0eb