Some XPConnect clean-ups

RESOLVED FIXED in Firefox 53

Status

()

Core
XPConnect
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
I have some clean-ups as follow-ups to recent changes I've made to XPConnect.
(Assignee)

Comment 1

a year ago
Created attachment 8819719 [details] [diff] [review]
Mark js::Class and related types as NONHEAP
Attachment #8819719 - Flags: review?(continuation)
(Assignee)

Updated

a year ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

a year ago
Created attachment 8819720 [details] [diff] [review]
(part 2) - Make XPC_MAP_FLAGS non-optional when using xpc_map_end.h

(I'd like to remove XPC_MAP_WANT_* altogether and use XPC_MAP_FLAGS for all the
nsIXPCScriptable flag setting, but I haven't worked out how to handle the
method definitions in xpc_map_end.h yet. In the meantime, it seems good to the
make the flag setting more consistent and explicit.

In particular, the three "Module" classes have "#define XPC_MAP_WANT_CALL" and
"#define XPC_MAP_FLAGS nsIXPCScriptable::WANT_CALL" which both have the same
effect. The patch removes the latter, to make them consistent with other
classes.
Attachment #8819720 - Flags: review?(continuation)
(Assignee)

Comment 3

a year ago
Created attachment 8819721 [details] [diff] [review]
(part 1) - Mark js::Class and related types as NONHEAP
Attachment #8819721 - Flags: review?(continuation)
(Assignee)

Updated

a year ago
Attachment #8819719 - Attachment is obsolete: true
Attachment #8819719 - Flags: review?(continuation)
(Assignee)

Comment 4

a year ago
Created attachment 8819722 [details] [diff] [review]
(part 2) - Make XPC_MAP_FLAGS non-optional when using xpc_map_end.h

I'd like to remove XPC_MAP_WANT_* altogether and use XPC_MAP_FLAGS for all the
nsIXPCScriptable flag setting, but I haven't worked out how to handle the
method definitions in xpc_map_end.h yet. In the meantime, it seems good to the
make the flag setting more consistent and explicit.

In particular, the three "Module" classes have "#define XPC_MAP_WANT_CALL" and
"#define XPC_MAP_FLAGS nsIXPCScriptable::WANT_CALL" which both have the same
effect. The patch removes the latter, to make them consistent with other
classes.
Attachment #8819722 - Flags: review?(continuation)
(Assignee)

Updated

a year ago
Attachment #8819720 - Attachment is obsolete: true
Attachment #8819720 - Flags: review?(continuation)
(Assignee)

Comment 5

a year ago
Created attachment 8819723 [details] [diff] [review]
(part 3) - Don't mix |Cant| and |Cannot| in function names in XPCWrappedNativeJSOps.cpp

Use |Cannot| consistently.
Attachment #8819723 - Flags: review?(continuation)
(Assignee)

Comment 6

a year ago
Created attachment 8819728 [details] [diff] [review]
(part 4) - Streamline GetClassDescription() definitions

Several classes that are implemented via xpc_map_end.h also implement
nsIClassInfo. All of these classes have GetClassDescription() definitions that
are identical to the GetClassName() definition in xpc_map_end.h.
Attachment #8819728 - Flags: review?(continuation)
(Assignee)

Comment 7

a year ago
Created attachment 8819729 [details] [diff] [review]
(part 5) - Remove some unused functions
Attachment #8819729 - Flags: review?(continuation)
(Assignee)

Comment 8

a year ago
Created attachment 8819730 [details] [diff] [review]
(part 6) - Remove nsDOMClassInfo::mName

It's identical to the mClass.name.
Attachment #8819730 - Flags: review?(continuation)
(Assignee)

Comment 9

a year ago
Created attachment 8820048 [details] [diff] [review]
(part 7) - Streamline nsIXPCScriptable flags

This patch adjusts the values so there are no skipped bits, and removes the
no-longer-used RESERVED flag.
Attachment #8820048 - Flags: review?(continuation)
Attachment #8819721 - Flags: review?(continuation) → review+
Attachment #8819723 - Flags: review?(continuation) → review+
Attachment #8819730 - Flags: review?(continuation) → review+
Attachment #8819729 - Flags: review?(continuation) → review+
Comment on attachment 8819728 [details] [diff] [review]
(part 4) - Streamline GetClassDescription() definitions

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

::: js/xpconnect/src/XPCComponents.cpp
@@ -165,5 @@
>  
>  NS_IMETHODIMP
>  nsXPCComponents_Interfaces::GetClassDescription(char * *aClassDescription)
>  {
> -    static const char classDescription[] = "XPCComponents_Interfaces";

Note that the related definition here is "nsXPCComponents_Interfaces", not "XPCComponents_Interfaces", and I think similarly for other things in this file. Hopefully it doesn't matter. I have no idea what this is used for...

With some code searching, it looks like this change will at least break this test:
http://searchfox.org/mozilla-central/source/js/xpconnect/tests/unit/test_bug677864.js#6

I don't see any other uses of that string in m-c JS, but you should at least look around on addons DXR to get a sense if anybody is using it.
Attachment #8819728 - Flags: review?(continuation) → review-
Comment on attachment 8820048 [details] [diff] [review]
(part 7) - Streamline nsIXPCScriptable flags

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

Somebody else should look at this. I have no idea why these bits are skipped like this. I could imagine it might break something if they are removed.
Attachment #8820048 - Flags: review?(continuation) → review?(peterv)
(Assignee)

Comment 12

a year ago
> Note that the related definition here is "nsXPCComponents_Interfaces", not
> "XPCComponents_Interfaces", and I think similarly for other things in this
> file. Hopefully it doesn't matter. I have no idea what this is used for...

Good catch. BackstagePass is the only case where this change is valid. I don't think it's worth making the change for a single case, so I'll eliminate that patch.
(Assignee)

Comment 13

a year ago
BTW, did you overlook part 2 unintentionally?
Flags: needinfo?(continuation)
(Assignee)

Updated

a year ago
Attachment #8819728 - Attachment is obsolete: true
(In reply to Nicholas Nethercote [:njn] from comment #13)
> BTW, did you overlook part 2 unintentionally?

Yeah, I haven't looked at any of that map_end stuff before, and the changes looked less trivial than the others, so I need to gird myself before I look into it. I'll hopefully get to it tomorrow.

(In reply to Nicholas Nethercote [:njn] from comment #12)
> Good catch. BackstagePass is the only case where this change is valid. I
> don't think it's worth making the change for a single case, so I'll
> eliminate that patch.

It might even be okay to make those other changes, but it probably isn't worth your time to figure out if there is any fallout and deal with it if it happens.
Flags: needinfo?(continuation)
(Assignee)

Comment 15

a year ago
> Yeah, I haven't looked at any of that map_end stuff before, and the changes
> looked less trivial than the others, so I need to gird myself before I look
> into it. I'll hopefully get to it tomorrow.

That change is pretty trivial. Not much girding should be necessary! :)
Comment on attachment 8819722 [details] [diff] [review]
(part 2) - Make XPC_MAP_FLAGS non-optional when using xpc_map_end.h

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

nit: "it seems good to the make"
That "the" is redundant.
Attachment #8819722 - Flags: review?(continuation) → review+
(Assignee)

Updated

a year ago
Blocks: 1325542
(Assignee)

Comment 18

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8921eeffbb3e0b4fe67041281b5e23f92eab270
Bug 1324330 (part 1) - Mark js::Class and related types as NONHEAP. r=mccr8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b05e5a264b4a5e8e8aabdcf7198bf541f618cb4
Bug 1324330 (part 2) - Make XPC_MAP_FLAGS non-optional when using xpc_map_end.h. r=mccr8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a193a2da927441688192f9ee07db640e51d0120b
Bug 1324330 (part 3) - Don't mix |Cant| and |Cannot| in function names in XPCWrappedNativeJSOps.cpp. r=mccr8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2066a8e8a822217907a86b0c10227a9214557d
Bug 1324330 (part 4) - Remove some unused functions. r=mccr8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/52e2571353b8ba1f023f1684662ece5f4f3ed785
Bug 1324330 (part 5) - Remove nsDOMClassInfo::mName. r=mccr8.
(Assignee)

Comment 19

a year ago
leave-open because part 6 hasn't landed yet.
Keywords: leave-open
Comment on attachment 8820048 [details] [diff] [review]
(part 7) - Streamline nsIXPCScriptable flags

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

The bits that were removed were skipped to keep binary compatibility with previous versions. I don't think we care much anymore about that, then again, I don't think we care about the skipped bits, but whatever.
Attachment #8820048 - Flags: review?(peterv) → review+
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/42c1ba6cb5f4
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 24

2 months ago
I stumbled across https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/js/public/Class.h#617 while doing some code reading. FYI there is an attribute which requires that a class is used only in statics, which is __attribute__((annotate("moz_global_class")))

This is currently exposed with a macro as MOZ_ONLY_USED_TO_AVOID_STATIC_CONSTRUCTORS: https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/mfbt/Attributes.h#675-681, though we should probably also have a MOZ_STATIC_CLASS alias.
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 25

2 months ago
Thank you for the info. I filed bug 1451658 about adding MOZ_STATIC_CLASS.
Flags: needinfo?(n.nethercote)
You need to log in before you can comment on or make changes to this bug.