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.

I've started working on this.

Feel free to attach a work in progress patch.

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

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

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.

I'd like to take a whack at this.

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?

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

Sorry, this isn't really working.

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

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?

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

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.

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

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

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

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

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

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

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

Comment on attachment 653910 [details] [diff] [review] OS X Battery HAL backend v5 Thanks for the review!

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;

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

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.

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

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/reuben.morais@gmail.com-80b2c36cc2a7

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.

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

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

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

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.

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/reuben.morais@gmail.com-3c3e38a4a36a

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?

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

(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

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/reuben.morais@gmail.com-5394bfe95b22

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

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

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

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/294643a303c1 Should this have a test?

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.

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

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.

Thanks for the update :)