Battery Status API: implementation for Mac OS.

content/browser/battery_status/battery_status_manager_mac.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/battery_status/battery_status_manager.h"
+
+#include <CoreFoundation/CoreFoundation.h>
+#include <IOKit/ps/IOPowerSources.h>
+#include <IOKit/ps/IOPSKeys.h>
+
+#include "base/mac/foundation_util.h"
+#include "base/mac/scoped_cftyperef.h"
+#include "base/memory/ref_counted.h"
+#include "base/time/time.h"
+#include "content/public/browser/browser_thread.h"
+#include "third_party/WebKit/public/platform/WebBatteryStatus.h"
+
+namespace content {
+
+namespace {
+
+typedef const BatteryStatusService::BatteryUpdateCallback BatteryCallback;
+
+// Returns the value corresponding to |key| in the dictionary |description|.
+// Returns |default_value| if the dictionary does not contain |key|, the
+// corresponding value is NULL or it could not be converted to CFIndexType.
+static CFIndex GetValueAsCFIndex(CFDictionaryRef description,

[Mark Mentovai, 2014/07/24 01:32:22]

This is already in an unnamed namespace, so the static doesn’t help at all. Same
for all of the other functions in here.

[Mark Mentovai, 2014/07/24 01:32:22]

Why did you choose CFIndex as the type for the CFNumbers? What’s CFNumberGetType
return for the numbers you’re interested in?

[timvolodine, 2014/07/24 16:03:19]

Done.

[timvolodine, 2014/07/24 16:03:19]

usually it is kCFNumberSInt64Type, but can also be kCFNumberSInt32Type (e.g.
when "time to empty" is -1). For practical use the CFIndex (which is a signed
long) should be sufficient.

[Mark Mentovai, 2014/07/24 16:08:52]

timvolodine wrote:
long is 32 bits on 32-bit x86. You should probably extract this as
kCFNumberSInt64Type, since that’s the largest type you’re seeing it stored as,
and that type is always capable of holding values of kCFNumberSInt32Type.

[timvolodine, 2014/07/24 16:33:42]

yes, my point was that the actual values are small and very unlikely to require
a full 64 bit integer. For example capacity is between 0 and 100 and times are
in minutes. I assume the conversion would simply fail if they cannot be
converted to long.

If you feel strongly about this I could introduce a SInt64 with the additional
complexity of using Math64.h for all operations on them (to make it work on
compilers that don't support long long).

[Mark Mentovai, 2014/07/24 17:03:46]

timvolodine wrote:
Are you serious? The API gives you a SInt64 already, and this is a
platform-specific file used on a platform where we know we have a usable
int64_t. So at least for the cases where you convert to double and divide one of
these by another, there doesn’t seem to be any problem at all, and I don’t know
why you wouldn’t just deal with then numbers that the system gives you. The
web-side API doesn’t need to change at all, and the 64-bit quantities wouldn’t
leave this file.

SInt64 has the same range as CFIndex on x86_64 anyway, but a different range on
32-bit x86. I don’t know why you think this is something that should vary based
on CPU architecture. I mean, if you really wanted to clamp the range to
something narrower than SInt64, then you should be uniformly using that type
(SInt32?)

[timvolodine, 2014/07/25 12:46:08]

Ok, changed to SInt32.

Technically casting a SInt64 to double can lead to loss in precision. Actually I
was looking at MacTypes.h 
(https://opensource.apple.com/source/CarbonHeaders/CarbonHeaders-18.1/MacTypes.h)
where SInt64 is defined as a struct for compilers that don't support it
natively. Not sure how conversion to double would work in that case. Generally
speaking, do you know if we can assume that we have 64bit integer compiler
support in chromium?

[Mark Mentovai, 2014/07/25 12:57:39]

timvolodine wrote:
That’d be fine. In this case, a loss of precision at the range’s extremities is
preferable to integer truncation to a completely unrelated quantity.
100% sure.

MacTypes.h has legacy stuff in it going back to the classic Mac OS and the 68k
family of CPUs. Compiler support for 64-bit integers is always present in Mac OS
X.

If you haven’t noticed, we use uint64_t with reckless abandon in Chrome.

[timvolodine, 2014/07/25 13:13:17]

right, thanks for the clarification and sorry for confusion. I've uploaded an
upgraded patch using SInt64.

+                                 CFStringRef key,
+                                 CFIndex default_value) {
+  CFNumberRef number =
+      base::mac::GetValueFromDictionary<CFNumberRef>(description, key);
+  CFIndex value;
+
+  if (number && CFNumberGetValue(number, kCFNumberCFIndexType, &value))
+    return value;
+
+  return default_value;
+}
+
+static Boolean GetValueAsBoolean(CFDictionaryRef description,

[Mark Mentovai, 2014/07/24 01:32:21]

I think that this should return a bool and take a bool as the third argument,
which is consistent with the type you’ve used elsewhere.

[timvolodine, 2014/07/24 16:03:19]

Done.

+                                 CFStringRef key,
+                                 Boolean default_value) {
+  CFBooleanRef boolean =
+      base::mac::GetValueFromDictionary<CFBooleanRef>(description, key);
+
+  return boolean ? CFBooleanGetValue(boolean) : default_value;
+}
+
+static void FetchBatteryStatus(CFDictionaryRef description,
+                               blink::WebBatteryStatus& status) {
+  CFStringRef currentState =

[Mark Mentovai, 2014/07/24 01:32:22]

Follow the C++ style guide. This should be named current_state.

[timvolodine, 2014/07/24 16:03:19]

Done.

+      base::mac::GetValueFromDictionary<CFStringRef>(description,
+          CFSTR(kIOPSPowerSourceStateKey));
+
+  bool on_battery_power = CFStringCompare(currentState,

[Mark Mentovai, 2014/07/24 01:32:22]

I don’t think this is going to be too happy if currentState is NULL.

[timvolodine, 2014/07/24 16:03:19]

actually kIOPSPowerSourceStateKey is a required key so should not be NULL, but
anyway I've made it more robust now.

+                                          CFSTR(kIOPSBatteryPowerValue),
+                                          0)
+                          == kCFCompareEqualTo;
+  bool is_charging =
+      GetValueAsBoolean(description, CFSTR(kIOPSIsChargingKey), true);

[Mark Mentovai, 2014/07/24 01:32:22]

Is true the best default here? I probably would have chosen false.

[timvolodine, 2014/07/24 16:03:19]

kIOPSIsChargingKey is also required so hopefully no need for default value, but
as according to specification I've chosen true to reflect the default value in
this case.

+  bool is_charged =
+      GetValueAsBoolean(description, CFSTR(kIOPSIsChargedKey), false);
+
+  // Set charging to false if on battery power and not charging. Otherwise leave
+  // the default value, which is true.
+  if (on_battery_power && !is_charging)

[Mark Mentovai, 2014/07/24 01:32:22]

Why not avoid the conditional and set this explicitly all the time, which avoid
the need for the awkward comment explaining the default:

  status.charging = !on_battery_power || is_charging;

This is clearer and functionally equivalent to what you wrote.

In fact, for all of these fields, I think it’d be preferable to set the values
explicitly, at least in most cases, rather than relying on the default values.

[timvolodine, 2014/07/24 16:03:19]

Done.
Re other fields: normally speaking the idea is to set the fields only if we can
do so reliably. Otherwise we'll have to explicitly specify defaults for each
platform-specific implementation. ChromeOS implementation is also done this way.

+    status.charging = false;
+
+  CFIndex current_capacity =
+      GetValueAsCFIndex(description, CFSTR(kIOPSCurrentCapacityKey), -1);
+  CFIndex max_capacity =
+      GetValueAsCFIndex(description, CFSTR(kIOPSMaxCapacityKey), -1);
+
+  // Set level if it is available and valid. Otherwise leave the default value,
+  // which is 1.
+  if (current_capacity != -1 && max_capacity != -1 &&
+      current_capacity <= max_capacity && max_capacity != 0)
+    status.level = current_capacity / static_cast<double>(max_capacity);
+
+  if (is_charging) {
+    CFIndex charging_time =
+        GetValueAsCFIndex(description, CFSTR(kIOPSTimeToFullChargeKey), -1);
+
+    // Battery is charging: set the charging time if it's available, otherwise
+    // set to +infinity.
+    status.chargingTime = (charging_time != -1)
+        ? base::TimeDelta::FromMinutes(charging_time).InSeconds()
+        : std::numeric_limits<double>::infinity();
+  } else {
+    // Battery is not charging.
+    // Set chargingTime to +infinity if the battery is not charged. Otherwise
+    // leave the default value, which is 0.
+    if (!is_charged)
+      status.chargingTime = std::numeric_limits<double>::infinity();
+
+    // Set dischargingTime if it's available and valid, i.e. when on battery
+    // power. Otherwise leave the default value, which is +infinity.
+    if (on_battery_power) {
+      CFIndex discharging_time =
+          GetValueAsCFIndex(description, CFSTR(kIOPSTimeToEmptyKey), -1);
+      if (discharging_time != -1) {
+        status.dischargingTime =
+            base::TimeDelta::FromMinutes(discharging_time).InSeconds();
+      }
+    }
+  }
+}
+
+static void OnBatteryStatusChanged(BatteryCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  blink::WebBatteryStatus status;
+  base::ScopedCFTypeRef<CFTypeRef> blob(IOPSCopyPowerSourcesInfo());
+  base::ScopedCFTypeRef<CFArrayRef> powerSourcesList(

[Mark Mentovai, 2014/07/24 01:32:22]

Naming: power_sources_list.

[timvolodine, 2014/07/24 16:03:19]

Done.

+      IOPSCopyPowerSourcesList(blob));
+  CFIndex count = CFArrayGetCount(powerSourcesList);
+
+  for (CFIndex i = 0; i < count; ++i) {
+    CFDictionaryRef description = IOPSGetPowerSourceDescription(blob,
+        CFArrayGetValueAtIndex(powerSourcesList, i));

[Mark Mentovai, 2014/07/24 01:32:21]

  if (!description)
    continue;

can go here, you don’t need to wait until line 125 after you’ve done
GetValueAsBoolean().

[timvolodine, 2014/07/24 16:03:19]

Done.

+
+    bool battery_present = GetValueAsBoolean(description,
+                                             CFSTR(kIOPSIsPresentKey),
+                                             false);
+
+    if (!description || !battery_present)
+      continue;
+
+    FetchBatteryStatus(description, status);
+    break;

[Mark Mentovai, 2014/07/24 01:32:22]

In theory, there can be more than one battery. If you don’t handle that, you
should put a comment here. You may also want to remove the break without
handling multiple batteries fully. You can let the loop continue to process all
power sources so that if it finds more than one battery, it can DCHECK to alert
you (or some future person) that the code needs attention.

[timvolodine, 2014/07/24 16:03:19]

that's right, I've added a comment. I am reluctant to put a DCHECK, because if
Apple suddenly comes up with devices with multiple batteries chrome will start
crashing (in debug), which sounds a bit extreme.

[Mark Mentovai, 2014/07/24 16:08:52]

timvolodine wrote:
That’s exactly the point of making it crash: otherwise, chances are nobody would
recognize that this code needs to be updated, and it would report incorrect and
inappropriate results.

The point of doing a DCHECK instead of a CHECK is that only developer builds
would be affected, so Chrome won’t start crashing on end user systems.

[timvolodine, 2014/07/24 16:33:42]

the current specification does not strictly impose multiple battery support,
it's a recommendation. probably not worth a DCHECK. We can add this feature
later if necessary, I can add a reference to crbug in order to not forget this.

[Mark Mentovai, 2014/07/24 17:03:46]

timvolodine wrote:
This is an area where we know ahead of time that we haven’t accounted for
something that doesn’t currently exist but may spring into existence in the
future. It’s better to catch these things at runtime when the condition to
trigger them begins existing than to have some bug logged in a database where
everyone can forget about it.

Please add the DCHECK now.

[timvolodine, 2014/07/25 12:46:08]

Done.

+  }
+
+  callback.Run(status);
+}
+
+class BatteryStatusObserver
+    : public base::RefCountedThreadSafe<BatteryStatusObserver> {
+ public:
+  explicit BatteryStatusObserver(BatteryCallback& callback)
+      : callback_(callback) {}
+
+  void Start() {
+    // Need thread with UI-type message loop for notifications to run.

[Mark Mentovai, 2014/07/24 01:32:21]

If you added two more words here, it’d be a complete sentence, which would be
easier to understand.

[timvolodine, 2014/07/24 16:03:19]

Done.

+    if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+      StartOnUI();
+    } else {
+      BrowserThread::PostTask(
+          BrowserThread::UI,
+          FROM_HERE,
+          base::Bind(&BatteryStatusObserver::StartOnUI, this));
+    }
+  }
+
+  void Stop() {
+    if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+      StopOnUI();
+    } else {
+      BrowserThread::PostTask(
+          BrowserThread::UI,
+          FROM_HERE,
+          base::Bind(&BatteryStatusObserver::StopOnUI, this));
+    }
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<BatteryStatusObserver>;
+  virtual ~BatteryStatusObserver() { DCHECK(!notifier_run_loop_); }
+
+  void StartOnUI() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+    if (notifier_run_loop_)
+      return;
+
+    OnBatteryStatusChangedUI((void*)&callback_);

[Mark Mentovai, 2014/07/24 01:32:22]

Use cplusplus<style>(casts). See the style guide. Same on line 177.

[timvolodine, 2014/07/24 16:03:19]

Done.

+
+    notifier_run_loop_.reset(
+        IOPSNotificationCreateRunLoopSource(OnBatteryStatusChangedUI,
+                                            (void*)&callback_));
+    if (notifier_run_loop_) {

[Mark Mentovai, 2014/07/24 01:32:22]

Do you need this conditional? If IOPSNotificationCreateRunLoopSource() failed,
we probably want to crash. If we’re actually seeing it fail in practice for some
reason, then you should say something about that in a comment.

[timvolodine, 2014/07/24 16:03:19]

I think we should avoid crashing the browser, instead I've changed the code such
that it returns the default values, which corresponds to "cannot provide battery
data" in the specification.

+      CFRunLoopAddSource(CFRunLoopGetCurrent(), notifier_run_loop_,
+                         kCFRunLoopDefaultMode);
+    }
+  }
+
+  void StopOnUI() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+    if (!notifier_run_loop_)
+      return;
+
+    CFRunLoopRemoveSource(CFRunLoopGetCurrent(), notifier_run_loop_,
+                          kCFRunLoopDefaultMode);
+    notifier_run_loop_.reset();
+  }
+
+  static void OnBatteryStatusChangedUI(void* callback) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    // Offload fetching of values and callback execution to the IO thread.
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&OnBatteryStatusChanged,
+                   *static_cast<BatteryCallback*>(callback)));
+  }
+
+  BatteryCallback callback_;
+  base::ScopedCFTypeRef<CFRunLoopSourceRef> notifier_run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(BatteryStatusObserver);
+};
+
+class BatteryStatusManagerMac : public BatteryStatusManager {
+ public:
+  explicit BatteryStatusManagerMac(BatteryCallback& callback)
+      : notifier_(new BatteryStatusObserver(callback)) {}
+
+  virtual ~BatteryStatusManagerMac() {
+    notifier_->Stop();
+  }
+
+  // BatteryStatusManager:
+  virtual bool StartListeningBatteryChange() OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    notifier_->Start();
+    return true;
+  }
+
+  virtual void StopListeningBatteryChange() OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    notifier_->Stop();
+  }
+
+ private:
+  scoped_refptr<BatteryStatusObserver> notifier_;
+
+  DISALLOW_COPY_AND_ASSIGN(BatteryStatusManagerMac);
+};
+
+} // end namespace
+
+// static
+scoped_ptr<BatteryStatusManager> BatteryStatusManager::Create(
+    const BatteryStatusService::BatteryUpdateCallback& callback) {
+  return scoped_ptr<BatteryStatusManager>(
+      new BatteryStatusManagerMac(callback));
+}
+
+}  // namespace content