All the information required to implement the usual device status bar should be exposed. Available cell network, wireless network status, etc.

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!

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.

Renaming this bug Battery API given that the network stuff my go with the Network API in bug 677166.

This patch has some changes and matches what the proposal in the wiki.

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?

(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? :)

With the fixes for Vivien and a few other minor changes.

This new draft patch is now based on bug 690670.

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/

Simple standalone C/C++ file reading sysfs files to get battery information.

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

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

Nice work here Mounir. Did you figure out how to get charge events working?

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

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.

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/

just as a side note, sdwilsh tried to make a battery api time ago in bug 454660, there may be something useful there yet.

IOW, when the battery state changes, the backend have to call NotifyObserver and BatteryManager will fire the correct events.

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.

marking sec-review-needed, sched review for 2011.11.03

(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?

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 on attachment 568363 [details] [diff] [review] Part A - Makefiles Review of attachment 568363 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 568364 [details] [diff] [review] Part B - BatteryManager interface Review of attachment 568364 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 568365 [details] [diff] [review] Part C - Plug mozBattery into .navigator Review of attachment 568365 [details] [diff] [review]: ----------------------------------------------------------------- r=me

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.

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

(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?

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?

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.

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.

It seems that the static IObserverList object in Hal.cpp is seen as a leak when running the test... :(

I forgot a delete in that patch...

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

(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! :)

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

That should be perfectly fine.

(Unless the object has a non-trivial constructor. Let me just review the patches ;)

I've added a GetObserversCount() method which is needed by the part F.

This is fixing the perceived leak.

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

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

(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).

(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 on attachment 570160 [details] [diff] [review] Part G - hal/dom relationship and events calling Review of attachment 570160 [details] [diff] [review]: ----------------------------------------------------------------- r=me

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.

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

OK. We could make a little inner helper object to dance around that.

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

sec-review-complete: notes https://wiki.mozilla.org/Security/Firefox/WebAPI/WebBattery

https://hg.mozilla.org/mozilla-central/rev/bd76c743731c https://hg.mozilla.org/mozilla-central/rev/4ce6025603c7 https://hg.mozilla.org/mozilla-central/rev/09da9af7e934 https://hg.mozilla.org/mozilla-central/rev/5d24f6d3df66 https://hg.mozilla.org/mozilla-central/rev/718e0171d5d2 https://hg.mozilla.org/mozilla-central/rev/6450bedb1f3c https://hg.mozilla.org/mozilla-central/rev/64f25813b372

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.

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

O.K. Thanks for the clarifications!

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

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

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.

(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 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?

Privacy was considered during security review (completed). This feature can be preffed off.

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.