chrome power profiler chrome side changes

content/public/browser/power_profiler_observer.h

Index: content/public/browser/power_profiler_observer.h
diff --git a/content/public/browser/power_profiler_observer.h b/content/public/browser/power_profiler_observer.h
new file mode 100644
index 0000000000000000000000000000000000000000..9ed401a47297e6bb2031fb154cd925d5b74c7b95
--- /dev/null
+++ b/content/public/browser/power_profiler_observer.h
@@ -0,0 +1,51 @@
+// 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_PROFILER_OBSERVER_H_
+#define CONTENT_PUBLIC_BROWSER_POWER_PROFILER_OBSERVER_H_
+
+#include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "content/common/content_export.h"
+#include "content/public/browser/power_data_provider.h"
+
+namespace content {
+
+// A class used to monitor the power usage and tell profiler the power data
+class CONTENT_EXPORT PowerProfilerObserver

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

I'm not sure to understand why the object should be
base::RefCountedThreadSafe<PowerProfilerObserver>

My guess is that you need this for your current implementation, and because this
is not a pure virtual base class, you cannot have multiple inheritance.

I would instead propose that this class becomes a pure virtual base class, so:

Remove Register and Unregister, the calls to AddObserver/RemoveObserver are
usual, and you do not need to have them here.

You can also remove the SetResolution, and instead add this as a parameter of
AddObserver. Another solution is to have the Resolution be a property of the
Service instead of one of the observer. It is simpler, and I do not understand
why you would need that at the observer level.

After that, you only have a single virtual method here.

[Pan, 2014/01/23 09:37:43]

it is there because previously, observer called in UI thread while register
itself in PowerProfiler thread. now we can remove it.
I'm thinking there may be kinds of observers that have different resolution
requirement, like HIGH for profile point(in DevTools), LOW for measurement
point(measure in a period). if resolution_ lives in a observer, we can not only
adjust profling QoS to HIGH when HIGH customer comes, but also back to LOW when
it leaves. 
Register and Unregister, I don't have strong preference to keep or remove them,
how do you think of it?

thanks 
Pan

[qsr, 2014/01/23 10:08:05]

 Then you should have a virtual method returning the resolution, and let
implementation decide what to do: either return a constant value, or have a
setter. You can also have a default implementation (in a .cc file) if you think
it makes sense.
 Then they should be removed.

+    : public base::RefCountedThreadSafe<PowerProfilerObserver> {
+ public:
+  enum Resolution {
+    LOW,
+    NORMAL,
+    HIGH,
+    TYPE_COUNT // this should be always the last one
+  };
+
+  PowerProfilerObserver();
+  ~PowerProfilerObserver();
+
+  // Register as an PowerProfilerService observer
+  bool Register();
+
+  // Unregister
+  void Unregister();
+
+  // Set resolution requirement, PowerProfilerService would change for it.
+  void SetResolution(Resolution);
+
+  Resolution resolution() const { return resolution_; }
+
+  // This method would be called on thread in WorkerPool
+  virtual void Send(PowerEvent*) { }

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

Make this a pure virtual method, as I do not see why you would implement this
and not override this method. Also you can probably change it to
OnPowerEvent(const PowerEvent&) 

+
+ private:
+  Resolution resolution_;
+
+  DISALLOW_COPY_AND_ASSIGN(PowerProfilerObserver);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_POWER_PROFILER_OBSERVER_H_