Classes should use nsStringFwd.h rather than ad-hoc definitions

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Attachment #8898986 - Flags: review?(nfroyd)
Sorry for the churn, this includes changes to nsStringFwd.h as well.
Attachment #8898999 - Flags: review?(nfroyd)
Attachment #8898986 - Attachment is obsolete: true
Attachment #8898986 - Flags: review?(nfroyd)
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.
Attachment #8898999 - Flags: review?(nfroyd) → feedback+
Priority: -- → P3
This removes the XUL API changes in favor of just removing the internal API restriction from nsStringFwd.h
Attachment #8899974 - Flags: review?(nfroyd)
Attachment #8898999 - Attachment is obsolete: true
(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!
Attachment #8899974 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/397cfed5073f34740aed9e20460810316ee8ec25
Bug 1391803 - Use nsStringFwd.h for forward declaring string classes. r=froydnj
Blocks: 1393230
https://hg.mozilla.org/mozilla-central/rev/397cfed5073f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.