Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(883)

Issue 1107333002: Wake Lock API implementation (Chromium part) (Closed)

Created:
5 years, 7 months ago by alogvinov
Modified:
4 years, 6 months ago
CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Wake Lock API implementation (Chromium part) This is Chromium part of Wake Lock API implementation as per specification: http://www.w3.org/TR/wake-lock/ The corresponding Blink part is submitted in issue 1084923002 Design document: https://docs.google.com/document/d/1KbIENP0wgxtSXDQFn9PbHZ_tAKZfR1Y8u4Hst8LpeaA/edit?usp=sharing R=tsepez@chromium.org R=nasko@chromium.org R=mlamouri@chromium.org R=jochen@chromium.org Committed: https://crrev.com/334a5a782b578f773f6cd95ee00918300725a2a8 Cr-Commit-Position: refs/heads/master@{#348602}

Patch Set 1 #

Total comments: 16

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 18

Patch Set 6 : #

Total comments: 17

Patch Set 7 : #

Total comments: 38

Patch Set 8 : #

Total comments: 18

Patch Set 9 : #

Total comments: 5

Patch Set 10 : #

Total comments: 2

Patch Set 11 : #

Patch Set 12 : #

Patch Set 13 : #

Total comments: 6

Patch Set 14 : #

Patch Set 15 : #

Patch Set 16 : added visibility check to browsertest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+845 lines, -1 line) Patch
M content/browser/frame_host/render_frame_host_delegate.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +13 lines, -0 lines 0 comments Download
A content/browser/wake_lock/wake_lock_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +392 lines, -0 lines 0 comments Download
A content/browser/wake_lock/wake_lock_service_context.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +67 lines, -0 lines 0 comments Download
A content/browser/wake_lock/wake_lock_service_context.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +89 lines, -0 lines 0 comments Download
A content/browser/wake_lock/wake_lock_service_context_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +96 lines, -0 lines 0 comments Download
A content/browser/wake_lock/wake_lock_service_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +40 lines, -0 lines 0 comments Download
A content/browser/wake_lock/wake_lock_service_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +33 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +4 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +7 lines, -0 lines 0 comments Download
M content/common/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
A content/common/wake_lock_service.mojom View 1 2 3 4 5 6 7 1 chunk +11 lines, -0 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -0 lines 0 comments Download
M content/content_common_mojo_bindings.gyp View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
M content/content_renderer.gypi View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 4 5 6 7 8 9 10 4 chunks +5 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +8 lines, -0 lines 0 comments Download
A content/renderer/wake_lock/wake_lock_dispatcher.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +31 lines, -0 lines 0 comments Download
A content/renderer/wake_lock/wake_lock_dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +30 lines, -0 lines 0 comments Download

Messages

