Convert devtools to be client of WakeLock mojo service.

content/browser/devtools/render_frame_devtools_agent_host.cc

Index: content/browser/devtools/render_frame_devtools_agent_host.cc
diff --git a/content/browser/devtools/render_frame_devtools_agent_host.cc b/content/browser/devtools/render_frame_devtools_agent_host.cc
index 951122cd18f3f2a4465aaf8a3bc3275517b5148a..6b52231533924e35fcee0467ac55463c972606ca 100644
--- a/content/browser/devtools/render_frame_devtools_agent_host.cc
+++ b/content/browser/devtools/render_frame_devtools_agent_host.cc
@@ -50,7 +50,7 @@
 
 #if defined(OS_ANDROID)
 #include "content/public/browser/render_widget_host_view.h"
-#include "device/power_save_blocker/power_save_blocker.h"
+#include "device/wake_lock/public/interfaces/wake_lock_context.mojom.h"
 #endif
 
 namespace content {
@@ -603,12 +603,14 @@ void RenderFrameDevToolsAgentHost::OnClientAttached() {
     return;
 
   frame_trace_recorder_.reset(new DevToolsFrameTraceRecorder());
-  CreatePowerSaveBlocker();
+#if defined(OS_ANDROID)
+  GetWakeLockService()->RequestWakeLock();
+#endif
 }
 
 void RenderFrameDevToolsAgentHost::OnClientDetached() {
 #if defined(OS_ANDROID)
-  power_save_blocker_.reset();
+  GetWakeLockService()->CancelWakeLock();
 #endif
   frame_trace_recorder_.reset();
   in_navigation_protocol_message_buffer_.clear();
@@ -823,19 +825,25 @@ bool RenderFrameDevToolsAgentHost::CheckConsistency() {
       handlers_frame_host_ == manager->pending_frame_host();
 }
 
-void RenderFrameDevToolsAgentHost::CreatePowerSaveBlocker() {
 #if defined(OS_ANDROID)
-  power_save_blocker_.reset(new device::PowerSaveBlocker(
-      device::PowerSaveBlocker::kPowerSaveBlockPreventDisplaySleep,
-      device::PowerSaveBlocker::kReasonOther, "DevTools",
-      BrowserThread::GetTaskRunnerForThread(BrowserThread::UI),
-      BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE)));
-  if (web_contents()->GetNativeView()) {
-    power_save_blocker_->InitDisplaySleepBlocker(
-        web_contents()->GetNativeView());
+device::mojom::WakeLockService*
+RenderFrameDevToolsAgentHost::GetWakeLockService() {

[dgozman, 2017/05/18 21:58:45]

I'm not very good at mojo yet, so let me ask a naive question: shouldn't there
be a more centralized place for the service, like WebContents? What's the
difference between WakeLockService and WakeLockContext?

[ke.he, 2017/05/18 23:58:47]

The design of WakeLock servicification is necessarily very complex. see design
doc:
https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c

WakeLockService is the instance that manages the PowerSaveBLocker. The
WakeLockContext is designed to identify WakeLockService to one instance of
WebContents, because different WebContents has different native_view, which is
needed by PowerSaveBlocker (only on Android). So in Android case, user needs to
get a WakeLockContext first, then use the WakeLockContext to create a
WakeLockService, in this way the WakeLockService knows which WebContents's
native_view it is operating on.

MediaWebContentsObsever is another client that is very similar to this See
https://codereview.chromium.org/2846813002/

+  // Here is a lazy binding, and will not reconnect after connection error.
+  if (!wake_lock_) {
+    device::mojom::WakeLockServiceRequest request =
+        mojo::MakeRequest(&wake_lock_);

[dgozman, 2017/05/18 21:58:45]

Inline this into line 841?

[ke.he, 2017/05/18 23:58:47]

We don't inline this on purpose, so the |wake_lock_| can always be bound by
"mojo::MakeRequest", then it is safe to make mojo call even if the connection is
not created.

[dgozman, 2017/05/19 19:15:45]

Do you mean that wake_lock_ will be functional (although noop) even when there
is no wake_lock_context? Interesting...

[ke.he, 2017/05/20 01:00:34]

Yes, mojo::MakeRequest() make it be functional, but noop without
wake_lock_context_ connect it to remote side.

+    device::mojom::WakeLockContext* wake_lock_context =
+        web_contents()->GetWakeLockContext();
+    if (wake_lock_context) {
+      wake_lock_context->GetWakeLock(
+          device::mojom::WakeLockType::PreventAppSuspension,

[ke.he, 2017/05/26 03:11:12]

Oh! The original PowerSaveBlocker type is "kPowerSaveBlockPreventDisplaySleep",
I mistakenly set it as "PreventAppSuspension" here!
I guess this is the reason mentioned in Issue 726290

+          device::mojom::WakeLockReason::ReasonOther, "DevTools",
+          std::move(request));
+    }
   }
-#endif
+  return wake_lock_.get();
 }
+#endif
 
 void RenderFrameDevToolsAgentHost::RenderProcessGone(
     base::TerminationStatus status) {
@@ -905,12 +913,14 @@ void RenderFrameDevToolsAgentHost::DidDetachInterstitialPage() {
 }
 
 void RenderFrameDevToolsAgentHost::WasShown() {
-  CreatePowerSaveBlocker();
+#if defined(OS_ANDROID)
+  GetWakeLockService()->RequestWakeLock();
+#endif
 }
 
 void RenderFrameDevToolsAgentHost::WasHidden() {
 #if defined(OS_ANDROID)
-  power_save_blocker_.reset();
+  GetWakeLockService()->CancelWakeLock();

[dgozman, 2017/05/18 21:58:45]

I think we should destroy the service at this point as well.

[ke.he, 2017/05/18 23:58:47]

we don't need to destroy the service here or in destructor. when the
|wake_lock_| gets destructed, the remote Service will trigger
WakeLockServiceImpl::OnConnectionError() and destroy itself.

[dgozman, 2017/05/19 19:15:44]

Shouldn't we do it from the performance standpoint? How expensive holding the
service is?

[ke.he, 2017/05/20 01:00:34]

In GetWakeLockService(), the |wake_lock_| is initialized(bind and connect) only
for the first time. 

In the remote service side, the WakeLockServiceImpl will be created when client
makes the first mojo call(RequestWakeLock() or CancelWakeLock()) and will be
destroyed when |wake_lock_| is destructed.

So if we destroy the |wake_lock_| here, we need to create and init it again
before next use. I think we can just keep the remote WakeLockServiceImpl alive
as long as this RenderFrameDevToolsAgentHost alive. So we can avoid creating and
deleting remote service instance again and again.

 #endif
 }