238088 - Support compile-time backwards compatible nsAString in Gecko SDK [was: nsAString.h is _completely_ different in gecko-sdk and mozilla trees.]

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Michael 'mig' Gerard]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; .NET CLR 1.0.3705; .NET CLR 1.1.4322)
Build Identifier: 

nsAString.h is totally different in the current (1.6) gecko-sdk and mozilla 
trees and pretty much incompatible with each other (code developed to work with 
the gecko-sdk tree refuses to compile under the mozilla tree, and vice versa).


The following is just one example of the differentiation between gecko and 
mozilla trees -- neither side of the elif will properly compile under both.  
Tried lots of different permutations (but because there are such an 
overwhelming number of possible permutations to do something as simple as 
extract the bloody pointer from the object, I will readily admit it possible 
that I just missed the magic one that actually works correctly):

#if defined(USING_GECKO_SDK)

#  include "nsEmbedString.h"
#  define __nsACString_To_UTF8String(in, out) \
 nsEmbedCString ___nsACString_To_UTF8String_##in(in); \
 out = (char *)___nsACString_To_UTF8String_##in.get(); 

#elif defined(USING_MOZILLA_SDK)

#  include "nsPromiseFlatString.h"
#  define __nsACString_To_UTF8String(in, out) out = (char *)PromiseFlatCString
(in).get()

#endif


Reproducible: Always
Steps to Reproduce:
1. Compile code under gecko-sdk.
2. Compile code under mozilla.
3. Attempt to make something that actually works under both.
4. Cry.

Actual Results:  
Much crying.

Expected Results:  
There should be the same set of headers in both trees.

Comment 1

[Olivier Cahagne]

this is possibly because the string implementation changed in 1.7, see the
notice at the beginning of http://www.mozilla.org/projects/xpcom/string-guide.html
and bug 231995 and bug 227240.
I believe the gecko-sdk will be updated to reflect this as soon as 1.7 is out.

Comment 2

[Michael 'mig' Gerard]

Great.... so.... when gecko-sdk 1.7 comes out is it just going to break 
everybody's current code and force them to recompile?  Or is there actually 
some middle ground here that will work for both older and newer SDKs?

The task of "given a reference to nsACString, get a flat pointer to its 
internals to send down into system API's" seems kinda basic for compilation 
incompatibilities, don't you think?

mig

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

hmm, what part of comment 0 does not compile? it looks like it should work...
does it fail to compile with current trunk (1.7b), or with 1.6?

Comment 4

[Michael 'mig' Gerard]

Apologies, I didn't give enough information.

The given nsEmbedString.h usage actually works in both SDK's.

Unfortunately, I also want to go in the other direction:

#  define __UTF8String_To_nsACString(in, out) out.Assign(in); 

The "Assign" method isn't defined under the 1.7 SDK with just nsEmbedString.h 
(this header is totally different between the two SDK's as well), and including 
nsAString.h before or after nsEmbedString.h causes nasty "Redefinition of class 
nsAString" errors.

In file included 
from /Users/mig/projects/mozilla/dist/sdk/string/include/nsEmbedString.h:43,
                 from /Volumes/PROJECTS/Unagi/plugin/Mozilla/xpcom.h:9,
                 from /Volumes/PROJECTS/Unagi/plugin/CUnagiCtrl.h:17,
                 from /Volumes/PROJECTS/Unagi/plugin/CUnagiCtrl.cpp:3:
/Users/mig/projects/mozilla/dist/sdk/string/include/nsStringAPI.h:66: error: 
redefinition
   of `class nsAString'
/Users/mig/projects/mozilla/dist/include/string/nsTAString.h:104: error: 
previous
   definition of `class nsAString'

This is why I have to split my code completely for both SDK's.

(And I got a "midair collision" typing this, heh, never seen that in bz before)

Comment 5

[Darin Fisher]

mig:

if you compile against 1.6, then your code will run fine against 1.6 and any
future GRE (binary compatible with all frozen interfaces).  however, if you
start compiling against the pre-1.7 string headers, then you may need to modify
your code.  see this newsgroup posting for more info:

http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&selm=c0v0sv%24p954%40ripley.netscape.com

NOTE: you cannot mix nsEmbedString.h (or nsStringAPI.h) with nsAString.h. 
nsAString and nsACString are abstract class types, and it is true that
nsStringAPI.h defines its own version of this class.  in fact, nsAString.h is
now no longer part of the SDK.  (it should only be used by files in the mozilla
codebase.)  this was done to simplify the string class API for embedders and
external component authors.  the mail thread has more details.

to go "the other way", you'll need to use the methods defined in nsStringAPI.h.
 note: using those methods restricts you to mozilla 1.7 and later.  the function
you want is: NS_CStringCopy.

the rule with the GRE is: compile your code against the earliest version of the
GRE that you want your application to support.

this bug is almost WONTFIX.  the change was intentional.  however, i could see
us supporting the old nsAString API methods on the nsAString class defined in
nsStringAPI.h in terms of the NS_ methods as a way of providing compile-time
backwards compatibility.  hmm...

Comment 6

[Darin Fisher]

> however, if you start compiling against the pre-1.7 string headers, then you may 
> need to modify your code.

i meant: "the 1.7 string headers"

Comment 7

[Darin Fisher]

Attachment: v1 patch

something like this perhaps...

Comment 8

[Darin Fisher]

The NS_DEFINE_ASTRING_METHODS macro helps keep nsStringAPI.cpp happy.

Comment 9

[Benjamin Smedberg]

Comment on attachment 144424 [details] [diff] [review]
v1 patch

r=me

I don't like the empty nsStringContainer constructor, though I haven't played
with MSVC's compiler bugs. Can we make nsStringContainer a struct and get rid
of the constructor?

Comment 10

[Darin Fisher]

Comment on attachment 144424 [details] [diff] [review]
v1 patch

this is a low risk change

Comment 11

[chris hofmann]

Comment on attachment 144424 [details] [diff] [review]
v1 patch

a=chofmann for 1.7

Comment 12

[Darin Fisher]

fixed-on-trunk for 1.7 final.

Comment 13

[Darin Fisher]

this change broke the debug seamonkey w/ gre build on WIN32 due to duplicate
symbols in string_s.lib and xpcomglue.lib :-(

i'm working on a fix.

Comment 14

[Darin Fisher]

Attachment: WIN32 bustage fix

Comment 15

[Darin Fisher]

WIN32 bustage fix checked in.