Remove XPCNativeScriptableFlags

RESOLVED FIXED in Firefox 53

Status

()

Core
XPConnect
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.)
(Assignee)

Comment 1

a year ago
Created attachment 8821435 [details] [diff] [review]
Remove XPCNativeScriptableFlags
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-
(Assignee)

Comment 3

11 months ago
(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.
(Assignee)

Updated

11 months ago
Depends on: 1324330
(Assignee)

Comment 4

11 months ago
Created attachment 8824852 [details] [diff] [review]
Remove XPCNativeScriptableFlags

As per comment 3, this has one change compared to the previous version.
Attachment #8824852 - Flags: review?(continuation)
(Assignee)

Updated

11 months ago
Attachment #8821435 - Attachment is obsolete: true
(Assignee)

Comment 5

11 months ago
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1bb94c8899754baa73f6731aed548c4d2c2c46f
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+
(Assignee)

Comment 7

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

Comment 8

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e2cfc2d396f
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.