1332172 - Remove XPC_MAP_WANT_*

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Nicholas Nethercote [inactive]]

Contrary to what I said in earlier bugs, I have one more XPConnect
simplification.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove XPC_MAP_WANT_*

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.

Comment 2

[Andrew McCreight [:mccr8]]

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?

Comment 3

[Nicholas Nethercote [inactive]]

> > +#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

[Masatoshi Kimura [:emk]]

(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

Comment 5

[Nicholas Nethercote [inactive]]

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.

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/538094a7900cdec31ad57faf9a6937104edce590
Bug 1332172 - Remove XPC_MAP_WANT_*. r=mccr8.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/538094a7900c