Closed Bug 581198 Opened 9 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla18

People

(Reporter: millerdevel, Assigned: Ms2ger)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
This removes the INCLUDE='s and adds the necessary header files to the list of exports from xpcom/io/.
Attachment #459603 - Flags: review?(me)
Comment on attachment 459603 [details] [diff] [review]
patch

Quick easy cleanup.  What's not to like?
Attachment #459603 - Flags: review?(me)
Attachment #459603 - Flags: review+
Attachment #459603 - Flags: approval2.0?
(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 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.
Attachment #459603 - Flags: review+
Attachment #459603 - Flags: approval2.0?
Going through XPCOM is often slower. We've had numerous deCOMtamination refactorings to do the exact opposite.
Sure, but we either need to deCOMtaminate the whole thing across the tree or fix up the callers not to cheat.
No we don't. Shades of grey are ok.
Attached patch Patch v2Splinter Review
Compiles locally
Assignee: nobody → Ms2ger
Attachment #459603 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #662583 - Flags: review?(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.
Attachment #662583 - Flags: review?(ted.mielczarek) → review+
(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.
-I$(srcdir) is already in INCLUDES, so you can skip that.
https://hg.mozilla.org/mozilla-central/rev/075745deb9a8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.