Remove XPCNativeScriptableCreateInfo

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments)

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 :)
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.
Attachment #8826498 - Flags: review?(continuation)
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 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
Attachment #8826498 - Flags: review?(continuation) → review-
Posted patch fixupsSplinter Review
This strikes me as strictly worse than the previous version, but oh well.
Attachment #8828158 - Flags: review?(continuation)
Comment on attachment 8828158 [details] [diff] [review]
fixups

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

Thanks.

The call sites are less magical, at least.
Attachment #8828158 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/a63edff17c8f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.