Use fallback files in hal/ when a backend isn't implemented instead of having randomly defined methods

RESOLVED FIXED in mozilla15

Status

()

Core
Hardware Abstraction Layer (HAL)
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla15
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 619057 [details] [diff] [review]
Patch v1

I tried to apply an old patch a volunteer wrote for a MacOS Battery API backend and the experience was terrible: given that most backends are currently defining methods for un-implemented stuff in a sort of generic cpp file (WindowsHal.cpp, LinuxHal.cpp, etc.) it is very annoying to add something especially given that MacOS is using FallbackHal.cpp which is basically including most of the methods.

Anyway, having multiple FallbackFoo.cpp files makes it easier to just implement a chunk in a backend.
Attachment #619057 - Flags: review?(justin.lebar+bug)
I'm ready to r+ this, but what do you think about this solution:

Everybody builds FallbackHal.cpp, which has code like

  #ifndef MOZ_HAS_HAL_POWER
  void FrobPower() { }
  void Reboot() { }
  #endif

  #ifndef MOZ_HAS_HAL_SCREEN
  void FrobScreen() { }
  #endif

This way, you never have to remember to add FallbackFoo.cpp to all the relevant platforms.  It'll just work, and if you mess up the #defines, it will fail to link on *every* platform, not just some of them.
(Assignee)

Comment 2

5 years ago
But that requires to #define MOZ_HAS_HAL_SCREEN somewhere to platforms implementing it so this is only moving the problem somewhere else, right?
Oh, I you're right -- we'd have to define it in a header file somewhere.  :-/  I had this delusion that we could define it in the CPP file.
What about if we had a single HalScreen.cpp file:

HalScreen.cpp:

  #if defined(XP_WIN)
  #include <hal/WindowsScreen.cpp>
  #else
  void FrobScreen();
  #endif

Again, this simplifies the make logic and, more importantly, ensures that every platform always gets an implementation of the full Hal interface.
Er, that should be:

  #if defined(XP_WIN)
  #include <hal/WindowsScreen.cpp>
  #else
  void FrobScreen() {}
  #endif
I have a very strong preference for the solution that Mounir has implemented here.
Attachment #619057 - Flags: review?(justin.lebar+bug) → review?(jones.chris.g)
Comment on attachment 619057 [details] [diff] [review]
Patch v1

Get a try run in, of course.
Attachment #619057 - Flags: review?(jones.chris.g) → review+
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Attachment #619057 - Flags: checkin+

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/cda84ca70452
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.