The default bug view has changed. See this FAQ.

don't export headers that aren't used outside

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: capella)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
msaa/Makefile.in

ARIAGridAccessibleWrap.h, nsTextAccessibleWrap.h, nsHTMLWin32ObjectAccessible.h, nsXULMenuAccessibleWrap.h, nsXULListboxAccessibleWrap.h, nsXULTreeGridAccessibleWrap.h, nsHTMLImageAccessibleWrap.h, nsHTMLTableAccessibleWrap.h, CAccessibleImage.h, CAccessibleTable.h, CAccessibleTableCell.h

atk/Makefile.in

ARIAGridAccessibleWrap.h, AtkSocketAccessible.h, nsTextAccessibleWrap.h, nsXULMenuAccessibleWrap.h, nsXULListboxAccessibleWrap.h, nsXULTreeGridAccessibleWrap.h, nsHTMLImageAccessibleWrap.h, nsHTMLTableAccessibleWrap.h

similar files for mac and other folders.
(Reporter)

Comment 1

5 years ago
note: bug 748716 should be fixed before doing this one
(Reporter)

Updated

5 years ago
Depends on: 748716
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 620860 [details] [diff] [review]
Patch (v1)

First try. I had to leave some exports in place to continue to build ...

   nstextaccessiblewrap.h is required by html/nshtmltextaccessible.h

   caccessibleimage.h is required by nshtmlimageaccessiblewrap.h which is required by html/nshtmlimagemapaccessible.h

   caccessibletable.h and caccessibletablecell.h are required by nsxultreegridaccessiblewrap.h which is required by xul/nsxultreegridaccessible.h
Attachment #620860 - Flags: feedback?(trev.saunders)
Comment on attachment 620860 [details] [diff] [review]
Patch (v1)

> EXPORTS = \
>-  ARIAGridAccessibleWrap.h \
>-  AtkSocketAccessible.h \
>   nsAccessNodeWrap.h \
>   nsAccessibleWrap.h \
>   nsDocAccessibleWrap.h \

what's this one for?

>   nsXULTreeGridAccessibleWrap.h \
>   nsHyperTextAccessibleWrap.h \

and these?

>   mozDocAccessible.h \
>   mozAccessible.h \
>   mozAccessibleProtocol.h \
>   mozActionElements.h \
>   mozTextAccessible.h \

what about these?

>   nsXULTreeGridAccessibleWrap.h \

what's this for?
(Assignee)

Comment 4

5 years ago
I pulled all the moz exports from above, and built / tested successfully.

Then I removed all 4 of the the nsDocAccessibleWrap.h exports and the build failed. Processing had exited the /msaa directory and entered the /generic directory and failed due to generic/RootAccessible.h needing that header.

Experimenting a little, I replaced the export of that header back into the msaa/makefile.in only, and the build then was successful, and mochitests ran ok.

Hmmm ... not sure if this is valid. Exports go the $OBJDIR/dist/include directory, so if we're exporting 4 different versions of the same header file to there (overlaying each other as they go) ... do subsequent directory builds need to use their own export version while the build is in progress?

Perhaps the way I have it now one of the other/mac/atk directories may use the msaa/ exported header version to build ok and that masks a potential error later?

To keep moving, I'm putting all four exports of this header back into the makefiles for now, and seeing if any of the remaining headers that you pointed out can be removed across the board. That should be safe.
(Reporter)

Comment 5

