1330904 - Remove XPCNativeScriptableCreateInfo

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Nicholas Nethercote [inactive]]

XPCNativeScriptableCreateInfo is now a very thin wrapper around
nsIXPCScriptable. Removing it simplifies things quite a bit.

Note especially the change to GatherScriptableCreateInfo(), which is a
confusing function. Previously its fourth argument was never touched it was
called, but it did have a return value. Now the fourth argument is touched and
effectively replaces that return value, and the function now returns void.

This is my last XPConnect change for now :)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove XPCNativeScriptableCreateInfo

mccr8: I'm not super confident about some of the refcounting stuff in this
patch, particular for GatherProtoScriptable() and GatherScriptable(). E.g. I
don't understand why the dont_AddRef() is required in the former. (My first try
push exposed a leak which I subsequently fixed.) So please check those
carefully.

Comment 2

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbdf09cb97665ad867b6072ba08e6c79d134fbc6

Comment 3

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

Comment on attachment 8826498 [details] [diff] [review]
Remove XPCNativeScriptableCreateInfo

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

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +630,5 @@
>  // static
> +void
> +XPCWrappedNative::GatherScriptable(nsISupports* obj,
> +                                   nsIClassInfo* classInfo,
> +                                   nsCOMPtr<nsIXPCScriptable>& scrProto,

Please use standard Foo** outparams

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8826498 [details] [diff] [review]
Remove XPCNativeScriptableCreateInfo

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

I should have complained about this in your other patches, but could you please call these new scriptable variables "scriptable" instead of "scr"? "scr" is not a good variable name.

r- just for the nsIFoo** argument stuff. The patch looks fine to me otherwise.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +630,5 @@
>  // static
> +void
> +XPCWrappedNative::GatherScriptable(nsISupports* obj,
> +                                   nsIClassInfo* classInfo,
> +                                   nsCOMPtr<nsIXPCScriptable>& scrProto,

As Ms2ger said, these arguments should both be nsIXPCScriptable**, and then call them with getter_AddRefs around the arguments. The easiest way to return into nsIFoo** variables is to have a local nsCOMPtr<nsIFoo> whatever, and then do whatever.forget(aWhatever).

@@ +689,4 @@
>  }
>  
>  bool
> +XPCWrappedNative::Init(nsIXPCScriptable* aScr)

aScriptable please

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: fixups

This strikes me as strictly worse than the previous version, but oh well.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8828158 [details] [diff] [review]
fixups

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

Thanks.

The call sites are less magical, at least.

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a63edff17c8f61d797f6c1defb77746ea6840249
Bug 1330904 - Remove XPCNativeScriptableCreateInfo. r=mccr8.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a63edff17c8f