gfxIFormats (see url for this bug) has no uuid declared, and yet claims to be scriptable. I'd argue that *any* interface without a uuid should fail to parse, but certainly, at least *scriptable* interfaces without uuid's should fail to parse.

I agree about the scriptable without a uuid I don't think that makes much sense. This interface appears to be used to just define constants, thus I could see allowing it to exist without scriptable and uuid. It would just be included by other IDL's that would need the constants. Did you file a bug on this specific interface as well? Lastly, I'd like to get a concensus. Should this be an error or a warning? My opinion is it's ill formed thus should be an error. I did a search and this _appears_ to be the only interface in this state, at least within spidermonkey.

We were originally trying to stay fairly compatible with the underlying Corba syntax; to not puke on legal Corba declarations that didn't really mean much to us. So, we silently ignore *lots* of stuff. Unfortunately those things we ignore are likely to just be mistakes and the programmers doesn't get what they expect. Maybe we should back off on such compatibility and start adding more checks. Do we want to go all out and flag stuff as errors? We could certainly add new warnings. Maybe we should add a '-s' for STRICT option that lets the user decide if some of these things should be treated as warnings or errors. Our build system would turn that option ON for sure. Note that we already have some warnings that we don't want to consider errors. So, we'd have to do some classification rather than just considering *all* warnings to be errors in STRICT mode. Among the things that we might warn about are.. - use of namespaces - [attributes] that we don't understand - and might be typos - or NOT! (there is a bug on misspelled [scriptable] with discussion on this) - unsupported Corba idl keywords. There is a list of those I noticed back in the day at the bottom of: http://www.mozilla.org/scriptable/xpidl/notes/keywords.txt I agreee: we should certainly, at least, add a warning for interface w/o uuid.

Yes, I filed bug 101697, which was duped against bug 77354, in which pav argues that the interface doesn't need a uuid. A bogus argument, IMO. I think the only way to keep people from doing this is to throw an error, breaking the build.

the "interface" doesn't have any members or classes. give me another way to export a bunch of const values to xpconnect that isn't an interface.

Like I just told Mr Parmenter to his face: Without a uuid the interface will not appear to JS via xpconnect; i.e. it won't do what he wants. It *is* legal to not inherit from nsISupports. Some code does this... http://lxr.mozilla.org/seamonkey/source/xpfe/components/autocomplete/public/nsIA utoCompleteListener.idl#31

The following is a list of interfaces that don't have uuid's: These have functions: http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsILoggingService.idl#70 http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/public/nsIAddbookUrl.idl#56 These just define constants: http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/public/nsIAbBooleanExpression.idl#78 http://lxr.mozilla.org/seamonkey/source/mailnews/absync/public/nsIAbSync.idl#49 The first two would concern me, I think the last two are fine, given they define just constants.

Even those *can* burn people if they decide to try to script using those constants and they discover they've been shipping typelibs where those constants are not available to script. In such cases they may want to only ship some addon chome JS code, but discover they'd need to either also ship and install additional typelibs or forego use of the constants and duplicate the values locally in the JS code. The fact that they made no attempt to mark the interfaces scriptable perhaps means they deserve what they get. But we *do* like our tools to remind us when we make mistakes :)

At least the second constant-only interface you listed is used in the scriptable interface right underneath it. Most likely both are used in scriptable interfaces, and the author did not intend to make these methods non-scriptable. I'm convinced we need to break the build for all interfaces without uuids. Is there ever a good reason to omit a uuid? It seems like a corner better left uncut.

I agree with Rob, I think the usefullness of allowing interfaces without uuid's is outweighed by the potential danger. UUID's are cheap anyway.

The patch looks reasonable to me. I assume you tested. You'll need a patch that also fixes the interfaces in the tree that will not compile with this fix in place. As an NS employee you should verify that this doesn't bust the commercial tree too.

Yes, I created patches to address the IDL files. Should I file separate bugs agains the two components, xpcom and mailnews? Or just put them here? Thanks for the commercial reminder. I'll build that as well.

I'd vote for one patch here. You can use the lxr blame stuff to make a guess at who *might* care that you are making those changes and then ping those persons as a courtesy. But, it is unlikely, I think, that they might objects to such a change. Nevertheless, it is good to have a combined patch that takes the tree from a buildable state to another (hopefully better :) buildable state.

Ok, I've built the commercial tree. And I'm set to build PSM2. Are there any other optional things I should check?

CC'ing the owners of XPCOM and Mail/News, since this patch contains changes to IDL files in those areas. The individual contributors for the code affected don't appear to be valid.

sr=mscott for the address book changes.

Swapping scc for dougt ;-)

Comment on attachment 53603 [details] [diff] [review] Previous patch with the addition of changes to the IDL xpcom changes look fine.

Comment on attachment 53603 [details] [diff] [review] Previous patch with the addition of changes to the IDL sr=jband

I have reviews from the respective components for changes to the idl and an sr for the patch itself. Can I get an r= for the full patch?

r=rginda

patch checked in

Marking Verified -