1325542 - Remove XPCNativeScriptableFlags

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Nicholas Nethercote [inactive]]

WantFoo() and the similar methods remain, but they've been moved from
XPCNativeScriptableFlags to nsIXPCScriptable.

One consequence of this change is that in places where we used to get the flags
from an XPCNativeScriptableCreateInfo we now need a null check on the
nsIXPCScriptable. (This isn't true when getting flags from
XPCNativeScriptableInfo, however, because nsIXPCScriptable is always non-null
within that type.)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove XPCNativeScriptableFlags

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8821435 [details] [diff] [review]
Remove XPCNativeScriptableFlags

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

Do you need to null-check GetCallback() everywhere? You aren't consistent about it.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +685,5 @@
>          // the siWrapper...
>  
> +        // Can't set WANT_PRECREATE on an instance scriptable without also
> +        // setting it on the class scriptable.
> +        MOZ_ASSERT_IF(scrWrapper->WantPreCreate(),

Isn't scrWrapper null here? You called forget on it above.

@@ +692,3 @@
>  
> +        // Can't set DONT_ENUM_QUERY_INTERFACE on an instance scriptable
> +        // without also setting it on the class scriptable (if present).

Why did you drop the "and shared" from these comments?

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Comment on attachment 8821435 [details] [diff] [review]
> Remove XPCNativeScriptableFlags
> 
> Review of attachment 8821435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you need to null-check GetCallback() everywhere? You aren't consistent
> about it.

The commit message explains that it's necessary for XPCNativeScriptableCreateInfo but not necessary for XPCNativeScriptableInfo because it's always non-null in the latter case. (If it helps, I have follow-up patches that will remove both of those types, so this will soon become a lot simpler.)
 

> > +        // Can't set WANT_PRECREATE on an instance scriptable without also
> > +        // setting it on the class scriptable.
> > +        MOZ_ASSERT_IF(scrWrapper->WantPreCreate(),
> 
> Isn't scrWrapper null here? You called forget on it above.

Good catch; indeed this problem crashes the browser during startup. I've moved the SetCallback() after all the assertions.


> > +        // Can't set DONT_ENUM_QUERY_INTERFACE on an instance scriptable
> > +        // without also setting it on the class scriptable (if present).
> 
> Why did you drop the "and shared" from these comments?

Because I figured it was referring to the sharing provided by XPCNativeScriptableShared, which bug 1321374 removed. But I can reinstate it if you think it's referring to something else.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Remove XPCNativeScriptableFlags

As per comment 3, this has one change compared to the previous version.

Comment 5

[Nicholas Nethercote [inactive]]

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

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8824852 [details] [diff] [review]
Remove XPCNativeScriptableFlags

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

Thanks for the explanations.

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2cfc2d396fd43d2a825b527656a60247912414
Bug 1325542 - Remove XPCNativeScriptableFlags. r=mccr8.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e2cfc2d396f