Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl

device/wake_lock/wake_lock_service_impl.cc

Index: device/wake_lock/wake_lock_service_impl.cc
diff --git a/device/wake_lock/wake_lock_service_impl.cc b/device/wake_lock/wake_lock_service_impl.cc
index ae998aa62f4e27a29e65b60d5c0d3b5eb1c6c026..79747dbc7f1c260d78ae39e7139c5431ed653878 100644
--- a/device/wake_lock/wake_lock_service_impl.cc
+++ b/device/wake_lock/wake_lock_service_impl.cc
@@ -6,31 +6,122 @@
 
 #include <utility>
 
-#include "device/wake_lock/wake_lock_service_context.h"
+#include "base/memory/ptr_util.h"
 
 namespace device {
 
-WakeLockServiceImpl::WakeLockServiceImpl(WakeLockServiceContext* context)
-    : context_(context), wake_lock_request_outstanding_(false) {}
+WakeLockServiceImpl::WakeLockServiceImpl(
+    mojom::WakeLockServiceRequest request,
+    device::PowerSaveBlocker::PowerSaveBlockerType type,
+    device::PowerSaveBlocker::Reason reason,
+    const std::string& description,
+    int context_id,
+    WakeLockContextCallback native_view_getter,
+    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner)
+    : type_(type),
+      reason_(reason),
+      description_(base::MakeUnique<std::string>(description)),
+      num_lock_requests_(0),
+#if defined(OS_ANDROID)
+      context_id_(context_id),
+      native_view_getter_(native_view_getter),
+#endif
+      main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      file_task_runner_(std::move(file_task_runner)) {
+  AddClient(std::move(request));
+  binding_set_.set_connection_error_handler(base::Bind(
+      &WakeLockServiceImpl::OnConnectionError, base::Unretained(this)));
+}
+
+WakeLockServiceImpl::~WakeLockServiceImpl() {}
 
-WakeLockServiceImpl::~WakeLockServiceImpl() {
-  CancelWakeLock();
+void WakeLockServiceImpl::AddClient(mojom::WakeLockServiceRequest request) {
+  // Multiple frames that associate to the same WebContents share the same one
+  // WakeLockServiceImpl instance. Two consecutive |RequestWakeLock| requests
+  // from the same frame should be coalesced as one request. Everytime a new

[blundell, 2017/05/05 12:49:38]

s/frame/client

[ke.he, 2017/05/05 14:56:36]

Done.

+  // client is being added into the bindingSet, we create an unique_ptr<bool>
+  // as its context, this context records the clients' outstanding status.

[blundell, 2017/05/05 12:49:37]

, which records this client's request status.

[ke.he, 2017/05/05 14:56:36]

Done.

+  binding_set_.AddBinding(this, std::move(request),
+                          base::MakeUnique<bool>(false));
 }
 
 void WakeLockServiceImpl::RequestWakeLock() {
-  if (wake_lock_request_outstanding_)
-    return;
+  DCHECK(main_task_runner_->RunsTasksOnCurrentThread());
+  DCHECK(binding_set_.dispatch_context());
 
-  wake_lock_request_outstanding_ = true;
-  context_->RequestWakeLock();
+  // Uses the Context to get the outstanding status of current binding.
+  // Two consecutive requests from the same client should be coalesced
+  // as one request.
+  if (*binding_set_.dispatch_context()) {
+    return;
+  }
+  *binding_set_.dispatch_context() = true;
+  num_lock_requests_++;
+  UpdateWakeLock();
 }
 
 void WakeLockServiceImpl::CancelWakeLock() {
-  if (!wake_lock_request_outstanding_)
+  DCHECK(main_task_runner_->RunsTasksOnCurrentThread());
+  DCHECK(binding_set_.dispatch_context());
+
+  if (!(*binding_set_.dispatch_context())) {
     return;
+  }
+  *binding_set_.dispatch_context() = false;
+  if (num_lock_requests_ > 0) {
+    num_lock_requests_--;
+    UpdateWakeLock();
+  }
+}
+
+void WakeLockServiceImpl::HasWakeLockForTests(
+    const HasWakeLockForTestsCallback& callback) {
+  callback.Run(!!wake_lock_);
+}
+void WakeLockServiceImpl::UpdateWakeLock() {
+  DCHECK(num_lock_requests_ >= 0);
+
+  if (num_lock_requests_) {
+    if (!wake_lock_)
+      CreateWakeLock();
+  } else {
+    if (wake_lock_)
+      RemoveWakeLock();
+  }
+}
+
+void WakeLockServiceImpl::CreateWakeLock() {
+  DCHECK(!wake_lock_);
+  wake_lock_ = base::MakeUnique<device::PowerSaveBlocker>(
+      type_, reason_, *description_, main_task_runner_, file_task_runner_);
+
+#if defined(OS_ANDROID)

[blundell, 2017/05/05 12:49:37]

What I'

+  gfx::NativeView native_view = native_view_getter_.Run(context_id_);

[blundell, 2017/05/05 12:49:37]

What I'm saying here is that this class should be internally consistent: Either
it has a generalized interface and properly implements that generalized
interface, or it hardcodes information internally and acts based on that
hardcoded information. Right now it's in the middle. So I'd either add a
condition here or move the hardcoding from WakeLockServiceContext to
WakeLockServiceImpl for this CL. Probably the former, since the latter would
just need to be undone in the next CL in any case.

[ke.he, 2017/05/05 14:56:36]

Done.

+  if (native_view) {
+    wake_lock_.get()->InitDisplaySleepBlocker(native_view);
+  }
+#endif
+}
+
+void WakeLockServiceImpl::RemoveWakeLock() {
+  DCHECK(wake_lock_);
+  wake_lock_.reset();
+}
+
+void WakeLockServiceImpl::OnConnectionError() {
+  if (binding_set_.dispatch_context()) {

[blundell, 2017/05/05 12:49:37]

Hmm, why the need for this if?

[ke.he, 2017/05/05 14:56:36]

it should be a DCHECK() instead of if().
DCHECK(binding_set_.dispatch_context());

+    // If the error-happening client's wakelock is in outstanding status,
+    // decrease the num_lock_requests and call UpdateWakeLock().
+    if (*binding_set_.dispatch_context() && num_lock_requests_ > 0) {
+      num_lock_requests_--;
+      UpdateWakeLock();
+    }
+  }
 
-  wake_lock_request_outstanding_ = false;
-  context_->CancelWakeLock();
+  // If |binding_set_| is empty, WakeLockServiceImpl should delele itself.
+  if (binding_set_.empty()) {
+    base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
+  }
 }
 
 }  // namespace device