581198 - Fix xpcom/io/ header exports and remove content/base/src INCLUDE=

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Alex Miller]

content/base/src requires a few headers from xpcom/io/ and instead of them being exported within xpcom/io/, the Makefile.in uses INCLUDE= -I$(topsrcdir)/xpcom/io. content/html/document/src does the same thing.

Comment 1

[Alex Miller]

Attachment: patch

This removes the INCLUDE='s and adds the necessary header files to the list of exports from xpcom/io/.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 459603 [details] [diff] [review]
patch

Quick easy cleanup.  What's not to like?

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #1)
> This removes the INCLUDE='s and adds the necessary header files to the list of
> exports from xpcom/io/.

At one time there were various efforts not to export the header files (e.g. bug 63083). IMO if these header files don't have exported symbols, then we shouldn't be exporting them as it'll mean it forms part of the headers for the libxul SDK and people may try and use those functions.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 459603 [details] [diff] [review]
patch

So taking a look at the code that uses these includes, there's no reason that they can't be converted to just use the IDL generated headers and the regular XPCOM component APIs.

Comment 5

[(not currently active) Ted Mielczarek]

Going through XPCOM is often slower. We've had numerous deCOMtamination refactorings to do the exact opposite.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Sure, but we either need to deCOMtaminate the whole thing across the tree or fix up the callers not to cheat.

Comment 7

[Benjamin Smedberg]

No we don't. Shades of grey are ok.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Compiles locally

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 662583 [details] [diff] [review]
Patch v2

Review of attachment 662583 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/Makefile.in
@@ +166,5 @@
>  include $(topsrcdir)/ipc/chromium/chromium-config.mk
>  include $(topsrcdir)/config/rules.mk
>  
> +LOCAL_INCLUDES += \
> +		-I$(srcdir)/../../base/src \

Isn't this just -I$(srcdir) ? Maybe you can just make these all $(topsrcdir)-relative while you're at it.

@@ +176,4 @@
>  		-I$(srcdir)/../../xul/content/src \
>  		-I$(srcdir)/../../xul/document/src \
> +		-I$(topsrcdir)/caps/include \
> +		-I$(topsrcdir)/dom/base \

Can you fix this to 2-space indent while you're here?

::: xpcom/io/Makefile.in
@@ +83,1 @@
>  		$(NULL)			

Kill this trailing whitespace while you're here.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Ted Mielczarek [:ted] from comment #9)
> Comment on attachment 662583 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 662583 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/Makefile.in
> @@ +166,5 @@
> >  include $(topsrcdir)/ipc/chromium/chromium-config.mk
> >  include $(topsrcdir)/config/rules.mk
> >  
> > +LOCAL_INCLUDES += \
> > +		-I$(srcdir)/../../base/src \
> 
> Isn't this just -I$(srcdir) ? Maybe you can just make these all
> $(topsrcdir)-relative while you're at it.

Heh, looks like it is. I kept $(srcdir) for the ones in the same top-level dir, but I can change too.

> @@ +176,4 @@
> >  		-I$(srcdir)/../../xul/content/src \
> >  		-I$(srcdir)/../../xul/document/src \
> > +		-I$(topsrcdir)/caps/include \
> > +		-I$(topsrcdir)/dom/base \
> 
> Can you fix this to 2-space indent while you're here?

Sure.

> ::: xpcom/io/Makefile.in
> @@ +83,1 @@
> >  		$(NULL)			
> 
> Kill this trailing whitespace while you're here.

Sure.

Comment 11

[Mike Hommey [:glandium]]

-I$(srcdir) is already in INCLUDES, so you can skip that.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/075745deb9a8