709584 - Make Hal observer management a bit more generic

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

IOW, this is using templates and a bit of virtualizations.

The idea is to be able to add new observing mechanisms in Hal without copy-pasting all the boiler plate. This is going to be used by bug 677166.

This patch is templatizing the three methods that should clearly be used by the dom/ to communicate with hal/: RegisterObserver, UnregisterObserver and GetCurrentInformation. NotifyChange is also templatized but it should only be used to communicate from the backends to hal/ [1]. All methods that have to be re-implemented by the backends are not templatized: EnableBatteryInformations and DisableBatteryInformations. In addition, because GetCurrentInformation need to do an IPC call, I've added a new GetCurrentBatteryInformation method [2].

This patch didn't reduce the boilerplate in sandbox/ but doing that would require using an enum to know which kind of information we are looking for and I don't really like the idea. Though, if someone wants to do that, that could be done in a follow-up I believe; it shouldn't enter in conflict with that work.

[1] I would be fine to have this un-templatized but it seem nicer to me to keep it that way.
[2] Actually, it's not new, but GetCurrentInformation is new...

Comment 1

[Mounir Lamouri (:mounir)]

Comment on attachment 580742 [details] [diff] [review]
Patch v1

Per discussion on IRC, Chris is ok with Justin doing this review. Still asking a feedback from Chris to make sure he agrees with the general idea of the patch.

Comment 2

[Justin Lebar (not reading bugmail)]

> This patch is templatizing the three methods that should clearly be used by the dom/ to communicate 
> with hal/: RegisterObserver, UnregisterObserver and GetCurrentInformation.

When you call RegisterObserver, UnregisterObserver, and GetCurrentInformation, you (very reasonably) pass the template parameter explicitly.

But I think

  RegisterObserver<hal::BatteryInformation>()

is less clear than

  RegisterBatteryObserver()

as a public API.  The fact that we use templates should be an implementation detail.

With a bit of hackery in the header, you could make

  {Register,Unregister,GetCurrent}Battery{Observer,Observer,Information}()

functions on the hal:: namespace only, and continue to declare the templated methods in all the hal namespaces (but not define them in all namespaces).  Then the {Register,Unregister,GetCurrent}Battery functions forward to the templated functions.

I'd feel a lot better about the API if we did it this way.

If you really don't want the header to contain these three forwarding functions (six with the network observer), you could define a macro which defines the functions.

Also, can we rename hal::{RegisterObserver,UnregisterObserver,GetCurrentInformation} so that the word "device" is in there?  RegisterDeviceObserver, UnregisterDeviceObserver, GetCurrentDeviceInfo, maybe?  And similarly s/ObserversManager/DeviceObserversManager.

Sorry to bikeshed on names.  The general approach seems fine; the template goop is well-contained.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 580742 [details] [diff] [review]
Patch v1

I agree with jlebar on both points.  Except, don't use macros to declare the forwarding functions in Hal.h; give them real C++ decls with good comments, and hide all the template-y/macro-y stuff in the .cpp.

Would like to see v2.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #2)
> Also, can we rename
> hal::{RegisterObserver,UnregisterObserver,GetCurrentInformation} so that the
> word "device" is in there?  RegisterDeviceObserver,
> UnregisterDeviceObserver, GetCurrentDeviceInfo, maybe?  And similarly
> s/ObserversManager/DeviceObserversManager.
> 
> Sorry to bikeshed on names.  The general approach seems fine; the template
> goop is well-contained.

I don't think the renaming is useful: hal is about abstraction the device interaction so it's not going to observe anything else than device stuff...

Comment 5

[Justin Lebar (not reading bugmail)]

I'm happy to entertain alternatives to "device".  But having a bare function called RegisterObserver or GetCurrentInformation is just confusing.  For example, it's not immediately clear that RegisterObserver(); calls a function in hal -- it could be in mozilla:: or in the empty namespace.

