Last Comment Bug 678694 - Battery API
: Battery API
Status: RESOLVED FIXED
[secr:curtisk]
: dev-doc-complete
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
https://wiki.mozilla.org/WebAPI/Batte...
Depends on: 690670 695287 699459 699741 699742 699743 699744 703568 705084 735114 751016 769571 1313580
Blocks: 475505 506975 webapi 506729 696038 696041 696042 696045 696049 714249
  Show dependency treegraph
 
Reported: 2011-08-12 23:36 PDT by Andreas Gal :gal
Modified: 2016-10-27 23:28 PDT (History)
19 users (show)
dveditz: sec‑review+
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Draft patch for a Battery API (48.86 KB, patch)
2011-09-23 01:15 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Draft patch (49.25 KB, patch)
2011-09-26 03:39 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Draft patch (53.67 KB, patch)
2011-09-27 09:47 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Draft patch (51.29 KB, patch)
2011-10-05 05:06 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Standalone sysfs reading POC (4.13 KB, text/plain)
2011-10-06 17:18 PDT, Mounir Lamouri (:mounir)
no flags Details
Standalone uevent POC (3.76 KB, text/x-c++src)
2011-10-06 17:19 PDT, Mounir Lamouri (:mounir)
no flags Details
Standalone uPower POC (3.25 KB, text/x-c++src)
2011-10-11 01:00 PDT, Mounir Lamouri (:mounir)
no flags Details
Part A - Makefiles (6.96 KB, patch)
2011-10-20 05:42 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part B - BatteryManager interface (2.79 KB, patch)
2011-10-20 05:43 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part C - Plug mozBattery into .navigator (9.60 KB, patch)
2011-10-20 05:44 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part D - Dummy DOM code (16.86 KB, patch)
2011-10-20 05:45 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part E - Make BatteryManager observing messages from backends (17.51 KB, patch)
2011-10-20 05:47 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part F - Hal boilerplate code (15.17 KB, patch)
2011-10-20 05:47 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part D - Dummy DOM code (16.90 KB, patch)
2011-10-27 19:17 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part E - Add IObserver and IObserverList (6.16 KB, patch)
2011-10-27 19:24 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part F - Hal code (13.21 KB, patch)
2011-10-27 19:25 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part G - hal/dom relationship and events calling (9.85 KB, patch)
2011-10-27 19:26 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part F - Hal code (13.27 KB, patch)
2011-10-28 05:25 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part E - Add IObserver and IObserverList (6.23 KB, patch)
2011-10-28 12:10 PDT, Mounir Lamouri (:mounir)
cjones.bugs: superreview+
Details | Diff | Splinter Review
Part F - Hal code (13.55 KB, patch)
2011-10-28 12:12 PDT, Mounir Lamouri (:mounir)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2011-08-12 23:36:12 PDT
All the information required to implement the usual device status bar should be exposed. Available cell network, wireless network status, etc.
Comment 1 Robin Berjon 2011-09-15 07:58:20 PDT
FYI a new snapshot of the Battery Status Events has been released:

  http://www.w3.org/TR/2011/WD-battery-status-20110915/

The approach has changed a fair bit based on feedback, and I hope that this would be reasonably stable at this point. Feedback would be very much welcome!
Comment 2 Mounir Lamouri (:mounir) 2011-09-23 01:15:24 PDT
Created attachment 561991 [details] [diff] [review]
Draft patch for a Battery API

Some choices in this implementation are different from the DAP Battery API specifications. I believe we will do some formal propositions later to the appropriate WC/TF.
Comment 3 Mounir Lamouri (:mounir) 2011-09-26 03:38:32 PDT
Renaming this bug Battery API given that the network stuff my go with the Network API in bug 677166.
Comment 4 Mounir Lamouri (:mounir) 2011-09-26 03:39:33 PDT
Created attachment 562395 [details] [diff] [review]
Draft patch

This patch has some changes and matches what the proposal in the wiki.
Comment 5 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-09-27 09:27:27 PDT
Comment on attachment 562395 [details] [diff] [review]
Draft patch

Review of attachment 562395 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/src/webapi/mozBatteryManager.cpp
@@ +94,5 @@
> +    if (AndroidBridge::Bridge()) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    AndroidBridge::Bridge()->RegisterBatteryManager(&mIsPlugged, &mLevel, hasBattery);

There is a typo bug here, you probably want to do the invert, otherwise the battery manager fails to initialize if call from the chrome process.

if (AndroidBridge::Bridge()) {
  AndroidBridge::Bridge()->RegisterBatteryManager(&mIsPlugged, &mLevel, hasBattery);
} else {
  return NS_ERROR_FAILURE;
}

@@ +174,5 @@
> +  if (mLevel <= sCriticalTreshold) {
> +    return BatteryStatus_Critical;
> +  }
> +
> +  // TODO: spces should change for <= !

