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/memory/ref_counted.h"
+#include "base/threading/thread.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,
+                                 CFStringRef key,
+                                 CFIndex default_value) {
+  CFNumberRef value_number =
+      (CFNumberRef)CFDictionaryGetValue(description, key);

[Mark Mentovai, 2014/07/17 14:08:31]

CFCast<CFNumberRef>.

[Robert Sesek, 2014/07/17 15:16:41]

Better yet: base::mac::GetValueFromDictionary<T>(dictionary, key).

[timvolodine, 2014/07/22 18:04:43]

Done.

[timvolodine, 2014/07/22 18:04:43]

Done.

+  CFIndex value;
+
+  if (value_number &&
+      CFNumberGetValue(value_number, kCFNumberCFIndexType, &value))
+    return value;
+
+  return default_value;
+}
+
+static void FetchBatteryStatus(CFDictionaryRef description,
+                               blink::WebBatteryStatus& status) {
+  CFStringRef currentState =
+      (CFStringRef)CFDictionaryGetValue(description,

[Mark Mentovai, 2014/07/17 14:08:31]

CFCast<CFStringRef>.

[timvolodine, 2014/07/22 18:04:44]

Done.

+                                        CFSTR(kIOPSPowerSourceStateKey));
+  bool on_battery_power =
+      CFStringCompare(currentState, CFSTR(kIOPSBatteryPowerValue), 0)
+      == kCFCompareEqualTo;
+  bool is_charging =
+      CFDictionaryGetValue(description, CFSTR(kIOPSIsChargingKey))

[Mark Mentovai, 2014/07/17 14:08:31]

This just tells you if kIOPSIsChargingKey was found in the dictionary, it
doesn’t tell you what its value is. Right now, I see

    | |           "FullyCharged" = Yes
[…]
    | |           "IsCharging" = No

By your logic here, both is_charging and is_charged would be true.

[timvolodine, 2014/07/22 18:04:44]

I've changed this to use CFBooleanGetValue, as you suggested in one of the later
comments. However the initial code works as well, I suspect that is because
CFDictionaryGetValue() returns a pointer which equals to kCFBooleanTrue or
kCFBooleanFalse if present.

+      == kCFBooleanTrue;
+  bool is_charged =
+      CFDictionaryGetValue(description, CFSTR(kIOPSIsChargedKey))
+      == kCFBooleanTrue;
+
+  // Set charging to false if on battery power and not charging.

[Mark Mentovai, 2014/07/17 14:08:31]

Why not set status.charging to is_charging?

Why haven’t you set status.charging to true if it’s charging?

[timvolodine, 2014/07/22 18:04:44]

yes this is on purpose. I've added a comment explaining the default value. The
idea is to set fields only when different from the default value.
Also, status.charging has slightly different semantics as compared to
is_charging, it is defined as being false only when battery is discharging and
true otherwise.

+  if (on_battery_power && !is_charging)
+    status.charging = false;
+
+  CFIndex current_capacity =
+      GetValueAsCFIndex(description, CFSTR(kIOPSCurrentCapacityKey), -1);
+  CFIndex max_capacity =
+      GetValueAsCFIndex(description, CFSTR(kIOPSMaxCapacityKey), -1);
+
+  if (current_capacity != -1 && max_capacity != -1)

[Mark Mentovai, 2014/07/17 14:08:31]

&& current_capacity <= max_capacity?
&& max_capacity != 0?

[timvolodine, 2014/07/22 18:04:44]

Done.

+    status.level = (double)current_capacity/(double)max_capacity;

[Mark Mentovai, 2014/07/17 14:08:31]

Don’t use (C_style)casts, use Cplusplus<style>(casts). These can be static_cast.
Actually, only one of them needs a static_cast.

But put some spaces around the / operator, so people can see what it’s operating
on.

[timvolodine, 2014/07/22 18:04:43]

Done.

+
+  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)

[Mark Mentovai, 2014/07/17 14:08:31]

WebBatteryStatus.h doesn’t have comments in it to help me understand the meaning
of the fields. I might have assumed that chargingTime meant “time already spent
charging.” I also don’t know if there’s an agreed-upon standard for the use of
infinity, how other platforms deal with this, etc. Where’s the documentation?

[timvolodine, 2014/07/22 18:04:44]

The specification is here:
https://dvcs.w3.org/hg/dap/raw-file/tip/battery/Overview.html. I'll probably add
some comments to WebBatteryStatus.h in a separate patch.

