Closed Bug 749645 Opened 11 years ago Closed 11 years ago

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


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

Not set





(Reporter: mounir, Assigned: mounir)



(1 file)

Attached patch Patch v1Splinter Review
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

  void FrobPower() { }
  void Reboot() { }

  void FrobScreen() { }

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.
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:


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

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>
  void FrobScreen() {}
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+
Flags: in-testsuite-
Target Milestone: --- → mozilla15
Attachment #619057 - Flags: checkin+
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.