Closed Bug 1325542 Opened 3 years ago Closed 3 years ago

Remove XPCNativeScriptableFlags

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(1 file, 1 obsolete file)

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.)
Attached patch Remove XPCNativeScriptableFlags (obsolete) — Splinter Review
Attachment #8821435 - Flags: review?(continuation)
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?
Attachment #8821435 - Flags: review?(continuation) → review-
(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.
As per comment 3, this has one change compared to the previous version.
Attachment #8824852 - Flags: review?(continuation)
Attachment #8821435 - Attachment is obsolete: true
Comment on attachment 8824852 [details] [diff] [review]
Remove XPCNativeScriptableFlags

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

Thanks for the explanations.
Attachment #8824852 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/4e2cfc2d396f
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.