+        ? 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.
+    if (!is_charged)
+      status.chargingTime = std::numeric_limits<double>::infinity();
+
+    // Set dischargingTime if it's available and valid, i.e. when on battery
+    // power.
+    if (on_battery_power) {
+      CFIndex discharging_time =
+          GetValueAsCFIndex(description, CFSTR(kIOPSTimeToEmptyKey), -1);
+      if (discharging_time != -1) {
+        status.dischargingTime =

[Mark Mentovai, 2014/07/17 14:08:31]

Should you be setting status.dischargingTime to some value always?

Same for the other members of status. I only see you setting the values
sometimes. What will they contain otherwise? Garbage?

[timvolodine, 2014/07/22 18:04:44]

no garbage ;). I've added some comments regarding default values to make it more
understandable.

+            base::TimeDelta::FromMinutes(discharging_time).InSeconds();
+      }
+    }
+  }
+}
+
+static void OnBatteryStatusChanged(BatteryCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  blink::WebBatteryStatus status;
+  CFTypeRef blob = IOPSCopyPowerSourcesInfo();

[Mark Mentovai, 2014/07/17 14:08:31]

Do you need to release this or does IOPSCopyPowerSourcesList take ownership of
it? I suspect that you need to release it. In that case, use ScopedCFTypeRef.

[timvolodine, 2014/07/22 18:04:44]

you are right, it needs to be released. added ScopedCFTypeRef.

+  CFArrayRef powerSourcesList = IOPSCopyPowerSourcesList(blob);

[Mark Mentovai, 2014/07/17 14:08:31]

This one almost definitely needs a ScopedCFTypeRef.

In CoreFoundaton-land, things with Copy in their name mean that a new copy is
made that you need to take ownership of and dispose of when you’re done with.

[timvolodine, 2014/07/22 18:04:44]

added ScopedCFTypeRef, done.

+  CFIndex count = CFArrayGetCount(powerSourcesList);
+
+  for (CFIndex i = 0; i < count; ++i) {
+    CFDictionaryRef description = IOPSGetPowerSourceDescription(blob,
+        CFArrayGetValueAtIndex(powerSourcesList, i));
+
+    bool battery_present =
+        CFDictionaryGetValue(description, CFSTR(kIOPSIsPresentKey))

[Mark Mentovai, 2014/07/17 14:08:31]

CFDictionaryGetValue doesn’t return kCFBooleanTrue or kCFBooleanFalse, it
returns a CFBooleanRef. Use CFBooleanGetValue to get the value of the boolean.

[timvolodine, 2014/07/22 18:04:44]

Done. (but it looks like it actually returns a pointer which has the value of
kCFBoolean{True,False}).

+        == kCFBooleanTrue;
+
+    if (!description || !battery_present)
+      continue;
+
+    FetchBatteryStatus(description, status);
+    break;
+  }
+
+  callback.Run(status);
+}
+
+class BatteryStatusObserver
+    : public base::RefCountedThreadSafe<BatteryStatusObserver> {
+ public:
+  explicit BatteryStatusObserver(BatteryCallback& callback)
+      : callback_(callback), notifier_run_loop_(0) {}

[Mark Mentovai, 2014/07/17 14:08:31]

Use NULL for null CFTypeRef-family values here and on line 179.

[timvolodine, 2014/07/22 18:04:43]

not necessary anymore because I made notifier_run_loop_ ScopedCFTypeRef as well.

+
+  void Start() {
+    // Need thread with UI-type message loop for notifications to run.
+    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() {}

[Mark Mentovai, 2014/07/17 14:08:31]

[D]CHECK that it’s stopped (by notifier_run_loop_ being NULL.

[timvolodine, 2014/07/22 18:04:44]

Done.

+
+  void StartOnUI() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+    if (notifier_run_loop_)

[Mark Mentovai, 2014/07/17 14:08:31]

Might this actually happen in practice? It may be better as a [D]CHECK. Same in
StopOnUI, line 173.

[timvolodine, 2014/07/22 18:04:44]

I don't think we should be that strict, users may invoke multiple start/stop
without any issues. Also the if-check in StopOnUI handles the case if we failed
to create the notifier_run_loop_ in StartOnUI for some reason.

+      return;
+
+    OnBatteryStatusChangedUI((void*)&callback_);
+
+    notifier_run_loop_ =
+        IOPSNotificationCreateRunLoopSource(OnBatteryStatusChangedUI,
+                                            (void*)&callback_);
+    if (notifier_run_loop_) {
+      CFRunLoopAddSource(CFRunLoopGetCurrent(), notifier_run_loop_,
+                         kCFRunLoopDefaultMode);
+    }
+  }
+
+  void StopOnUI() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+    if (!notifier_run_loop_)
+      return;
+
+    CFRunLoopRemoveSource(CFRunLoopGetCurrent(), notifier_run_loop_,
+                          kCFRunLoopDefaultMode);
+    CFRelease(notifier_run_loop_);
+    notifier_run_loop_ = 0;
+  }
+
+  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_;

[Robert Sesek, 2014/07/17 15:16:41]

Why is this stored by reference?

[timvolodine, 2014/07/22 18:04:43]

technically this is ok because the callback is owned by BatteryStatusService.
However I've changed this to value semantics now because it arguably looks
better and is also more extendable.

+  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