Last Comment Bug 750216 - don't export headers that aren't used outside
: don't export headers that aren't used outside
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
:
Mentors:
Depends on: 748716
Blocks: cleana11y 748719
  Show dependency treegraph
 
Reported: 2012-04-30 05:06 PDT by alexander :surkov
Modified: 2012-05-11 11:42 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (3.68 KB, patch)
2012-05-03 14:49 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (9.68 KB, patch)
2012-05-07 21:24 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (9.65 KB, patch)
2012-05-07 22:40 PDT, Mark Capella [:capella]
roc: review+
Details | Diff | Splinter Review
Patch (v4) (7.94 KB, patch)
2012-05-08 03:03 PDT, Mark Capella [:capella]
surkov.alexander: review+
tbsaunde+mozbugs: feedback+
Details | Diff | Splinter Review

Description alexander :surkov 2012-04-30 05:06:39 PDT
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 2012-04-30 05:07:17 PDT
note: bug 748716 should be fixed before doing this one
Comment 2 Mark Capella [:capella] 2012-05-03 14:49:14 PDT
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
Comment 3 Trevor Saunders (:tbsaunde) 2012-05-07 02:00:35 PDT
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] 2012-05-07 03:54:13 PDT
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 2012-05-07 04:44:13 PDT
(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) 2012-05-07 06:12:03 PDT
(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] 2012-05-07 06:46:27 PDT
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 2012-05-07 07:50:22 PDT
(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] 2012-05-07 21:24:31 PDT
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
Comment 10 alexander :surkov 2012-05-07 21:32:33 PDT
(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] 2012-05-07 22:40:09 PDT
Created attachment 621889 [details] [diff] [review]
Patch (v3)

Ok, I made the a11y includes conditional.
Comment 12 Mark Capella [:capella] 2012-05-07 22:41:17 PDT
Comment on attachment 621889 [details] [diff] [review]
Patch (v3)

Asking :roc for review? for the /widgets changes...
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2012-05-08 00:54:01 PDT
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 2012-05-08 01:01:59 PDT
(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] 2012-05-08 03:03:35 PDT
Created attachment 621924 [details] [diff] [review]
Patch (v4)

Changed back to bare minimum use of EXPORTS, only exporting files that /widgets needs.
Comment 16 Mark Capella [:capella] 2012-05-08 04:01:31 PDT
FYI

https://tbpl.mozilla.org/?tree=Try&rev=17852a7b64c2
Comment 17 Trevor Saunders (:tbsaunde) 2012-05-08 09:12:23 PDT
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 2012-05-10 06:45:06 PDT
(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 20 Matt Brubeck (:mbrubeck) 2012-05-11 11:42:22 PDT
https://hg.mozilla.org/mozilla-central/rev/a5d587b25033

Note You need to log in before you can comment on or make changes to this bug.