397929 - Allow changing the C++ method names generated for attributes or methods to avoid naming conflicts

Status: RESOLVED FIXED

Description

[Benjamin Smedberg]

Created attachment 282734 [details] [diff] [review]
Damn MS, rev. 1

This is for moz2:

windows.h ends up redefining a bunch of very common tokens such as

GetMessage
ERROR
CompareString
GetClassName

Because MMgc.h ends up #including <windows.h> we end up redefining these a lot in places where we previously didn't. Some of these places can be fixed with a simple #undef, but GetMessage in particular can't: GetMessage is used in ATL headers and must not be undefined. So I've hacked XPIDL to emit a different binary method name for that particular case.

I didn't rename the signature in nsIConsoleMessage because 1) that's hard to get right with all the JS callers and 2) there appear to be places which expect JS exception .message to match the .message of nsIScriptError->nsIConsoleMessage

Comment 1

[Jeff Walden [:Waldo]]

I think I may have a principled fix; it works on my one testcase so far.  I'm rebuilding now to check...

Comment 2

[Jeff Walden [:Waldo]]

Created attachment 301449 [details] [diff] [review]
Patch

Should be simple enough to understand...just add binaryname(Name) to the bracketed attributes for a method or attribute, and then you either use Class::Name or [GS]etName to define the method.  This patch works for me with postMessage, and it worked by visual inspection of the generated header on the following attribute in an IDL interface:

  [binaryname(MessageMoz)] attribute AString message;

Comment 3

[Jeff Walden [:Waldo]]

I don't see a reason to bump the iid since the patch preserves binary compatibility.

Comment 4

[Benjamin Smedberg]

Comment on attachment 301449 [details] [diff] [review]
Patch

Beautiful.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 301449 [details] [diff] [review]
Patch

(XPCOM doesn't require sr, just in case you, Mr. Anonymous Approval-Granter, didn't know.  :-) )

Comment 6

[Jeff Walden [:Waldo]]

I'll pick this up later today, most likely tonight.

Comment 7

[Jeff Walden [:Waldo]]

I documented this in:

http://www.mozilla.org/scriptable/xpidl/idl-authors-guide/rules.html

I think we should leave renaming individual names like className, message, etc. to followup bugs, for the most part, but I'd like to add this to at least one attribute so it's demonstrated that the attribute stuff works.  Patch for nsIConsoleMessage.message to do this in a sec...

Comment 8

[Jeff Walden [:Waldo]]

Created attachment 302036 [details] [diff] [review]
Add binaryname(MessageMoz) to nsIConsoleMessage.message

Comment 9

[Jeff Walden [:Waldo]]

For the record, I'm trying to fight tryserver into verifying that Windows will run with this patch, particularly with the #undef GetMessage hunk of it.  It's been slow going due to lots of recent tryserver bustage (I'm on my second bug report to get the Windows one working).  I'll land once I get verification that nothing else depends on that #undef-inition.

Comment 10

[Jeff Walden [:Waldo]]

Created attachment 302735 [details] [diff] [review]
Builds on Windows tryserver

Comment 11

[Jeff Walden [:Waldo]]

In on trunk.  Now that both methods and attributes are being used here, I'm going to mark this FIXED; further cleanup of Windows's mess can go in other bugs.

Comment 12

[aaronr]

How do I know if I need to put this [binaryname(MessageMoz)] thing in a .idl or not?  For example, webservices has a message attribute in nsISOAPMessage.idl but there are no undef GetMessage's anywhere in the webservices code.  And it returns a nsIDOMDocument, not a string.

Comment 13

[Benjamin Smedberg]

Until we start having compiler errors, don't worry about it.

Comment 14

[aaronr]

(In reply to comment #13)
> Until we start having compiler errors, don't worry about it.
> 

Well, reed changed nsSOAPMessage in webservices to have GetMessageMoz which caused compile errors because he missed some spots.  So I was trying determine if his change was even needed.  Since webservices compiled fine without his change, I'll probably back it out.  But I will still need to fix webservices proxy because has a class that extends nsIException which did change to use GetMessageMoz.  I'll open a bug on webservices to make these changes.

Comment 15

[Reed Loden [:reed] (use needinfo?)]

(In reply to comment #14)
> Well, reed changed nsSOAPMessage in webservices to have GetMessageMoz which
> caused compile errors because he missed some spots.  So I was trying determine
> if his change was even needed.  Since webservices compiled fine without his
> change, I'll probably back it out.

This was why I changed it.

<Ubulette> [reed], trunk is broken http://paste.ubuntu.com/4517/

...I make change...

<Ubulette> [reed], still broken
<Ubulette> nsSOAPMessage.cpp:68: error: no 'nsresult nsSOAPMessage::GetMessageMoz(nsIDOMDocument**)' member function declared in class 'nsSOAPMessage'
<Ubulette> before, it was
<Ubulette> nsSOAPException.cpp:63: error: no 'nsresult nsSOAPException::GetMessage(char**)' member function declared in class 'nsSOAPException'
<Ubulette> http://paste.ubuntu.com/4525/

So, webservices does need some change somewhere, it seems.

Comment 16

[aaronr]

Thanks Reed, that helps.  I see why nsSOAPException would give an error, but not nsSOAPMessage.  Maybe it is a platform thing.  I guess I'll leave nsSOAPMessage alone and just add what I need to get my windows machine building again.

Comment 17

[Boris Zbarsky [:bzbarsky, bz on IRC]]

So...  Over here, I'm not seeing any GetMessageMoz in the generated nsISoapMessage.h.  On the other hand, the C++ was changed to use GetMessageMoz, so the whole thing can't possibly compile...

Anyone care to shed some light on how the heck this is supposed to work?

Comment 18

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Hmm.  Looks like I'd somehow managed to pick up the C++ changes but not the idl changes...  repulling tip from toplevel made it work...