Closed Bug 1486772 Opened Last year Closed Last year

HalScreenConfiguration.h implicitly includes everything and the kitchen sink

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

defect
Not set

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
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 gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/000a3f46f36c
Refactor the screen-orientation types and headers r=smaug
https://hg.mozilla.org/mozilla-central/rev/000a3f46f36c
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee: nobody → gsvelto
You need to log in before you can comment on or make changes to this bug.