|
|
DescriptionConvert devtools to be client of WakeLock mojo service.
Wake Lock is a Mojo interface that wraps PowerSaveBlocker. As part of the
creation of standalone Device Service, all browser-side clients of
PowerSaveBlocker should be converted to be clients of the Wake Lock Mojo
interface instead.
BUG=689413
Review-Url: https://codereview.chromium.org/2893793003
Cr-Commit-Position: refs/heads/master@{#473833}
Committed: https://chromium.googlesource.com/chromium/src/+/f6fd56037a18d025817064d21222bc078b081428
Patch Set 1 : Convert devtools to be client of WakeLock mojo service. #
Total comments: 11
Messages
Total messages: 22 (14 generated)
The CQ bit was checked by ke.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by ke.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
ke.he@intel.com changed reviewers: + dgozman@chromium.org
Hi, dgozman@, Could you PTAL on this, thanks!
https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:830: RenderFrameDevToolsAgentHost::GetWakeLockService() { 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? https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:834: mojo::MakeRequest(&wake_lock_); Inline this into line 841? https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:923: GetWakeLockService()->CancelWakeLock(); I think we should destroy the service at this point as well.
dgozman@, Thanks! https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:830: RenderFrameDevToolsAgentHost::GetWakeLockService() { On 2017/05/18 21:58:45, dgozman wrote: > 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? 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/ https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:834: mojo::MakeRequest(&wake_lock_); On 2017/05/18 21:58:45, dgozman wrote: > Inline this into line 841? 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. https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:923: GetWakeLockService()->CancelWakeLock(); On 2017/05/18 21:58:45, dgozman wrote: > I think we should destroy the service at this point as well. 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.
Description was changed from ========== Convert devtools to be client of WakeLock mojo service. Wake Lock is a Mojo interface that wraps PowerSaveBlocker. As part of the creation of standalone Device Service, all browser-side clients of PowerSaveBlocker should be converted to be clients of the Wake Lock Mojo interface instead. BUG=689410 ========== to ========== Convert devtools to be client of WakeLock mojo service. Wake Lock is a Mojo interface that wraps PowerSaveBlocker. As part of the creation of standalone Device Service, all browser-side clients of PowerSaveBlocker should be converted to be clients of the Wake Lock Mojo interface instead. BUG=689413 ==========
Thank you for explanations! lgtm with some questions. https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:834: mojo::MakeRequest(&wake_lock_); On 2017/05/18 23:58:47, ke.he wrote: > On 2017/05/18 21:58:45, dgozman wrote: > > Inline this into line 841? > > 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. Do you mean that wake_lock_ will be functional (although noop) even when there is no wake_lock_context? Interesting... https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:923: GetWakeLockService()->CancelWakeLock(); On 2017/05/18 23:58:47, ke.he wrote: > On 2017/05/18 21:58:45, dgozman wrote: > > I think we should destroy the service at this point as well. > 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. Shouldn't we do it from the performance standpoint? How expensive holding the service is?
https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:834: mojo::MakeRequest(&wake_lock_); On 2017/05/19 19:15:45, dgozman wrote: > On 2017/05/18 23:58:47, ke.he wrote: > > On 2017/05/18 21:58:45, dgozman wrote: > > > Inline this into line 841? > > > > 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. > > Do you mean that wake_lock_ will be functional (although noop) even when there > is no wake_lock_context? Interesting... > Yes, mojo::MakeRequest() make it be functional, but noop without wake_lock_context_ connect it to remote side. https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:923: GetWakeLockService()->CancelWakeLock(); On 2017/05/19 19:15:44, dgozman wrote: > On 2017/05/18 23:58:47, ke.he wrote: > > On 2017/05/18 21:58:45, dgozman wrote: > > > I think we should destroy the service at this point as well. > > 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. > > Shouldn't we do it from the performance standpoint? How expensive holding the > service is? 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.
The CQ bit was checked by ke.he@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495523301008570, "parent_rev": "513553ac9db297eeb9c6cc4965d53bec81dd6a66", "commit_rev": "f6fd56037a18d025817064d21222bc078b081428"}
Message was sent while issue was closed.
Description was changed from ========== Convert devtools to be client of WakeLock mojo service. Wake Lock is a Mojo interface that wraps PowerSaveBlocker. As part of the creation of standalone Device Service, all browser-side clients of PowerSaveBlocker should be converted to be clients of the Wake Lock Mojo interface instead. BUG=689413 ========== to ========== Convert devtools to be client of WakeLock mojo service. Wake Lock is a Mojo interface that wraps PowerSaveBlocker. As part of the creation of standalone Device Service, all browser-side clients of PowerSaveBlocker should be converted to be clients of the Wake Lock Mojo interface instead. BUG=689413 Review-Url: https://codereview.chromium.org/2893793003 Cr-Commit-Position: refs/heads/master@{#473833} Committed: https://chromium.googlesource.com/chromium/src/+/f6fd56037a18d025817064d21222... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f6fd56037a18d025817064d21222...
Message was sent while issue was closed.
https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2893793003/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:839: device::mojom::WakeLockType::PreventAppSuspension, Oh! The original PowerSaveBlocker type is "kPowerSaveBlockPreventDisplaySleep", I mistakenly set it as "PreventAppSuspension" here! I guess this is the reason mentioned in Issue 726290 |