Chrome power profiler service

content/browser/power_profiler/power_profiler_service.cc

Index: content/browser/power_profiler/power_profiler_service.cc
diff --git a/content/browser/power_profiler/power_profiler_service.cc b/content/browser/power_profiler/power_profiler_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..441dacdb9bb93df388565577080883202ae989cf
--- /dev/null
+++ b/content/browser/power_profiler/power_profiler_service.cc
@@ -0,0 +1,120 @@
+// 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.
+
+#include "content/public/browser/power_profiler_service.h"
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace content {
+
+PowerProfilerService::PowerProfilerService()
+    : status_(UNINITIALIZED)
+    , data_provider_(PowerDataProviderFactory::Create()) {
+  if (data_provider_.get()) {
+      status_ = INITIALIZED;
+      InitializeDelays();
+      task_runner_ = BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(
+          BrowserThread::GetBlockingPool()->GetSequenceToken());
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
+
+bool PowerProfilerService::IsAvailable() {
+  return status_ !=  UNINITIALIZED;
+}
+
+void PowerProfilerService::InitializeDelays() {
+  delays_[PowerProfilerObserver::LOW] = base::TimeDelta::FromMilliseconds(4000);
+  delays_[PowerProfilerObserver::NORMAL]
+      = base::TimeDelta::FromMilliseconds(200);
+  delays_[PowerProfilerObserver::HIGH] = base::TimeDelta::FromMilliseconds(50);
+}
+
+PowerProfilerService* PowerProfilerService::GetInstance() {
+  return Singleton<PowerProfilerService>::get();
+}
+
+bool PowerProfilerService::AddObserver(PowerProfilerObserver* obs) {

[qsr, 2014/01/23 12:50:06]

I don't think you need a return value here. If someone is interested to know if
using this makes sense, it does have the IsAvailable method.

[Pan, 2014/01/25 09:35:53]

Done.

+  if (UNINITIALIZED == status_)
+    return false;
+
+  observers_.AddObserver(obs);
+  UpdateResolution();
+  if (PROFILING != status_)
+    Start();
+
+  return true;
+}
+
+void PowerProfilerService::RemoveObserver(PowerProfilerObserver* obs) {
+  observers_.RemoveObserver(obs);
+  UpdateResolution();
+  if (!observers_.might_have_observers())
+    Stop();
+}
+
+void PowerProfilerService::UpdateResolution() {
+  if (UNINITIALIZED == status_)
+    return;
+
+  PowerProfilerObserver::Resolution resolution = PowerProfilerObserver::LOW;
+  FOR_EACH_OBSERVER(PowerProfilerObserver, observers_,
+      GetHigherResolution(resolution));

[qsr, 2014/01/23 12:50:06]

I don't really like that each observer needs to implement the same comparaison
code. Can you have it return a value instead? You will have to iterate yourself
over the list and get the maximal value.

[Pan, 2014/01/25 09:35:53]

Done.

+
+  // if delay_ will be reset, reschedule the timer immediately
+  if (delay_ != delays_[resolution]) {
+      delay_ = delays_[resolution];
+
+      if (query_power_timer_.IsRunning())

[qsr, 2014/01/23 12:50:06]

You might want to check on your status, instead of finding that out from the
timer status)

[Pan, 2014/01/25 09:35:53]

Done.

+          query_power_timer_.Start(FROM_HERE,
+              delay_, this, &PowerProfilerService::OnTimer);
+  }
+}
+
+void PowerProfilerService::Start() {
+  DCHECK (!query_power_timer_.IsRunning());

[qsr, 2014/01/23 12:50:06]

Maybe also check for your status.

[Pan, 2014/01/25 09:35:53]

Done.

+  status_ = PROFILING;
+
+  // send out power events immediately.
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&PowerProfilerService::ProcessData, base::Unretained(this)));
+
+  query_power_timer_.Start(FROM_HERE,
+      delay_, this, &PowerProfilerService::OnTimer);
+}
+
+void PowerProfilerService::Stop() {
+  // stop timer, set status to INITIALIZED
+  if (PROFILING == status_) {

[qsr, 2014/01/23 12:50:06]

Why is that a test instead of a DCHECK? Are you expecting stop to be called when
you are not running?

[Pan, 2014/01/25 09:35:53]

Done.

+    query_power_timer_.Stop();
+    status_ = INITIALIZED;
+  }
+}
+
+void PowerProfilerService::OnTimer() {
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&PowerProfilerService::ProcessData, base::Unretained(this)));
+}
+
+void PowerProfilerService::Notify(const PowerEvent& event) {
+  FOR_EACH_OBSERVER(PowerProfilerObserver, observers_, OnPowerEvent(event));
+}
+
+void PowerProfilerService::ProcessData() {

[qsr, 2014/01/23 12:50:06]

you could DCHECK  that you are on your task_runner. Also maybe rename this
method QueryData instead of ProcessData. And even:

rename OnTimer QueryData
rename ProcessData QueryDataOnTaskRunner

and in Start, instead of writing the PostTask, you can call QueryData

[Pan, 2014/01/25 09:35:53]

Done.

+  if (PROFILING != status_)
+    return;
+
+  // Get Data and Notify
+  PowerEvent event;
+  if(data_provider_->GetData(&event))
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
+        &PowerProfilerService::Notify, base::Unretained(this), event));
+}
+
+}  // namespace content