Have desktop Macs report not being on battery power

Bad logic in PowerMonitorDeviceSource::IsOnBatteryPowerImpl()
meant that Macs with no batteries were reported as being on
battery power.

Fixed: 1132422
Change-Id: Ifa174a0164c4d4d31bdd2dc494b2b7f83fd48bca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2432765
Commit-Queue: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Auto-Submit: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811893}

base/power_monitor/power_monitor_device_source_mac.mm

@@ -25,9 +25,14 @@ void ProcessPowerEventHelper(PowerMonitorSource::PowerEvent event) {
 
 bool PowerMonitorDeviceSource::IsOnBatteryPowerImpl() {
   base::ScopedCFTypeRef<CFTypeRef> info(IOPSCopyPowerSourcesInfo());
+  if (!info)
+    return false;
   base::ScopedCFTypeRef<CFArrayRef> power_sources_list(
       IOPSCopyPowerSourcesList(info));
+  if (!power_sources_list)
+    return false;
 
+  bool found_battery = false;
   const CFIndex count = CFArrayGetCount(power_sources_list);
   for (CFIndex i = 0; i < count; ++i) {
     const CFDictionaryRef description = IOPSGetPowerSourceDescription(
@@ -37,18 +42,29 @@ bool PowerMonitorDeviceSource::IsOnBatteryPowerImpl() {
 
     CFStringRef current_state = base::mac::GetValueFromDictionary<CFStringRef>(
         description, CFSTR(kIOPSPowerSourceStateKey));
-
     if (!current_state)
       continue;
 
     // We only report "on battery power" if no source is on AC power.
-    if (CFStringCompare(current_state, CFSTR(kIOPSBatteryPowerValue), 0) !=
+    if (CFStringCompare(current_state, CFSTR(kIOPSOffLineValue), 0) ==
         kCFCompareEqualTo) {
+      continue;
+    } else if (CFStringCompare(current_state, CFSTR(kIOPSACPowerValue), 0) ==
+               kCFCompareEqualTo) {
       return false;
     }
+
+    DCHECK_EQ(CFStringCompare(current_state, CFSTR(kIOPSBatteryPowerValue), 0),
+              kCFCompareEqualTo)
+        << "Power source state is not one of 3 documented values";
+
+    found_battery = true;
   }
 
-  return true;
+  // At this point, either there were no readable batteries found, in which case
+  // this Mac is not on battery power, or all the readable batteries were on
+  // battery power, in which case, count this as being on battery power.
+  return found_battery;
 }
 
 PowerObserver::DeviceThermalState

chrome/browser/metrics/power_metrics_provider_mac.mm

@@ -435,7 +435,7 @@ class PowerMetricsProvider::Impl : public base::RefCountedThreadSafe<Impl> {
     bool is_on_battery = false;
     if (base::PowerMonitor::IsInitialized())
       is_on_battery = base::PowerMonitor::IsOnBatteryPower();
-    UMA_HISTOGRAM_BOOLEAN("Power.Mac.IsOnBattery", is_on_battery);
+    UMA_HISTOGRAM_BOOLEAN("Power.Mac.IsOnBattery2", is_on_battery);
   }
 
   void RecordThermal() API_AVAILABLE(macos(10.10.3)) {

tools/metrics/histograms/histograms_xml/power/histograms.xml

@@ -708,9 +708,25 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
 
 <histogram name="Power.Mac.IsOnBattery" enum="BooleanOnBattery"
     expires_after="2021-02-21">
+  <obsolete>
+    Replaced with Power.Mac.IsOnBattery2 09/2020 as it was broken.
+  </obsolete>
   <owner>lgrey@chromium.org</owner>
   <summary>
     Whether the user's machine is on battery power. Sampled once per minute.
+    Warning: This incorrectly logs desktop Macs as being on battery power.
+    Replaced by Power.Mac.IsOnBattery2.
+  </summary>
+</histogram>
+
+<histogram name="Power.Mac.IsOnBattery2" enum="BooleanOnBattery"
+    expires_after="2021-02-21">
+  <owner>avi@chromium.org</owner>
+  <owner>lgrey@chromium.org</owner>
+  <summary>
+    Whether the user's machine is on battery power. Sampled once per minute.
+    Unlike Power.Mac.IsOnBattery, this correctly logs desktop Macs as not being
+    on battery.
   </summary>
 </histogram>