Closed
Bug 1486772
Opened 6 years ago
Closed 6 years ago
HalScreenConfiguration.h implicitly includes everything and the kitchen sink
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Core
Hardware Abstraction Layer (HAL)
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: gsvelto, Assigned: gsvelto)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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 [1] because it couldn't include ScreenConfiguration.h.
[1] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/docshell/base/nsDocShell.h#72
Assignee | ||
Comment 1•6 years ago
|
||
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 2•6 years ago
|
||
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 gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/000a3f46f36c
Refactor the screen-orientation types and headers r=smaug
Comment 4•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
Assignee: nobody → gsvelto
You need to log in
before you can comment on or make changes to this bug.
Description
•