750216 - don't export headers that aren't used outside

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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.

Comment 1

[alexander :surkov (:asurkov)]

note: bug 748716 should be fixed before doing this one

Comment 2

[Mark Capella [:capella]]

Attachment: 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

Comment 3

[Trevor Saunders (:tbsaunde)]

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?

Comment 4

[Mark Capella [:capella]]

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.

Comment 5

[alexander :surkov (:asurkov)]

(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.

Comment 6

[Trevor Saunders (:tbsaunde)]

(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

Comment 7

[Mark Capella [:capella]]

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

Comment 8

[alexander :surkov (:asurkov)]

(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.

Comment 9

[Mark Capella [:capella]]

Attachment: 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

Comment 10

[alexander :surkov (:asurkov)]

(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.

Comment 11

[Mark Capella [:capella]]

Attachment: Patch (v3)

Ok, I made the a11y includes conditional.

Comment 12

[Mark Capella [:capella]]

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

Asking :roc for review? for the /widgets changes...

Comment 13

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

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.

Comment 14

[alexander :surkov (:asurkov)]

(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)?

Comment 15

[Mark Capella [:capella]]

Attachment: Patch (v4)

Changed back to bare minimum use of EXPORTS, only exporting files that /widgets needs.

Comment 16

[Mark Capella [:capella]]

FYI

https://tbpl.mozilla.org/?tree=Try&rev=17852a7b64c2

Comment 17

[Trevor Saunders (:tbsaunde)]

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.

Comment 18

[alexander :surkov (:asurkov)]

(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.

Comment 19

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d587b25033

Comment 20

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/a5d587b25033