Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843353003:
Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl (Closed)
|
Created:
3 years, 7 months ago by ke.he Modified:
3 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, jam, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl
This is the first step of "Allowing WakeLock to Serve Browser Process Clients"
Design doc by blundell@chromium.org:
https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c
In this CL:
1) Move ownership of PowerSaveBlocker from WakeLockServiceContext to
WakeLockServiceImpl.
2) Refactor WakeLockServiceImpl, one WakeLockServiceImpl supports managing
multiple client bindings.
3) Make WebContentsImpl coalesce multiple client requests into one
WakeLockServiceImpl.
The concrete motivation for this move is to make it trivial to implement a
generalization of the GetWakeLock() API that passes in the parameters for the
PowerSaveBlocker (needed by browser process clients). That interface extension
would be complex to implement in the current architecture, which is multiplexing
one PowerSaveBlocker for multiple WakeLockServiceImpl instances.
BUG=689410
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2843353003
Cr-Commit-Position: refs/heads/master@{#470241}
Committed: https://chromium.googlesource.com/chromium/src/+/98b761e7e4ffdfa7344bd3de7e3203d2bec235f0
Patch Set 1 #Patch Set 2 : android gn fix #Patch Set 3 : webkit_unit_tests pass #Patch Set 4 : error fix, non-frame client. #
Total comments: 16
Patch Set 5 : 1)Use bindingset manage all bindings. 2)Move test interface from WakeLockContext to WakeLockService. #Patch Set 6 : code rebase #Patch Set 7 : Allowing WakeLock to Serve Browser Process Clients #Patch Set 8 : seperate this CL into 2CLs #
Total comments: 14
Patch Set 9 : RemoveBinding() and |binding_ids| are not necessary in WakeLockServiceImpl. #Patch Set 10 : Use std::unique_ptr<bool> as the context of bindingSet. #Patch Set 11 : correct some trivial #includes #
Total comments: 17
Patch Set 12 : Addressed Colin's comments. #Patch Set 13 : Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl #
Total comments: 6
Patch Set 14 : Addressed Ken's comments. #Patch Set 15 : code rebase #
Total comments: 3
Messages
Total messages: 98 (69 generated)
Description was changed from ========== Allowing WakeLock to Serve Browser Process Clients BUG=689410 ========== to ========== Allowing WakeLock to Serve Browser Process Clients BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Description was changed from ========== Allowing WakeLock to Serve Browser Process Clients BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Allowing WakeLock to Serve Browser Process Clients Design doc by blundell@chromium.org: https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c In this CL: 1) Allow for creating PowerSaveBlockers with arbitrary (type, reason, description) parameters in the WakeLockContext mojo interface. 2) Continue to support having one PowerSaveBlocker per WebContents in which all connections from frames associated with that WebContents are coalesced. BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ke.he@intel.com changed reviewers: + blundell@chromium.org
Hi, Colin, thanks very much for your help:) This is the core refactoring part. PTAL.
Sorry, errors are found in https://codereview.chromium.org/2846813002/ I should fix that error first. Sorry.
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: This issue passed the CQ dry run.
Hi, Colin, PTAL. Thanks :)
Hi Ke He, This is a great start! This CL is necessarily pretty complex and is doing two things: (1) Moving ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl to make it trivial to generalize the API of WakeLockContext::GetWakeLock(). (2) Generalizing the API of WakeLockContext::GetWakeLock(). Let's separate (1) and (2) into separate CLs so that we can isolate the refactoring in (1). That means that the API extension and related typemap stuff would get split out into a followup CL. All of my comments on the code are targeting (1) above. https://codereview.chromium.org/2843353003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2843353003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:196: virtual void ConnectToBlinkWakeLock( nit: On seeing this, I'm inclined to have the interface instead be // Gets the WakeLockService that services wake lock requests from the // renderer. WakeLockService* GetRendererWakeLock(); and move the AddClient() calls to RenderFrameHostImpl. I think that will make what's happening slightly more transparent. https://codereview.chromium.org/2843353003/diff/60001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_context_host.cc (right): https://codereview.chromium.org/2843353003/diff/60001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_context_host.cc:44: context_provider->GetWakeLockContextForID( nit: Let's make this naming change in the CL that introduces the corresponding method for getting a wake lock without context. https://codereview.chromium.org/2843353003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2843353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:1518: device::mojom::WakeLockServicePtr blink_wake_lock_; nit: let's name this |renderer_wake_lock_| actually. https://codereview.chromium.org/2843353003/diff/60001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2843353003/diff/60001/content/public/browser/... content/public/browser/web_contents.h:664: virtual device::mojom::WakeLockContext* GetWakeLockContext() = 0; nit: Nothing in this CL is using this method, right? It seems like we could introduce it in the next CL (i.e., the one that uses it). https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/public... File device/wake_lock/public/interfaces/wake_lock_service.mojom (right): https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/public... device/wake_lock/public/interfaces/wake_lock_service.mojom:12: // TODO(heke): The AddClient() interface is only used by WebContents for the Actually, I think it's fine to leave this method on this interface. There is a use case for coalescing requests from multiple clients. In the future, maybe there will be clients other than Blink for that use case. https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... File device/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... device/wake_lock/wake_lock_service_context.h:55: void HasWakeLockForTests( IIUC, the only reason that this class still has any complexity is because of this method. Let's move this method into WakeLockService; the browsertest can access it by accessing the WebContent's |blink_wake_lock_|. Then this class (i.e., WakeLockServiceContext) becomes dead-simple: it just has a StrongBinding to its request, it doesn't need to track the WakeLockServiceImpls at all; WakeLockServiceImpl can instead have a BindingSet to manage all its requests and delete itself when that binding set becomes empty. https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... File device/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... device/wake_lock/wake_lock_service_impl.h:76: mojo::BindingSet<mojom::WakeLockService, int> frames_binding_set_; 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.
https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... File device/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... device/wake_lock/wake_lock_service_impl.h:76: mojo::BindingSet<mojom::WakeLockService, int> frames_binding_set_; On 2017/05/02 11:27:43, blundell wrote: > 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. 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!
https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... File device/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... device/wake_lock/wake_lock_service_impl.h:76: mojo::BindingSet<mojom::WakeLockService, int> frames_binding_set_; On 2017/05/03 06:25:37, ke.he wrote: > On 2017/05/02 11:27:43, blundell wrote: > > 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. > > 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(); I'm confused: what prevents the client just using |wake_lock_| directly in this scenario? > > 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!
On 2017/05/03 09:13:13, blundell wrote: > https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... > File device/wake_lock/wake_lock_service_impl.h (right): > > https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... > device/wake_lock/wake_lock_service_impl.h:76: > mojo::BindingSet<mojom::WakeLockService, int> frames_binding_set_; > On 2017/05/03 06:25:37, ke.he wrote: > > On 2017/05/02 11:27:43, blundell wrote: > > > 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. > > > > 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(); > > I'm confused: what prevents the client just using |wake_lock_| directly in this > scenario? > > > > > 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! Potentially WakeLockServiceImpl at least serves two kinds of clients, one is frames in blink, another is MediaWebContentsObsever. For frames in blink, it is the WebContentsImpl create a blink_wake_lock_ first, then all the WakeLockServiceRequests from blink are added into the bindingSet of WakeLockServiceImpl via blink_wake_lock_.AddClient(). For MediaWebContentsObsever, it calls "webcontent()->GetWakeLockContext()->GetWakeLock(type, reason, description, mojo::MakeRequest(&wake_lock_))", note that the wake_lock requests are Not added into the bindingSet, because the AddClient() has never been called. Actually it is a strongbinding, see "WakeLockServiceContext::GetWakeLock()". So in WakeLockServiceImpl, it have to cope with 2 kinds of bindings, one kind is in bindingset, another is strongbinding. You mentioned that we should make the WakeLockServiceContext to be dead-simple and to stuff all the requests into bindingset, so I guess that MediaWebContentsObsever should call AddClient() to add its binding into bindingset instead of using strongbinding. that's why it has to create two WakeLockServicePtrs, the first one is very similar to blink_wake_lock_, to create the WakeLockServiceImpl instance and add the second WakeLockServicePtr's Request into bindingset.
On 2017/05/03 10:29:00, ke.he wrote: > On 2017/05/03 09:13:13, blundell wrote: > > > https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... > > File device/wake_lock/wake_lock_service_impl.h (right): > > > > > https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... > > device/wake_lock/wake_lock_service_impl.h:76: > > mojo::BindingSet<mojom::WakeLockService, int> frames_binding_set_; > > On 2017/05/03 06:25:37, ke.he wrote: > > > On 2017/05/02 11:27:43, blundell wrote: > > > > 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. > > > > > > 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(); > > > > I'm confused: what prevents the client just using |wake_lock_| directly in > this > > scenario? > > > > > > > > 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! > > Potentially WakeLockServiceImpl at least serves two kinds of clients, one is > frames in blink, another is MediaWebContentsObsever. > For frames in blink, it is the WebContentsImpl create a blink_wake_lock_ first, > then all the WakeLockServiceRequests from blink are added into the bindingSet of > WakeLockServiceImpl via blink_wake_lock_.AddClient(). > For MediaWebContentsObsever, it calls > "webcontent()->GetWakeLockContext()->GetWakeLock(type, reason, description, > mojo::MakeRequest(&wake_lock_))", note that the wake_lock requests are Not added > into the bindingSet, because the AddClient() has never been called. Actually it > is a strongbinding, see "WakeLockServiceContext::GetWakeLock()". > > So in WakeLockServiceImpl, it have to cope with 2 kinds of bindings, one kind is > in bindingset, another is strongbinding. > You mentioned that we should make the WakeLockServiceContext to be dead-simple > and to stuff all the requests into bindingset, so I guess that > MediaWebContentsObsever should call AddClient() to add its binding into > bindingset instead of using strongbinding. that's why it has to create two > WakeLockServicePtrs, the first one is very similar to blink_wake_lock_, to > create the WakeLockServiceImpl instance and add the second WakeLockServicePtr's > Request into bindingset. Ah, I see the confusion here. What I meant was to also put the *initial request* in WakeLockServiceImpl's binding set. So WakeLockServiceContext will just pass that request on to the constructor of WakeLockServiceImpl. Does that make sense?
On 2017/05/03 10:30:59, blundell wrote: > On 2017/05/03 10:29:00, ke.he wrote: > > On 2017/05/03 09:13:13, blundell wrote: > > > > > > https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... > > > File device/wake_lock/wake_lock_service_impl.h (right): > > > > > > > > > https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... > > > device/wake_lock/wake_lock_service_impl.h:76: > > > mojo::BindingSet<mojom::WakeLockService, int> frames_binding_set_; > > > On 2017/05/03 06:25:37, ke.he wrote: > > > > On 2017/05/02 11:27:43, blundell wrote: > > > > > 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. > > > > > > > > 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(); > > > > > > I'm confused: what prevents the client just using |wake_lock_| directly in > > this > > > scenario? > > > > > > > > > > > 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! > > > > Potentially WakeLockServiceImpl at least serves two kinds of clients, one is > > frames in blink, another is MediaWebContentsObsever. > > For frames in blink, it is the WebContentsImpl create a blink_wake_lock_ > first, > > then all the WakeLockServiceRequests from blink are added into the bindingSet > of > > WakeLockServiceImpl via blink_wake_lock_.AddClient(). > > For MediaWebContentsObsever, it calls > > "webcontent()->GetWakeLockContext()->GetWakeLock(type, reason, description, > > mojo::MakeRequest(&wake_lock_))", note that the wake_lock requests are Not > added > > into the bindingSet, because the AddClient() has never been called. Actually > it > > is a strongbinding, see "WakeLockServiceContext::GetWakeLock()". > > > > So in WakeLockServiceImpl, it have to cope with 2 kinds of bindings, one kind > is > > in bindingset, another is strongbinding. > > You mentioned that we should make the WakeLockServiceContext to be dead-simple > > and to stuff all the requests into bindingset, so I guess that > > MediaWebContentsObsever should call AddClient() to add its binding into > > bindingset instead of using strongbinding. that's why it has to create two > > WakeLockServicePtrs, the first one is very similar to blink_wake_lock_, to > > create the WakeLockServiceImpl instance and add the second > WakeLockServicePtr's > > Request into bindingset. > > Ah, I see the confusion here. What I meant was to also put the *initial request* > in WakeLockServiceImpl's binding set. So WakeLockServiceContext will just pass > that request on to the constructor of WakeLockServiceImpl. Does that make sense? Yeah, make sense! Thanks very much:)
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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 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: This issue passed the CQ dry run.
Description was changed from ========== Allowing WakeLock to Serve Browser Process Clients Design doc by blundell@chromium.org: https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c In this CL: 1) Allow for creating PowerSaveBlockers with arbitrary (type, reason, description) parameters in the WakeLockContext mojo interface. 2) Continue to support having one PowerSaveBlocker per WebContents in which all connections from frames associated with that WebContents are coalesced. BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl This is the first step of "Allowing WakeLock to Serve Browser Process Clients" Design doc by blundell@chromium.org: https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c In this CL: 1) Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl. 2) Refactor WakeLockServiceImpl, one WakeLockServiceImpl supports managing multiple client bindings. 3) Make WebContentsImpl coalesce multiple client requests into one WakeLockServiceImpl. BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl This is the first step of "Allowing WakeLock to Serve Browser Process Clients" Design doc by blundell@chromium.org: https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c In this CL: 1) Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl. 2) Refactor WakeLockServiceImpl, one WakeLockServiceImpl supports managing multiple client bindings. 3) Make WebContentsImpl coalesce multiple client requests into one WakeLockServiceImpl. BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl This is the first step of "Allowing WakeLock to Serve Browser Process Clients" Design doc by blundell@chromium.org: https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c In this CL: 1) Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl. 2) Refactor WakeLockServiceImpl, one WakeLockServiceImpl supports managing multiple client bindings. 3) Make WebContentsImpl coalesce multiple client requests into one WakeLockServiceImpl. BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Colin, I split the "Generalize mojo interface" part out of this CL. PTAL. Thanks! https://codereview.chromium.org/2843353003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2843353003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:196: virtual void ConnectToBlinkWakeLock( On 2017/05/02 11:27:43, blundell wrote: > nit: On seeing this, I'm inclined to have the interface instead be > > // Gets the WakeLockService that services wake lock requests from the // > renderer. > WakeLockService* GetRendererWakeLock(); > > and move the AddClient() calls to RenderFrameHostImpl. I think that will make > what's happening slightly more transparent. Done. https://codereview.chromium.org/2843353003/diff/60001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_context_host.cc (right): https://codereview.chromium.org/2843353003/diff/60001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_context_host.cc:44: context_provider->GetWakeLockContextForID( On 2017/05/02 11:27:43, blundell wrote: > nit: Let's make this naming change in the CL that introduces the corresponding > method for getting a wake lock without context. Done. https://codereview.chromium.org/2843353003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2843353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:1518: device::mojom::WakeLockServicePtr blink_wake_lock_; On 2017/05/02 11:27:43, blundell wrote: > nit: let's name this |renderer_wake_lock_| actually. Done. https://codereview.chromium.org/2843353003/diff/60001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2843353003/diff/60001/content/public/browser/... content/public/browser/web_contents.h:664: virtual device::mojom::WakeLockContext* GetWakeLockContext() = 0; On 2017/05/02 11:27:43, blundell wrote: > nit: Nothing in this CL is using this method, right? It seems like we could > introduce it in the next CL (i.e., the one that uses it). It is used by WebContentsImpl::GetRendererWakeLock(), when init the |renderer_wake_lock_|. I feel it is also good to add the declaration in webcontent.h, Although it is ok to declare it only in web_content_impl.h for now. https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/public... File device/wake_lock/public/interfaces/wake_lock_service.mojom (right): https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/public... device/wake_lock/public/interfaces/wake_lock_service.mojom:12: // TODO(heke): The AddClient() interface is only used by WebContents for the On 2017/05/02 11:27:43, blundell wrote: > Actually, I think it's fine to leave this method on this interface. There is a > use case for coalescing requests from multiple clients. In the future, maybe > there will be clients other than Blink for that use case. Yes, Totally understand after the mail discussion. https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... File device/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... device/wake_lock/wake_lock_service_context.h:55: void HasWakeLockForTests( On 2017/05/02 11:27:43, blundell wrote: > IIUC, the only reason that this class still has any complexity is because of > this method. Let's move this method into WakeLockService; the browsertest can > access it by accessing the WebContent's |blink_wake_lock_|. Then this class > (i.e., WakeLockServiceContext) becomes dead-simple: it just has a StrongBinding > to its request, it doesn't need to track the WakeLockServiceImpls at all; > WakeLockServiceImpl can instead have a BindingSet to manage all its requests and > delete itself when that binding set becomes empty. Done. https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... File device/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/2843353003/diff/60001/device/wake_lock/wake_l... device/wake_lock/wake_lock_service_impl.h:76: mojo::BindingSet<mojom::WakeLockService, int> frames_binding_set_; On 2017/05/03 09:13:13, blundell wrote: > On 2017/05/03 06:25:37, ke.he wrote: > > On 2017/05/02 11:27:43, blundell wrote: > > > 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. > > > > 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(); > > I'm confused: what prevents the client just using |wake_lock_| directly in this > scenario? > > > > > 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! > Understand after the mail discussion, Done.
This is looking great! Thanks for all your hard work here! In addition to the comments below, could you augment the CL description with something like: The concrete motivation for this move is to make it trivial to implement a generalization of the GetWakeLock() API that passes in the parameters for the PowerSaveBlocker (needed by browser process clients). That interface extension would be complex to implement in the current architecture, which is multiplexing one PowerSaveBlocker for multiple WakeLockServiceImpl instances. https://codereview.chromium.org/2843353003/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2843353003/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2620: if (!wake_lock_context) { will this ever happen? https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/publi... File device/wake_lock/public/interfaces/wake_lock_service.mojom (right): https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:11: AddClient(WakeLockService& wake_lock); Let's document all 3 of these methods. https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:13: // Test-only method that returns whethere a wake lock is currently active. nit: s/whethere/whether https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_context.h:49: mojo::Binding<mojom::WakeLockContext> context_binding_; We can just make this a StrongBinding now. https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:68: int client_id = binding_set_.dispatch_context(); 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. https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:117: gfx::NativeView native_view = native_view_getter_.Run(context_id_); 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. https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:147: binding_set_.RemoveBinding(it2->second); 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.
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 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 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...
Colin, Thanks very much! CL is updated, PTAL. https://codereview.chromium.org/2843353003/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2843353003/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2620: if (!wake_lock_context) { On 2017/05/04 15:40:50, blundell wrote: > will this ever happen? Yes, in https://cs.chromium.org/chromium/src/content/browser/wake_lock/wake_lock_cont... There is a condition "if (ServiceManagerConnection::GetForProcess())" that possibly skip executing "mojo::MakeRequest(&wake_lock_context_)" https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/publi... File device/wake_lock/public/interfaces/wake_lock_service.mojom (right): https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:11: AddClient(WakeLockService& wake_lock); On 2017/05/04 15:40:50, blundell wrote: > Let's document all 3 of these methods. Done. https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:13: // Test-only method that returns whethere a wake lock is currently active. On 2017/05/04 15:40:50, blundell wrote: > nit: s/whethere/whether Done. https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_context.h:49: mojo::Binding<mojom::WakeLockContext> context_binding_; On 2017/05/04 15:40:50, blundell wrote: > We can just make this a StrongBinding now. Yes. and OnContextBindingError() is not needed either. Done. https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:68: int client_id = binding_set_.dispatch_context(); On 2017/05/04 15:40:50, blundell wrote: > 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. Yes, |outstandings_| is not needed. Done. https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:117: gfx::NativeView native_view = native_view_getter_.Run(context_id_); On 2017/05/04 15:40:50, blundell wrote: > 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. The type, reason and description has already been hardcoded in WakeLockServiceContext::GetWakeLock() which is the only place to create WakeLockServiceImpl. https://codereview.chromium.org/2843353003/diff/140001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:147: binding_set_.RemoveBinding(it2->second); On 2017/05/04 15:40:50, blundell wrote: > 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. Yes. we don't need |binding_ids_|. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl This is the first step of "Allowing WakeLock to Serve Browser Process Clients" Design doc by blundell@chromium.org: https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c In this CL: 1) Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl. 2) Refactor WakeLockServiceImpl, one WakeLockServiceImpl supports managing multiple client bindings. 3) Make WebContentsImpl coalesce multiple client requests into one WakeLockServiceImpl. BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl This is the first step of "Allowing WakeLock to Serve Browser Process Clients" Design doc by blundell@chromium.org: https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c In this CL: 1) Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl. 2) Refactor WakeLockServiceImpl, one WakeLockServiceImpl supports managing multiple client bindings. 3) Make WebContentsImpl coalesce multiple client requests into one WakeLockServiceImpl. The concrete motivation for this move is to make it trivial to implement a generalization of the GetWakeLock() API that passes in the parameters for the PowerSaveBlocker (needed by browser process clients). That interface extension would be complex to implement in the current architecture, which is multiplexing one PowerSaveBlocker for multiple WakeLockServiceImpl instances. BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Looks great! Just a few more minor things. https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/publi... File device/wake_lock/public/interfaces/wake_lock_service.mojom (right): https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:9: // Request WakeLock Service from WakeLockServiceImpl. nit: I would write something like: "Requests that a wake lock be applied on behalf of this client. Has no effect if the client has previously called this method without subsequently calling RequestWakeLock()." https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:15: // Cancel WakeLock Service. "Cancels any outstanding wake lock request from this client. If there are no more outstanding requests from any clients, the active wake lock (if there is one) will be turned off)." https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:18: // Add other clients to current WakeLockServiceImpl so it can serve multiple "Adds a client to this WakeLockService instance. Clients are grouped on a per-WakeLockService instance basis: that is, a given WakeLockService instance turns on its wake lock whenever *any* of its clients make a request to do so and turns off its wake lock only when *all* its clients that had previously called RequestWakelock() cancel their requests." https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:41: // from the same frame should be coalesced as one request. Everytime a new s/frame/client https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:43: // as its context, this context records the clients' outstanding status. , which records this client's request status. https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:98: #if defined(OS_ANDROID) What I' https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:99: gfx::NativeView native_view = native_view_getter_.Run(context_id_); 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. https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:112: if (binding_set_.dispatch_context()) { Hmm, why the need for this if? https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.h:23: using WakeLockContextCallback = base::Callback<gfx::NativeView(int)>; Just include wake_lock_service_context.h?
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...
Hi, Colin, Thanks very much for your help! PTAL:) https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/publi... File device/wake_lock/public/interfaces/wake_lock_service.mojom (right): https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:9: // Request WakeLock Service from WakeLockServiceImpl. On 2017/05/05 12:49:37, blundell wrote: > nit: I would write something like: > > "Requests that a wake lock be applied on behalf of this client. Has no effect if > the client has previously called this method without subsequently calling > RequestWakeLock()." Thanks very much. I'm sorry for my poor english. "subsequently calling RequestWakeLock()" -> "subsequently calling CancelWakeLock()", right? https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:15: // Cancel WakeLock Service. On 2017/05/05 12:49:37, blundell wrote: > "Cancels any outstanding wake lock request from this client. If there are no > more outstanding requests from any clients, the active wake lock (if there is > one) will be turned off)." Done. https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:18: // Add other clients to current WakeLockServiceImpl so it can serve multiple On 2017/05/05 12:49:37, blundell wrote: > "Adds a client to this WakeLockService instance. Clients are grouped on a > per-WakeLockService instance basis: that is, a given WakeLockService instance > turns on its wake lock whenever *any* of its clients make a request to do so and > turns off its wake lock only when *all* its clients that had previously called > RequestWakelock() cancel their requests." Done. https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:41: // from the same frame should be coalesced as one request. Everytime a new On 2017/05/05 12:49:38, blundell wrote: > s/frame/client Done. https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:43: // as its context, this context records the clients' outstanding status. On 2017/05/05 12:49:37, blundell wrote: > , which records this client's request status. Done. https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:99: gfx::NativeView native_view = native_view_getter_.Run(context_id_); On 2017/05/05 12:49:37, blundell wrote: > 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. Done. https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:112: if (binding_set_.dispatch_context()) { On 2017/05/05 12:49:37, blundell wrote: > Hmm, why the need for this if? it should be a DCHECK() instead of if(). DCHECK(binding_set_.dispatch_context()); https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/2843353003/diff/200001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.h:23: using WakeLockContextCallback = base::Callback<gfx::NativeView(int)>; On 2017/05/05 12:49:38, blundell wrote: > Just include wake_lock_service_context.h? Done.
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...
blundell@chromium.org changed reviewers: + jam@chromium.org, rockot@chromium.org, tsepez@chromium.org
LGTM, thanks! +rockot@ for //device OWNERS +jam@ for //content OWNERS +tsepez@ for mojom changes https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:99: return; I would do a NOTREACHED() here with a TODO(ke.he@intel.com): Fully generalize the WakeLock interface and impl.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
device lgtm https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/publi... File device/wake_lock/public/interfaces/wake_lock_service.mojom (right): https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:14: // Cancels any outstanding wake lock request from this client. If there are nit: How about "all outstanding wake lock requests" instead of "any outstanding wake lock request". https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.h:62: mojo::BindingSet<mojom::WakeLockService, std::unique_ptr<bool>> binding_set_; Please document the meaning of the context here instead of in the constructor impl.
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...)
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: This issue passed the CQ dry run.
ke.he@intel.com changed reviewers: + haraken@chromium.org
Jam@, PTAL on the //content. Haraken@, PTAL on the "third_party/WebKit/Source/web/tests/ScreenWakeLockTest.cpp" Thanks all! https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/publi... File device/wake_lock/public/interfaces/wake_lock_service.mojom (right): https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/publi... device/wake_lock/public/interfaces/wake_lock_service.mojom:14: // Cancels any outstanding wake lock request from this client. If there are On 2017/05/05 17:55:31, Ken Rockot wrote: > nit: How about "all outstanding wake lock requests" instead of "any outstanding > wake lock request". Done. https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:99: return; On 2017/05/05 16:28:04, blundell wrote: > I would do a NOTREACHED() here with a mailto:TODO(ke.he@intel.com): Fully generalize > the WakeLock interface and impl. Done. https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/2843353003/diff/240001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.h:62: mojo::BindingSet<mojom::WakeLockService, std::unique_ptr<bool>> binding_set_; On 2017/05/05 17:55:31, Ken Rockot wrote: > Please document the meaning of the context here instead of in the constructor > impl. Done.
WebKit LGTM
lgtm
The CQ bit was checked by ke.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, rockot@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2843353003/#ps260001 (title: "Addressed Ken's comments.")
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by ke.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, rockot@chromium.org, haraken@chromium.org, jam@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2843353003/#ps280001 (title: "code rebase")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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": 280001, "attempt_start_ts": 1494301283901130,
"parent_rev": "aa79e549a918ff2530db858e281eb9f1515c2d5b", "commit_rev":
"98b761e7e4ffdfa7344bd3de7e3203d2bec235f0"}
Message was sent while issue was closed.
Description was changed from ========== Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl This is the first step of "Allowing WakeLock to Serve Browser Process Clients" Design doc by blundell@chromium.org: https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c In this CL: 1) Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl. 2) Refactor WakeLockServiceImpl, one WakeLockServiceImpl supports managing multiple client bindings. 3) Make WebContentsImpl coalesce multiple client requests into one WakeLockServiceImpl. The concrete motivation for this move is to make it trivial to implement a generalization of the GetWakeLock() API that passes in the parameters for the PowerSaveBlocker (needed by browser process clients). That interface extension would be complex to implement in the current architecture, which is multiplexing one PowerSaveBlocker for multiple WakeLockServiceImpl instances. BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl This is the first step of "Allowing WakeLock to Serve Browser Process Clients" Design doc by blundell@chromium.org: https://docs.google.com/document/d/1AEmiwtEj1nfTCmqSneht0xad2k3huxZQsIRHvVz0S8c In this CL: 1) Move ownership of PowerSaveBlocker from WakeLockServiceContext to WakeLockServiceImpl. 2) Refactor WakeLockServiceImpl, one WakeLockServiceImpl supports managing multiple client bindings. 3) Make WebContentsImpl coalesce multiple client requests into one WakeLockServiceImpl. The concrete motivation for this move is to make it trivial to implement a generalization of the GetWakeLock() API that passes in the parameters for the PowerSaveBlocker (needed by browser process clients). That interface extension would be complex to implement in the current architecture, which is multiplexing one PowerSaveBlocker for multiple WakeLockServiceImpl instances. BUG=689410 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2843353003 Cr-Commit-Position: refs/heads/master@{#470241} Committed: https://chromium.googlesource.com/chromium/src/+/98b761e7e4ffdfa7344bd3de7e32... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/98b761e7e4ffdfa7344bd3de7e32...
Message was sent while issue was closed.
some final nits that I just saw for a followup. Thanks! https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:66: if (num_lock_requests_ > 0) { This can be a DCHECK instead of an if statement, right? You've just verified above that at least one client (this one) has an outstanding request. https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:115: DCHECK(binding_set_.dispatch_context()); We don't need this: It's a basic property of BindingSet that this DCHECK will be true here, and the proper place to check it would be in tests of BindingSet itself. I think it's more confusing to the reader to have it here than not. https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:124: // If |binding_set_| is empty, WakeLockServiceImpl should delele itself. delete
Message was sent while issue was closed.
some final nits that I just saw for a followup. Thanks! https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... File device/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:66: if (num_lock_requests_ > 0) { This can be a DCHECK instead of an if statement, right? You've just verified above that at least one client (this one) has an outstanding request. https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:115: DCHECK(binding_set_.dispatch_context()); We don't need this: It's a basic property of BindingSet that this DCHECK will be true here, and the proper place to check it would be in tests of BindingSet itself. I think it's more confusing to the reader to have it here than not. https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... device/wake_lock/wake_lock_service_impl.cc:124: // If |binding_set_| is empty, WakeLockServiceImpl should delele itself. delete
Message was sent while issue was closed.
On 2017/05/09 08:22:44, blundell wrote: > some final nits that I just saw for a followup. Thanks! > > https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... > File device/wake_lock/wake_lock_service_impl.cc (right): > > https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... > device/wake_lock/wake_lock_service_impl.cc:66: if (num_lock_requests_ > 0) { > This can be a DCHECK instead of an if statement, right? You've just verified > above that at least one client (this one) has an outstanding request. > > https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... > device/wake_lock/wake_lock_service_impl.cc:115: > DCHECK(binding_set_.dispatch_context()); > We don't need this: It's a basic property of BindingSet that this DCHECK will be > true here, and the proper place to check it would be in tests of BindingSet > itself. I think it's more confusing to the reader to have it here than not. > > https://codereview.chromium.org/2843353003/diff/280001/device/wake_lock/wake_... > device/wake_lock/wake_lock_service_impl.cc:124: // If |binding_set_| is empty, > WakeLockServiceImpl should delele itself. > delete Colin, I'll correct them soon, thanks! |