Total messages: 52 (4 generated)
alogvinov
5 years, 7 months ago (2015-04-28 10:22:53 UTC) #1
Tom Sepez
https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/wake_lock_state.h File content/browser/wake_lock/wake_lock_state.h (right): https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/wake_lock_state.h#newcode17 content/browser/wake_lock/wake_lock_state.h:17: // Incapsulates wake lock state decision logic for WebContents. ...
5 years, 7 months ago (2015-04-28 16:22:54 UTC) #2
nasko
Just some initial comments, still need to read through most of the newly added code. ...
5 years, 7 months ago (2015-04-28 23:57:37 UTC) #3
mlamouri (slow - plz ping)
A had a quick look and left a few comments, see below. I will have ...
5 years, 7 months ago (2015-05-05 14:08:34 UTC) #4
alogvinov
https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/wake_lock_dispatcher_host.cc File content/browser/wake_lock/wake_lock_dispatcher_host.cc (right): https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/wake_lock_dispatcher_host.cc#newcode43 content/browser/wake_lock/wake_lock_dispatcher_host.cc:43: NotifyUpdate(); On 2015/05/05 14:08:33, Mounir Lamouri wrote: > Should ...
5 years, 7 months ago (2015-05-06 14:30:38 UTC) #5
mlamouri (slow - plz ping)
Is this ready to be reviewed again?
5 years, 4 months ago (2015-08-05 13:55:56 UTC) #6
alogvinov
On 2015/08/05 13:55:56, Mounir Lamouri wrote: > Is this ready to be reviewed again? Yes, ...
5 years, 4 months ago (2015-08-05 14:06:13 UTC) #7
mlamouri (slow - plz ping)
Having a dispatcher_host with a mojo service is unusual. Is that design used in the ...
5 years, 4 months ago (2015-08-06 09:18:51 UTC) #8
alogvinov
On 2015/08/06 09:18:51, Mounir Lamouri wrote: > Having a dispatcher_host with a mojo service is ...
5 years, 4 months ago (2015-08-11 13:06:34 UTC) #9
alogvinov
https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lock/wake_lock_dispatcher_host.h File content/browser/wake_lock/wake_lock_dispatcher_host.h (right): https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lock/wake_lock_dispatcher_host.h#newcode27 content/browser/wake_lock/wake_lock_dispatcher_host.h:27: ~WakeLockDispatcherHost() override; On 2015/08/06 09:18:51, Mounir Lamouri wrote: > ...
5 years, 4 months ago (2015-08-11 13:06:43 UTC) #10
mlamouri (slow - plz ping)
Sorry, for the delay, I left you a few comments. It generally looks pretty good! ...
5 years, 4 months ago (2015-08-21 10:03:36 UTC) #12
alogvinov
https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_lock/wake_lock_service_context.h File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_lock/wake_lock_service_context.h#newcode43 content/browser/wake_lock/wake_lock_service_context.h:43: class CONTENT_EXPORT Observer { On 2015/08/21 10:03:35, Mounir Lamouri ...
5 years, 3 months ago (2015-08-26 14:47:19 UTC) #13
mlamouri (slow - plz ping)
lgtm with comments applied. Thank you for working on that! https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_lock/wake_lock_browsertest.cc File content/browser/wake_lock/wake_lock_browsertest.cc (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_lock/wake_lock_browsertest.cc#newcode71 ...
5 years, 3 months ago (2015-08-31 14:59:16 UTC) #14
Tom Sepez
mojom lgtm
5 years, 3 months ago (2015-08-31 16:13:35 UTC) #15
alogvinov
https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_lock/wake_lock_browsertest.cc File content/browser/wake_lock/wake_lock_browsertest.cc (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_lock/wake_lock_browsertest.cc#newcode71 content/browser/wake_lock/wake_lock_browsertest.cc:71: void LockScreenInMainFrame() { On 2015/08/31 14:59:15, Mounir Lamouri wrote: ...
5 years, 3 months ago (2015-09-02 17:06:23 UTC) #16
alogvinov
5 years, 3 months ago (2015-09-02 17:06:24 UTC) #17
nasko
Thanks for the thorough test coverage! It is mostly there, but I have a few ...
5 years, 3 months ago (2015-09-02 22:40:15 UTC) #18
alogvinov
https://codereview.chromium.org/1107333002/diff/140001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/frame_host/render_frame_host_impl.cc#newcode1600 content/browser/frame_host/render_frame_host_impl.cc:1600: base::Unretained(this))); On 2015/09/02 22:40:14, nasko (slow to review) wrote: ...
5 years, 3 months ago (2015-09-03 16:25:00 UTC) #19
nasko
https://codereview.chromium.org/1107333002/diff/160001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1107333002/diff/160001/content/browser/frame_host/render_frame_host_impl.cc#newcode1602 content/browser/frame_host/render_frame_host_impl.cc:1602: // RenderFrameHost pointers. Additionally, WakeLockServiceContext observes This comment is ...
5 years, 3 months ago (2015-09-03 17:53:37 UTC) #20
alogvinov
https://codereview.chromium.org/1107333002/diff/160001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1107333002/diff/160001/content/browser/frame_host/render_frame_host_impl.cc#newcode1602 content/browser/frame_host/render_frame_host_impl.cc:1602: // RenderFrameHost pointers. Additionally, WakeLockServiceContext observes On 2015/09/03 17:53:37, ...
5 years, 3 months ago (2015-09-04 15:39:26 UTC) #21
nasko
https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_lock/wake_lock_service_context.cc File content/browser/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_lock/wake_lock_service_context.cc#newcode32 content/browser/wake_lock/wake_lock_service_context.cc:32: void WakeLockServiceContext::RenderFrameDeleted( On 2015/09/04 15:39:26, alogvinov wrote: > On ...
5 years, 3 months ago (2015-09-04 21:06:45 UTC) #22
alogvinov
On 2015/09/04 21:06:45, nasko (out until Sept 11) wrote: > https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_lock/wake_lock_service_context.cc > File content/browser/wake_lock/wake_lock_service_context.cc (right): ...
5 years, 3 months ago (2015-09-08 14:44:54 UTC) #23
alogvinov
https://codereview.chromium.org/1107333002/diff/180001/content/browser/wake_lock/wake_lock_service_context.h File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/180001/content/browser/wake_lock/wake_lock_service_context.h#newcode52 content/browser/wake_lock/wake_lock_service_context.h:52: // Set of all frame ids currently requesting wake ...
5 years, 3 months ago (2015-09-08 14:49:44 UTC) #24
nasko
In general, I'd love to see an improvement of the test to avoid the Sleep ...
5 years, 3 months ago (2015-09-08 17:57:52 UTC) #25
alogvinov
Jochen, can you please take a look at this CL?
5 years, 3 months ago (2015-09-10 09:10:01 UTC) #26
jochen (gone - plz use gerrit)
nasko already owns all of this
5 years, 3 months ago (2015-09-14 09:36:51 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1107333002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1107333002/220001
5 years, 3 months ago (2015-09-14 10:05:35 UTC) #30
alogvinov
On 2015/09/14 09:36:51, jochen wrote: > nasko already owns all of this Thank you!
5 years, 3 months ago (2015-09-14 10:12:31 UTC) #31
commit-bot: I haz the power
Committed patchset #12 (id:220001)
5 years, 3 months ago (2015-09-14 11:31:00 UTC) #32
commit-bot: I haz the power
Patchset 12 (id:??) landed as https://crrev.com/334a5a782b578f773f6cd95ee00918300725a2a8 Cr-Commit-Position: refs/heads/master@{#348602}
5 years, 3 months ago (2015-09-14 11:31:33 UTC) #33
Devlin
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1345563002/ by rdevlin.cronin@chromium.org. ...
5 years, 3 months ago (2015-09-14 19:43:48 UTC) #34
alogvinov
The original CL had a problem with cross-site navigation because it used frame node ids ...
5 years, 3 months ago (2015-09-16 14:07:02 UTC) #35
alogvinov
The original CL had a problem with cross-site navigation because it used frame node ids ...
5 years, 3 months ago (2015-09-16 14:07:05 UTC) #36
alogvinov
The original CL had a problem with cross-site navigation because it used frame node ids ...
5 years, 3 months ago (2015-09-16 14:07:06 UTC) #37
nasko
On 2015/09/16 14:07:06, alogvinov wrote: > The original CL had a problem with cross-site navigation ...
5 years, 3 months ago (2015-09-16 14:11:03 UTC) #38
nasko
LGTM with a few nits. https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_lock/wake_lock_service_context.h File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_lock/wake_lock_service_context.h#newcode37 content/browser/wake_lock/wake_lock_service_context.h:37: // Requests wake lock ...
5 years, 3 months ago (2015-09-16 14:17:03 UTC) #39
alogvinov
https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_lock/wake_lock_service_context.h File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_lock/wake_lock_service_context.h#newcode37 content/browser/wake_lock/wake_lock_service_context.h:37: // Requests wake lock for render frame identified by ...
5 years, 3 months ago (2015-09-16 16:30:19 UTC) #40
alogvinov
I'm still experiencing content_browsertests WakeLockTest flakiness isolated to Mac builds, like for example here: http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_dbg_ng/builds/463 ...
5 years, 3 months ago (2015-09-17 06:50:09 UTC) #41
alogvinov
I've confirmed flakiness of WakeLockTest on Mac to be a visibility issue by adding a ...
5 years, 3 months ago (2015-09-17 07:46:17 UTC) #42
commit-bot: I haz the power
Patchset 16 (id:??) landed as https://crrev.com/334a5a782b578f773f6cd95ee00918300725a2a8 Cr-Commit-Position: refs/heads/master@{#348602}
5 years, 3 months ago (2015-09-23 12:31:39 UTC) #43
jdduke (slow)
Is there no bug for this feature? Mounir, I thought we had agreed not to ...
5 years, 2 months ago (2015-10-05 20:19:34 UTC) #45
mlamouri (slow - plz ping)
On 2015/10/05 at 20:19:34, jdduke wrote: > Is there no bug for this feature? > ...
5 years, 2 months ago (2015-10-05 22:51:15 UTC) #46
mlamouri (slow - plz ping)
On 2015/10/05 at 22:51:15, Mounir Lamouri wrote: > On 2015/10/05 at 20:19:34, jdduke wrote: > ...
5 years, 2 months ago (2015-10-05 22:52:16 UTC) #47
alogvinov
On 2015/10/05 22:52:16, Mounir Lamouri wrote: > On 2015/10/05 at 22:51:15, Mounir Lamouri wrote: > ...
5 years, 2 months ago (2015-10-12 07:36:46 UTC) #48
mlamouri (slow - plz ping)
On 2015/10/12 at 07:36:46, alogvinov wrote: > On 2015/10/05 22:52:16, Mounir Lamouri wrote: > > ...
5 years, 2 months ago (2015-10-12 10:39:13 UTC) #49
jochen (gone - plz use gerrit)
On 2015/10/12 at 10:39:13, mlamouri wrote: > On 2015/10/12 at 07:36:46, alogvinov wrote: > > ...
5 years, 2 months ago (2015-10-12 11:02:48 UTC) #50
alogvinov
On 2015/10/12 11:02:48, jochen wrote: > On 2015/10/12 at 10:39:13, mlamouri wrote: > > On ...
5 years, 2 months ago (2015-10-12 13:02:39 UTC) #51
Ben Goodger (Google)
4 years, 6 months ago (2016-06-21 04:03:41 UTC) #52
Message was sent while issue was closed.
On 2015/09/08 17:57:52, nasko (Out until 24 June) wrote:
> In general, I'd love to see an improvement of the test to avoid the Sleep
call,
> but I don't have a good suggestion at this point. nick@ might be someone to
> consult on that.
> 
> LGTM

Indeed. We should not be adding tests like this as they are the source of flake.
I ran into this one today, and had I not corrected other issues first I might
have disabled this whole suite having seen this line.

Here's my recommendation:

- WakeLockServiceContext should offer a test API to register a callback or
interface to be notified of changes to the context.
- This test should use that, and spin a runloop until change is observed.

This is how we do basically all async Mojo tests. See //services/shell/tests for
many examples.

I just filed: http://crbug.com/621778 - help triaging would be appreciated.

Powered by Google App Engine
This is Rietveld 408576698