Closed Bug 1329846 Opened 3 years ago Closed 3 years ago

Remove XPCNativeScriptableInfo

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(2 files)

XPCScriptableInfo is now a very thin wrapper around nsIXPCScriptable. It can be removed.
Summary: Remove XPCScriptableInfo → Remove XPCNativeScriptableInfo
PCNativeScriptableInfo is now a very thin wrapper around nsIXPCScriptable, and
it uses manual memory management. Removing it simplifies things quite a bit.

In particular, when setting XPCWrappedNative::mScriptable in
XPCWrappedNative::WrapNewGlobal() and XPCWrappedNative::Init() we no longer
have to worry about sharing the XPCNativeScriptableInfo object with the proto.
And XPCWrappedNative::{Init,Destroy}() have similar simplifications.
Attachment #8825606 - Flags: review?(continuation)
It's only used in three places, and it no longer makes the code more readable.
Attachment #8825607 - Flags: review?(continuation)
Comment on attachment 8825606 [details] [diff] [review]
(part 1) - Remove XPCNativeScriptableInfo

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

::: js/xpconnect/src/XPCCallContext.cpp
@@ +71,5 @@
>                                 XPC_WN_TEAROFF_FLAT_OBJECT_SLOT).toObject());
>      }
>      if (mWrapper) {
>          if (mTearOff)
> +            mScriptable = nullptr;

This case isn't needed, because mScriptable is a COM ptr. You should be able to simplify this code a little. (Something like if (mWrapper && !mTearOff))

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +535,5 @@
>  XPCWrappedNative::XPCWrappedNative(already_AddRefed<nsISupports>&& aIdentity,
>                                     XPCWrappedNativeProto* aProto)
>      : mMaybeProto(aProto),
>        mSet(aProto->GetSet()),
> +      mScriptable()

Nit: remove this initalizer.

@@ +553,5 @@
>                                     already_AddRefed<XPCNativeSet>&& aSet)
>  
>      : mMaybeScope(TagScope(aScope)),
>        mSet(aSet),
> +      mScriptable()

Nit: remove this initalizer.

@@ +572,5 @@
>  
>  void
>  XPCWrappedNative::Destroy()
>  {
> +    mScriptable = nullptr;

Hooray for simplifications!

@@ +926,5 @@
>  
>      if (HasProto())
>          proto->SystemIsBeingShutDown();
>  
> +    // We don't null mScriptable here. The destructor will do it.

"null" seems a little odd as a verb to me. Maybe "clear"?

::: js/xpconnect/src/XPCWrappedNativeProto.cpp
@@ +21,5 @@
>      : mScope(Scope),
>        mJSProtoObject(nullptr),
>        mClassInfo(ClassInfo),
>        mSet(Set),
> +      mScriptable()

Nit: remove this initalizer.

@@ +54,5 @@
>  XPCWrappedNativeProto::Init(const XPCNativeScriptableCreateInfo* scriptableCreateInfo,
>                              bool callPostCreatePrototype)
>  {
>      AutoJSContext cx;
>      nsIXPCScriptable* callback = scriptableCreateInfo ?

Maybe make this an nsCOMPtr to be consistent? and you could then use forget() below.

@@ +57,5 @@
>      AutoJSContext cx;
>      nsIXPCScriptable* callback = scriptableCreateInfo ?
>                                   scriptableCreateInfo->GetCallback() :
>                                   nullptr;
>      if (callback) {

nit: no braces
Attachment #8825606 - Flags: review?(continuation) → review+
Attachment #8825607 - Flags: review?(continuation) → review+
You need to log in before you can comment on or make changes to this bug.