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...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Hardware Abstraction Layer (HAL) (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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

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

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.
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>
  #else
  void FrobScreen() {}
  #endif
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
https://hg.mozilla.org/mozilla-central/rev/cda84ca70452

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