[chromeos] New PowerManagerClient observer to collect power data.

chromeos/power/power_data_collector.h

Index: chromeos/power/power_data_collector.h
diff --git a/chromeos/power/power_data_collector.h b/chromeos/power/power_data_collector.h
new file mode 100644
index 0000000000000000000000000000000000000000..3c40b67a4e2cf3909d086159bccbe9417336421f
--- /dev/null
+++ b/chromeos/power/power_data_collector.h
@@ -0,0 +1,98 @@
+// 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.
+
+#ifndef CHROMEOS_DBUS_POWER_DATA_COLLECTOR_H_

[Daniel Erat, 2013/12/11 01:06:52]

nit: update these to reflect the new path (don't forget the #endif at the end of
the file)

[Siva Chandra, 2013/12/11 20:32:02]

Done.

+#define CHROMEOS_DBUS_POWER_DATA_COLLECTOR_H_
+
+#include <vector>
+
+#include "base/gtest_prod_util.h"
+#include "base/synchronization/lock.h"
+#include "chromeos/dbus/power_manager_client.h"
+
+namespace power_manager {
+class PowerSupplyProperties;
+}
+
+namespace chromeos {
+
+// A class which starts collecting power metrics, like the battery charge, as
+// soon as it is initialized via Initialize().
+//
+// The constructor for this class is private as we want only a single global
+// instance to be created via Initialize(). Also, Initialize() should be called
+// only after the DBusThreadManager is initialized. The single global instance
+// should be destroyed via Shutdown(), and before the DBusThreadManager is
+// shutdown.
+class PowerDataCollector
+    : public PowerManagerClient::Observer,
+      public base::RefCountedThreadSafe<PowerDataCollector> {
+ public:
+  struct PowerSupplySnapshot {
+    // Generated by Time::ToInternalValue.
+    int64 time;

[Daniel Erat, 2013/12/11 01:06:52]

why not just use base::TimeTicks here? the internal value is there for
serialization

[Siva Chandra, 2013/12/11 20:32:02]

Done.

How does one convert time ticks to human understandable time deltas?

[Daniel Erat, 2013/12/11 20:59:20]

base::TimeDelta is a class that represents an interval of time (e.g. 5 seconds,
1 hour, etc.). you can add and subtract TimeDeltas from Times and TimeTicks.

base::TimeTicks represents a point in time since the system booted. it's based
on the kernel's mononotically-increasing clock, so it won't e.g. go backwards if
the system clock gets adjusted.

base::Time represents a point in time based on the system clock (a.k.a. "wall
time"). it can go backwards.

in general, you should use TimeTicks for everything. you can get a TimeDelta by
subtracting one TimeTicks from another TimeTicks and then convert that to a
human-understandable interval of time.

+
+    // true if connected to externel power at the time of the snapshot.

[Daniel Erat, 2013/12/11 01:06:52]

nit: s/externel/external/

[Siva Chandra, 2013/12/11 20:32:02]

Done.

+    bool external_power;
+
+    // The battery charge as a percentage of full charge in range [0.0, 100.00].
+    double battery_charge;
+  };
+
+ public:
+  // Can be called only after DBusThreadManager is initialized.
+  static void Initialize();
+
+  // Can be called only if initialized via Initialize, and before
+  // DBusThreadManager is destroyed.
+  static void Shutdown();
+
+  // Call this when processing the power supply data got from

[Daniel Erat, 2013/12/11 01:06:52]

nit: s/got/received/

[Siva Chandra, 2013/12/11 20:32:02]

Done.

+  // GetPowerSupplyData. Call UnlockPowerSupplyData after done with processing.
+  static void LockPowerSupplyData(void);

[Daniel Erat, 2013/12/11 01:06:52]

this lock/unlock mechanism seems error-prone. why is it necessary? are you
planning to access this data from outside of the UI thread? are you planning to
hang onto the reference for an extended period of time? if the answer to both of
those questions is "no", i don't see why the lock is necessary.

[Siva Chandra, 2013/12/11 20:32:02]

Why is it error-prone?
Correct me if I am wrong in my understanding here.

My understanding here is that UI thread (which is a process actually) has a
number of threads running. So, in our case, we want the UI thread displaying the
about:power page be able to read the data being being accumulated by the power
data collector. So, potentially, a read from the UI thread could be happening
while there is a write happening via the DBusThreadManager.
What is "extended period of time". In my opinion, if there is a possibility of
the vector's size being updated while someone is reading it, then we should have
synchronization.

[Daniel Erat, 2013/12/11 20:59:20]

it is easy for a caller to acquire the lock and later forget to release it is.
it is easy to accidentally introduce deadlocks.
there is a browser process with a UI thread (and some other threads).
DBusThreadManager notifies observers on the UI thread (so that is where
PowerChanged() will run). the code that IPCs the data to javascript will also be
running on the UI thread (so that is where GetPowerSupplyData() will run). all
of these things run as tasks on the same message loop; there's no need for
synchronization here.
there is no possibility of this happening unless whatever is going to call
GetPowerSupplyData() needs to hang on to the data and continue accessing it
after control returns to the message loop (which you shouldn't need to do).

+
+  // Returns a reference to a vector of power supply data. Invoke this function
+  // after locking the data with LockPowerSupplyData, and unlock after done
+  // processing the data with UnlockPowerSupplyData.
+  static std::vector<PowerSupplySnapshot> &GetPowerSupplyData(void);

[Daniel Erat, 2013/12/11 01:06:52]

instead of having a bunch of static methods, i think that the typical pattern is
to have a single static Get() method that returns the singleton and implement
everything else as instance methods

[Siva Chandra, 2013/12/11 20:32:02]

Done.

+
+  // Call this after done with processing the power supply data got from
+  // &GetPowerSupplyData.
+  static void UnlockPowerSupplyData(void);
+
+ private:
+  friend class base::RefCountedThreadSafe<PowerDataCollector>;
+  friend class PowerDataCollectorTest;
+  FRIEND_TEST_ALL_PREFIXES(PowerDataCollectorTest, AppendPowerSupplySnapshot);
+
+  // When the 'testing' argument is 'true', the instance is created without
+  // checking for an initialized DBusThreadManager and hence it does not listen
+  // to any PowerManagerClient notifications.
+  explicit PowerDataCollector(const bool testing);
+
+  virtual ~PowerDataCollector() {}
+
+  // Used in tests. Initializes the PowerDataCollector so that unit tests
+  // can be performed without depending on the DBusThreadManager.
+  static void InitializeTesting();
+
+  // Used in tests. Returns the global instance of PowerDataCollector.
+  static PowerDataCollector *Get();
+
+  virtual void PowerChanged(const power_manager::PowerSupplyProperties& prop);

[Daniel Erat, 2013/12/11 01:06:52]

list virtual methods after other methods, add "// PowerManagerClient::Observer
implementation:" comment, and use OVERRIDE (see any other chrome classes for an
example)

[Siva Chandra, 2013/12/11 20:32:02]

Done.

+  void AppendPowerSupplySnapshot(const PowerSupplySnapshot& snapshot);
+  void Shutdown_Instance(void);

[Daniel Erat, 2013/12/11 01:06:52]

google c++ naming style would be ShutdownInstance

[Siva Chandra, 2013/12/11 20:32:02]

Done.

+
+  // Is true for testing instance.
+  bool testing_;
+
+  base::Lock power_supply_lock_;
+  std::vector<PowerSupplySnapshot> power_supply_;
+
+  DISALLOW_COPY_AND_ASSIGN(PowerDataCollector);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_DBUS_POWER_DATA_COLLECTOR_H_