Chrome power profiler service

content/public/browser/power_data_provider.h

Index: content/public/browser/power_data_provider.h
diff --git a/content/public/browser/power_data_provider.h b/content/public/browser/power_data_provider.h
new file mode 100644
index 0000000000000000000000000000000000000000..76cb6f6f0533beb3d8581b033eb47b643145f6b5
--- /dev/null
+++ b/content/public/browser/power_data_provider.h
@@ -0,0 +1,57 @@
+// 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 CONTENT_PUBLIC_BROWSER_POWER_DATA_PROVIDER_H_
+#define CONTENT_PUBLIC_BROWSER_POWER_DATA_PROVIDER_H_
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time/time.h"
+
+namespace content {
+
+struct PowerEvent {
+  enum Type {

[qsr, 2014/01/27 10:01:47]

You should remove Type. Right now you have a single type, so it doesn't give
anything. Moreover, your API for the provider doesn't handle multiple type (you
are secifying you are getting back a single PowerEvent for each call to the
provider), nor does the definition of the |value| field (do I need to keep the
last value for each type to be able to get anything, or something else?)

 So, right now, remove this, and when we have a provider that returns multiple
type of power event, we will refactor this to handle it.

[Pan, 2014/01/28 09:58:04]

Yes, it is a little weird to have only one type in the enum.
Previously, I left 3 types there, and GetData() aims to fill all 3 type event,
but seems not all of them are useful, so only SOC_PACKAGE is provided.
I agree to remove Type, while, a little concern is that, the values that
provider support is always associated with a type. E.g, currently, SOC_PACKAGE
power contains power consumes in SoC, including CPU, GPU etc, but it doesn't
contains modules out of SoC, such as screen, so the values doesn't mean
"battery". 
So if we remove the type here, I suggest a comment in the service to say, it is
SoC_Package power, when other type added, resume Type, make sense?

thanks
Pan

+    // Total power of SoC. including CPU, GT and others on the chip.
+    SOC_PACKAGE,
+
+    // Count the number of known PowerEvent.
+    ID_COUNT
+  };
+
+  Type type;
+  base::TimeTicks time; // Time that power data was read.
+
+  // Power value between last event to this one, in watt,

[qsr, 2014/01/27 10:01:47]

What does Power value mean? Power consumption might be a better comment. Also
specify what the sign means as I guess negative means the device is charging.

[Pan, 2014/01/28 09:58:04]

For SOC_PACKAGE power, it means power consumed in SoC, so it is always a
positive one.
Scenario of events is:
e1 {t1,v1};
e2 {t2, v2};
e3 {t3, v3};
once an observer registered, it got e1 immediately, after a delay_, it is e2,
then e3.
v2 means the power(watt) between t1 and t2, v3 means power between v2 and t3.
For v1, it may mean power between "last time you read" and t1, may mean nothing,
so just forget it.
I will document this.

+  // so value of the first event that observer received should be ignored.
+  double value;
+};
+
+// a class used to GET power usage.
+class PowerDataProvider {
+ public:
+  PowerDataProvider() { }

[qsr, 2014/01/27 10:01:47]

No spaces inside {}, here and everywhere on this CL.

[Pan, 2014/01/28 13:32:58]

Done.

+  virtual ~PowerDataProvider() { }
+
+  // Return true when success, it fills a single PowerEvent, which should be
+  // allocated by its caller.
+  virtual bool GetData(PowerEvent*) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PowerDataProvider);
+};
+
+class PowerDataProviderFactory {
+ public:
+  // Create a provider, and transfer the ownership to PowerProfilerService.
+  static scoped_ptr<PowerDataProvider> Create();
+
+ private:
+  PowerDataProviderFactory() { }
+  DISALLOW_COPY_AND_ASSIGN(PowerDataProviderFactory);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_POWER_DATA_PROVIDER_H_