Part of the libxul plan to improve performance and reduce embedding headaches is to stop exporting the nonfrozen symbols. This bug is about the XPCOM symbols.

This uses a new macro for the frozen XPCOM API, and keeps NS_COM for internal symbols. It doesn't actually stop exporting the NS_COM symbols from libxul yet, pending tree fixup (see blocking bugs).

Comment on attachment 201843 [details] [diff] [review] Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 1 >+# Can't build internal xptcall tests that use symbols which are not exported. >+ifndef MOZ_ENABLE_LIBXUL >+TOOL_DIRS += \ > reflect/xptinfo/tests \ > reflect/xptcall/tests \ >- proxy/tests >+ $(NULL) >+endif > endif We need to figure out how to expose xptcall symbols like XPTC_InokeByIndex because it is needed to do things like implement python and java bindings. >Index: xpcom/string/public/nsStringAPI.h >+inline char* >+ToNewUTF8String(const nsAString& aSource) >+{ >+ nsCString temp; >+ CopyUTF16toUTF8(aSource, temp); >+ return NS_CStringCloneData(temp); >+} So, we have to decide whether nsStringAPI.h should be an attempt to implement an API-compatible equivalent to our internal string API, or if it should be a clean API ;-) I vote for a cleaner API. Can we please avoid bringing in baggage from the internal string API? Maybe move this function and others like it into a separate compat header file? >+#define PromiseFlatString nsString >+#define PromiseFlatCString nsCString >+ >+inline char* >+ToNewCString(const nsACString& aStr) >+{ >+ return NS_CStringCloneData(aStr); >+} ditto for these... >+typedef nsCString nsCAutoString; >+typedef nsString nsAutoString; >+ >+#define nsXPIDLString nsString >+#define nsXPIDLCString nsCString and these... why not use typedef for nsXPIDLString? >Index: xpcom/tests/TestMinStringAPI.cpp > NS_StringSetData(s, kUnicodeData, PR_UINT32_MAX); > len = NS_StringGetData(s, &ptr); >+ if (len != sizeof(kUnicodeData)/2 - 1) > { > NS_ERROR("unexpected result"); > return PR_FALSE; > } >+ if (ptr == nsnull || memcmp(ptr, kUnicodeData, sizeof(kUnicodeData)/2 - 1) != 0) This looks wrong. If you are using memcmp, then you are comparing bytes, so you should pass a length value that expresses the number of bytes. I think that sizeof(kUnicodeData) is correct because it also includes the null terminator, which we can assert exists. >+ if (ptr == nsnull || memcmp(ptr, kUnicodeData, sizeof(kUnicodeData)/2 - 1) != 0) ditto. >- nsMemory::Free(clone); >+ NS_Free(clone); Is nsMemory::Free no longer part of the glue?

> We need to figure out how to expose xptcall symbols like > XPTC_InokeByIndex because it is needed to do things like > implement python and java bindings. Are we going to freeze them? I don't mind freezing XPTC_InvokeByIndex as-is, but nsXPTCStubBase/nsXPTCVariant should probably be in the glue since there's no good way to freeze it as it depends on the C++ ABI. > Is nsMemory::Free no longer part of the glue? It is, but it inlines directly to NS_Free

> Are we going to freeze them? I don't mind freezing XPTC_InvokeByIndex as-is, > but nsXPTCStubBase/nsXPTCVariant should probably be in the glue since there's > no good way to freeze it as it depends on the C++ ABI. Last time I asked about this, shaver said that these should not be frozen as-is. I don't know the issues, but I do care about supporting language bindings via extensions, so we need to figure out what we want to do. > > Is nsMemory::Free no longer part of the glue? > > It is, but it inlines directly to NS_Free OK, that's what I figured. You were just changing the code to use NS_Free for aesthetic reasons then ;-)

Shaver, can we just move all of xptcall into the glue (actually probably its own glue-like library)?

After discussing with darin, we are going to leave a lot of the CopyUTF8toUTF16 glue in nsStringAPI.h, and mark certain things (nsXPIDLString and nsAutoString) as deprecated. This patch also happens to include bits of bug 315401.

