Chrome power profiler service

content/browser/power_profiler/power_profiler_service.cc

+// 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()) {

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

comma should be on the previous line.

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

thanks, done.

+  if (data_provider_.get()) {
+      status_ = INITIALIZED;
+      InitializeDelays();
+      task_runner_ = BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(
+          BrowserThread::GetBlockingPool()->GetSequenceToken());
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
+
+PowerProfilerService::PowerProfilerService(
+    scoped_ptr<PowerDataProvider> provider)

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

I sent you a patch off-review to show exactly what I mean, but you should also
pass the task_runner here, as it would help you for test (making them single
threaded).

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

thanks, I missed the one to run a nested loop, it is really helpful.

+    : status_(UNINITIALIZED)
+    , data_provider_(provider.Pass()) {
+  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]

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

'=' should be on the previous line, more generally you should cut after the
operators, not before.

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

thanks, done

+      = base::TimeDelta::FromMilliseconds(200);
+  delays_[PowerProfilerObserver::HIGH] = base::TimeDelta::FromMilliseconds(50);
+}
+
+PowerProfilerService* PowerProfilerService::GetInstance() {
+  return Singleton<PowerProfilerService>::get();
+}
+
+void PowerProfilerService::AddObserver(PowerProfilerObserver* obs) {
+  if (UNINITIALIZED == status_)
+    return;
+
+  observers_.AddObserver(obs);
+  UpdateResolution();
+  if (PROFILING != status_)
+    Start();
+}
+
+void PowerProfilerService::RemoveObserver(PowerProfilerObserver* obs) {
+  observers_.RemoveObserver(obs);
+  UpdateResolution();
+  if (!observers_.might_have_observers())
+    Stop();
+}
+
+void PowerProfilerService::UpdateResolution() {

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

I've been thinking about resolution again, and I finally found out why I dislike
the way it is done here:

Suppose I have on observer that needs to be called infrequently, but would do
some significant work on each call, let's say around ~50 ms of work. I will
define it with a low resolution, but that would not work, because if another
observer is registered with a high resolution, I will be called every 50ms and
mainly pegged the CPU. So in the current state, even if I ask for a low
resolution, I still need to apply my own mechanism to only do my work every few
seconds. So if you really want resolution, you need to ensure that a low
resolution observer is not called on a high frequency.

Another issue is that the max frequency you can attain does not depend on the
observer, but in fact depends on the provider. Some provider might only be able
to give you result every couple of seconds, so having this run every 50ms is not
useful at all.

At this point, I would advise to do one of 2 things:
1) Forget about resolution for the moment. Let the service decide of the
resolution itself, and right now just have a single 50ms resolution. We will be
able to change that to depend on the provider later on when the necessity
happens.
2) Have multiple queues for each resolution, and make sure you only call each
observer at the right rate.

Given that we have a single observer and a single provider right now, I would
strongly choose 1 over 2. We will be able to update this for 2 later on if found
necessary.

If instead you go for 2, then you will also have to update the tests to check
you are doing the right thing.

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

I've thought it should works similar to Android sensor API, observer request a
resolution, while it is a hint to service, the real service should be same or
better.
I agree with the resolution depends on provider in future, so request is
designed to "HIGH" "NORMAL" "LOW"
, although actually it is a 50, 200, 4000 internally.
Given we don't have too many cases with diff resolution requirement right now, I
agree to remove resolution related code in this CL.

+  if (UNINITIALIZED == status_)
+    return;
+
+  PowerProfilerObserver::Resolution resolution = PowerProfilerObserver::LOW;
+  ObserverListBase<PowerProfilerObserver>::Iterator it_inside_observer_macro(
+      observers_);
+  PowerProfilerObserver* obs;
+  while ((obs = it_inside_observer_macro.GetNext()) != 0)
+    if (obs->resolution() > resolution)
+      resolution = obs->resolution();
+
+  // if delay_ will be reset, reschedule the timer immediately
+  if (delay_ != delays_[resolution]) {
+    delay_ = delays_[resolution];
+
+    if (PROFILING == status_)

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

Add {} around statement as it is more than one line long. Here and below.

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

Done.

+      query_power_timer_.Start(FROM_HERE,
+          delay_, this, &PowerProfilerService::QueryData);
+  }
+}
+
+void PowerProfilerService::Start() {
+  DCHECK(INITIALIZED == status_);
+  status_ = PROFILING;
+
+  // send out power events immediately.
+  QueryData();
+
+  query_power_timer_.Start(FROM_HERE,
+      delay_, this, &PowerProfilerService::QueryData);
+}
+
+void PowerProfilerService::Stop() {
+  DCHECK(PROFILING == status_);
+
+  // stop timer, set status to INITIALIZED
+  query_power_timer_.Stop();
+  status_ = INITIALIZED;
+}
+
+void PowerProfilerService::QueryData() {
+  task_runner_->PostTask(
+      FROM_HERE, base::Bind(&PowerProfilerService::QueryDataOnTaskRunner,
+      base::Unretained(this)));
+}
+
+void PowerProfilerService::Notify(const PowerEvent& event) {
+  FOR_EACH_OBSERVER(PowerProfilerObserver, observers_, OnPowerEvent(event));
+}
+
+void PowerProfilerService::QueryDataOnTaskRunner() {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+  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