Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(27)

Issue 2734943003: Device Service: Decouple Wake Lock from //content (Closed)

Created:
2 years, 5 months ago by blundell
Modified:
2 years, 5 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.

Description

Device Service: Decouple Wake Lock from //content Wake Lock presents challenges in decoupling from //content: (1) It is naturally a per-WebContents interface, as it coalesces requests made from frames within the same WebContents. (2) On Android, it needs the NativeView of the WebContents in order to block the screen from going to sleep. This CL takes the following approach: (1) Adds the notion of a context ID, which is set on WakeLockServiceContext and which the Device Service can use to map contexts to NativeViews (via a callback that is passed in at Device Service construction time by its embedder). (2) Adds a WakeLockContext Mojo interface. This interface can be used to forward requests to bind wake locks from untrusted clients that are made within the same (trusted-client specific) context. (3) Adds a WakeLockContextProvider Mojo interface. This interface allows for getting a WakeLockContext that is associated with a specific context ID. The WakeLockContext impl uses that context ID on Android to obtain the NativeView for its context. Note that the lifetime model of WakeLockContext is somewhat complex: It must stay alive as long as either (1) Its Mojo connection is still valid (as the client might make future GetWakeLock() calls) OR (2) There are still live WakeLock instances that it has instantiated (since they call into it when they receive Mojo requests from *their* clients). Consequently, WakeLockContext monitors the state of the connections described in (1) and (2), dying only when *all* of those connections go away. As a final subtlety, WakeLockServiceImpl calls back into WakeLockContext from its destructor, which executes immediately *after* WakeLockContext receives the callback that that WakeLockServiceImpl instance has experienced a connection error. WakeLockContext thus posts a task to delete itself rather than deleting itself synchronously. BUG=689403 TEST=On Chrome on Android, go to about://flags and turn on "Experimental Web Platform features." Restart Chrome. Go to https://colinblundell.github.io/wake_lock.html and verify that the page says "Wake Lock API found". Verify that if you wait ~40 seconds, the device goes to sleep. Tap "Block sleep." Verify that if you wait longer than the above period, the device does not go to sleep. Tap "Unblock sleep" and verify again that the device goes to sleep after waiting. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2734943003 Cr-Commit-Position: refs/heads/master@{#459719} Committed: https://chromium.googlesource.com/chromium/src/+/e75a8f91ce3a7a09d0da15f1c1fc83a79a668b8e

Patch Set 1 #

Patch Set 2 : Bugfix #

Total comments: 7

Patch Set 3 : Rebase #

Patch Set 4 : Response to review + rebase #

Total comments: 34

Patch Set 5 : Rebase #

Patch Set 6 : Response to reviews #

Total comments: 1

Patch Set 7 : Rebase #

Total comments: 4

