Closed
Bug 1325542
Opened 7 years ago
Closed 7 years ago
Remove XPCNativeScriptableFlags
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(1 file, 1 obsolete file)
26.41 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Attachment #8821435 -
Flags: review?(continuation)
Comment 2•7 years ago
|
||
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•7 years 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 | ||
Comment 4•7 years ago
|
||
As per comment 3, this has one change compared to the previous version.
Attachment #8824852 -
Flags: review?(continuation)
Assignee | ||
Updated•7 years ago
|
Attachment #8821435 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1bb94c8899754baa73f6731aed548c4d2c2c46f
Comment 6•7 years ago
|
||
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•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2cfc2d396fd43d2a825b527656a60247912414 Bug 1325542 - Remove XPCNativeScriptableFlags. r=mccr8.
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4e2cfc2d396f
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•