chrome power profiler chrome side changes

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..c362ebc2c7da80cb9b30d3eeb2325ac42c17df70
--- /dev/null
+++ b/content/public/browser/power_data_provider.h
@@ -0,0 +1,77 @@
+// 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_BROWSER_POWER_DATA_PROVIDER_H_
+#define CONTENT_BROWSER_POWER_DATA_PROVIDER_H_
+
+#include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "base/time/time.h"
+
+namespace content {
+
+// A class used to monitor the power usage and tell profiler the power data

[qsr, 2014/01/22 15:30:01]

The comment seems not to apply to this object. This is just a data point, it
doesn't do anything.

Moreover, comments must be sentences and so end with a period.

+struct PowerEvent {
+  enum Type {
+    // 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;
+  double value; // power
+};
+
+// A class used to GET power usage

[qsr, 2014/01/22 15:30:01]

No need for uppercase.

+class PowerDataProvider {
+ public:
+  virtual ~PowerDataProvider() { }
+
+  // Return true when success

[qsr, 2014/01/22 15:30:01]

Can you be more precise and what this method those? Does it return a single
power event? Does it needs to be initialized beforehand?

+  virtual bool GetData(PowerEvent*) = 0;
+
+  virtual bool IsDummy() const { return false; }

[qsr, 2014/01/22 15:30:01]

You might not nee a dummy provider in fact, and so won't need such a method: the
only user of the provider is the service, so the factory can simply return NULL
if there is no provider, and the service can check for this.

 If you still prefer to use a dummy implementation, this method should be
renamed to something like: IsAvailable()

+
+ protected:
+  PowerDataProvider() { }

[qsr, 2014/01/22 15:30:01]

I'm not sure you want to protect this. When writing test, having the factory
being the only one able to create a provider will be cumbersome.

+
+ private:
+  friend class PowerDataProviderFactory;
+  DISALLOW_COPY_AND_ASSIGN(PowerDataProvider);
+};
+
+// A dummy data provider
+class PowerDataProviderDummy : public PowerDataProvider {
+ public:
+  virtual ~PowerDataProviderDummy() { }
+
+  virtual bool IsDummy() const OVERRIDE { return true; }
+
+  virtual bool GetData(PowerEvent* data) OVERRIDE {
+    return false;
+  }
+
+ protected:
+  PowerDataProviderDummy() { }
+
+ private:
+  friend class PowerDataProviderFactory;
+  DISALLOW_COPY_AND_ASSIGN(PowerDataProviderDummy);
+};
+
+class PowerDataProviderFactory {
+ public:
+  static PowerDataProvider* Create();

[qsr, 2014/01/22 15:30:01]

You can return a scoped_ptr<PowerDataProvider> here, documenting the ownership.

+
+ private:
+  PowerDataProviderFactory() { }
+  DISALLOW_COPY_AND_ASSIGN(PowerDataProviderFactory);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_POWER_DATA_PROVIDER_H_