5 years ago
(In reply to Mark Capella [:capella] from comment #4)
> I pulled all the moz exports from above, and built / tested successfully.
> 
> Then I removed all 4 of the the nsDocAccessibleWrap.h exports and the build
> failed. Processing had exited the /msaa directory and entered the /generic
> directory and failed due to generic/RootAccessible.h needing that header.

you need to add platform specific folders to generic Makefile.in as you did for base/Makefile.in

> Experimenting a little, I replaced the export of that header back into the
> msaa/makefile.in only, and the build then was successful, and mochitests ran
> ok.

btw, if it doesn't have build errors then mochitest must be ok.

> Hmmm ... not sure if this is valid. Exports go the $OBJDIR/dist/include
> directory, so if we're exporting 4 different versions of the same header
> file to there (overlaying each other as they go) ... do subsequent directory
> builds need to use their own export version while the build is in progress?

nope, only one platforms folder is built so you don't run into collision. for example, if you're on windows then only msaa directory is built.
(In reply to alexander :surkov from comment #5)
> (In reply to Mark Capella [:capella] from comment #4)
> > I pulled all the moz exports from above, and built / tested successfully.
> > 
> > Then I removed all 4 of the the nsDocAccessibleWrap.h exports and the build
> > failed. Processing had exited the /msaa directory and entered the /generic
> > directory and failed due to generic/RootAccessible.h needing that header.
> 
> you need to add platform specific folders to generic Makefile.in as you did
> for base/Makefile.in

I wonder if it would make more sense to just leave them and work on killing wrap classes?

> > Experimenting a little, I replaced the export of that header back into the
> > msaa/makefile.in only, and the build then was successful, and mochitests ran
> > ok.
> 
> btw, if it doesn't have build errors then mochitest must be ok.

well, technically it could if something very odd is happening, but its pretty unlikely
(Assignee)

Comment 7

5 years ago
Ah, ok....

More research / testing shows we can pretty much remove all EXPORTS if we add platform specific folders to all makefiles under /src ... /atk, /base, /generic, /html, (/jsat not required), /mac, /msaa, /other, /xforms, /xpcom, and /xul.

We still need to leave mozAccessibleProtocol.h, and nsAccessible.h and any dependencies they have (ie: nsaccessnodewrap.h) as they are being used alongside

/accessible 

by:

/widget/cocoa/nsChildView.h 
/widget/gtk2/nsWindow.h
/widget/windows/nsWindow.h
(Reporter)

Comment 8

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> I wonder if it would make more sense to just leave them and work on killing
> wrap classes?

it doesn't look like we can do that soon (until you suggest to simply move all wraps code into ifdefs under base/generic/etc folders). In either case they shouldn't be exported :) so we can do that now in consistent way as we do it now for base folder.
(Assignee)

Comment 9

5 years ago
Created attachment 621874 [details] [diff] [review]
Patch (v2)

Check out this approach ! Gets rid of all EXPORTS, though I had to make minor patchs into the /widget makefiles.

https://tbpl.mozilla.org/?tree=Try&rev=bbfd9289a27f
Attachment #620860 - Attachment is obsolete: true
Attachment #620860 - Flags: feedback?(trev.saunders)
Attachment #621874 - Flags: feedback?(trev.saunders)
(Reporter)

Comment 10

5 years ago
(In reply to Mark Capella [:capella] from comment #9)
> Created attachment 621874 [details] [diff] [review]
> Patch (v2)
> 
> Check out this approach ! Gets rid of all EXPORTS, though I had to make
> minor patchs into the /widget makefiles.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=bbfd9289a27f

you need to get review from widgets peer (Roc for example). I don't have strong opinion whether it's ok to have a11y dependencies in other modules.

btw, you should ifdef that include in case of non a11y build.
(Assignee)

Comment 11

5 years ago
Created attachment 621889 [details] [diff] [review]
Patch (v3)

Ok, I made the a11y includes conditional.
Attachment #621874 - Attachment is obsolete: true
Attachment #621874 - Flags: feedback?(trev.saunders)
(Assignee)

Comment 12

5 years ago
Comment on attachment 621889 [details] [diff] [review]
Patch (v3)

Asking :roc for review? for the /widgets changes...
Attachment #621889 - Flags: review?(roc)
Attachment #621889 - Flags: review?(roc) → review+
(Reporter)

Updated

5 years ago
Attachment #621889 - Flags: review?(trev.saunders)
Comment on attachment 621889 [details] [diff] [review]
Patch (v3)

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

::: widget/gtk2/Makefile.in
@@ +144,5 @@
>  INCLUDES   	+= -I$(srcdir)/../shared/x11
>  endif
> +ifdef ACCESSIBILITY
> +INCLUDES   	+= -I$(topsrcdir)/accessible/src/atk
> +endif

I don't think this is a good idea... If you need something from a11y outside a11y, that's what EXPORTS are for.
(Reporter)

Comment 14

5 years ago
(In reply to Ms2ger from comment #13)

> I don't think this is a good idea... If you need something from a11y outside
> a11y, that's what EXPORTS are for.

Mark, could you change that back please (export everything what's used outside a11y module)?
(Assignee)

Comment 15

5 years ago
Created attachment 621924 [details] [diff] [review]
Patch (v4)

Changed back to bare minimum use of EXPORTS, only exporting files that /widgets needs.
Attachment #621889 - Attachment is obsolete: true
Attachment #621889 - Flags: review?(trev.saunders)
(Assignee)

Updated

5 years ago
Attachment #621924 - Flags: feedback?(trev.saunders)
(Assignee)

Comment 16

5 years ago
FYI

https://tbpl.mozilla.org/?tree=Try&rev=17852a7b64c2
Comment on attachment 621924 [details] [diff] [review]
Patch (v4)

>+ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
>+LOCAL_INCLUDES += \
>+  -I$(srcdir)/../atk \
>+  $(NULL)
>+else
>+ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
>+LOCAL_INCLUDES += \
>+  -I$(srcdir)/../msaa \
>+  $(NULL)
>+else
>+ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
>+LOCAL_INCLUDES += \
>+  -I$(srcdir)/../mac \
>+  $(NULL)
>+else
>+LOCAL_INCLUDES += \
>+  -I$(srcdir)/../other \
>+  $(NULL)
>+endif
>+endif
>+endif


spamming this all over sort of sucks, but I guess its a little better than exporting stuff nobody uses.
Attachment #621924 - Flags: feedback?(trev.saunders) → feedback+
(Assignee)

Updated

5 years ago
Blocks: 748719
(Reporter)

Comment 18

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #17)

> spamming this all over sort of sucks, but I guess its a little better than
> exporting stuff nobody uses.

It'd be nice to have something like rules.mk that contains these ifdefs and included into each a11y makefile.
(Reporter)

Updated

5 years ago
Attachment #621924 - Flags: review+
(Reporter)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d587b25033
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/a5d587b25033
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.