spces => specs?
Comment 6 Mounir Lamouri (:mounir) 2011-09-27 09:30:55 PDT
(In reply to Vivien Nicolas (:vingtetun) from comment #5)
> Comment on attachment 562395 [details] [diff] [review] [diff] [details] [review]
> Draft patch
> 
> Review of attachment 562395 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/src/webapi/mozBatteryManager.cpp
> @@ +94,5 @@
> > +    if (AndroidBridge::Bridge()) {
> > +      return NS_ERROR_FAILURE;
> > +    }
> > +
> > +    AndroidBridge::Bridge()->RegisterBatteryManager(&mIsPlugged, &mLevel, hasBattery);
> 
> There is a typo bug here, you probably want to do the invert, otherwise the
> battery manager fails to initialize if call from the chrome process.
> 
> if (AndroidBridge::Bridge()) {
>   AndroidBridge::Bridge()->RegisterBatteryManager(&mIsPlugged, &mLevel,
> hasBattery);
> } else {
>   return NS_ERROR_FAILURE;
> }

It's a typo, I should have write "if (!AndroidBridge::Bridge())".
Will fix that.

> @@ +174,5 @@
> > +  if (mLevel <= sCriticalTreshold) {
> > +    return BatteryStatus_Critical;
> > +  }
> > +
> > +  // TODO: spces should change for <= !
> 
> spces => specs?

Seriously? :)
Comment 7 Mounir Lamouri (:mounir) 2011-09-27 09:47:04 PDT
Created attachment 562796 [details] [diff] [review]
Draft patch

With the fixes for Vivien and a few other minor changes.
Comment 8 Mounir Lamouri (:mounir) 2011-10-05 05:06:16 PDT
Created attachment 564798 [details] [diff] [review]
Draft patch

This new draft patch is now based on bug 690670.
Comment 9 Mounir Lamouri (:mounir) 2011-10-06 03:31:55 PDT
For the battery backends, the plan is going to be the following:
- Android: using the Battery API provided by the SDK;
- GNU/Linux: using upower [1] with a fallback to a plain sysfs implementation if upower is disabled on compile time (should we have a run-time fallback? maybe later);
- B2G: for the moment, stick with a sysfs implementation.
- MacOS X and Windows: will come later. The goal is to have more than one backend implementation to make sure the API on top of them are generic enough. Given that I have a Mac, I could try to implement the MacOS X backend. I have no Windows system out of a VM so I will let someone else working on this.

upower is simply a layer on top of sysfs using dbus to inform of battery status (and changes). sysfs implementation will require listening to uevents to update the battery status.

[1] http://upower.freedesktop.org/
Comment 10 Mounir Lamouri (:mounir) 2011-10-06 17:18:48 PDT
Created attachment 565405 [details]
Standalone sysfs reading POC

Simple standalone C/C++ file reading sysfs files to get battery information.
Comment 11 Mounir Lamouri (:mounir) 2011-10-06 17:19:59 PDT
Created attachment 565406 [details]
Standalone uevent POC

Simple standalone C/C++ file using uevents to react to battery changes.