Comment on attachment 202133 [details] [diff] [review] Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2 >Index: xpcom/string/public/nsStringAPI.h >+#define nsXPIDLString nsXPIDLString_external >+#define nsXPIDLCString nsXPIDLCString_external Since these are deprecated, let's move them down to where nsXPIDLString is defined. >+inline char* >+ToNewCString(const nsACString& aStr) >+{ >+ return NS_CStringCloneData(aStr); >+} >+ >+inline PRUnichar* >+ToNewUnicode(const nsAString& aStr) >+{ >+ return NS_StringCloneData(aStr); >+} I think these should be in the deprecated section since the names of these functions are terrible. "ToNewCString"... sounds like "new nsCString" but it isn't. "ToNewUnicode"... unicode... what?!? >+/** >+ * The following declarations are *deprecated*, and are included here only >+ * to make porting from existing code that doesn't use the frozen string API >+ * easier. They may disappear in the future. > */ > >+class nsXPIDLString : public nsString >+{ >+public: >+ operator const PRUnichar*() { return get(); } >+}; >+ >+class nsXPIDLCString : public nsCString >+{ >+public: >+ operator const char*() { return get(); } >+}; If you are going to support these, what about supporting the NULL return value from .get() and the cast for nsXPIDLString? That would seem to be a very important part of nsXPIDLString that is not being supported here. > #define EmptyCString() nsCString() > #define EmptyString() nsString() EmptyCString/EmptyString are not deprecated. please move them out of this section.

> >+class nsXPIDLString : public nsString > >+ operator const PRUnichar*() { return get(); } > If you are going to support these, what about supporting the NULL return > value from .get() and the cast for nsXPIDLString? That would seem to be > a very important part of nsXPIDLString that is not being supported here. I don't know what you mean, especially WRT to casting. I will try removing these casts and see whether my existing code compiles, until I can figure out what you're talking about ;-)

> > If you are going to support these, what about supporting the NULL return > > value from .get() and the cast for nsXPIDLString? That would seem to be > > a very important part of nsXPIDLString that is not being supported here. > > I don't know what you mean, especially WRT to casting. I will try removing > these casts and see whether my existing code compiles, until I can figure out > what you're talking about ;-) Take this code for example: { nsXPIDLString s; foo->GetBar(getter_Copies(s)); if (s) { // do stuff with |s| } } Notice that this code depends on "nsXPIDLString::operator const PRUnichar*()" evaluating to null when GetBar returns a null out param. nsString::get() never returns null, so nsString is not a substitute for nsXPIDLString in the above sample code. This is a very common use-case for nsXPIDLString, so I suspect that you must have encountered code that is depending on this.

OK, I didn't need the xpidlstring stuff... Since this doesn't flip the NS_COM switch yet, I'd to land this as-is and then look at how to deal with the xptcall stuff.

Comment on attachment 202253 [details] [diff] [review] Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2.1 [checked in] r=darin with the typedefs for nsXPIDLC?String removed from nsStringAPI.h -- it's just really worrisome to have those there if they do not have the "null" property that I mentioned previously. they are likely to be misused.

Comment on attachment 202253 [details] [diff] [review] Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2.1 [checked in] Attachment 202253 [details] [diff] checked in with xpidlc?string removed and CopyUTF16toASCII renamed with a Lossy to match the nonfrozen version.

I'm thinking that we never want to override IMETHOD_VISIBILITY to NS_VISIBILITY_DEFAULT, always preferring to define it to empty and use the class-scope visibility declaration.

> I'm thinking that we never want to override IMETHOD_VISIBILITY to > NS_VISIBILITY_DEFAULT, always preferring to define it to empty and use the > class-scope visibility declaration. And that works properly on Linux? NS_COM is empty on Linux, no?

No, http://lxr.mozilla.org/mozilla/source/xpcom/base/nscore.h#282

Comment on attachment 218722 [details] [diff] [review] Various XPCOM macros, rev. 1 [checked in] ok, but i think bryner should review this. i thought that older compilers lacked support for specifying visibility at the class level.

They do, but on those compilers we don't use pragmas nor -fvisibility=hidden, so the default visibility is fine.

Comment on attachment 218722 [details] [diff] [review] Various XPCOM macros, rev. 1 [checked in] I wonder if we should change the way the visibility macros work so that it's not necessary to duplicate this somewhat non-intuitive #defining. In any case, sr=me.

The widget classes are not NS_EXPORT or called across dll boundaries, no need to override the default visibility. The NSReg symbols are all statically linked, no need for dllexport mojo. The OJI symbol just needed visibility love.

This is done well enough to call it fixed.