Closed
Bug 1332172
Opened 7 years ago
Closed 7 years ago
Remove XPC_MAP_WANT_*
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(1 file)
57.10 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
Contrary to what I said in earlier bugs, I have one more XPConnect simplification.
Assignee | ||
Comment 1•7 years ago
|
||
nsIXPCScriptable flags handling in xpc_map_end.h is a bit of a mess. - Half the flags relate to whether various functions are defined (PreCreate, GetProperty, etc). These are set using the XPC_MAP_WANT_* macros; for each one xpc_map_end.h inserts the corresponding flag using the preprocessor (see XPC_MAP_CLASSNAME::GetScriptableFlags()). - The other half of the flags relate to other things (IS_GLOBAL_OBJECT, DONT_REFLECT_INTERFACE_NAMES, etc). These are set using the XPC_MAP_FLAGS macro. Having two similar but different mechanisms to set the flags for a class is confusing. (Indeed, until recently we had some classes where a single flag was redundantly specified via both mechanisms.) Note also that the classes done in dom/base/nsIDOMClassInfo.h also specify all the flags in a single value, similar to how XPC_MAP_FLAGS works. This patch removes the XPC_MAP_WANT_* macros. All flags are now set via XPC_MAP_FLAGS. This is a significant simplification to xpc_map_end.h and all the places that use it. The downside of this change is that I had to change the flag constants from class constants (i.e. nsIXPCScriptable::FOO) to macros (i.e. NSIXPCSCRIPTABLE_FOO) because they need to be used in #if statements like this in xpc_map_end.h: #if !((XPC_MAP_FLAGS) & NSIXPCSCRIPTABLE_WANT_PRECREATE) and you can't use a '::'-qualified name inside a #if. I think this downside is outweighed by the simplification described above. The patch also makes XPC_MAP_QUOTED_CLASSNAME optional; it's now required only in the case where it doesn't match XPC_MAP_CLASSNAME, which is in only two places. Overall the patch removes ~100 lines of code.
Attachment #8828215 -
Flags: review?(continuation)
Comment 2•7 years ago
|
||
Comment on attachment 8828215 [details] [diff] [review] Remove XPC_MAP_WANT_* Review of attachment 8828215 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/idl/nsIXPCScriptable.idl @@ +87,5 @@ > %{ C++ > + // Bitflags used for 'flags' (only 32 bits available!). They are defined > + // via #defines so they can be used in #ifndef guards in xpc_map_end.h. > + > + #define NSIXPCSCRIPTABLE_WANT_PRECREATE (1 << 0) Please move these definitions before the interface, as they are not actually defined inside of the interface. Also, I know this is a bit of a pain, but please name these NSIXPCSCRIPTABLE_* macros to XPC_SCRIPTABLE_*. The NSIXPC stuff is really hard to read when it is all caps, and the XPC_SCRIPTABLE_ should be sufficiently disambiguating. ::: js/xpconnect/public/xpc_map_end.h @@ +10,5 @@ > #ifndef XPC_MAP_CLASSNAME > #error "Must #define XPC_MAP_CLASSNAME before #including xpc_map_end.h" > #endif > > +#define QUOTE(name) #name This should probably be more like XPC_QUOTE_NAME or something to avoid conflicts. (Is it really needed? Looks kind of odd.) You also need to #undef it somewhere. ::: js/xpconnect/src/XPCComponents.cpp @@ +2263,5 @@ > NS_IMPL_RELEASE(nsXPCComponents_Utils) > > // The nsIXPCScriptable map declaration that will generate stubs for us... > +#define XPC_MAP_CLASSNAME nsXPCComponents_Utils > +#define XPC_MAP_FLAGS (NSIXPCSCRIPTABLE_ALLOW_PROP_MODS_DURING_RESOLVE) It looks a little weird to me to have these parens here. Maybe get rid of them when there's only a single flag?
Attachment #8828215 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 3•7 years ago
|
||
> > +#define QUOTE(name) #name
>
> This should probably be more like XPC_QUOTE_NAME or something to avoid
> conflicts. (Is it really needed? Looks kind of odd.) You also need to #undef
> it somewhere.
Turns out this didn't work the way I expected -- it ended up always producing "XPC_MAP_CLASSNAME". So I reverted the XPC_MAP_QUOTED_CLASSNAME change.
Comment 4•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #3) > > > +#define QUOTE(name) #name > > > > This should probably be more like XPC_QUOTE_NAME or something to avoid > > conflicts. (Is it really needed? Looks kind of odd.) You also need to #undef > > it somewhere. > > Turns out this didn't work the way I expected -- it ended up always > producing "XPC_MAP_CLASSNAME". So I reverted the XPC_MAP_QUOTED_CLASSNAME > change. You need to define two-level macros to get the expected result: #define QUOTE(name) XQUOTE(name) #define XQUOTE(name) #name
Assignee | ||
Comment 5•7 years ago
|
||
Thanks for the tip, emk! It was only a minor part of the patch so I won't put it back in, but good to know for next time.
Assignee | ||
Comment 6•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/538094a7900cdec31ad57faf9a6937104edce590 Bug 1332172 - Remove XPC_MAP_WANT_*. r=mccr8.
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/538094a7900c
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
•