Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2893873002:
Convert PowerSaveBlockResourceThrottle to be client of WakeLock mojo service. (Closed)
DescriptionConvert PowerSaveBlockResourceThrottle 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
TBR=jam@chromium.org, for mechanical changes to //content/browser/BUILD.gn
Review-Url: https://codereview.chromium.org/2893873002
Cr-Commit-Position: refs/heads/master@{#473076}
Committed: https://chromium.googlesource.com/chromium/src/+/91eabe25b470e6b042b78a333f4bbc62900d2916
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed Charlie's comments. #
Total comments: 2
Patch Set 3 : Convert PowerSaveBlockResourceThrottle to be client of WakeLock mojo service. #
Messages
Total messages: 30 (18 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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ke.he@intel.com changed reviewers: + csharrison@chromium.org
Hi, Charlie, Could you PTAL on this, thanks! https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... File content/browser/loader/wake_lock_resource_throttle.cc (right): https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:49: DCHECK_CURRENTLY_ON(BrowserThread::IO); I think the original PowerSaveBlockResourceThrottle always runs in IO thread, right? https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:50: DCHECK(!wake_lock_); This ActivateWakeLock() will never be called twice, right? So I DCHECK the wake_lock_ be initiallized only once.
Hey ke.he this looks reasonable but I'm not thrilled about the design changes to the PowerSaveBlocker class. The benefit of the old style is that since PowerSaveBlocker is RAII we know that the wake lock will not be held when the class goes away. Without that guarantee I'm afraid we'll have to guard the destructor with: if (wake_lock_) wake_lock_->CancelWakeLock() This is really dangerous, because it is easy for users of the API to forget this, and holding on to the wake lock for too long will provide the user with a really bad experience. On the other hand, I could be misunderstanding how the mojo interface works. https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... File content/browser/loader/wake_lock_resource_throttle.cc (right): https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. Copyright 2017 https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:49: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/05/18 13:40:09, ke.he wrote: > I think the original PowerSaveBlockResourceThrottle always runs in IO thread, > right? Yep, all ResourceThrottles run on the IO thread. https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:50: DCHECK(!wake_lock_); On 2017/05/18 13:40:09, ke.he wrote: > This ActivateWakeLock() will never be called twice, right? So I DCHECK the > wake_lock_ be initiallized only once. Yep, WillStartRequest should only be called once. https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... File content/browser/loader/wake_lock_resource_throttle.h (right): https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. Copyright 2017
On 2017/05/18 13:56:42, Charlie Harrison wrote: > Hey ke.he this looks reasonable but I'm not thrilled about the design changes to > the PowerSaveBlocker class. > > The benefit of the old style is that since PowerSaveBlocker is RAII we know that > the wake lock will not be held when the class goes away. Without that guarantee > I'm afraid we'll have to guard the destructor with: > > if (wake_lock_) > wake_lock_->CancelWakeLock() > > This is really dangerous, because it is easy for users of the API to forget > this, and holding on to the wake lock for too long will provide the user with a > really bad experience. > > On the other hand, I could be misunderstanding how the mojo interface works. Thanks for your quick response:) Actually we don't need to call CancelWakeLock() in destructor. When this instance is destructed, the free of |wake_lock_| will trigger the WakeLockServiceImpl::OnConnectionError() in remote service side. In which the WakeLockServiceImpl is responsible for making everything is correct. So it has the same effect as calling CancelWakeLock() in destructor. So we still keep the RAII style here.
https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... File content/browser/loader/wake_lock_resource_throttle.cc (right): https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2017/05/18 13:56:42, Charlie Harrison wrote: > Copyright 2017 Actually this file is renamed from the original power_save_block_resource_throttle.cc, I think maybe we would keep the "2013" here? Same in the ".h" file. https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:49: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/05/18 13:56:42, Charlie Harrison wrote: > On 2017/05/18 13:40:09, ke.he wrote: > > I think the original PowerSaveBlockResourceThrottle always runs in IO thread, > > right? > > Yep, all ResourceThrottles run on the IO thread. Thanks!
Thanks for clearing up my understanding, I feel a lot better about this. Would you document the RAII behavior in the header above |wake_lock_|? Generally this l-g-t-m but now that we have a mojo interface, is this now testable in an easier way? We've been meaning to write tests for this class for a while. Would you have time to add a test harness? The core logic should be similar (but simpler) than the ThrottlingResourceHandlerTest, with just a single throttle (this one). https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... File content/browser/loader/wake_lock_resource_throttle.cc (right): https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:48: void WakeLockResourceThrottle::ActivateWakeLock() { Maybe s/Activate/Request ? https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:59: mojo::MakeRequest(&wake_lock_provider)); Do we need to include mojo/public/cpp/bindings/interface_request.h ?
Charlie, thanks for your comments. CL is updated. I'll have a look on the ThrottlingResourceHandlerTest later, sorry the code in /loader is quite new for me. I feel the mojofication make it harder instead of easier to write unittest. In original code, we can check the power_save_blocker_ pointer is nullptr or not directly, while in mojo case, sometimes we have to create a fake WakeLockServiceImpl to implement the mojo interface. https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... File content/browser/loader/wake_lock_resource_throttle.cc (right): https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:48: void WakeLockResourceThrottle::ActivateWakeLock() { On 2017/05/18 14:46:17, Charlie Harrison wrote: > Maybe s/Activate/Request ? Done. https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... content/browser/loader/wake_lock_resource_throttle.cc:59: mojo::MakeRequest(&wake_lock_provider)); On 2017/05/18 14:46:17, Charlie Harrison wrote: > Do we need to include mojo/public/cpp/bindings/interface_request.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...
On 2017/05/18 15:35:15, ke.he wrote: > Charlie, thanks for your comments. CL is updated. > > I'll have a look on the ThrottlingResourceHandlerTest later, sorry the code in > /loader is quite new for me. > > I feel the mojofication make it harder instead of easier to write unittest. In > original code, we can check the power_save_blocker_ pointer is nullptr or not > directly, while in mojo case, sometimes we have to create a fake > WakeLockServiceImpl to implement the mojo interface. It's a fair point, but that would require exposing the internals of the resource throttle to the test. Presumably with this change we can observe the changes outside by monitoring the actual wake lock service so tests don't have to introspect the throttle state. Let me know if this blocks you unduly, we could maybe land tests in a followup. > > https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... > File content/browser/loader/wake_lock_resource_throttle.cc (right): > > https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... > content/browser/loader/wake_lock_resource_throttle.cc:48: void > WakeLockResourceThrottle::ActivateWakeLock() { > On 2017/05/18 14:46:17, Charlie Harrison wrote: > > Maybe s/Activate/Request ? > > Done. > > https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... > content/browser/loader/wake_lock_resource_throttle.cc:59: > mojo::MakeRequest(&wake_lock_provider)); > On 2017/05/18 14:46:17, Charlie Harrison wrote: > > Do we need to include mojo/public/cpp/bindings/interface_request.h ? > > Done.
On 2017/05/18 15:47:14, Charlie Harrison wrote: > On 2017/05/18 15:35:15, ke.he wrote: > > Charlie, thanks for your comments. CL is updated. > > > > I'll have a look on the ThrottlingResourceHandlerTest later, sorry the code in > > /loader is quite new for me. > > > > I feel the mojofication make it harder instead of easier to write unittest. In > > original code, we can check the power_save_blocker_ pointer is nullptr or not > > directly, while in mojo case, sometimes we have to create a fake > > WakeLockServiceImpl to implement the mojo interface. > It's a fair point, but that would require exposing the internals of the resource > throttle to the test. > Presumably with this change we can observe the changes outside by monitoring the > actual wake lock service so tests don't have to introspect the throttle state. > > Let me know if this blocks you unduly, we could maybe land tests in a followup. > > > > > https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... > > File content/browser/loader/wake_lock_resource_throttle.cc (right): > > > > > https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... > > content/browser/loader/wake_lock_resource_throttle.cc:48: void > > WakeLockResourceThrottle::ActivateWakeLock() { > > On 2017/05/18 14:46:17, Charlie Harrison wrote: > > > Maybe s/Activate/Request ? > > > > Done. > > > > > https://codereview.chromium.org/2893873002/diff/1/content/browser/loader/wake... > > content/browser/loader/wake_lock_resource_throttle.cc:59: > > mojo::MakeRequest(&wake_lock_provider)); > > On 2017/05/18 14:46:17, Charlie Harrison wrote: > > > Do we need to include mojo/public/cpp/bindings/interface_request.h ? > > > > Done. I'll add the unittest, no problem. Yes I prefer to add it in a followup, thank you:)
LGTM https://codereview.chromium.org/2893873002/diff/20001/content/browser/loader/... File content/browser/loader/wake_lock_resource_throttle.h (right): https://codereview.chromium.org/2893873002/diff/20001/content/browser/loader/... content/browser/loader/wake_lock_resource_throttle.h:36: // Destruction of wake_lock_ will trigger Suggested rephrasing: Destruction of wake_lock_ will trigger WakeLockServiceImpl::OnConnection error on the service side, so there is no need to call CancelWakeLock() in the destructor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 ke.he@intel.com
https://codereview.chromium.org/2893873002/diff/20001/content/browser/loader/... File content/browser/loader/wake_lock_resource_throttle.h (right): https://codereview.chromium.org/2893873002/diff/20001/content/browser/loader/... content/browser/loader/wake_lock_resource_throttle.h:36: // Destruction of wake_lock_ will trigger On 2017/05/18 16:43:49, Charlie Harrison wrote: > Suggested rephrasing: > Destruction of wake_lock_ will trigger WakeLockServiceImpl::OnConnection error > on the service side, so there is no need to call CancelWakeLock() in the > destructor. Done.
Description was changed from ========== Convert PowerSaveBlockResourceThrottle 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 PowerSaveBlockResourceThrottle 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 TBR=jam@chromium.org, for mechanical changes to //content/browser/BUILD.gn ==========
ke.he@intel.com changed reviewers: + jam@chromium.org
The CQ bit was checked by ke.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2893873002/#ps40001 (title: "Convert PowerSaveBlockResourceThrottle to be client of WakeLock mojo service.")
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": 40001, "attempt_start_ts": 1495166138108920,
"parent_rev": "de85aa9a225e731e84cc0837767c5c88bfead6c1", "commit_rev":
"91eabe25b470e6b042b78a333f4bbc62900d2916"}
Message was sent while issue was closed.
Description was changed from ========== Convert PowerSaveBlockResourceThrottle 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 TBR=jam@chromium.org, for mechanical changes to //content/browser/BUILD.gn ========== to ========== Convert PowerSaveBlockResourceThrottle 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 TBR=jam@chromium.org, for mechanical changes to //content/browser/BUILD.gn Review-Url: https://codereview.chromium.org/2893873002 Cr-Commit-Position: refs/heads/master@{#473076} Committed: https://chromium.googlesource.com/chromium/src/+/91eabe25b470e6b042b78a333f4b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/91eabe25b470e6b042b78a333f4b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