Next step is to hook those two things together in Gecko to have our uevent/sysfs backend implementation
Comment 12 Mounir Lamouri (:mounir) 2011-10-06 17:30:39 PDT
Hmm, looks like this code doesn't react to battery charge changing or events are not sent very often. Will have to verify that tomorrow...
Comment 13 Andreas Gal :gal 2011-10-10 22:19:15 PDT
Nice work here Mounir. Did you figure out how to get charge events working?
Comment 14 Mounir Lamouri (:mounir) 2011-10-11 00:58:33 PDT
(In reply to Andreas Gal :gal from comment #13)
> Nice work here Mounir. Did you figure out how to get charge events working?

Actually, I did some search and it seems that Android is getting some uevents for battery charge changing but it is not expected to happen for all systems. For what I understood, there are two types of uevents here, POWER_SUPPLY_CHANGED when the battery is plugged/unplugged and POWER_SUPPLY_UEVENT when the battery charge changes. It seems that to receive the latter, a loglevel has to be changed, see [1]. I'm not 100% sure this information is correct nor that I understood it correctly but what is certain is that we can't assume that uevents will occur regularly on all systems. Most Linux applications are regularly reading the sysfs (or ACPI...) information, for what I've seen.

I think we should still be able to use this implementation (sysfs/uevent) for B2G. Though, we will not be able to use it on Linux [2]. I did work on a upower implementation (PoC is done, I've to integrate that in Gecko now).

[1] https://groups.google.com/group/android-porting/browse_thread/thread/2d870f671bb68697/3f6a63bddb3bc5e7?#3f6a63bddb3bc5e7
[2] We could read the sysfs information regularly but that would be a bad idea because we would do something that upower is already doing and reduce the battery life...
Comment 15 Mounir Lamouri (:mounir) 2011-10-11 01:00:57 PDT
Created attachment 566142 [details]
Standalone uPower POC

I began putting this code in Gecko but unfortunately, I have to work on a presentation (and a lighting talk) I'm giving at the end of the week. Might make slow me a bit for the next days.
Comment 16 Mounir Lamouri (:mounir) 2011-10-11 16:46:56 PDT
Did a quick search for MacOS API. It seems that we could/should use this:
https://developer.apple.com/library/mac/#documentation/Darwin/Reference/IOKit/IOPowerSources_h/
Comment 17 Marco Bonardo [::mak] 2011-10-19 12:27:44 PDT
just as a side note, sdwilsh tried to make a battery api time ago in bug 454660, there may be something useful there yet.
Comment 18 Mounir Lamouri (:mounir) 2011-10-20 05:42:56 PDT
Created attachment 568363 [details] [diff] [review]
Part A - Makefiles
Comment 19 Mounir Lamouri (:mounir) 2011-10-20 05:43:48 PDT
Created attachment 568364 [details] [diff] [review]
Part B - BatteryManager interface
Comment 20 Mounir Lamouri (:mounir) 2011-10-20 05:44:32 PDT
Created attachment 568365 [details] [diff] [review]
Part C - Plug mozBattery into .navigator
Comment 21 Mounir Lamouri (:mounir) 2011-10-20 05:45:18 PDT
Created attachment 568366 [details] [diff] [review]
Part D - Dummy DOM code
Comment 22 Mounir Lamouri (:mounir) 2011-10-20 05:47:10 PDT
Created attachment 568367 [details] [diff] [review]
Part E - Make BatteryManager observing messages from backends

IOW, when the battery state changes, the backend have to call NotifyObserver and BatteryManager will fire the correct events.
Comment 23 Mounir Lamouri (:mounir) 2011-10-20 05:47:58 PDT
Created attachment 568368 [details] [diff] [review]
Part F - Hal boilerplate code
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-25 18:15:16 PDT
Comment on attachment 568368 [details] [diff] [review]
Part F - Hal boilerplate code

Sorry for the review latency.

There are a couple of things I don't quite understand about this patch

 - how does this implementation work when multiple DOM windows request
   battery notifications?  What happens if window A registers, then
   window B registers, then window A unregisters?  Will B continue to
   get updates?

 - is there any mechanism for turning off battery notifications in
   this patch?  That is, in the example above, if A unregisters, then
   B unregisters, will OS battery updates continue to be listened to
   in chrome and sent to content processes?

I'm not enthusiastic about building these notifications around the
observer service since that gets lots of little XPCOM tentacles into
hal/, but until I understand the above I can't fully evaluate.

>diff --git a/dom/battery/BatteryInformationIPCSerializer.h b/dom/battery/BatteryInformationIPCSerializer.h

>+template<>
>+struct ParamTraits<mozilla::dom::battery::BatteryInformation>
>+{
>+  typedef mozilla::dom::battery::BatteryInformation paramType;
>+
>+  // Serialize.
>+  static void Write(Message *aMsg, const paramType& aParam)
>+  {
>+    WriteParam(aMsg, aParam.mLevel);
>+    WriteParam(aMsg, aParam.mCharging);
>+  }
>+
>+  // Deserialize.
>+  static bool Read(const Message* aMsg, void **aIter, paramType* aResult)
>+  {
>+    return (ReadParam(aMsg, aIter, &aResult->mLevel) &&
>+            ReadParam(aMsg, aIter, &aResult->mCharging));
>+  }
>+};

This code is unnecessary; you can define BatteryInformation as an IPDL
struct in PHal and IPDL will generate optimal
serializers/deserializers for it.  This code is fairly tricky in
general and we've had bugs from incorrect hand-written
serializer/deserializer.

In general, prefer defining these structures in IPDL because (i)
autogen serializer/deserializer per above, lower maintenance burden,
and (ii) IPDL's type system is more powerful than C++'s default
features so you can define things like type-safe (mostly)
discriminated unions.

Clearing request for answers to questions above.
Comment 25 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-10-25 18:39:29 PDT
marking sec-review-needed, sched review for 2011.11.03
Comment 26 Mounir Lamouri (:mounir) 2011-10-26 02:39:29 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> Comment on attachment 568368 [details] [diff] [review] [diff] [details] [review]
> Part F - Hal boilerplate code
> 
> Sorry for the review latency.
> 
> There are a couple of things I don't quite understand about this patch
> 
>  - how does this implementation work when multiple DOM windows request
>    battery notifications?  What happens if window A registers, then
>    window B registers, then window A unregisters?  Will B continue to
>    get updates?

A BatteryManager will get battery updates as soon as there is an access to navigator.mozBattery. At this point, the BatteryManager will register itself to listen to the "battery-change" notifications. It unregisters when the window is closed.
If two windows are opened and one is closed, the other one will still get updates.

>  - is there any mechanism for turning off battery notifications in
>    this patch?  That is, in the example above, if A unregisters, then
>    B unregisters, will OS battery updates continue to be listened to
>    in chrome and sent to content processes?

Yes, the |UnregisterBatteryObserver| is here for that: backends have to keep a counter of observers and are expected to stop sending notifications if there is no observers.

> I'm not enthusiastic about building these notifications around the
> observer service since that gets lots of little XPCOM tentacles into
> hal/, but until I understand the above I can't fully evaluate.

I think the observer service is exactly what we want here: we have one central points that sends notifications and we can have 0 to n observers. We could rebuild a similar mechanism but I don't know how important that would be.
For example, I could keep track of currently living BatteryManager and instead of firing a notification with the observer service, I could call a method that would notify all those living instances.

> In general, prefer defining these structures in IPDL because (i)
> autogen serializer/deserializer per above, lower maintenance burden,
> and (ii) IPDL's type system is more powerful than C++'s default
> features so you can define things like type-safe (mostly)
> discriminated unions.

My draf of Battery API patches was using that but I had to include ContentFoo.h in my dom/ and widget/ code which I thought was a bit ugly. Is there a way I'm missing to prevent this or do you really want me to include HalFoo.h in dom/ and widget/ code?
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-26 04:04:40 PDT
Thanks for the clarifications, I understand better now.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #26)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> > Comment on attachment 568368 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > I'm not enthusiastic about building these notifications around the
> > observer service since that gets lots of little XPCOM tentacles into
> > hal/, but until I understand the above I can't fully evaluate.
> 
> I think the observer service is exactly what we want here: we have one
> central points that sends notifications and we can have 0 to n observers. We
> could rebuild a similar mechanism but I don't know how important that would
> be.
> For example, I could keep track of currently living BatteryManager and
> instead of firing a notification with the observer service, I could call a
> method that would notify all those living instances.
> 

Here are my concerns about using the observer service for this

 - the namespace is global
 - observer service isn't type safe by default, needs effort to get that through XPCOM indirection (which adds code overhead too)
 - other listeners can register to these observer service notifications without necessarily tracking the hal API
 - JS code can listen to these notifications (I can almost guarantee that extensions will do this ...).  That brings on a world of pain from re-entry and perf issues.
 - RegisterBatteryListener/UnregisterBatteryListener don't actually register listeners; they eventually increment a counter in the chrome process backend, and it's up to client code to set up observer-service listeners on the side
 - moves listener accounting into each platform backend, instead of a central platform-neutral place
 - ties all listeners and broadcasted data to XPCOM for no gain AFAICT

There's another subtlety with the RegisterBatteryObserver() API, which is that the first returned value has to be the "last cached" value instead of the current value, or else listeners can have inconsistent results for a long time (until the next broadcasted update).  Right now, maintaining that invariant is distributed to each backend.

Ideally, I'd like a system something like the following

 - the RegisterListener/UnregisterListener API stays the same, but takes an actual IBatteryListener argument or something like that
 - IBatteryListener is an instantiation of an IObserver<T>, where T in this case is BatteryInfo.  IObserver has a |virtual void Notify(T) = 0| method, and something for refcounting.  Best would be RefCounted<T>.
 - there's an IObserverList<T> (or something like that) that tracks IObservers<T>.  It has a Broadcast(T) interface that Notify(T)'s all the registered observers.
 - (the IObserver/IObserverList code is all generic, and lives outside of hal/)
 - there's a central chunk of code (BatteryListenerManager?  better name TODO) responsible for tracking battery-status listeners and the most recent battery state.  That centralizes the code in all the current backend implementations.
 - there's one listener-manager per process.  It's local to Hal.cpp.
 - when the number of listeners goes from 0->1, then the listener manager calls a semi-private hal API like EnableBatteryNotifications() that each backend implements.  This would be a simplified version of the current RegisterBatteryListener() API.
 - when the number of listeners goes from 1->0, then the listener manager calls something like DisableBatteryNotifications().

This also allows us to make the RegisterBatteryObserver() implementation asynchronous for content processes, since BatteryListenerManager (or whatever) always keeps the most up-to-date battery state.

Does all this make sense?  I don't want to overburden the landing of the battery API, but I also want to make sure we build the right foundation for maintainable code in hal/.  If all this makes sense, I'm happy to help build the new helper code above.

> > In general, prefer defining these structures in IPDL because (i)
> > autogen serializer/deserializer per above, lower maintenance burden,
> > and (ii) IPDL's type system is more powerful than C++'s default
> > features so you can define things like type-safe (mostly)
> > discriminated unions.
> 
> My draf of Battery API patches was using that but I had to include
> ContentFoo.h in my dom/ and widget/ code which I thought was a bit ugly. Is
> there a way I'm missing to prevent this or do you really want me to include
> HalFoo.h in dom/ and widget/ code?

Nope --- these declarations should be available transparently through Hal.h, by way of PHal.h.  If PHal.h isn't already being included already through Hal.h we can add it to HalSandbox.h, and it will be picked up Hal.h automagically.
Comment 28 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-26 11:44:00 PDT
Comment on attachment 568363 [details] [diff] [review]
Part A - Makefiles

Review of attachment 568363 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Comment 29 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-26 11:44:28 PDT
Comment on attachment 568364 [details] [diff] [review]
Part B - BatteryManager interface

Review of attachment 568364 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Comment 30 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-26 13:11:06 PDT
Comment on attachment 568365 [details] [diff] [review]
Part C - Plug mozBattery into .navigator

Review of attachment 568365 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Comment 31 Mounir Lamouri (:mounir) 2011-10-27 09:34:56 PDT
Chris, I think with your proposal navigator.mozBattery.level will not return the correct value the first time it is called. If the backend doesn't send events while there is no BatteryManager objects and we do not return the current state while adding an observer, we basically can't know the current state when creating a BatteryManager object.

We could try to keep the API clean by adding a new IPDL method that would get the BatterInfo instead of doing that in a method that isn't really related but (like EnableBatteryNotifications) but I don't think we can prevent the sync method except if we have all battery backends running and notifying all the time but I think that would be worse.
Comment 32 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-27 11:13:02 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #31)
> Chris, I think with your proposal navigator.mozBattery.level will not return
> the correct value the first time it is called. If the backend doesn't send
> events while there is no BatteryManager objects and we do not return the
> current state while adding an observer, we basically can't know the current
> state when creating a BatteryManager object.
> 
> We could try to keep the API clean by adding a new IPDL method that would
> get the BatterInfo instead of doing that in a method that isn't really
> related but (like EnableBatteryNotifications) but I don't think we can
> prevent the sync method except if we have all battery backends running and
> notifying all the time but I think that would be worse.

Yes, that's correct: the first battery listener would get the kDefault* values.  If that's a problem for the API design, then yes, we can add the sync fetcher on the 0->1 listener transitions.
Comment 33 Mounir Lamouri (:mounir) 2011-10-27 11:24:06 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #27)
>  - there's a central chunk of code (BatteryListenerManager?  better name
> TODO) responsible for tracking battery-status listeners and the most recent
> battery state.  That centralizes the code in all the current backend
> implementations.
>  - there's one listener-manager per process.  It's local to Hal.cpp.
>  - when the number of listeners goes from 0->1, then the listener manager
> calls a semi-private hal API like EnableBatteryNotifications() that each
> backend implements.  This would be a simplified version of the current
> RegisterBatteryListener() API.
>  - when the number of listeners goes from 1->0, then the listener manager
> calls something like DisableBatteryNotifications().

I'm not sure I understand this correctly but for what I understand, there is something wrong... If we have one object per process tracking the IBatteryObserver's and calling EnableBatteryNotifications() and DisableBatteryNotifications() we will have unexpected behavior as soon as we have two processes (which we have with a content process and chrome process). Let say we have an observer registering to the chrome process, it will enable the notifications. Then one appears in the content process: the content process will try to enable the battery notifications too. Then, this same observer unregister: the content process will ask for stopping notifications and the chrome process' observer will not get notified anymore.
So should we add another check in the parent process or did I miss something?
Comment 34 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-27 11:35:17 PDT
When the content process listeners go from 1->0, then Hal.cpp would issue a DisableBatterNotifications() call implemented by SandboxHal.cpp.  That would send a message to chrome that resulted in an IPDL actor-listener being unregistered from the battery listener list in chrome.  Since there was already another listener in chrome, then the listeners would go 2->1 and there wouldn't be a DisableBatteryNotifications() issued in the chrome process.  Make sense?
Comment 35 Mounir Lamouri (:mounir) 2011-10-27 19:17:45 PDT
Created attachment 570157 [details] [diff] [review]
Part D - Dummy DOM code

Small update of the patch:
- renamed BatteryConstants.h Constants.h so it's nicer to include.
- add something that was in a following patch to prevent an assert.
Comment 36 Mounir Lamouri (:mounir) 2011-10-27 19:24:12 PDT
Created attachment 570158 [details] [diff] [review]
Part E - Add IObserver and IObserverList

I'm not sure that ObserverList should be prefixed by 'I' but I guess it depends what 'I' means here.

In IObserverList, the methods are not virtual because it wasn't required for the moment and it could easily be prevented. It could easily be changed later too...
I didn't add the refcounting because I think it makes thing worse in some situations (for example, BatteryManager is a nsISupports, should it be RefCounted in addition?). And, it happens to me and to other people (jlebar for example) to remove an observer from the observer service in the destructor, assuming the observer service wasn't taking a strong ref. I think we should at least consider asking callers to make sure the observer doesn't get freed until RemoveObserver hasn't been called.
Comment 37 Mounir Lamouri (:mounir) 2011-10-27 19:25:08 PDT
Created attachment 570159 [details] [diff] [review]
Part F - Hal code
Comment 38 Mounir Lamouri (:mounir) 2011-10-27 19:26:18 PDT
Created attachment 570160 [details] [diff] [review]
Part G - hal/dom relationship and events calling
Comment 39 Mounir Lamouri (:mounir) 2011-10-27 19:29:39 PDT
It seems that the static IObserverList object in Hal.cpp is seen as a leak when running the test... :(
Comment 40 Mounir Lamouri (:mounir) 2011-10-28 05:25:11 PDT
Created attachment 570225 [details] [diff] [review]
Part F - Hal code

I forgot a delete in that patch...
Comment 41 Mounir Lamouri (:mounir) 2011-10-28 11:43:24 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #39)
> It seems that the static IObserverList object in Hal.cpp is seen as a leak
> when running the test... :(

So the only way to fix that would be to have a static pointer instead of a static object. Then, we would be able to call delete before the leak analysis tool runs.
Comment 42 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-28 11:48:25 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #41)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #39)
> > It seems that the static IObserverList object in Hal.cpp is seen as a leak
> > when running the test... :(
> 
> So the only way to fix that would be to have a static pointer instead of a
> static object. Then, we would be able to call delete before the leak
> analysis tool runs.

Yes, let's just delete the observer list when the last observer is unregistered.

I'll get to these reviews as soon as I can today.  Thanks for persevering! :)
Comment 43 Mounir Lamouri (:mounir) 2011-10-28 11:52:00 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #42)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #41)
> > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #39)
> > > It seems that the static IObserverList object in Hal.cpp is seen as a leak
> > > when running the test... :(
> > 
> > So the only way to fix that would be to have a static pointer instead of a
> > static object. Then, we would be able to call delete before the leak
> > analysis tool runs.
> 
> Yes, let's just delete the observer list when the last observer is
> unregistered.

This is exactly what I'm trying (compiling right now). But we will still have an object in memory (with two null pointers). I just hope the leak analysis tool will not catch it.
Comment 44 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-28 12:00:25 PDT
That should be perfectly fine.
Comment 45 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-28 12:01:26 PDT
(Unless the object has a non-trivial constructor.  Let me just review the patches ;)
Comment 46 Mounir Lamouri (:mounir) 2011-10-28 12:10:49 PDT
Created attachment 570325 [details] [diff] [review]
Part E - Add IObserver and IObserverList

I've added a GetObserversCount() method which is needed by the part F.
Comment 47 Mounir Lamouri (:mounir) 2011-10-28 12:12:21 PDT
Created attachment 570326 [details] [diff] [review]
Part F - Hal code

This is fixing the perceived leak.
Comment 48 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-28 14:41:25 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #35)
> Created attachment 570157 [details] [diff] [review] [diff] [details] [review]
> Part D - Dummy DOM code
> 
> Small update of the patch:
> - renamed BatteryConstants.h Constants.h so it's nicer to include.

Is there a reason to keep these outside of hal?  It seems to make sense for hal to own these constants, like it owns BatteryInfo.  They can go into Hal.h, and save the boilerplate of a new file.
Comment 49 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-28 14:54:59 PDT
Comment on attachment 570325 [details] [diff] [review]
Part E - Add IObserver and IObserverList

I don't much care about IObserver vs. Observer.

>diff --git a/xpcom/glue/IObserver.h b/xpcom/glue/IObserver.h

>+template <class T>
>+class IObserver
>+{
>+public:

IObserver needs a virtual dtor

  virtual ~IObserver() { }

>+  virtual void Notify(T aParam) = 0;

  const T& aParam

>diff --git a/xpcom/glue/IObserverList.h b/xpcom/glue/IObserverList.h

I don't think it makes sense to have these in separate files.  Let's
move the IObserverList code into IObserver.h.

>+namespace mozilla {
>+
>+/**
>+ * IObserverList<T> tracks IObserver<T> and can notify them when Broadcast() is
>+ * called.
>+ * T represents the type of the object passed in argument to Broadcast() and
>+ * sent to IObserver<T> objects through Notify().
>+ *
>+ * @see IObserver.
>+ */
>+template <class T>
>+class IObserverList
>+{
>+public:
>+  /**
>+   * Note: When calling AddObserver, it's up to the caller to make sure the
>+   * object isn't going to be release as long as RemoveObserver hasn't been
>+   * called.
>+   *

This makes me pretty nervous.  I'm OK with landing this for a v0, but
I think we'll want IObserverList to hold a strong ref.

Did you have trouble making RefCounted<T> work with the DOM code?

>+  PRUint32 GetObserversCount() {

PRUint32 Length()

>+  void Broadcast(T aParam) {

const T&

sr=me with those changes.  You're not technically allowed to do r+sr anymore, but sr=me is enough.
Comment 50 Mounir Lamouri (:mounir) 2011-10-28 16:00:45 PDT
BatteryManager.cpp doesn't compile on Win64 Opt:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7087904&tree=Try#error0

Does that speak to anyone?
Comment 51 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-28 16:25:08 PDT
Comment on attachment 570326 [details] [diff] [review]
Part F - Hal code

>diff --git a/dom/battery/Types.h b/dom/battery/Types.h

My taste would be to define these in maybe BatteryManager.h, but
that's not my module so doesn't really matter.

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp
 
>+private:
>+  IObserverList<BatteryObserverParam>* mObservers;

This can be an inline member.

>+  BatteryInformation*                  mBatteryInfo;

This can be an inline member alongside a |bool mHaveBatteryInfo|
field, no need to heap allocate it.

>+};
>+
>+static BatteryObserversManager sBatteryObservers;
>+

This can be an nsAutoPtr<>.  You can destroy sBatteryObservers when
the last observer goes away.  That should fix the leak you were seeing
and is slightly simpler.

>diff --git a/hal/Hal.h b/hal/Hal.h

>+namespace hal {
>+  class BatteryInformation;

Nit: no indent here.

>diff --git a/hal/fallback/FallbackHal.cpp b/hal/fallback/FallbackHal.cpp

> #include "Hal.h"
>+#include <mozilla/dom/battery/Constants.h>
> 

I would prefer to have these constants defined in Hal.h.

r=me with those fixes.
Comment 52 Mounir Lamouri (:mounir) 2011-10-31 08:17:36 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #48)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #35)
> > Created attachment 570157 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Part D - Dummy DOM code
> > 
> > Small update of the patch:
> > - renamed BatteryConstants.h Constants.h so it's nicer to include.
> 
> Is there a reason to keep these outside of hal?  It seems to make sense for
> hal to own these constants, like it owns BatteryInfo.  They can go into
> Hal.h, and save the boilerplate of a new file.

Those constants are required per specs. I would prefer to have them owned by the DOM code.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #49)
> Comment on attachment 570325 [details] [diff] [review] [diff] [details] [review]
> Part E - Add IObserver and IObserverList
> 
> I don't much care about IObserver vs. Observer.

Would you be okay with IObserver and ObserverList? I don't like having "I" prefixing a non virtual class.
Otherwise, I will go with Observer and ObserverList.

> >+public:
> >+  /**
> >+   * Note: When calling AddObserver, it's up to the caller to make sure the
> >+   * object isn't going to be release as long as RemoveObserver hasn't been
> >+   * called.
> >+   *
> 
> This makes me pretty nervous.  I'm OK with landing this for a v0, but
> I think we'll want IObserverList to hold a strong ref.
> 
> Did you have trouble making RefCounted<T> work with the DOM code?

Didn't try but it seems odd to use RefCounted on an already refcounted object. And I really think there are some use cases where having a weak ref would be better (it would prevent creating a Shutdown method just to call |RemoveObserver| for example).
Comment 53 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-31 15:45:32 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #52)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #48)
>
> > I don't much care about IObserver vs. Observer.
> 
> Would you be okay with IObserver and ObserverList? I don't like having "I"
> prefixing a non virtual class.
> Otherwise, I will go with Observer and ObserverList.
> 

Any combination is fine with me.

> > >+public:
> > >+  /**
> > >+   * Note: When calling AddObserver, it's up to the caller to make sure the
> > >+   * object isn't going to be release as long as RemoveObserver hasn't been
> > >+   * called.
> > >+   *
> > 
> > This makes me pretty nervous.  I'm OK with landing this for a v0, but
> > I think we'll want IObserverList to hold a strong ref.
> > 
> > Did you have trouble making RefCounted<T> work with the DOM code?
> 
> Didn't try but it seems odd to use RefCounted on an already refcounted
> object. And I really think there are some use cases where having a weak ref
> would be better (it would prevent creating a Shutdown method just to call
> |RemoveObserver| for example).

Oh, is your BatteryManager an XPCOM class?  Does it need to be?  If so, then yeah we'll need kind of refcount traits to handle both.  For weak vs. strong refs, you're right that we'd want to support weak refs, but we don't need them for the battery API.  That could be added as a followup.  The code right now is unsafe-by-default, which is bad and makes me nervous.
Comment 54 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-31 16:44:30 PDT
Comment on attachment 570160 [details] [diff] [review]
Part G - hal/dom relationship and events calling

Review of attachment 570160 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Comment 55 Mounir Lamouri (:mounir) 2011-11-01 04:56:29 PDT
For those interested in testing, you can get a Linux or an Android build from here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mlamouri@mozilla.com-ea3868f72e03/

If you try a Linux, make sure to have upower service installed and running.
Comment 56 Mounir Lamouri (:mounir) 2011-11-01 05:00:17 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #53)
> Oh, is your BatteryManager an XPCOM class?  Does it need to be?  If so, then
> yeah we'll need kind of refcount traits to handle both.

DOM classes have to be XPCOM classes.
Comment 57 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-01 06:56:57 PDT
OK.  We could make a little inner helper object to dance around that.
Comment 58 Mounir Lamouri (:mounir) 2011-11-02 06:29:32 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #51)
> Comment on attachment 570326 [details] [diff] [review] [diff] [details] [review]
> Part F - Hal code
> 
> >diff --git a/dom/battery/Types.h b/dom/battery/Types.h
> 
> My taste would be to define these in maybe BatteryManager.h, but
> that's not my module so doesn't really matter.

I would prefer to keep Types.h so I will not have to export BatteryManager.h. I prefer to not make it public.

> >diff --git a/hal/Hal.cpp b/hal/Hal.cpp
>  
> >+private:
> >+  IObserverList<BatteryObserverParam>* mObservers;
> 
> This can be an inline member.
> 
> >+  BatteryInformation*                  mBatteryInfo;
> 
> This can be an inline member alongside a |bool mHaveBatteryInfo|
> field, no need to heap allocate it.
> 
> >+};
> >+
> >+static BatteryObserversManager sBatteryObservers;
> >+
> 
> This can be an nsAutoPtr<>.  You can destroy sBatteryObservers when
> the last observer goes away.  That should fix the leak you were seeing
> and is slightly simpler.

As discussed on IRC, we are going to keep the current scheme. I prefer to keep move the logic inside Register/Unregister methods.
Comment 59 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-11-03 10:22:01 PDT
sec-review-complete: notes https://wiki.mozilla.org/Security/Firefox/WebAPI/WebBattery
Comment 61 John Hammink 2011-11-11 18:56:38 PST
Regarding the JS implementation in Fennec Native 11, I had a few open questions regarding battery API and how to use it in JS.

1. Does the battery need to be explicitly coupled (in JS code) to get real values for .chargingTime or .dischargingTime other than Infinity?

2. Does the battery need to be explitly enabled (e.g. 99459 - Add a pref to disable Battery API ).  If so, how is this done?
  
3. ....and can the battery be explicitly disabled (e.g. 699459 - Add a pref to disable Battery API ), and if so, what's the code for doing it?  It seems there was a patch added to this bug.
Comment 62 Mounir Lamouri (:mounir) 2011-11-12 01:05:58 PST
(In reply to John Hammink from comment #61)
> Regarding the JS implementation in Fennec Native 11, I had a few open
> questions regarding battery API and how to use it in JS.
> 
> 1. Does the battery need to be explicitly coupled (in JS code) to get real
> values for .chargingTime or .dischargingTime other than Infinity?

I don't know what you mean by 'coupling' but please, have a look at my comment in bug 699743.

> 2. Does the battery need to be explitly enabled (e.g. 99459 - Add a pref to
> disable Battery API ).  If so, how is this done?

No, Battery API is enabled by default.

> 3. ....and can the battery be explicitly disabled (e.g. 699459 - Add a pref
> to disable Battery API ), and if so, what's the code for doing it?  It seems
> there was a patch added to this bug.

Yes, in about:config, you can disable Battery API by setting "dom.battery.enabled" to false. You need to restart your browser after setting this pref. After doing so, navigator.mozBattery is going to return undefined, as if you were using a browser without this feature.
Comment 63 John Hammink 2011-11-12 01:27:43 PST
O.K. Thanks for the clarifications!
Comment 64 John Hammink 2011-11-15 16:56:03 PST
Actual "litmus" cases (manual testcases with pointers to hosted testpages) for all api functionality are now here:
https://caseconductor.allizom.org/manage/testcases/?pagenumber=1&pagesize=20&filter-product=2
Comment 65 Mounir Lamouri (:mounir) 2011-11-18 08:11:16 PST
(In reply to John Hammink from comment #64)
> Actual "litmus" cases (manual testcases with pointers to hosted testpages)
> for all api functionality are now here:
> https://caseconductor.allizom.org/manage/testcases/
> ?pagenumber=1&pagesize=20&filter-product=2

I see Firefox Desktop isn't listed. Why? Battery API is working on Linux (if upower daemon is installed) and there is some work happening to get it working on Mac and Windows.
Comment 66 John Hammink 2011-11-18 12:40:30 PST
Noted:  I'm currently updating my test plan with a coverage matrix for each of the landed APIs- in the meantime I'll add these environements to the CC cases.
Comment 67 Mounir Lamouri (:mounir) 2011-11-18 15:38:32 PST
(In reply to John Hammink from comment #66)
> Noted:  I'm currently updating my test plan with a coverage matrix for each
> of the landed APIs- in the meantime I'll add these environements to the CC
> cases.

Awesome! Thanks :)
Comment 68 neil@parkwaycc.co.uk 2011-12-05 03:43:18 PST
Comment on attachment 570160 [details] [diff] [review]
Part G - hal/dom relationship and events calling

>+  hal::BatteryInformation* batteryInfo = new hal::BatteryInformation();
>+  hal::GetCurrentBatteryInformation(batteryInfo);
>+
>+  UpdateFromBatteryInfo(*batteryInfo);
>+
>+  delete batteryInfo;
Was it really necessary to stack-allocate this?
Comment 69 Sid Stamm [:geekboy or :sstamm] 2011-12-30 12:56:11 PST
Privacy was considered during security review (completed).  This feature can be preffed off.
Comment 70 Florian Scholz [:fscholz] (MDN) 2012-03-24 11:53:32 PDT
Listed on:

https://developer.mozilla.org/en/DOM/window.navigator

Documented:

https://developer.mozilla.org/en/DOM/window.navigator.mozBattery

Mentioned on Firefox 10 and 11 for developers.

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