Also, these are not the only kind of observer one might conceivably have in hal, so it's misleading not to have some modifier like "device".

Comment 6

[Mounir Lamouri (:mounir)]

Actually, if I keep the RegisterBatteryObserver et al. methods, I can also remove the templatized methods. I mean, there is no reason to keep them. I could just use the correct observer manager from the definitions and call the manager's method.

Comment 7

[Justin Lebar (not reading bugmail)]

But then would you need to define RegisterBatteryObserver in both hal:: and hal_sandbox::?  I thought the point of the templates was to simplify this.

If it's just a one-liner, then sure, that sounds great.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

AFAIUI, you guys are looking for something like that?

Comment 9

[Justin Lebar (not reading bugmail)]

r=me on the design; if you're happy with it, I quite like it.

But r- because I'd like to take a look at it again after we work through other changes.

>-class BatteryObserversManager
>+template <class InfoType>
>+class ObserversManager
> {
> public:
>-  void AddObserver(BatteryObserver* aObserver) {
>+  void AddObserver(Observer<InfoType>* aObserver) {
>     if (!mObservers) {
>-      mObservers = new ObserverList<BatteryInformation>();
>+      mObservers = new ObserverList<InfoType>();
>     }
> 
>     mObservers->AddObserver(aObserver);
> 
>     if (mObservers->Length() == 1) {
>-      PROXY_IF_SANDBOXED(EnableBatteryNotifications());
>+      EnableNotifications();
>     }
>   }
> 
>-  void RemoveObserver(BatteryObserver* aObserver) {
>+  void RemoveObserver(Observer<InfoType>* aObserver) {
>     mObservers->RemoveObserver(aObserver);

We'll crash if RemoveObserver is called before AddObserver, right?

Maybe you should just make mObservers stored inline.

>-  void CacheBatteryInformation(const BatteryInformation& aBatteryInfo) {
>-    if (mBatteryInfo) {
>-      delete mBatteryInfo;
>+  const InfoType& GetCurrentInformation() {
>+    if (mInfo) {
>+      return *mInfo;
>     }
>-    mBatteryInfo = new BatteryInformation(aBatteryInfo);
>+
>+    mInfo = new InfoType();
>+    GetCurrentInformationInternal(mInfo);
>+    return *mInfo;
>   }

I don't like returning InfoType& here, because its lifetime is not clear.  (It
lives until we get a new notification.)  So someone could easily hold onto this
reference for too long. 

How about we return InfoType here, or, if that's too big to copy, we properly
refcount InfoType?

> 
>-  bool HasCachedBatteryInformation() const {
>-    return mBatteryInfo;
>+  void CacheInformation(const InfoType& aInfo) {
>+    if (mInfo) {
>+      delete mInfo;
>+    }

You can safely delete a null pointer, btw.

>+protected:
>+  virtual void EnableNotifications() = 0;
>+  virtual void DisableNotifications() = 0;
>+  virtual void GetCurrentInformationInternal(InfoType*) = 0;
>+
>+private:
>+  ObserverList<InfoType>* mObservers;
>+  InfoType*               mInfo;

Seems like you want both mInfo and mObservers to be inline.  You copy aInfo during
CacheInformation anyway, so it's not that big, right?  But if it is big, we
should refcount it.

>-GetCurrentBatteryInformation(BatteryInformation* aBatteryInfo)
>+GetCurrentBatteryInformation(BatteryInformation* aInfo)
> {
>   AssertMainThread();
>-
>-  if (sBatteryObservers.HasCachedBatteryInformation()) {
>-    sBatteryObservers.GetCachedBatteryInformation(aBatteryInfo);
>-  } else {
>-    PROXY_IF_SANDBOXED(GetCurrentBatteryInformation(aBatteryInfo));
>-    sBatteryObservers.CacheBatteryInformation(*aBatteryInfo);
>-  }
>+  *aInfo = sBatteryObservers.GetCurrentInformation();
> }

It's kind of weird to ask the "battery observers" for the current battery
information, but I guess this is OK.

>-void NotifyBatteryChange(const BatteryInformation& aBatteryInfo)
>+void
>+NotifyBatteryChange(const BatteryInformation& aInfo)
> {
>-  AssertMainThread();
>-
>-  sBatteryObservers.CacheBatteryInformation(aBatteryInfo);
>-  sBatteryObservers.Broadcast(aBatteryInfo);
>+  sBatteryObservers.CacheInformation(aInfo);
>+  sBatteryObservers.Broadcast(aInfo);
> }

Seems like Broadcast() should not take any arguments and just broadcast the cached information.  Is that reasonable?

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #9)
> >-GetCurrentBatteryInformation(BatteryInformation* aBatteryInfo)
> >+GetCurrentBatteryInformation(BatteryInformation* aInfo)
> > {
> >   AssertMainThread();
> >-
> >-  if (sBatteryObservers.HasCachedBatteryInformation()) {
> >-    sBatteryObservers.GetCachedBatteryInformation(aBatteryInfo);
> >-  } else {
> >-    PROXY_IF_SANDBOXED(GetCurrentBatteryInformation(aBatteryInfo));
> >-    sBatteryObservers.CacheBatteryInformation(*aBatteryInfo);
> >-  }
> >+  *aInfo = sBatteryObservers.GetCurrentInformation();
> > }
> 
> It's kind of weird to ask the "battery observers" for the current battery
> information, but I guess this is OK.

I agree but having a method doing that was required. We could bikeshed on the naming though.

> >-void NotifyBatteryChange(const BatteryInformation& aBatteryInfo)
> >+void
> >+NotifyBatteryChange(const BatteryInformation& aInfo)
> > {
> >-  AssertMainThread();
> >-
> >-  sBatteryObservers.CacheBatteryInformation(aBatteryInfo);
> >-  sBatteryObservers.Broadcast(aBatteryInfo);
> >+  sBatteryObservers.CacheInformation(aInfo);
> >+  sBatteryObservers.Broadcast(aInfo);
> > }
> 
> Seems like Broadcast() should not take any arguments and just broadcast the
> cached information.  Is that reasonable?

I prefer to pass the information that needs to be broadcasted even if in the code we only broadcast the cached value. If we rename the method BroadcastCachedInformation() I will be okay with passing no argument ;)

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

Comment 12

[Justin Lebar (not reading bugmail)]

> +  void GetCurrentInformation(InfoType* aInfo) {

Why not just return InfoType?  Bona fide return values are better than out params.

> Seems like you want both mInfo and mObservers to be inline.

What about this change?  At the very least, they should be AutoPtrs, but I don't understand why either member should be heap-allocated.  (Not only is it cleaner, but avoiding heap allocations is virtuous in general, since they fragment the heap.)

> If we rename the method BroadcastCachedInformation() I will be okay with passing no argument ;)

Fine with me!

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.2

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 581879 [details] [diff] [review]
Patch v2.2

Thanks.

Comment 15

[Mounir Lamouri (:mounir)]

Comment on attachment 581879 [details] [diff] [review]
Patch v2.2

I had to make mObservers a pointer in the patch I checked in because mObservers has a nsTArray which is saw as leaking if not destroyed before shutdown. Using ClearOnShutdown() just for that seems a bit too much unless the heap fragmentation is a very big deal here?
Justin, feel free to open a follow-up.

Comment 16

[Justin Lebar (not reading bugmail)]

Why didn't you make it an nsAutoPtr?

Also, it seems like you're relying on every consumer calling RemoveObserver upon shutdown.  Is that right?  Maybe ObserversManager should register a shutdown listener and destroy itself upon xpcom shutdown; otherwise, you're just pushing the work out to consumers.

Comment 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/08543c8312d6