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..4f71c287389388a3e0b73d09d8c86484c0074f87 100644
--- a/device/wake_lock/wake_lock_service_impl.cc
+++ b/device/wake_lock/wake_lock_service_impl.cc
@@ -6,31 +6,152 @@
 
 #include <utility>
 
+#include "base/atomic_sequence_num.h"
+#include "base/memory/ptr_util.h"
+#include "device/power_save_blocker/power_save_blocker.h"
 #include "device/wake_lock/wake_lock_service_context.h"
 
 namespace device {
 
-WakeLockServiceImpl::WakeLockServiceImpl(WakeLockServiceContext* context)
-    : context_(context), wake_lock_request_outstanding_(false) {}
+namespace {
 
-WakeLockServiceImpl::~WakeLockServiceImpl() {
-  CancelWakeLock();
+base::StaticAtomicSequenceNumber g_unique_client_id;
+
+}  // namespace
+
+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() {}
+
+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
+  // client is being added into the bindingSet, we create an unique client_id
+  // as its context, this context is used as the key of the map |outstandings_|
+  // and |binding_ids| which record the clients' last requested status and
+  // BindingId.
+  int client_id = g_unique_client_id.GetNext();
+  mojo::BindingId binding_id =
+      binding_set_.AddBinding(this, std::move(request), client_id);
+  outstandings_[client_id] = false;
+  binding_ids_[client_id] = binding_id;
 }
 
 void WakeLockServiceImpl::RequestWakeLock() {
-  if (wake_lock_request_outstanding_)
-    return;
+  DCHECK(main_task_runner_->RunsTasksOnCurrentThread());
 
-  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.
+  int client_id = binding_set_.dispatch_context();

[blundell, 2017/05/04 15:40:50]

Ken and I discussed, and we think it should work fine to use a unique_ptr as the
dispatch context (we haven't tried though). Assuming that that works, you can
use a unique_ptr<bool> directly as the dispatch context, and you can do away
with |outstandings_| altogether. That would be nice.

[ke.he, 2017/05/05 08:58:01]

Yes, |outstandings_| is not needed.
Done.

+  DCHECK(outstandings_.find(client_id) != outstandings_.end());
+
+  if (outstandings_[client_id]) {
+    return;
+  }
+  outstandings_[client_id] = true;
+  num_lock_requests_++;
+  UpdateWakeLock();
 }
 
 void WakeLockServiceImpl::CancelWakeLock() {
-  if (!wake_lock_request_outstanding_)
+  DCHECK(main_task_runner_->RunsTasksOnCurrentThread());
+
+  int client_id = binding_set_.dispatch_context();
+  DCHECK(outstandings_.find(client_id) != outstandings_.end());
+
+  if (!outstandings_[client_id]) {
     return;
+  }
+  outstandings_[client_id] = 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)
+  gfx::NativeView native_view = native_view_getter_.Run(context_id_);

[blundell, 2017/05/04 15:40:50]

You either need to do this conditionally based on the type here or hardcode the
information passed to the PowerSaveBlocker in WakeLockServiceImpl in this CL and
generalize it only in the next CL.

[ke.he, 2017/05/05 08:58:01]

The type, reason and description has already been hardcoded in
WakeLockServiceContext::GetWakeLock() which is the only place to create
WakeLockServiceImpl.

+  if (native_view) {
+    wake_lock_.get()->InitDisplaySleepBlocker(native_view);
+  }
+#endif
+}
+
+void WakeLockServiceImpl::RemoveWakeLock() {
+  DCHECK(wake_lock_);
+  wake_lock_.reset();
+}
+
+void WakeLockServiceImpl::OnConnectionError() {
+  int client_id = binding_set_.dispatch_context();
+
+  auto it = outstandings_.find(client_id);
+  if (it != outstandings_.end()) {
+    // If the error-happening client's wakelock is in outstanding status,
+    // decrease the num_lock_requests and call UpdateWakeLock().
+    if (it->second && num_lock_requests_ > 0) {
+      num_lock_requests_--;
+      UpdateWakeLock();
+    }
+    // Remove the entry of client_id from map.
+    outstandings_.erase(client_id);
+  }
+
+  // Remove error-happening client's binding from bindingSet.
+  auto it2 = binding_ids_.find(client_id);
+  if (it2 != binding_ids_.end()) {
+    binding_set_.RemoveBinding(it2->second);

[blundell, 2017/05/04 15:40:50]

BindingSet has already done this at the time of calling this callback (cf.
BindingSet::OnConnectionError()), so this code is not necessary. This also means
we don't need |binding_ids_| at all unless I'm missing something.

[ke.he, 2017/05/05 08:58:01]

Yes. we don't need |binding_ids_|.
Done.

+    binding_ids_.erase(client_id);
+  }
 
-  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