696045 - Battery API: MacOS X backend

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Comment 1

[Mounir Lamouri (:mounir)]

Maybe we should use this API:
https://developer.apple.com/library/mac/#documentation/Darwin/Reference/IOKit/IOPowerSources_h/

Also, bug 454660 has a draft patch of something quite similar: some kind of system information (including battery info) service with a MacOS X backend.

Comment 2

[Håkan Waara]

I've started working on this.

Comment 3

[Mounir Lamouri (:mounir)]

Feel free to attach a work in progress patch.

Comment 4

[Håkan Waara]

Attachment: Implementation v1

This patch successfully makes http://oldworld.fr/mozilla/battery.html detect whether we are plugged in. I'm not sure if that page should reflect battery level too? 

http://oldworld.fr/mozilla/battery-test.html successfully detects the battery level (0.0-1.0) so I think the former page is broken in that case.

The remaining time is specified in seconds, is that correct?

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 574919 [details] [diff] [review]
Implementation v1

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

In general, this patch looks very good. Most of my comments are nits or very easy to fix (like the AC checking).
However, I'm afraid the general behavior might not match what we are expecting: when BeginListening is called we should immediately get some valid values. In other words, GetCurrentBatteryInformation should return valid battery information. I've the feeling that your patch waits for the first update which is unlikely to happen between BeginListening() and GetCurrentBatteryInformation.
If there is some black magic (like registering for the event immediately calls the callback), please let me know and add a comment in the code. Otherwise, is there a way to get those information synchronously that we could use when BeginListening is called?

And I think it would be better to have MacHal.cpp implementing the basic/dummy hal stuff (like Vibrate) and have something like MacPower.cpp or MacBattery.cpp (or whatever name you want) for Battery API backend.

::: hal/mac/MacHal.cpp
@@ +47,5 @@
> +/**
> + * Used to make sure we always release CFTypeRef.
> + */
> +template < >
> +class nsAutoRefTraits<CFTypeRef>

Why not inheriting from nsPointerRefTraits<CFTypeRef>?
It would prevent typedef'ing RawRef and defining Void().

@@ +54,5 @@
> +  typedef CFTypeRef RawRef;
> +  static void Release(CFTypeRef ref) { ::CFRelease(ref); }
> +  static CFTypeRef Void() { return NULL; }
> +};
> +typedef nsAutoRef<CFTypeRef> CFTypeAutoRelease;

Do we really need this typedef?

@@ +60,5 @@
> +/*
> + * Used to make sure we always release CFArrayRef.
> + */
> +template < >
> +class nsAutoRefTraits<CFArrayRef>

Same as above.

@@ +67,5 @@
> +  typedef CFArrayRef RawRef;
> +  static void Release(CFArrayRef ref) { ::CFRelease(ref); }
> +  static CFArrayRef Void() { return NULL; }
> +};
> +typedef nsAutoRef<CFArrayRef> CFArrayAutoRelease;

Same as above.

