Last Comment Bug 749645 - Use fallback files in hal/ when a backend isn't implemented instead of having randomly defined methods
: Use fallback files in hal/ when a backend isn't implemented instead of having...
Product: Core
Classification: Components
Component: Hardware Abstraction Layer (HAL) (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mounir Lamouri (:mounir)
Depends on:
  Show dependency treegraph
Reported: 2012-04-27 08:48 PDT by Mounir Lamouri (:mounir)
Modified: 2012-05-04 03:38 PDT (History)
2 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (16.51 KB, patch)
2012-04-27 08:48 PDT, Mounir Lamouri (:mounir)
cjones.bugs: review+
mounir: checkin+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-04-27 08:48:17 PDT
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.
Comment 1 Justin Lebar (not reading bugmail) 2012-04-27 10:43:19 PDT
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.
Comment 2 Mounir Lamouri (:mounir) 2012-04-27 10:52:31 PDT
But that requires to #define MOZ_HAS_HAL_SCREEN somewhere to platforms implementing it so this is only moving the problem somewhere else, right?
Comment 3 Justin Lebar (not reading bugmail) 2012-04-27 11:04:37 PDT
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.
Comment 4 Justin Lebar (not reading bugmail) 2012-04-27 14:29:21 PDT
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.
Comment 5 Justin Lebar (not reading bugmail) 2012-04-27 14:29:44 PDT
Er, that should be:

  #if defined(XP_WIN)
  #include <hal/WindowsScreen.cpp>
  void FrobScreen() {}
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-27 23:21:51 PDT
I have a very strong preference for the solution that Mounir has implemented here.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-30 23:09:03 PDT
Comment on attachment 619057 [details] [diff] [review]
Patch v1

Get a try run in, of course.
Comment 8 Ed Morley [:emorley] 2012-05-04 03:38:04 PDT

Note You need to log in before you can comment on or make changes to this bug.