Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl

device/wake_lock/wake_lock_service_impl.h

Index: device/wake_lock/wake_lock_service_impl.h
diff --git a/device/wake_lock/wake_lock_service_impl.h b/device/wake_lock/wake_lock_service_impl.h
index c66ec8d1a8fe73c2f111791f007c6f6afb35eafa..12e6d050b41e049cea3f7eca99ce4d339ef258ca 100644
--- a/device/wake_lock/wake_lock_service_impl.h
+++ b/device/wake_lock/wake_lock_service_impl.h
@@ -5,27 +5,77 @@
 #ifndef DEVICE_WAKE_LOCK_WAKE_LOCK_SERVICE_IMPL_H_
 #define DEVICE_WAKE_LOCK_WAKE_LOCK_SERVICE_IMPL_H_
 
+#include <map>
+
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "device/power_save_blocker/power_save_blocker.h"
 #include "device/wake_lock/public/interfaces/wake_lock_service.mojom.h"
-#include "mojo/public/cpp/bindings/interface_request.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
+#include "ui/gfx/native_widget_types.h"
 
 namespace device {
 
 class WakeLockServiceContext;
 
+// Callback that maps a context ID to the NativeView associated with
+// that context. This callback is provided to the Device Service by its
+// embedder.
+using WakeLockContextCallback = base::Callback<gfx::NativeView(int)>;
+
 class WakeLockServiceImpl : public mojom::WakeLockService {
  public:
-  explicit WakeLockServiceImpl(WakeLockServiceContext* context);
+  WakeLockServiceImpl(
+      WakeLockServiceContext* context,
+      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);
   ~WakeLockServiceImpl() override;
 
   // WakeLockSevice implementation.
   void RequestWakeLock() override;
   void CancelWakeLock() override;
+  void AddClient(mojom::WakeLockServiceRequest request) override;
 
  private:
+  void UpdateWakeLock();
+  void CreateWakeLock();
+  void RemoveWakeLock();
+  void OnConnectionError();
+
   // Will outlive this instance.
   WakeLockServiceContext* context_;
-  bool wake_lock_request_outstanding_;
+
+  device::PowerSaveBlocker::PowerSaveBlockerType type_;
+  device::PowerSaveBlocker::Reason reason_;
+  std::unique_ptr<std::string> description_;
+  int num_lock_requests_;
+
+#if defined(OS_ANDROID)
+  int context_id_;
+  WakeLockContextCallback native_view_getter_;
+#endif
+
+  scoped_refptr<base::SequencedTaskRunner> main_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;
+
+  // The actual power save blocker for screen.
+  std::unique_ptr<PowerSaveBlocker> wake_lock_;
+
+  // Wake lock requests from frames are coalesced into |frames_binding_set_|,
+  // While other requests *strongbinds* this WakeLockServiceImpl and don't use
+  // the |frames_binding_set_| at all. So we have to use a |outstandings_| map
+  // and a |outstanding_| to help handle both cases.
+
+  // TODO(heke): Split WakeLockServiceImpl into two derived classes that both
+  // implements mojom::WakeLockService interface. One to handle requests from
+  // frames, another to handle other kinds requests.
+  mojo::BindingSet<mojom::WakeLockService, int> frames_binding_set_;

[blundell, 2017/05/02 11:27:43]

See my comment on wake_lock_service_context.h. I think that we can just stuff
all of the requests into this BindingSet and have this object delete itself when
it detects that the BindingSet is empty.

[ke.he, 2017/05/03 06:25:37]

If we stuff all the requests into the BindingSet, then the clients have to
create two WakeLockServicePtrs, the code might be like:
mojom::WakeLockServicePtr wake_lock_;
mojom::WakeLockServicePtr wake_lock_2_;
webcontent()->GetWakeLockContext()->GetWakeLock(mojo::MakeRequest(&wake_lock_));
wake_lock_->AddClient(mojo::MakeRequest(&wake_lock_2_));
wake_lock_2_->RequestWakeLock();
...
wake_lock_2_->CancelWakeLock();

Or how about we split AddClient to a new mojo interface
WakeLockServiceConnection, so the code will be like:
mojom::WakeLockServiceConnectionPtr wake_lock_conn_;
mojom::WakeLockServicePtr wake_lock_;
...
wake_lock_conn_->AddClient(mojo::MakeRequest(&wake_lock_)).
WakeLockServiceImpl should implements both of the interfaces.

WDYT? thanks!

[blundell, 2017/05/03 09:13:13]

I'm confused: what prevents the client just using |wake_lock_| directly in this
scenario?

[ke.he, 2017/05/04 11:54:14]

Understand after the mail discussion, Done.

+  std::map<int, bool> outstandings_;
+  bool outstanding_;
 
   DISALLOW_COPY_AND_ASSIGN(WakeLockServiceImpl);
 };