[chromeos] New PowerManagerClient observer to collect power data.

chromeos/power/power_data_collector.cc

+// Copyright (c) 2013 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 "chromeos/power/power_data_collector.h"
+
+#include "base/logging.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/power_manager/power_supply_properties.pb.h"
+
+namespace chromeos {
+
+namespace {
+// The global PowerDataCollector instance.
+PowerDataCollector* power_data_collector = NULL;
+

[Daniel Erat, 2013/12/12 01:31:52]

nit: i'd either remove this blank line or add another blank line after
"namespace {" to match it

[Siva Chandra, 2013/12/13 22:10:39]

Done.

+}  // Anonymous namespace

[Daniel Erat, 2013/12/12 01:31:52]

nit: s/Anonymous namespace/namespace/

[Siva Chandra, 2013/12/13 22:10:39]

Done.

+
+PowerDataCollector::PowerDataCollector() {
+  DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this);
+}
+
+PowerDataCollector::~PowerDataCollector() {
+  DBusThreadManager *dbus_manager = DBusThreadManager::Get();

[Daniel Erat, 2013/12/12 01:31:52]

nit: move '*' to left of space

[Siva Chandra, 2013/12/13 22:10:39]

Done.

+  CHECK(dbus_manager);
+  dbus_manager->GetPowerManagerClient()->RemoveObserver(this);
+}
+
+void PowerDataCollector::PowerChanged(
+    const power_manager::PowerSupplyProperties& prop) {
+  PowerSupplySnapshot snapshot;
+  snapshot.time = base::TimeTicks::Now();

[Daniel Erat, 2013/12/12 01:31:52]

you don't need it yet, but if/when you have tests that depend on the timestamps
of the recorded data, you'll probably either want to replace this call with one
to a class that lets you set the time for tests (i'm not sure if chrome has
something like this already; probably) or just add a |now_for_testing_| member
and a set_now_for_testing() setter that tests can use, and then change this code
to something like:

  snapshot.time = now_for_testing_.null() ?
      base::TimeTicks::Now() :
      now_for_testing_;

[Siva Chandra, 2013/12/13 22:10:39]

Thanks. Will keep this in mind. Your and Steven's comments used |<var name>|
when commenting. What does it mean?

[Daniel Erat, 2013/12/13 23:57:09]

this is the style usually used in chrome for referring to variable names within
comments. it makes them easier to understand when the variable name is a regular
word, e.g. "Toggles |active|." rather than "Toggles active."

+  snapshot.external_power = (prop.external_power() !=
+      power_manager::PowerSupplyProperties::DISCONNECTED);
+  snapshot.battery_charge = prop.battery_percent();
+
+  power_supply_data_.push_back(snapshot);
+}
+
+void PowerDataCollector::Initialize() {
+  // Check that power data collector is initialized only after the
+  // DBusThreadManager is initialized.
+  CHECK(DBusThreadManager::Get());
+  CHECK(power_data_collector == NULL);
+  power_data_collector = new PowerDataCollector();
+}
+
+PowerDataCollector *PowerDataCollector::Get() {

[Daniel Erat, 2013/12/12 01:31:52]

nit: move '*' to left of space

[Siva Chandra, 2013/12/13 22:10:39]

Done.

+  CHECK(power_data_collector);
+  return power_data_collector;
+}
+
+void PowerDataCollector::Shutdown() {
+  // Shutdown only if initialized.
+  CHECK(power_data_collector);
+  delete power_data_collector;
+  // NULL it to protect from improper usage after shutdown.

[Daniel Erat, 2013/12/12 01:31:52]

nit: remove unnecessary comment

+  power_data_collector = NULL;
+}
+
+PowerDataCollector::PowerSupplySnapshot::PowerSupplySnapshot()
+    : time(base::TimeTicks::Now()), external_power(false), battery_charge(0) {

[Daniel Erat, 2013/12/12 01:31:52]

nit: one parameter per line in initialization list if they don't all fit on the
same line as the c'tor signature 
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...)

[Siva Chandra, 2013/12/13 22:10:39]

Done.

+}
+
+}  // namespace chromeos