Closed
Bug 750216
Opened 12 years ago
Closed 12 years ago
don't export headers that aren't used outside
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: surkov, Assigned: capella)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])
Attachments
(1 file, 3 obsolete files)
7.94 KB,
patch
|
surkov
:
review+
tbsaunde
:
feedback+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
note: bug 748716 should be fixed before doing this one
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•12 years ago
|
||
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 3•12 years ago
|
||
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•12 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•12 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.
Comment 6•12 years ago
|
||
(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•12 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•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
Ok, I made the a11y includes conditional.
Attachment #621874 -
Attachment is obsolete: true
Attachment #621874 -
Flags: feedback?(trev.saunders)
Assignee | ||
Comment 12•12 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•12 years ago
|
Attachment #621889 -
Flags: review?(trev.saunders)
Comment 13•12 years ago
|
||
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•12 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•12 years ago
|
||
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•12 years ago
|
Attachment #621924 -
Flags: feedback?(trev.saunders)
Assignee | ||
Comment 16•12 years ago
|
||
FYI https://tbpl.mozilla.org/?tree=Try&rev=17852a7b64c2
Comment 17•12 years ago
|
||
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+
Reporter | ||
Comment 18•12 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•12 years ago
|
Attachment #621924 -
Flags: review+
Reporter | ||
Comment 19•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d587b25033
Target Milestone: --- → mozilla15
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a5d587b25033
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•