We should not be declaring forward declarations for nsString classes directly, instead we should use nsStringFwd.h. This will make changing the underlying types easier.

Sorry for the churn, this includes changes to nsStringFwd.h as well.

Comment on attachment 8898999 [details] [diff] [review] Use nsStringFwd.h for forward declaring string classes Review of attachment 8898999 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/test/gtest/TestCSPParser.cpp @@ -8,5 @@ > > #include <string.h> > #include <stdlib.h> > > -#ifndef MOZILLA_INTERNAL_API This is built as a xul gtest... ::: widget/tests/TestChromeMargin.cpp @@ +16,5 @@ > > #include "TestHarness.h" > > #ifndef MOZILLA_INTERNAL_API > +#error This test needs MOZILLA_INTERNAL_API (see bug 652123) Technically we don't build this test at all. ::: xpcom/build/nsGeckoProcessType.h @@ +1,2 @@ > +#ifndef xpcom_build_nsGeckoProcessType_h > +#define xpcom_build_nsGeckoProcessType_h I split this out so the sandbox and plugin process can include it directly without needing nsXULApi which in turn included nsStringFwd which used to assert MOZILLA_INTERNAL_API. Since I changed nsStringFwd I can probably revert it. ::: xpcom/string/nsStringFwd.h @@ -12,5 @@ > #include "nscore.h" > > -#ifndef MOZILLA_INTERNAL_API > -#error Internal string headers are not available from external-linkage code. > -#endif This was more hassle than it's worth. Given the concrete classes assert this I don't think it's a big deal if non-internal API stuff gets these forward declarations.

Comment on attachment 8898999 [details] [diff] [review] Use nsStringFwd.h for forward declaring string classes Review of attachment 8898999 [details] [diff] [review]: ----------------------------------------------------------------- I have questions, below; I'm also curious if we can just discard the nsGeckoProcessType.h (should probably be GeckoProcessType.h nowadays?) changes, as you already mentioned in your comment on that file. ::: dom/security/test/gtest/TestCSPParser.cpp @@ -13,5 @@ > -// some of the includes make use of internal string types > -#define nsAString_h___ > -#define nsString_h___ > -#define nsStringFwd_h___ > -#define nsReadableUtils_h___ Yuuuuck. ::: xpcom/build/nsGeckoProcessType.h @@ +17,5 @@ > + GeckoProcessType_End, > + GeckoProcessType_Invalid = GeckoProcessType_End > +}; > + > +static const char* const kGeckoProcessTypeString[] = { Reverting the file would be fine, as you mention above, but if we're going to keep it, let's move the definition of kGeckoProcessTypeString into a separate .cpp, so we don't wind up with a separate copy in every file that uses it? I guess we only use the array in a single file at the moment anyway and we currently define the array in a header, but defining static arrays in headers is bad form regardless. ::: xpcom/build/nsXPCOM.h @@ +156,5 @@ > */ > > #ifdef __cplusplus > > +XPCOM_API(nsresult) NS_NewLocalFile(const nsTSubstring<char16_t>& aPath, Can we not use nsStringFwd.h above if __cplusplus, and then use the normal nsA*String types here and below? I am confused.

This removes the XUL API changes in favor of just removing the internal API restriction from nsStringFwd.h

(In reply to Nathan Froyd [:froydnj] from comment #4) > Comment on attachment 8898999 [details] [diff] [review] > > ::: xpcom/build/nsGeckoProcessType.h > @@ +17,5 @@ > > + GeckoProcessType_End, > > + GeckoProcessType_Invalid = GeckoProcessType_End > > +}; > > + > > +static const char* const kGeckoProcessTypeString[] = { > > Reverting the file would be fine, as you mention above, but if we're going > to keep it, let's move the definition of kGeckoProcessTypeString into a > separate .cpp, so we don't wind up with a separate copy in every file that > uses it? > > I guess we only use the array in a single file at the moment anyway and we > currently define the array in a header, but defining static arrays in > headers is bad form regardless. I chose to just revert it for simplicity. > ::: xpcom/build/nsXPCOM.h > @@ +156,5 @@ > > */ > > > > #ifdef __cplusplus > > > > +XPCOM_API(nsresult) NS_NewLocalFile(const nsTSubstring<char16_t>& aPath, > > Can we not use nsStringFwd.h above if __cplusplus, and then use the normal > nsA*String types here and below? I am confused. Yeah we can use it now that the internal API restriction is removed, I'll update it.

Comment on attachment 8899974 [details] [diff] [review] Use nsStringFwd.h for forward declaring string classes Review of attachment 8899974 [details] [diff] [review]: ----------------------------------------------------------------- Great, thank you!

https://hg.mozilla.org/integration/mozilla-inbound/rev/397cfed5073f34740aed9e20460810316ee8ec25 Bug 1391803 - Use nsStringFwd.h for forward declaring string classes. r=froydnj