Closed Bug 1486772 Opened 2 years ago Closed 2 years ago
Screen Configuration .h implicitly includes everything and the kitchen sink
46 bytes, text/x-phabricator-request
|Details | Review|
While trying to refactoring the HAL code in bug 1476250 I ran into include header clashes with X11 system headers. The issue was caused by an unfortunate (and unneeded) chain of inclusions: - Hal.h includes HalScreenConfiguration.h - HalScreenConfiguration.h includes dom/base/ScreenOrientation.h because it needs the ScreenOrientationInternal type (more on this later) - ScreenOrientation.h includes quite a bit of headers including ClientBinding.h which clashes with certain X11 headers As it turns out there's an almost circular dependency between HalScreenConfiguration.h and ScreenOrientation.h because the latter's implementation uses the HAL function provided in HalScreenConfiguration.h and the former needs the type declared in ScreenOrientation.h. The best way to clean this up is to make the dependencies go in only one direction, so I'd move ScreenOrientationInternal into HalScreenConfiguration.h - which is the lowest level of the whole stack - and have all the code that depends on it include that instead of ScreenConfiguration.h. This will also fix the code that re-declared (!) ScreenOreitnationInternal  because it couldn't include ScreenConfiguration.h.  https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/docshell/base/nsDocShell.h#72
This patch removes the 'ScreenOrientationInternal' type from dom/base/ScreenOrientation.h and moves it into the HalScreenConfiguration.h header, renaming it simply to 'ScreenOrientation' in the process. This has several knock-off effects: - It allows files that needed ScreenOrientationInternal to include a much smaller header than before - It greatly reduces the number of headers pulled in when including Hal.h - It clarifies the role of the type. The 'Internal' part in the name had nothing to do with it being part of the implementation. The type was public and called that way only to avoid clashing with the 'ScreenOrientation' class. Since we moved it into a different namespace it can be renamed safely. - It allows a file that was manually re-declaring 'ScreenConfigurationInternal' type to use the original one - Finally this fixes a few files which were missing headers they actually required but that would still build because unified compilation put them into units that already had those headers thanks to ScreenConfiguration.h
Comment on attachment 9004592 [details] Bug 1486772 - Refactor the screen-orientation types and headers Olli Pettay [:smaug] has approved the revision.
Attachment #9004592 - Flags: review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/000a3f46f36c Refactor the screen-orientation types and headers r=smaug
You need to log in before you can comment on or make changes to this bug.