[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 "base/logging.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/power_manager/power_supply_properties.pb.h"
+#include "chromeos/power/power_data_collector.h"

[Daniel Erat, 2013/12/11 22:26:47]

move this include to the top of the list and separate it from the subsequent
includes with a blank line (to match the rest of chrome)

[Siva Chandra, 2013/12/12 00:35:59]

Done.

+
+namespace chromeos {
+
+// The global PowerDataCollector instance.
+static PowerDataCollector* power_data_collector;

[Daniel Erat, 2013/12/11 22:26:47]

put this in a nested anonymous namespace instead of using 'static' and
explicitly initialize it to NULL

[Siva Chandra, 2013/12/12 00:35:59]

Done.

+
+PowerDataCollector::PowerDataCollector(const bool testing)
+    : testing_(testing) {
+  AddRef();

[Daniel Erat, 2013/12/11 22:26:47]

delete this

[Siva Chandra, 2013/12/12 00:35:59]

Done.

+
+  if (!testing) {
+    DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this);

[Daniel Erat, 2013/12/11 22:26:47]

i don't think you need this |testing| flag; can't you just use the mock or stub
DBusThreadManager when running on tests?

[Siva Chandra, 2013/12/12 00:35:59]

Done.

+  }
+}
+
+void PowerDataCollector::ShutdownInstance() {

[Daniel Erat, 2013/12/11 22:26:47]

(mentioned earlier: put this in the destructor instead)

[Siva Chandra, 2013/12/12 00:35:59]

Done.

+  if (!testing_) {
+    DBusThreadManager *dbus_manager = DBusThreadManager::Get();
+
+    // Destroy the power data collector before the DBusThreadManager.
+    CHECK(dbus_manager);
+
+    dbus_manager->GetPowerManagerClient()->RemoveObserver(this);
+  }
+
+  Release();

[Daniel Erat, 2013/12/11 22:26:47]

delete this

[Siva Chandra, 2013/12/12 00:35:59]

Done.

+}
+
+void PowerDataCollector::PowerChanged(
+    const power_manager::PowerSupplyProperties& prop) {
+  PowerSupplySnapshot snapshot = {

[Daniel Erat, 2013/12/11 22:26:47]

nit: this would be a bit clearer:

PowerSupplySnapshot snapshot;
snapshot.time = base::TimeTicks::Now();
etc.

[Siva Chandra, 2013/12/12 00:35:59]

Done.

+      base::TimeTicks::Now(),
+      prop.external_power() !=
+          power_manager::PowerSupplyProperties::DISCONNECTED,
+      prop.battery_percent()
+  };
+
+  power_supply_.push_back(snapshot);
+}
+
+void PowerDataCollector::Initialize() {
+  // Check that power data collector is initialized only after the
+  // DBusThreadManager is initialized.
+  CHECK(DBusThreadManager::Get());
+
+  // Do no allow intializing twice.

[Daniel Erat, 2013/12/11 22:26:47]

nit: s/no/not/

(but i don't think that this comment is necessary; i'd probably just delete it
and remove the blank lines between the three statements in this method)

[Siva Chandra, 2013/12/12 00:35:59]

Done.

+  CHECK(power_data_collector == NULL);
+
+  power_data_collector = new PowerDataCollector(false);
+}
+
+void PowerDataCollector::InitializeTesting() {

[Daniel Erat, 2013/12/11 22:26:47]

remove this (see above)

[Siva Chandra, 2013/12/12 00:35:59]

Done.

+  power_data_collector = new PowerDataCollector(true);
+}
+
+PowerDataCollector *PowerDataCollector::Get() {
+  CHECK(power_data_collector);
+
+  return power_data_collector;
+}
+
+void PowerDataCollector::Shutdown() {
+  // Shutdown only if initialized.
+  CHECK(power_data_collector);
+
+  power_data_collector->ShutdownInstance();
+
+  // NULL it to protect from improper usage after shutdown.

[Daniel Erat, 2013/12/11 22:26:47]

nit: delete unnecessary comments and blank lines here too

[Siva Chandra, 2013/12/12 00:35:59]

Done.

+  power_data_collector = NULL;
+}
+
+std::vector<PowerDataCollector::PowerSupplySnapshot>
+&PowerDataCollector::GetPowerSupplyData() {
+  // Do not allow calling this method after shutdown or before initialization.
+  CHECK(power_data_collector);
+
+  return power_data_collector->power_supply_;

[Daniel Erat, 2013/12/11 22:26:47]

(see earlier comment about just making this be an inlined accessor, but note
that in general, you'd want to just return |power_supply_| here instead of
having an instance method return data from a global instance instead of from
itself)

[Siva Chandra, 2013/12/12 00:35:59]

My bad.
Fixed.

+}
+
+}  // namespace chromeos