Patch Set 8 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+441 lines, -126 lines) Patch
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +3 lines, -1 line 0 comments Download
M content/browser/DEPS View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.h View 1 2 3 2 chunks +5 lines, -2 lines 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 2 chunks +4 lines, -3 lines 0 comments Download
M content/browser/service_manager/service_manager_context.cc View 1 2 3 4 5 6 7 2 chunks +11 lines, -0 lines 0 comments Download
M content/browser/wake_lock/wake_lock_browsertest.cc View 1 2 3 4 5 3 chunks +13 lines, -3 lines 0 comments Download
A content/browser/wake_lock/wake_lock_context_host.h View 1 2 3 4 5 1 chunk +48 lines, -0 lines 0 comments Download
A content/browser/wake_lock/wake_lock_context_host.cc View 1 2 3 4 5 1 chunk +60 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 2 3 4 5 6 7 5 chunks +4 lines, -3 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 4 5 6 7 3 chunks +4 lines, -6 lines 0 comments Download
M content/public/app/mojo/content_browser_manifest.json View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M device/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M device/wake_lock/BUILD.gn View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M device/wake_lock/public/interfaces/BUILD.gn View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A device/wake_lock/public/interfaces/README.md View 1 2 3 4 5 1 chunk +19 lines, -0 lines 0 comments Download
A device/wake_lock/public/interfaces/wake_lock_context.mojom View 1 2 3 1 chunk +17 lines, -0 lines 0 comments Download
A device/wake_lock/public/interfaces/wake_lock_context_provider.mojom View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
A device/wake_lock/wake_lock_context_provider.h View 1 2 3 4 5 1 chunk +44 lines, -0 lines 0 comments Download
A device/wake_lock/wake_lock_context_provider.cc View 1 2 3 4 5 1 chunk +40 lines, -0 lines 0 comments Download
M device/wake_lock/wake_lock_service_context.h View 1 2 3 4 5 3 chunks +41 lines, -11 lines 0 comments Download
M device/wake_lock/wake_lock_service_context.cc View 1 2 3 4 5 4 chunks +36 lines, -11 lines 0 comments Download
D device/wake_lock/wake_lock_service_context_unittest.cc View 1 chunk +0 lines, -64 lines 0 comments Download
M device/wake_lock/wake_lock_service_impl.h View 1 2 3 3 chunks +3 lines, -3 lines 0 comments Download
M device/wake_lock/wake_lock_service_impl.cc View 1 2 3 3 chunks +3 lines, -4 lines 0 comments Download
M services/device/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M services/device/DEPS View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M services/device/device_service.h View 1 2 3 4 5 6 7 5 chunks +23 lines, -1 line 0 comments Download
M services/device/device_service.cc View 1 2 3 4 5 6 7 6 chunks +35 lines, -9 lines 0 comments Download
M services/device/manifest.json View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 80 (54 generated)
blundell
As I started putting this together, I realized that the natural context granularity for WakeLock ...
2 years, 5 months ago (2017-03-07 20:41:15 UTC) #11
Ken Rockot(use gerrit already)
This pattern makes me super sad, but it highlights a real problem we need to ...
2 years, 5 months ago (2017-03-10 16:23:25 UTC) #12
ke.he
https://codereview.chromium.org/2734943003/diff/100001/content/browser/wake_lock/wake_lock_context_host.cc File content/browser/wake_lock/wake_lock_context_host.cc (right): https://codereview.chromium.org/2734943003/diff/100001/content/browser/wake_lock/wake_lock_context_host.cc#newcode32 content/browser/wake_lock/wake_lock_context_host.cc:32: : id_(g_unique_id.GetNext()), web_contents_(web_contents) { Glad to see the ID ...
2 years, 5 months ago (2017-03-15 08:09:38 UTC) #23
ke.he
https://codereview.chromium.org/2734943003/diff/100001/device/wake_lock/wake_lock_service_context.cc File device/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/2734943003/diff/100001/device/wake_lock/wake_lock_service_context.cc#newcode68 device/wake_lock/wake_lock_service_context.cc:68: device::PowerSaveBlocker::kPowerSaveBlockPreventDisplaySleep, See below link where it creates 2 PowerSaveBlocker ...
2 years, 5 months ago (2017-03-15 09:33:42 UTC) #24
blundell
Hi Ke He, Thanks for the comments! I think that your reasoning broadly makes sense, ...
2 years, 5 months ago (2017-03-15 11:06:11 UTC) #25
blundell
Hi Ken, This is ready for re-review now. My responses to your comments are on ...
2 years, 5 months ago (2017-03-15 15:25:10 UTC) #39
blundell
Daniel, could you do a review of //device/wake_lock/public/interfaces? I'm interested to get your thoughts (you'll ...
2 years, 5 months ago (2017-03-15 15:26:31 UTC) #41
Ken Rockot(use gerrit already)
Looks good (well, as good as this general approach *can* look) overall. https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/public/interfaces/README.mojom File device/wake_lock/public/interfaces/README.mojom ...
2 years, 5 months ago (2017-03-16 17:48:40 UTC) #42
dcheng
It seems reasonable overall. I am a little sad that we need to plumb through ...
2 years, 5 months ago (2017-03-17 06:55:26 UTC) #43
blundell
Thanks for the reviews! Daniel: I agree with the philosophy of avoiding "context IDs" and ...
2 years, 5 months ago (2017-03-17 12:28:23 UTC) #53
blundell
Ken/Daniel: ping.
2 years, 5 months ago (2017-03-21 08:44:06 UTC) #54
dcheng
https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_lock/wake_lock_context_host.cc File content/browser/wake_lock/wake_lock_context_host.cc (right): https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_lock/wake_lock_context_host.cc#newcode36 content/browser/wake_lock/wake_lock_context_host.cc:36: if (ServiceManagerConnection::GetForProcess()) { On 2017/03/17 12:28:21, blundell wrote: > ...
2 years, 5 months ago (2017-03-22 10:09:55 UTC) #55
blundell
https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_lock/wake_lock_context_host.cc File content/browser/wake_lock/wake_lock_context_host.cc (right): https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_lock/wake_lock_context_host.cc#newcode36 content/browser/wake_lock/wake_lock_context_host.cc:36: if (ServiceManagerConnection::GetForProcess()) { On 2017/03/22 10:09:55, dcheng wrote: > ...
2 years, 5 months ago (2017-03-22 13:03:40 UTC) #56
Ken Rockot(use gerrit already)
https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_lock_service_context.cc File device/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_lock_service_context.cc#newcode18 device/wake_lock/wake_lock_service_context.cc:18: mojom::WakeLockContextRequest request, On 2017/03/22 at 13:03:40, blundell wrote: > ...
2 years, 5 months ago (2017-03-22 18:25:05 UTC) #57
Ken Rockot(use gerrit already)
(I was supposed to LGTM with that last comment. Oops. LGTM.)
2 years, 5 months ago (2017-03-22 20:01:57 UTC) #58
dcheng
https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_lock_service_context.cc File device/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_lock_service_context.cc#newcode18 device/wake_lock/wake_lock_service_context.cc:18: mojom::WakeLockContextRequest request, On 2017/03/22 18:25:05, Ken Rockot wrote: > ...
2 years, 5 months ago (2017-03-22 20:07:32 UTC) #59
Ken Rockot(use gerrit already)
On 2017/03/22 at 20:07:32, dcheng wrote: > https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_lock_service_context.cc > File device/wake_lock/wake_lock_service_context.cc (right): > > https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_lock_service_context.cc#newcode18 ...
2 years, 5 months ago (2017-03-22 20:20:45 UTC) #60
dcheng
On 2017/03/22 20:20:45, Ken Rockot wrote: > On 2017/03/22 at 20:07:32, dcheng wrote: > > ...
2 years, 5 months ago (2017-03-22 20:27:54 UTC) #61
Ken Rockot(use gerrit already)
On Wed, Mar 22, 2017 at 1:27 PM, <dcheng@chromium.org> wrote: > On 2017/03/22 20:20:45, Ken ...
2 years, 5 months ago (2017-03-22 20:29:11 UTC) #62
dcheng
Thanks for helping me understand. LGTM =)
2 years, 5 months ago (2017-03-22 20:30:01 UTC) #63
blundell
jam@: Can you review //content? Thanks!
2 years, 5 months ago (2017-03-23 09:38:33 UTC) #65
jam
lgtm
2 years, 5 months ago (2017-03-24 14:28:57 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2734943003/200001
2 years, 5 months ago (2017-03-24 14:36:46 UTC) #68
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/176971) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, ...
2 years, 5 months ago (2017-03-24 14:39:58 UTC) #70
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2734943003/220001
2 years, 5 months ago (2017-03-27 08:07:16 UTC) #77
commit-bot: I haz the power
2 years, 5 months ago (2017-03-27 08:12:50 UTC) #80
Message was sent while issue was closed.
Committed patchset #8 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/e75a8f91ce3a7a09d0da15f1c1fc...

Powered by Google App Engine
This is Rietveld 408576698