chrome power profiler chrome side changes

content/browser/power_profiler/power_profiler_service.h

+// Copyright 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 CONTENT_BROWSER_POWER_PROFILER_SERVICE_H_
+#define CONTENT_BROWSER_POWER_PROFILER_SERVICE_H_
+
+#include <set>
+
+#include "base/basictypes.h"
+#include "base/timer/timer.h"
+#include "content/browser/power_profiler/power_profiler_helper.h"
+#include "content/browser/power_profiler/power_profiler_host.h"
+
+namespace content {
+
+// A class used to monitor the power usage and tell profiler the power data
+class PowerProfilerService {
+ public:
+  enum Status {
+    UNINITIALIZED, // Unitialized,
+    INITIALIZED, // Initialized, not in profiling
+    PROFILING, // In profiling
+  };
+
+  PowerProfilerService();
+  ~PowerProfilerService();
+
+  // Add and remove an observer.
+  void AddObserver(PowerProfilerHost* observer);

[pfeldman, 2014/01/09 11:38:58]

AddObserver(PowerProfilerObserver*)?

+  void RemoveObserver(PowerProfilerHost* observer);
+  void UpdateResolution();
+
+  void Snapshot();
+  double GetEnergy(PowerEvent::Type);
+
+  static PowerProfilerService* Get();
+  Status status();

[Pan, 2014/01/18 04:30:37]

I'm thinking, Public API of PowerProfilerService should be:

void AddObserver(PowerProfilerObserver*)
void RemoveObserver(PowerProfilerObserver*)
void UpdateResolution();

static PowerProfilerService* GetInstance(); // singleton usage
Status status(); // service status.

Snapshot() and GetEnergy(PowerEvent::Type) is designed for Power Monitor API I
proposed to measure energy in JavaScript, won't be used here, so they can be
removed for the time being.

In this way, PowerProfilerObservers are async notified when timer queried power
data, is it ok?

[qsr, 2014/01/20 09:16:55]

 Mostly yes. The only one I would ask about is status. Can you get this
information synchronously if the service is mostly delegating everything to
another thread? If yes, then the API is fine, otherwise, the method should take
a callback.

 Also, when documenting this class, please specify on which threads those
methods can be called. It will be a lot simpler if it can only be called on the
main thread, otherwise, you will need to remember on which thread the
AddObserver was called to be able to call all the observer method from the same
thread.

+
+ private:
+  void Start();
+  void Stop();
+  void ProcessData();
+  void Notify(PowerEvent* buffer, int count);
+  void InitializeDelays();
+
+  void ResetAccumulationEnergies();
+  void AccumulateEnergies(double* energies, int count);

[qsr, 2014/01/17 11:50:14]

All of those methods await an array of the same size, instead of using ptr +
count, you can defined near your PowerEvent::Type:

typedef int PowerEventArray[ID_COUNT];

and then, use:

void AccumulateEnergies(PowerEventArray* energies)

+
+  scoped_ptr<base::OneShotTimer<PowerProfilerService> > query_power_timer_;

[qsr, 2014/01/17 11:50:14]

Why is that a scoped_ptr, instead of an instance variable?

+
+  size_t delay_; // milliseconds
+  size_t delays_[PowerProfilerHost::TYPE_COUNT]; // kinds of delays
+  Status status_;
+  double accumulated_enegries_[PowerProfilerHost::TYPE_COUNT];
+  std::set<PowerProfilerHost* > observers_;
+
+  scoped_refptr<PowerProfilerHelper> profiler_helper_;
+
+  DISALLOW_COPY_AND_ASSIGN(PowerProfilerService);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_POWER_PROFILER_SERVICE_H_