1324330 - Some XPConnect clean-ups

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Nicholas Nethercote [inactive]]

I have some clean-ups as follow-ups to recent changes I've made to XPConnect.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Mark js::Class and related types as NONHEAP

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Mark js::Class and related types as NONHEAP

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Don't mix |Cant| and |Cannot| in function names in XPCWrappedNativeJSOps.cpp

Use |Cannot| consistently.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove some unused functions

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Remove nsDOMClassInfo::mName

It's identical to the mClass.name.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Streamline nsIXPCScriptable flags

This patch adjusts the values so there are no skipped bits, and removes the
no-longer-used RESERVED flag.

Comment 10

[Andrew McCreight [:mccr8]]

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.

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Nicholas Nethercote [inactive]]

> 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.

Comment 13

[Nicholas Nethercote [inactive]]

BTW, did you overlook part 2 unintentionally?

Comment 14

[Andrew McCreight [:mccr8]]

(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.

Comment 15

[Nicholas Nethercote [inactive]]

> 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 16

[Andrew McCreight [:mccr8]]

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.

Comment 17

[Nicholas Nethercote [inactive]]

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

Comment 18

[Nicholas Nethercote [inactive]]

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.

Comment 19

[Nicholas Nethercote [inactive]]

leave-open because part 6 hasn't landed yet.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8921eeffbb3
https://hg.mozilla.org/mozilla-central/rev/5b05e5a264b4
https://hg.mozilla.org/mozilla-central/rev/a193a2da9274
https://hg.mozilla.org/mozilla-central/rev/3a2066a8e8a8
https://hg.mozilla.org/mozilla-central/rev/52e2571353b8

Comment 21

[Peter Van der Beken [:peterv]]

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.

Comment 22

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42c1ba6cb5f400c7e4a8a68fe11713de6a69ba38
Bug 1324330 (part 6) - Streamline nsIXPCScriptable flags. r=mccr8.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42c1ba6cb5f4

Comment 24

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 25

[Nicholas Nethercote [inactive]]

Thank you for the info. I filed bug 1451658 about adding MOZ_STATIC_CLASS.