Battery impact UMA

content/browser/power_usage_monitor_impl.h

Index: content/browser/power_usage_monitor_impl.h
diff --git a/content/browser/power_usage_monitor_impl.h b/content/browser/power_usage_monitor_impl.h
new file mode 100644
index 0000000000000000000000000000000000000000..2bcdd95cdf093796144c025d708eca1c5fa6fc05
--- /dev/null
+++ b/content/browser/power_usage_monitor_impl.h
@@ -0,0 +1,125 @@
+// 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.
+
+#ifndef CHROME_CONTENT_BROWSER_POWER_USAGE_MONITOR_IMPL_H_
+#define CHROME_CONTENT_BROWSER_POWER_USAGE_MONITOR_IMPL_H_
+
+#include "base/basictypes.h"
+#include "base/containers/hash_tables.h"
+#include "base/memory/singleton.h"
+#include "base/power_monitor/power_monitor.h"
+#include "base/time/time.h"
+#include "content/browser/battery_status/battery_status_service.h"
+#include "content/public/browser/browser_message_filter.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "third_party/WebKit/public/platform/WebBatteryStatus.h"
+
+namespace content {
+
+// Record statistics on power usage.
+//
+// Two main statics are recorded by this class:
+// * Power.BatteryDischarge_{5,15,30} - delta between battery level when
+//   unplugged from wallpower, over the specified period - in minutes.
+// * Power.BatteryDischargeRateWhenUnplugged - the rate of battery discharge
+//   from the device being unplugged until it's plugged back in, if said period
+//   was longer than 30 minutes.
+//
+// Heuristics:
+// * Metrics are only collected once per day.
+// * If the machine goes to sleep or all renderers are closed then the current
+//   measurement is cancelled.
+//
+// Threading: Most methods are called on the IO thread.

[Avi (use Gerrit), 2014/10/21 15:12:49]

This is not a helpful comment. Don't use descriptive language (e.g. "this is
called on this thread") in an API. Use prescriptive language (e.g. "call this
function on this thread").

Something like "Methods must be called on the IO thread unless otherwise noted"
would be helpful. And then you'd have to mark the methods that require other
thread uses.

[jeremy, 2014/10/23 14:32:47]

Thanks, that is indeed clearer - I've removed the comment and added DCHECK()s to
anotate.

+class PowerUsageMonitor : public base::PowerObserver, NotificationObserver {

[Daniel Erat, 2014/10/21 16:01:21]

add 'public' to NotificationObserver too (not sure whether it'll go back to
private by default, but better to be explicit)

[jeremy, 2014/10/23 14:32:47]

Done.

+ public:
+  class SystemInterface {
+   public:
+    SystemInterface();
+    virtual ~SystemInterface();
+
+    void SetPowerUsageMonitor(PowerUsageMonitor* monitor) {

[Daniel Erat, 2014/10/21 16:01:21]

set_power_usage_monitor

[jeremy, 2014/10/23 14:32:47]

Done.

+      power_usage_monitor_ = monitor;
+    }
+
+    virtual void ScheduleHistogramReport(base::TimeDelta delay);

[Daniel Erat, 2014/10/21 16:01:21]

make this class be a pure interface, please

[jeremy, 2014/10/23 14:32:47]

Done.

+    virtual void CancelPendingHistgramReports();

[Daniel Erat, 2014/10/21 16:01:21]

s/Histgram/Histogram/

[jeremy, 2014/10/23 14:32:47]

Done.

+
+    // Allow tests to override clock.
+    virtual base::Time Now();
+
+   protected:
+    virtual void ReportBatteryLevelHistogram(base::TimeDelta discharge_time);
+
+   private:
+    PowerUsageMonitor* power_usage_monitor_;

[Daniel Erat, 2014/10/21 16:01:21]

document that this isn't owned

[jeremy, 2014/10/23 14:32:47]

Done.

+
+    // Used to cancel in progress timers.
+    base::WeakPtrFactory<SystemInterface> weak_ptr_factory_;
+  };
+
+ public:
+  PowerUsageMonitor();
+  virtual ~PowerUsageMonitor();
+
+  // Start monitoring power usage.
+  void Start();
+  void StartOnIOThread();
+
+  void OnBatteryStatusUpdate(const blink::WebBatteryStatus& status);
+
+  double DischargeAmount() {
+    return initial_battery_level_ - current_battery_level_;
+  }
+
+  // Called by SystemInterface after a histogram was succesfully recorded.
+  void BatteryLevelReported();
+
+  void SetSystemInterfaceForTest(scoped_ptr<SystemInterface> recorder);

[Daniel Erat, 2014/10/21 16:01:21]

s/recorder/interface/

[jeremy, 2014/10/23 14:32:47]

Done.

+  void SetNumLiveRenderersForTest(int num_renderers);
+
+  // Overridden from base::PowerObserver:
+  virtual void OnPowerStateChange(bool on_battery_power) override {};
+  virtual void OnResume() override {};
+  virtual void OnSuspend() override;

[Avi (use Gerrit), 2014/10/21 15:12:50]

The new style is to drop the "virtual" and only have "override". I hope that at
this point the clang plugin is updated to allow this.

[jeremy, 2014/10/23 14:32:46]

Done.

+
+  // Overridden from NotificationObserver:
+  virtual void Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) override;
+
+ private:
+  void ReportBatteryLevelHistogram(base::Time discharge_time);
+  void DischargeStarted(double battery_level);
+  void WallPowerConnected(double battery_level);
+
+  void CancelPendingHistogramRecordingOnIOThread();
+
+  BatteryStatusService::BatteryUpdateCallback callback_;
+  scoped_ptr<BatteryStatusService::BatteryUpdateSubscription> subscription_;
+
+  NotificationRegistrar registrar_;
+
+  scoped_ptr<SystemInterface> system_interface_;
+
+  bool was_on_battery_power_;

[Daniel Erat, 2014/10/21 16:01:21]

add a comment describing what this represents -- when was it on battery power?

[jeremy, 2014/10/23 14:32:47]

Done.

+  double initial_battery_level_;

[Daniel Erat, 2014/10/21 16:01:21]

initial when -- when the object was first created?

[jeremy, 2014/10/23 14:32:47]

Done.

+  double current_battery_level_;
+  base::Time start_discharge_time_;
+
+  // Timestamp for when the first histogram is reported in a 24 hour period.
+  // Used for rate-limiting.
+  base::Time first_histogram_report_timestamp_;
+
+  base::hash_set<int> live_renderer_ids_;
+  size_t num_live_renderers_;

[Avi (use Gerrit), 2014/10/21 15:12:50]

It bothers me that we have num_live_renderers_ as a separate variable to
live_renderer_ids_ even though in normal use it must always be equal to
live_renderer_ids_.size(). Is there a way to remove this redundant variable?

[Daniel Erat, 2014/10/21 16:01:21]

agreed; please remove this.

[jeremy, 2014/10/23 14:32:47]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PowerUsageMonitor);
+};
+
+}  // namespace content
+
+#endif  // CHROME_CONTENT_BROWSER_POWER_USAGE_MONITOR_IMPL_H_