@@ +76,5 @@
> +namespace mozilla {
> +namespace hal_impl {
> +
> +
> +/* no implementations of vibration on mac */

Better to have comments with uppercase at the beginning and period at the end ;)

@@ +92,5 @@
> +  void StopListening();
> +
> +  double GetLevel();
> +  bool   IsCharging();
> +  double GetRemainingTime();

I think you can remove those methods given that they are not called from outside and only return a member variable (I should do the same in UPowerClient).

@@ +101,5 @@
> +
> +private:
> +  MacPowerInformationService(); 
> +
> +  // the reference to the runloop that is notified of power changes

// The reference [...] changes.

@@ +198,5 @@
> +    static_cast<MacPowerInformationService *>(aContext);
> +
> +  CFTypeAutoRelease data(::IOPSCopyPowerSourcesInfo());
> +  if (!data)
> +    return;

The coding style require brackets:
if !(data) {
  return;
}

@@ +200,5 @@
> +  CFTypeAutoRelease data(::IOPSCopyPowerSourcesInfo());
> +  if (!data)
> +    return;
> +
> +  // get the list of power sources 

// Get the [...] sources.

@@ +203,5 @@
> +
> +  // get the list of power sources 
> +  CFArrayAutoRelease list(::IOPSCopyPowerSourcesList(data));
> +  if (!list)
> +    return;

Missing brackets.

@@ +206,5 @@
> +  if (!list)
> +    return;
> +
> +  // default values. these will be used if we have 0 sources (probably wall power)
> +  // or if we can't find better information.

// Default [...]

@@ +212,5 @@
> +  double charging = kDefaultCharging;
> +  double remainingTime = kUnknownRemainingTime;
> +
> +  // look for the first battery or AC power to give us the information we need.
> +  // usually there's only 1 available, depending on current power source.

// Look [...]
// Usually [...]

@@ +218,5 @@
> +    CFTypeRef source = ::CFArrayGetValueAtIndex(list, i);
> +    CFDictionaryRef desc = ::IOPSGetPowerSourceDescription(data, source);
> +    CFStringRef powerType = static_cast<CFStringRef>(::CFDictionaryGetValue(desc, CFSTR(kIOPSPowerSourceStateKey)));
> +
> +    // only get information from a battery or AC power.

// Only [...]

@@ +220,5 @@
> +    CFStringRef powerType = static_cast<CFStringRef>(::CFDictionaryGetValue(desc, CFSTR(kIOPSPowerSourceStateKey)));
> +
> +    // only get information from a battery or AC power.
> +    if (::CFStringCompare(powerType, CFSTR(kIOPSACPowerValue), 0) == kCFCompareEqualTo ||
> +        ::CFStringCompare(powerType, CFSTR(kIOPSBatteryPowerValue), 0) == kCFCompareEqualTo) {

Do we want to check for AC values? For AC, we want to use default values only, no need to read anything. In addition, we might have wrong values here if AC shows up before battery in the list (we will read AC info and stop without reading battery ones).

@@ +222,5 @@
> +    // only get information from a battery or AC power.
> +    if (::CFStringCompare(powerType, CFSTR(kIOPSACPowerValue), 0) == kCFCompareEqualTo ||
> +        ::CFStringCompare(powerType, CFSTR(kIOPSBatteryPowerValue), 0) == kCFCompareEqualTo) {
> +
> +      // se if we can get a time estimate.

// See [...]
(note the missing 'e' too)

@@ +229,5 @@
> +          estimate != kIOPSTimeRemainingUnlimited) {
> +        remainingTime = estimate;
> +      }
> +
> +      // get a battery level estimate

// Get [...] estimate.

@@ +239,5 @@
> +      cfRef = ::CFDictionaryGetValue(desc, CFSTR(kIOPSMaxCapacityKey));
> +      ::CFNumberGetValue((CFNumberRef)cfRef, kCFNumberSInt32Type, &maxCapacity);
> +
> +      if (maxCapacity > 0)
> +        level = double(currentCapacity)/double(maxCapacity);

Missing brackets.

@@ +241,5 @@
> +
> +      if (maxCapacity > 0)
> +        level = double(currentCapacity)/double(maxCapacity);
> +
> +      // find out if we're charging

// Find [...] charging.

@@ +252,5 @@
> +  power->mRemainingTime = remainingTime;
> +  power->mCharging = charging;
> +  power->mLevel = level;
> +  
> +  // notify the observers that stuff changed

// Notify [..] changed.

Comment 6

[Mounir Lamouri (:mounir)]

Håkan, is there anything I can do to help you with that patch?

Comment 7

[Mounir Lamouri (:mounir)]

I sent an email to Håkan today and he has too much stuff to do to get back on this soon.
If anyone wants to fix the issues I pointed, I would be glad to mentor this bug.

Comment 8

[Dirkjan Ochtman (:djc)]

I'd like to take a whack at this.

Comment 9

[Dirkjan Ochtman (:djc)]

Okay, I just took a stab at taking the patch here and addressing the comments from comment 5. However, I'm having a hard time seeing through the whole PointerRef templating shebang... there seems to be a lot of stuff there, and I'm not sure (even while looking at the comments for nsPointerRefTraits and nsAutoRef) how this is supposed to fit together. Mounir (or someone else), can you provide some guidance?

Also, is there a better IRC channel to talk about this stuff than just #developers?

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to Dirkjan Ochtman (:djc) from comment #9)
> Okay, I just took a stab at taking the patch here and addressing the
> comments from comment 5. However, I'm having a hard time seeing through the
> whole PointerRef templating shebang... there seems to be a lot of stuff
> there, and I'm not sure (even while looking at the comments for
> nsPointerRefTraits and nsAutoRef) how this is supposed to fit together.
> Mounir (or someone else), can you provide some guidance?

nsAutoRefTraits allows you to create a guard object that will handle creation and destruction of a pointer (for example, a pointer of type CFTypeRef for nsAutoRefTraits<CFTypeRef>). That way, you don't have to worry about the lifetime of a pointer you got and you own, you can assume nsAutoRefTraits will release the memory for you when needed as if it was an object in the stack (that's because the nsAutoRefTraits is actually in the stack even if the pointed object isn't).

> Also, is there a better IRC channel to talk about this stuff than just
> #developers?

#developers is a good place. You can also ping me (mounir) in #introduction or even directly send me a private message.

Comment 11

[Dirkjan Ochtman (:djc)]

Sorry, this isn't really working.

Comment 12

[Luke Crouch [:groovecoder]]

Any updates on this? Wanted to show a demo for it (http://davidwalsh.name/dw-content/battery-api.php) at OKC.js but it doesn't work on my MacBook Pro. :(

Comment 13

[Reuben Morais [:reuben]]

A friend of mine had a few questions about the Battery API on Firefox and I hit this bug. I'll attach an unbitrot patch with the review changes as soon as this build is done and I can test it.

Mounir: we now have hal/cocoa/ with the sensor implementation, so I put MacBattery.cpp there. Is that OK?

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to Reuben Morais [:reuben] from comment #13)
> Mounir: we now have hal/cocoa/ with the sensor implementation, so I put
> MacBattery.cpp there. Is that OK?

CocoaBattery.cpp would be good.

Comment 15

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend

Slowest build ever.

The API used in HandleChange is synchronous so I'm simply calling the method manually in EnableBatteryNotifications so we have valid data in case GetCurrentBatteryInformation gets called before the next power source change.

>Why not inheriting from nsPointerRefTraits<CFTypeRef>?
>It would prevent typedef'ing RawRef and defining Void().

nsPointerRefTraits defines RawRef as a T*, and CF*Ref are already pointers so we can't inherit from that.

Comment 16

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend

(In reply to Mounir Lamouri (:mounir) from comment #14)
> CocoaBattery.cpp would be good.

Somehow I completely missed this comment before attaching the patch, sorry for the bug spam.

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 640518 [details] [diff] [review]
OS X Battery HAL backend

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

It looks pretty great except that the notification happens a bit too often.
You should be good to go (regarding DOM API) if that is fixed. You will still need a Cocoa widget peer's review to make sure the platforms API uses are fine.

::: hal/cocoa/CocoaBattery.cpp
@@ +154,5 @@
> +
> +void
> +MacPowerInformationService::HandleChange(void *aContext) {
> +  MacPowerInformationService *power = 
> +    static_cast<MacPowerInformationService *>(aContext);

nit: use "Type*" instead of "Type *".

@@ +181,5 @@
> +    CFDictionaryRef desc = ::IOPSGetPowerSourceDescription(data, source);
> +    CFStringRef powerType = static_cast<CFStringRef>(::CFDictionaryGetValue(desc, CFSTR(kIOPSPowerSourceStateKey)));
> +
> +    // Only get information from a battery power source.
> +    if (::CFStringCompare(powerType, CFSTR(kIOPSBatteryPowerValue), 0) == kCFCompareEqualTo) {

You could do:
if (!::CFStringCompare(powerType, CFSTR(kIOPSBatteryPowerValue), 0) == kCFCompareEqualTo) {
  continue;
}

// The code when the source is battery there.

@@ +182,5 @@
> +    CFStringRef powerType = static_cast<CFStringRef>(::CFDictionaryGetValue(desc, CFSTR(kIOPSPowerSourceStateKey)));
> +
> +    // Only get information from a battery power source.
> +    if (::CFStringCompare(powerType, CFSTR(kIOPSBatteryPowerValue), 0) == kCFCompareEqualTo) {
> +

nit: remove that empty line.

@@ +199,5 @@
> +      int maxCapacity = 0;
> +      cfRef = ::CFDictionaryGetValue(desc, CFSTR(kIOPSMaxCapacityKey));
> +      ::CFNumberGetValue((CFNumberRef)cfRef, kCFNumberSInt32Type, &maxCapacity);
> +
> +      if (maxCapacity > 0) {

Shouldn't you check |currentCapacity| too?

@@ +216,5 @@
> +  power->mLevel = level;
> +  
> +  // Notify the observers that stuff changed.
> +  hal::NotifyBatteryChange(hal::BatteryInformation(power->mLevel,
> +                                                   power->mCharging,

You should not notify observers if there is no actual changes.

In addition, the first HandleChange() called from BeginListening() shouldn't fire this event.

Comment 18

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend v2

>Shouldn't you check |currentCapacity| too?

The |maxCapacity| check is there to avoid a division by zero. I don't think current capacity is ever reported as 0 (OS X hibernates before that), but if that happens, then |level| should be reported as 0 as well.

Comment 19

[Benoit Girard (:BenWa)]

Comment on attachment 640682 [details] [diff] [review]
OS X Battery HAL backend v2

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

::: hal/cocoa/CocoaBattery.cpp
@@ +15,5 @@
> +/**
> + * Used to make sure we always release CFTypeRef.
> + */
> +template < >
> +class nsAutoRefTraits<CFTypeRef>

I'm not a fan of using these, esp. for just a single use below.

@@ +83,5 @@
> +
> +void
> +DisableBatteryNotifications()
> +{
> +  MacPowerInformationService::GetInstance()->StopListening();

Is this a good place to release sInstance?

@@ +106,5 @@
> +/* static */ MacPowerInformationService* 
> +MacPowerInformationService::GetInstance()
> +{
> +  if (!sInstance) {
> +    sInstance = new MacPowerInformationService();

leak on shutdown.

@@ +141,5 @@
> +
> +  // Invoke our callback now so we have data if GetCurrentBatteryInformation is
> +  // called before a change happens.
> +  HandleChange(this);
> +  mShouldNotify = true;

I don't see the point of this flag. Should you set it to false in StopListening()?

@@ +177,5 @@
> +  double charging = kDefaultCharging;
> +  double remainingTime = kUnknownRemainingTime;
> +
> +  // Look for the first battery power source to give us the information we need.
> +  // Usually there's only 1 available, depending on current power source.

I'm surprised that we're not interested in the current power source.

@@ +180,5 @@
> +  // Look for the first battery power source to give us the information we need.
> +  // Usually there's only 1 available, depending on current power source.
> +  for (int i=0; i<(int)::CFArrayGetCount(list); i++) {
> +    CFTypeRef source = ::CFArrayGetValueAtIndex(list, i);
> +    CFDictionaryRef desc = ::IOPSGetPowerSourceDescription(data, source);

Check for null. A more descriptive name would be nice 'currPowerSourceDesc'.

@@ +185,5 @@
> +    CFStringRef powerType = static_cast<CFStringRef>(::CFDictionaryGetValue(desc, CFSTR(kIOPSPowerSourceStateKey)));
> +    // Only get information from a battery power source.
> +    if (::CFStringCompare(powerType, CFSTR(kIOPSBatteryPowerValue), 0) != kCFCompareEqualTo) {
> +  		continue;
> +  	}

tabs instead of spaces here.

@@ +196,5 @@
> +    }
> +
> +    // Get a battery level estimate.
> +    int currentCapacity = 0;
> +    const void *cfRef = ::CFDictionaryGetValue(desc, CFSTR(kIOPSCurrentCapacityKey));

This key is required, this should be documented.

@@ +200,5 @@
> +    const void *cfRef = ::CFDictionaryGetValue(desc, CFSTR(kIOPSCurrentCapacityKey));
> +    ::CFNumberGetValue((CFNumberRef)cfRef, kCFNumberSInt32Type, &currentCapacity);
> +
> +    int maxCapacity = 0;
> +    cfRef = ::CFDictionaryGetValue(desc, CFSTR(kIOPSMaxCapacityKey));

This key is required, this should be documented.

@@ +208,5 @@
> +      level = double(currentCapacity)/double(maxCapacity);
> +    }
> +
> +    // Find out if we're charging.
> +    charging = ::CFBooleanGetValue((CFBooleanRef)::CFDictionaryGetValue(desc, CFSTR(kIOPSIsChargingKey)));

This key is optional, this should be documented and checked.

Comment 20

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #19)
::: hal/cocoa/CocoaBattery.cpp
> @@ +15,5 @@
> > +/**
> > + * Used to make sure we always release CFTypeRef.
> > + */
> > +template < >
> > +class nsAutoRefTraits<CFTypeRef>
> 
> I'm not a fan of using these, esp. for just a single use below.
> 

I see this was suggested above in Comment 5. I don't fully agree but I can let this go.

Comment 21

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend v3

(In reply to Benoit Girard (:BenWa) from comment #19)
> ::: hal/cocoa/CocoaBattery.cpp
> @@ +15,5 @@
> > +/**
> > + * Used to make sure we always release CFTypeRef.
> > + */
> > +template < >
> > +class nsAutoRefTraits<CFTypeRef>
> 
> I'm not a fan of using these, esp. for just a single use below.

Me neither, I kept them just because they were in the original patch.
 
> @@ +83,5 @@
> > +
> > +void
> > +DisableBatteryNotifications()
> > +{
> > +  MacPowerInformationService::GetInstance()->StopListening();
> 
> Is this a good place to release sInstance?

Not really, unless we want to delete and create a new instance of MacPowerInformationService whenever a page uses the Battery API.

> @@ +106,5 @@
> > +/* static */ MacPowerInformationService* 
> > +MacPowerInformationService::GetInstance()
> > +{
> > +  if (!sInstance) {
> > +    sInstance = new MacPowerInformationService();
> 
> leak on shutdown.

Do we care about this? All the allocated memory is going to be released to the OS anyway.

> @@ +141,5 @@
> > +
> > +  // Invoke our callback now so we have data if GetCurrentBatteryInformation is
> > +  // called before a change happens.
> > +  HandleChange(this);
> > +  mShouldNotify = true;
> 
> I don't see the point of this flag. Should you set it to false in
> StopListening()?

It's there so HandleChange doesn't call hal::NotifyBatteryChange on this first call. See comment 17.
I can split HandleChange in three methods (HandleChange/Notify/HandleAndNotify), but I'd rather use the flag for simplicity.

Comment 22

[Benoit Girard (:BenWa)]

(In reply to Reuben Morais [:reuben] from comment #21)
> > @@ +106,5 @@
> > > +/* static */ MacPowerInformationService* 
> > > +MacPowerInformationService::GetInstance()
> > > +{
> > > +  if (!sInstance) {
> > > +    sInstance = new MacPowerInformationService();
> > 
> > leak on shutdown.
> 
> Do we care about this? All the allocated memory is going to be released to
> the OS anyway.
> 

We care because we look for leaks on shutdown.

The rest looks fine.

Comment 23

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend v4

Other than deleting the singleton instance on shutdown, I've also removed the checks to see if the power source information comes from a battery. IOKit reports changes in a charging battery as being from an AC power source, which means the old code only fired chargingchange events when you unplug the cable, not when you plug it. It also prevented chargingtimechange events from being fired.

Comment 24

[Benoit Girard (:BenWa)]

Comment on attachment 652468 [details] [diff] [review]
OS X Battery HAL backend v4

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

::: hal/cocoa/CocoaBattery.cpp
@@ +84,5 @@
> + * Following is the implementation of MacPowerInformationService.
> + */
> +
> +MacPowerInformationService* MacPowerInformationService::sInstance = nullptr;
> +MacPowerInformationService::SingletonDestroyer MacPowerInformationService::sDestroyer(sInstance);

Static destructors will run after we check for leak so this doesn't solve the problem, it's a real pain I know :(. Add MOZ_COUNT_CTOR/MOZ_COUNT_DTOR to the new object that are introduced here so we can better track for leaks. I think the general design is to use a module to destroy singleton.

Comment 25

[Mounir Lamouri (:mounir)]

Comment on attachment 652468 [details] [diff] [review]
OS X Battery HAL backend v4

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

I would like to review a version that was r+'d by Benoit, I don't have much to review and I want to have a look at the final version.

Comment 26

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend v5

Using xpcom-shutdown for the clean up as discussed on IRC.

Comment 27

[Reuben Morais [:reuben]]

Comment on attachment 653910 [details] [diff] [review]
OS X Battery HAL backend v5

Thanks for the review!

Comment 28

[Mounir Lamouri (:mounir)]

Comment on attachment 653910 [details] [diff] [review]
OS X Battery HAL backend v5

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

Unfortunately, I have to r- this patch because of the errors in the remainingTime field. Everything else is nits or minor changes.
Hopefully, it will not be that much work to make remainingTime work as expected.

::: hal/cocoa/CocoaBattery.cpp
@@ +32,5 @@
> +
> +  ~MacPowerInformationService();
> +
> +private:
> +  MacPowerInformationService(); 

nit: trailing space.

@@ +43,5 @@
> +  double mRemainingTime;
> +  bool mShouldNotify;
> +  
> +  friend void GetCurrentBatteryInformation(hal::BatteryInformation* aBatteryInfo);
> +  

nit: trailing spaces on the lines before a after "friend void GetCurrentBatteryInformation(...);"

@@ +99,5 @@
> +  return NS_OK;
> +}
> +} // anonymous namespace
> +
> +/* static */ MacPowerInformationService* 

nit: trailing space

@@ +104,5 @@
> +MacPowerInformationService::GetInstance()
> +{
> +  if (!sInstance) {
> +    sInstance = new MacPowerInformationService();
> +    

nit: trailing spaces

@@ +125,5 @@
> +
> +MacPowerInformationService::MacPowerInformationService()
> +  : mLevel(kDefaultLevel)
> +  , mCharging(kDefaultCharging)
> +  , mRemainingTime(kUnknownRemainingTime)

You should use |kDefaultRemainingTime|.

@@ +141,5 @@
> +void
> +MacPowerInformationService::BeginListening()
> +{
> +  // Set ourselves up to be notified about changes.
> +  mRunLoopSource = ::IOPSNotificationCreateRunLoopSource(HandleChange, this);

Maybe you could fire an assertion if mRunLoopSource is already set?

@@ +156,5 @@
> +
> +void
> +MacPowerInformationService::StopListening()
> +{
> +  if (mRunLoopSource) {

Maybe you could fire an assertion if mRunLoopSource isn't set?

@@ +165,5 @@
> +}
> +
> +void
> +MacPowerInformationService::HandleChange(void* aContext) {
> +  MacPowerInformationService* power = 

nit: trailing space

@@ +185,5 @@
> +  // Default values. These will be used if we have 0 sources (likely wall power)
> +  // or if we can't find better information.
> +  double level = kDefaultLevel;
> +  double charging = kDefaultCharging;
> +  double remainingTime = 0; // kUnknownRemainingTime is -1 but BatteryManager asserts on 0 if we're currently charging.

You should use |kDefaultRemainingTime|.

@@ +188,5 @@
> +  double charging = kDefaultCharging;
> +  double remainingTime = 0; // kUnknownRemainingTime is -1 but BatteryManager asserts on 0 if we're currently charging.
> +
> +  // Look for the first battery power source to give us the information we need.
> +  // Usually there's only 1 available, depending on current power source.

So I assume |list| can't contain the AC power source?

@@ +200,5 @@
> +    // See if we can get a time estimate.
> +    CFTimeInterval estimate = ::IOPSGetTimeRemainingEstimate();
> +    if (estimate != kIOPSTimeRemainingUnknown &&
> +        estimate != kIOPSTimeRemainingUnlimited) {
> +      remainingTime = estimate;

You should default to |kUnknownRemainingTime| when the battery is full.

@@ +214,5 @@
> +    cfRef = ::CFDictionaryGetValue(currPowerSourceDesc, CFSTR(kIOPSMaxCapacityKey));
> +    ::CFNumberGetValue((CFNumberRef)cfRef, kCFNumberSInt32Type, &maxCapacity);
> +
> +    if (maxCapacity > 0) {
> +      level = double(currentCapacity)/double(maxCapacity);

use static_cast<double>() instead of double()

@@ +228,5 @@
> +  }
> +
> +  bool isNewData = false;
> +  if (level != power->mLevel || charging != power->mCharging || remainingTime != power->mRemainingTime) {
> +    isNewData = true;

You could also write:
bool isNewData = level != power->mLevel || charging != power->mCharging || remainingTime != power->mRemainingTime;

Comment 29

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend v6 WIP

(In reply to Mounir Lamouri (:mounir) from comment #28)
> @@ +188,5 @@
> > +  double charging = kDefaultCharging;
> > +  double remainingTime = 0; // kUnknownRemainingTime is -1 but BatteryManager asserts on 0 if we're currently charging.
> > +
> > +  // Look for the first battery power source to give us the information we need.
> > +  // Usually there's only 1 available, depending on current power source.
> 
> So I assume |list| can't contain the AC power source?

Like I mentioned earlier – |list| will contain an AC power source if the battery is charging. We have to filter out AC power sources in order to get notifications for battery discharging and charging <-> not charging transitions, but inspecting the behavior closely after your review comment, I noticed that when charging, the capacity information is stored on the AC power source. So in order to properly report battery level when charging, extra logic will be required. This is a WIP patch with all the other review comments addressed.

Comment 30

[Reuben Morais [:reuben]]

Attachment: Battery API test

Also, I have a feeling that wasn't the behavior on 10.7. I'd be awesome if someone could test patch v5 on Lion and see if the test page reports charging and discharging notifications and accurate levels on both cases.

Comment 31

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend v6

Alright, so the problem was that when you plug the charger the system will report the change from battery to AC power first, and only then the change from not charging to charging. OS X reports charging batteries as an AC power source, so there's no need to filter only battery power sources. This patch addresses all your review comments, and has minor fixes for style consistency.

Comment 32

[Mounir Lamouri (:mounir)]

Comment on attachment 658355 [details] [diff] [review]
OS X Battery HAL backend v6

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

r=me with the comments addressed.

Thank you for this contribution Reuben! :)

Do you have enough rights to this patch to the try server or do you want me to do that for you?

::: hal/cocoa/CocoaBattery.cpp
@@ +102,5 @@
> +
> +/* static */ MacPowerInformationService*
> +MacPowerInformationService::GetInstance()
> +{
> +  if (!sInstance) {

nit: I would do:
if (sInstance) {
  return sInstance;
}

sInstance = new MacPowerInformationService();
[...]

@@ +143,5 @@
> +{
> +  // Set ourselves up to be notified about changes.
> +  MOZ_ASSERT(!mRunLoopSource, "IOPS Notification Loop Source already set up. "
> +                              "(StopListening should have been called)");
> +  mRunLoopSource = ::IOPSNotificationCreateRunLoopSource(HandleChange, this);

nit: live an empty line between MOZ_ASSERT and mRunLoopSource assignment.

I realize that you might want to assert if mRunLoopSource isn't set. Seems like you would assert when StopListening() would be called anyway, right? In which situations could that happen?

@@ +183,5 @@
> +    ::CFRelease(list);
> +    return;
> +  }
> +
> +  // Default values. These will be used if we there are 0 sources or we can't

nit: s/if we there/if there/

Comment 33

[Mozilla RelEng Bot]

Try run for 80b2c36cc2a7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=80b2c36cc2a7
Results (out of 22 total builds):
    success: 15
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-80b2c36cc2a7

Comment 34

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend v6.1

Carrying r=BenWa,mounir.

(In reply to Mounir Lamouri (:mounir) from comment #32)
> Comment on attachment 658355 [details] [diff] [review]
> OS X Battery HAL backend v6
> 
> Review of attachment 658355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the comments addressed.
> 
> Thank you for this contribution Reuben! :)
> 
> Do you have enough rights to this patch to the try server or do you want me
> to do that for you?

Try submission looks good to me.

> @@ +143,5 @@
> > +{
> > +  // Set ourselves up to be notified about changes.
> > +  MOZ_ASSERT(!mRunLoopSource, "IOPS Notification Loop Source already set up. "
> > +                              "(StopListening should have been called)");
> > +  mRunLoopSource = ::IOPSNotificationCreateRunLoopSource(HandleChange, this);
> 
> nit: live an empty line between MOZ_ASSERT and mRunLoopSource assignment.
> 
> I realize that you might want to assert if mRunLoopSource isn't set. Seems
> like you would assert when StopListening() would be called anyway, right? In
> which situations could that happen?

Right, when adding those asserts I checked for possible cases where the run loop was already initialized. BatteryManager shouldn't call EnableBatteryNotifications() twice in a row.

Comment 35

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend v6.1

Oops, forgot to qref one of the changes. Sorry for bugspam.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

(In reply to Mozilla RelEng Bot from comment #33)
>     https://tbpl.mozilla.org/?tree=Try&rev=80b2c36cc2a7

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/52120672ee11

Should this have a test?

Comment 37

[Ryan VanderMeulen [:RyanVM]]

And backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d28e07f4bec6

Comment 38

[Reuben Morais [:reuben]]

Apparently Lion debug builds don't set a specific SDK version, so the try debug run used the 10.7 SDK and the opt run on inbound used 10.6. I filed bug 790491 on that.

On the bright side, that made me aware that IOPSGetTimeRemainingEstimate (and the related constants) are only available on 10.7 and later.

Comment 39

[Mozilla RelEng Bot]

Try run for 3c3e38a4a36a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3c3e38a4a36a
Results (out of 8 total builds):
    success: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3c3e38a4a36a

Comment 40

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend v7

I've #ifdef'd the API call that's only available on 10.7, and it passed try (both opt and debug this time :).

@BenWa, is this OK?

Comment 41

[Benoit Girard (:BenWa)]

(In reply to Reuben Morais [:reuben] from comment #40)
> Created attachment 660305 [details] [diff] [review]
> OS X Battery HAL backend v7
> 
> I've #ifdef'd the API call that's only available on 10.7, and it passed try
> (both opt and debug this time :).
> 
> @BenWa, is this OK?

Actually it's not :(. This can't be a compile check since we ship the same binary to 10.6 and 10.7+. This needs to be a runtime check and we need to make sure we can still build with the 10.6 SDK.

I think the right thing to do is to copy the declaration and add '__attribute__((weak_import))' see nsChildView.h. Then you can detect the feature at run time by checking if the symbol is null. Is this right Steven?

Comment 42

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend© v8 OneMoreTime Edition™

(In reply to Benoit Girard (:BenWa) from comment #41)
> Actually it's not :(. This can't be a compile check since we ship the same
> binary to 10.6 and 10.7+. This needs to be a runtime check and we need to
> make sure we can still build with the 10.6 SDK.
> 
> I think the right thing to do is to copy the declaration and add
> '__attribute__((weak_import))' see nsChildView.h. Then you can detect the
> feature at run time by checking if the symbol is null. Is this right Steven?

Oh boy, let's try this again. The attached patch works on 10.8, but I don't have access to a Snow Leopard machine to test it there. Sent to try: https://tbpl.mozilla.org/?tree=Try&rev=5394bfe95b22

Comment 43

[Mozilla RelEng Bot]

Try run for 5394bfe95b22 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5394bfe95b22
Results (out of 5 total builds):
    success: 3
    warnings: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5394bfe95b22

Comment 44

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #41)

> I think the right thing to do is to copy the declaration and add
> '__attribute__((weak_import))' see nsChildView.h. Then you can
> detect the feature at run time by checking if the symbol is null. Is
> this right Steven?

Sounds good to me.

Comment 45

[Benoit Girard (:BenWa)]

Comment on attachment 661537 [details] [diff] [review]
OS X Battery HAL backend© v8 OneMoreTime Edition™

r+ as long as the 10.8 failure fix is straight forward.

Comment 46

[Reuben Morais [:reuben]]

(In reply to Benoit Girard (:BenWa) from comment #45)
> Comment on attachment 661537 [details] [diff] [review]
> OS X Battery HAL backend© v8 OneMoreTime Edition™
> 
> r+ as long as the 10.8 failure fix is straight forward.

Thanks! I can't reproduce that failure locally, probably because I'm building against the 10.7 SDK. I'll see if I can find my Snow Leopard installation CD and copy its SDK when I get home. It looks like clang is trying to strongly link the symbol even though I declare it as a weak import.

Comment 47

[Reuben Morais [:reuben]]

Attachment: OS X Battery HAL backend v9

Turns out weak imports aren't the tool we want here. Weakly imported symbols must exist at compile time, so they're OK if you're using an API that will be removed in the future (like nsChildView uses), but they're not OK when the symbol doesn't exist at compile time.

For the purposes of this bug, I'm using dlopen/dlsym to find the symbol at runtime, following the style used by gfx/2d/QuartzSupport.mm. Since this isn't a trivial fix, I'm requesting review again. Try results should be available soon at https://tbpl.mozilla.org/?tree=Try&rev=a1991c0468a6

Comment 48

[Benoit Girard (:BenWa)]

Comment on attachment 662798 [details] [diff] [review]
OS X Battery HAL backend v9

That looks good. Sorry for pointing you in the wrong direction with weak_import.

Comment 49

[Reuben Morais [:reuben]]

Thanks!

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/294643a303c1

Should this have a test?

Comment 51

[Mounir Lamouri (:mounir)]

It's really hard to have automatic test for this.
I'm not sure if marionette is ready for that. Otherwise, we might need basic litmus tests.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/294643a303c1

Comment 53

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Firefox_18_for_developers#.C2.A0DOM
https://developer.mozilla.org/en-US/docs/DOM/window.navigator.battery#Gecko_notes

Comment 54

[Justin Lebar (not reading bugmail)]

For everyone's edification: With respect to singletons which leak until shutdown, the shiny new way of doing this is

static StaticAutoPtr<T> myT;

void Init() {
  myT = new T();
  ClearOnShutdown(&myT);
}

This has no static constructors/destructors.

Comment 55

[Benoit Girard (:BenWa)]

Thanks for the update :)