Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1107333002:
Wake Lock API implementation (Chromium part) (Closed)
|
Created:
5 years, 7 months ago by alogvinov Modified:
4 years, 6 months ago Reviewers:
Tom Sepez, jdduke (slow), jochen (gone - plz use gerrit), nasko, mlamouri (slow - plz ping) 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. |
DescriptionWake 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 #Messages
Total messages: 52 (4 generated)
https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... File content/browser/wake_lock/wake_lock_state.h (right): https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... content/browser/wake_lock/wake_lock_state.h:17: // Incapsulates wake lock state decision logic for WebContents. Clients update Sp: Encapsulates? https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... content/browser/wake_lock/wake_lock_state.h:24: void SetFrameLock(const RenderFrameHost* frame, bool intention); Nit: cleaner if we removed |intention| and had this always lock the frame, and used RemoveFrame to clear it? https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... File content/browser/wake_lock/wake_lock_state_unittest.cc (right): https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... content/browser/wake_lock/wake_lock_state_unittest.cc:13: static const RenderFrameHost* MakeFakeRFHPtr(int fake_ptr_value) { Does this build everywhere without something like "pointer value too small" warnings? Maybe use uintptr_t as the type for fake_ptr_value. https://codereview.chromium.org/1107333002/diff/1/content/common/wake_lock_se... File content/common/wake_lock_service.mojom (right): https://codereview.chromium.org/1107333002/diff/1/content/common/wake_lock_se... content/common/wake_lock_service.mojom:9: KeepScreenAwake(bool intention); Nit: again, mauybe it would be clearer with two methods rather than passing a bool?
Just some initial comments, still need to read through most of the newly added code. https://codereview.chromium.org/1107333002/diff/1/content/content_renderer.gypi File content/content_renderer.gypi (right): https://codereview.chromium.org/1107333002/diff/1/content/content_renderer.gy... content/content_renderer.gypi:434: 'renderer/wake_lock/wake_lock_dispatcher.cc', nit: Do we need a full new directory for just a couple of small files? https://codereview.chromium.org/1107333002/diff/1/content/renderer/wake_lock/... File content/renderer/wake_lock/wake_lock_dispatcher.cc (right): https://codereview.chromium.org/1107333002/diff/1/content/renderer/wake_lock/... content/renderer/wake_lock/wake_lock_dispatcher.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1107333002/diff/1/content/renderer/wake_lock/... File content/renderer/wake_lock/wake_lock_dispatcher.h (right): https://codereview.chromium.org/1107333002/diff/1/content/renderer/wake_lock/... content/renderer/wake_lock/wake_lock_dispatcher.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1107333002/diff/1/content/renderer/wake_lock/... content/renderer/wake_lock/wake_lock_dispatcher.h:14: // WakeLockDispatcher is a delegate for WakeLock messages used by WebKit. s/WebKit/Blink/ https://codereview.chromium.org/1107333002/diff/1/content/test/data/wake_lock... File content/test/data/wake_lock/empty_page.html (right): https://codereview.chromium.org/1107333002/diff/1/content/test/data/wake_lock... content/test/data/wake_lock/empty_page.html:1: <!DOCTYPE html> Why do we need wake lock specific empty page? There are few generic pages, such as title1.html in the root test data directory. Is there a reason why those don't work?
A had a quick look and left a few comments, see below. I will have a deeper look when the Blink CL will be ready/landed. https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... File content/browser/wake_lock/wake_lock_dispatcher_host.cc (right): https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... content/browser/wake_lock/wake_lock_dispatcher_host.cc:43: NotifyUpdate(); Should this only be called when there is an actual update? https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... File content/browser/wake_lock/wake_lock_state.cc (right): https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... content/browser/wake_lock/wake_lock_state.cc:19: RemoveFrame(frame); It's fairly odd that inserting a frame is done inline while removing isn't. Maybe you could keep it consistent? https://codereview.chromium.org/1107333002/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1107333002/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.h:189: return wake_lock_dispatcher_host_.get(); You don't seem to use that except for tests. Maybe you could add a friend or a wake_lock_dispatcher_host_for_testing() method instead? https://codereview.chromium.org/1107333002/diff/1/content/common/wake_lock_se... File content/common/wake_lock_service.mojom (right): https://codereview.chromium.org/1107333002/diff/1/content/common/wake_lock_se... content/common/wake_lock_service.mojom:9: KeepScreenAwake(bool intention); On 2015/04/28 at 16:22:54, Tom Sepez wrote: > Nit: again, mauybe it would be clearer with two methods rather than passing a bool? +1 https://codereview.chromium.org/1107333002/diff/1/content/renderer/wake_lock/... File content/renderer/wake_lock/wake_lock_dispatcher.h (right): https://codereview.chromium.org/1107333002/diff/1/content/renderer/wake_lock/... content/renderer/wake_lock/wake_lock_dispatcher.h:18: NON_EXPORTED_BASE(public blink::WebWakeLockClient) { Do you need NON_EXPORTED_BASE?
https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... File content/browser/wake_lock/wake_lock_dispatcher_host.cc (right): https://codereview.chromium.org/1107333002/diff/1/content/browser/wake_lock/w... content/browser/wake_lock/wake_lock_dispatcher_host.cc:43: NotifyUpdate(); On 2015/05/05 14:08:33, Mounir Lamouri wrote: > Should this only be called when there is an actual update? The whole purpose of NotifyUpdate and AddObserver/RemoveObserver is to facilitate wake lock browser test (we needed a notification to see if a request from renderer came through, regardless of wake lock state change). Maybe it's an overkill and some less intrusive facility would suffice. https://codereview.chromium.org/1107333002/diff/1/content/content_renderer.gypi File content/content_renderer.gypi (right): https://codereview.chromium.org/1107333002/diff/1/content/content_renderer.gy... content/content_renderer.gypi:434: 'renderer/wake_lock/wake_lock_dispatcher.cc', On 2015/04/28 23:57:37, nasko wrote: > nit: Do we need a full new directory for just a couple of small files? The same is true for mojo and push_messaging (just two files if you don't count OWNERS). As there is also browser/wake_lock, having the corresponding renderer/wake_lock seems logical to me.
Is this ready to be reviewed again?
On 2015/08/05 13:55:56, Mounir Lamouri wrote: > Is this ready to be reviewed again? Yes, I've kept this CL in sync with changes in the Blink part.
Having a dispatcher_host with a mojo service is unusual. Is that design used in the code base? I thought that we usually had a service context for things like that. For example, you can look at PermissionService or GeolocationService. I think PermissionService is closer to what you are trying to do. Also, I would add jochen@ as a reviewer given that he reviewed the Blink CL. Otherwise, I left a few comments below. https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_dispatcher_host.h (right): https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_dispatcher_host.h:27: ~WakeLockDispatcherHost() override; = default https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_service_impl.h:32: // WakeLockSevice // WakeLockService implementation. https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_state.h (right): https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_state.h:22: ~WakeLockState(); Do you need the dtor if you make the class "final"? It might be required if it's not considered trivial. Though, in that case, can you make it = default? Same for the ctor above. https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_state.h:28: bool GetLockState() const; Hmm, that's a bit odd. Why not "HasFrame()"? I think it would be more explicit. https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_state.h:31: typedef std::set<const RenderFrameHost*> FramesContainer; nit: using FramesContainer = std::set<const RenderFrameHost*>; https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... File content/renderer/wake_lock/wake_lock_dispatcher.h (right): https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... content/renderer/wake_lock/wake_lock_dispatcher.h:14: // WakeLockDispatcher is a delegate for WakeLock messages used by Blink. "dispatcher" is a "delegate" is odd. Why not just say that it sends messages received by Blink to the browser process, using the Mojo WakeLockService? https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... content/renderer/wake_lock/wake_lock_dispatcher.h:21: virtual ~WakeLockDispatcher(); ~WakeLockDispatcher() override = default; https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... content/renderer/wake_lock/wake_lock_dispatcher.h:24: // WebWakeLockClient // WebWakeLockClient implementation. https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... content/renderer/wake_lock/wake_lock_dispatcher.h:25: void requestKeepScreenAwake(bool keepScreenAwake) override; no "override" for Blink API implementation. Use "virtual" instead.
On 2015/08/06 09:18:51, Mounir Lamouri wrote: > Having a dispatcher_host with a mojo service is unusual. Is that design used in > the code base? I thought that we usually had a service context for things like > that. For example, you can look at PermissionService or GeolocationService. I > think PermissionService is closer to what you are trying to do. > > Also, I would add jochen@ as a reviewer given that he reviewed the Blink CL. > > Otherwise, I left a few comments below. > > https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... > File content/browser/wake_lock/wake_lock_dispatcher_host.h (right): > > https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... > content/browser/wake_lock/wake_lock_dispatcher_host.h:27: > ~WakeLockDispatcherHost() override; > = default > > https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... > File content/browser/wake_lock/wake_lock_service_impl.h (right): > > https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... > content/browser/wake_lock/wake_lock_service_impl.h:32: // WakeLockSevice > // WakeLockService implementation. > > https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... > File content/browser/wake_lock/wake_lock_state.h (right): > > https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... > content/browser/wake_lock/wake_lock_state.h:22: ~WakeLockState(); > Do you need the dtor if you make the class "final"? It might be required if it's > not considered trivial. Though, in that case, can you make it = default? > > Same for the ctor above. > > https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... > content/browser/wake_lock/wake_lock_state.h:28: bool GetLockState() const; > Hmm, that's a bit odd. Why not "HasFrame()"? I think it would be more explicit. > > https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... > content/browser/wake_lock/wake_lock_state.h:31: typedef std::set<const > RenderFrameHost*> FramesContainer; > nit: using FramesContainer = std::set<const RenderFrameHost*>; > > https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... > File content/renderer/wake_lock/wake_lock_dispatcher.h (right): > > https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... > content/renderer/wake_lock/wake_lock_dispatcher.h:14: // WakeLockDispatcher is a > delegate for WakeLock messages used by Blink. > "dispatcher" is a "delegate" is odd. Why not just say that it sends messages > received by Blink to the browser process, using the Mojo WakeLockService? > > https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... > content/renderer/wake_lock/wake_lock_dispatcher.h:21: virtual > ~WakeLockDispatcher(); > ~WakeLockDispatcher() override = default; > > https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... > content/renderer/wake_lock/wake_lock_dispatcher.h:24: // WebWakeLockClient > // WebWakeLockClient implementation. > > https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... > content/renderer/wake_lock/wake_lock_dispatcher.h:25: void > requestKeepScreenAwake(bool keepScreenAwake) override; > no "override" for Blink API implementation. Use "virtual" instead. Regarding the dispatcher host vs service context: I've renamed WakeLockDispatcherHost to WakeLockServiceContext, as suggested, though there is still difference with PermissionService as for the latter the service context is associated with render frame host, while for the former it is necessarily associated with web contents, as PowerSaveBlocker needs WebContents instance to associate itself with (at least on Android). Is this acceptable?
https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_dispatcher_host.h (right): https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_dispatcher_host.h:27: ~WakeLockDispatcherHost() override; On 2015/08/06 09:18:51, Mounir Lamouri wrote: > = default Doesn't work. "[chromium-style] Complex destructor has an inline body.". https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_service_impl.h:32: // WakeLockSevice On 2015/08/06 09:18:51, Mounir Lamouri wrote: > // WakeLockService implementation. Done. https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_state.h (right): https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_state.h:22: ~WakeLockState(); On 2015/08/06 09:18:51, Mounir Lamouri wrote: > Do you need the dtor if you make the class "final"? It might be required if it's > not considered trivial. Though, in that case, can you make it = default? > > Same for the ctor above. Making dtor = default gives "[chromium-style] Complex constructor has an inlined body." error, ditto for the ctor. I believe the std::set member is what's making this class complex. https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_state.h:28: bool GetLockState() const; On 2015/08/06 09:18:51, Mounir Lamouri wrote: > Hmm, that's a bit odd. Why not "HasFrame()"? I think it would be more explicit. Done. https://codereview.chromium.org/1107333002/diff/80001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_state.h:31: typedef std::set<const RenderFrameHost*> FramesContainer; On 2015/08/06 09:18:51, Mounir Lamouri wrote: > nit: using FramesContainer = std::set<const RenderFrameHost*>; Hm, I'd rather remove the typedef as it is private and the type alias is not used anywhere else. https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... File content/renderer/wake_lock/wake_lock_dispatcher.h (right): https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... content/renderer/wake_lock/wake_lock_dispatcher.h:14: // WakeLockDispatcher is a delegate for WakeLock messages used by Blink. On 2015/08/06 09:18:51, Mounir Lamouri wrote: > "dispatcher" is a "delegate" is odd. Why not just say that it sends messages > received by Blink to the browser process, using the Mojo WakeLockService? Done. https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... content/renderer/wake_lock/wake_lock_dispatcher.h:21: virtual ~WakeLockDispatcher(); On 2015/08/06 09:18:51, Mounir Lamouri wrote: > ~WakeLockDispatcher() override = default; This produces "[chromium-style] Complex destructor has an inline body." error. https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... content/renderer/wake_lock/wake_lock_dispatcher.h:24: // WebWakeLockClient On 2015/08/06 09:18:51, Mounir Lamouri wrote: > // WebWakeLockClient implementation. Done. https://codereview.chromium.org/1107333002/diff/80001/content/renderer/wake_l... content/renderer/wake_lock/wake_lock_dispatcher.h:25: void requestKeepScreenAwake(bool keepScreenAwake) override; On 2015/08/06 09:18:51, Mounir Lamouri wrote: > no "override" for Blink API implementation. Use "virtual" instead. Done.
alogvinov@yandex-team.ru changed reviewers: + jochen@chromium.org
Sorry, for the delay, I left you a few comments. It generally looks pretty good! And I really enjoy seeing all these tests :) https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:94: FOR_EACH_OBSERVER(Observer, observer_list_, OnUpdate()); Why is that needed? https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:43: class CONTENT_EXPORT Observer { Not sure what the Observer is here for. https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:63: WakeLockState wake_lock_state_; Instead of WakeLockState, couldn't you simply have a std::set<> of RFH here? It seems that it would be more or less the same thing, right? Basically, have frames_requesting_lock_ in here instead of in WakeLockState. https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:65: scoped_ptr<PowerSaveBlocker> blocker_; FWIW, I think the blocker_ name is hard to grasp. https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_impl.h:39: mojo::InterfaceRequest<WakeLockService> request); I think you should pass the WakeLockServiceContext to the WakeLockServiceImpl so you don't need that callback interface in between. https://codereview.chromium.org/1107333002/diff/100001/content/common/wake_lo... File content/common/wake_lock_service.mojom (right): https://codereview.chromium.org/1107333002/diff/100001/content/common/wake_lo... content/common/wake_lock_service.mojom:7: // WebLockService receives per-frame wake lock preference from its client nit: you can use git cl upload --no-find-copies to prevent having this pretended copy from sw_event_status.mojom https://codereview.chromium.org/1107333002/diff/100001/content/content_common... File content/content_common_mojo_bindings.gyp (right): https://codereview.chromium.org/1107333002/diff/100001/content/content_common... content/content_common_mojo_bindings.gyp:25: 'common/wake_lock_service.mojom', nit: as long as you are here, could you re-order that list in alphabetical order? https://codereview.chromium.org/1107333002/diff/100001/content/renderer/wake_... File content/renderer/wake_lock/wake_lock_dispatcher.cc (right): https://codereview.chromium.org/1107333002/diff/100001/content/renderer/wake_... content/renderer/wake_lock/wake_lock_dispatcher.cc:8: #include "content/public/renderer/render_frame.h" nit: not sure you need that header. https://codereview.chromium.org/1107333002/diff/100001/content/renderer/wake_... content/renderer/wake_lock/wake_lock_dispatcher.cc:25: if (keepScreenAwake) { nit: add an empty line before this condition
https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:43: class CONTENT_EXPORT Observer { On 2015/08/21 10:03:35, Mounir Lamouri wrote: > Not sure what the Observer is here for. It was needed for the browser test, but I figured out how to do without it, so removing this. https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:63: WakeLockState wake_lock_state_; On 2015/08/21 10:03:35, Mounir Lamouri wrote: > Instead of WakeLockState, couldn't you simply have a std::set<> of RFH here? It > seems that it would be more or less the same thing, right? > > Basically, have frames_requesting_lock_ in here instead of in WakeLockState. Done. https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:65: scoped_ptr<PowerSaveBlocker> blocker_; On 2015/08/21 10:03:35, Mounir Lamouri wrote: > FWIW, I think the blocker_ name is hard to grasp. Done. https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_impl.h (right): https://codereview.chromium.org/1107333002/diff/100001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_impl.h:39: mojo::InterfaceRequest<WakeLockService> request); On 2015/08/21 10:03:35, Mounir Lamouri wrote: > I think you should pass the WakeLockServiceContext to the WakeLockServiceImpl so > you don't need that callback interface in between. Done. https://codereview.chromium.org/1107333002/diff/100001/content/common/wake_lo... File content/common/wake_lock_service.mojom (right): https://codereview.chromium.org/1107333002/diff/100001/content/common/wake_lo... content/common/wake_lock_service.mojom:7: // WebLockService receives per-frame wake lock preference from its client On 2015/08/21 10:03:35, Mounir Lamouri wrote: > nit: you can use git cl upload --no-find-copies to prevent having this pretended > copy from sw_event_status.mojom Acknowledged. https://codereview.chromium.org/1107333002/diff/100001/content/content_common... File content/content_common_mojo_bindings.gyp (right): https://codereview.chromium.org/1107333002/diff/100001/content/content_common... content/content_common_mojo_bindings.gyp:25: 'common/wake_lock_service.mojom', On 2015/08/21 10:03:35, Mounir Lamouri wrote: > nit: as long as you are here, could you re-order that list in alphabetical > order? Done. https://codereview.chromium.org/1107333002/diff/100001/content/renderer/wake_... File content/renderer/wake_lock/wake_lock_dispatcher.cc (right): https://codereview.chromium.org/1107333002/diff/100001/content/renderer/wake_... content/renderer/wake_lock/wake_lock_dispatcher.cc:8: #include "content/public/renderer/render_frame.h" On 2015/08/21 10:03:36, Mounir Lamouri wrote: > nit: not sure you need that header. That header is needed, see render_frame()->GetServiceRegistry() below. https://codereview.chromium.org/1107333002/diff/100001/content/renderer/wake_... content/renderer/wake_lock/wake_lock_dispatcher.cc:25: if (keepScreenAwake) { On 2015/08/21 10:03:35, Mounir Lamouri wrote: > nit: add an empty line before this condition Done.
lgtm with comments applied. Thank you for working on that! https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_browsertest.cc (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:71: void LockScreenInMainFrame() { nit: rename ScreenWakeLockInMainFrame() (name is confusing otherwise) https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:152: IN_PROC_BROWSER_TEST_F(WakeLockTest, LockScreenInMainFrameAndNestedFrame) { nit: rename to "LockInMainFrameandNestedFrame" https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:162: EXPECT_TRUE(HasWakeLock()); I'm not sure we want that to work like that by default, see: https://github.com/w3c/wake-lock/issues/51 I don't think this should block this CL but it should be addressed in order to ship. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:219: CrashTab(shell()->web_contents()); nit: GetWebContents() https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:236: IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterNavigationToSelf) { Could you check that in-page navigation keeps the lock? https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:28: registry->AddService<WakeLockService>( Could you do that in RenderFrameHostImpl::RegisterMojoServices instead? https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:37: frames_requesting_lock_.erase(render_frame_host); Call ::CancelWakeLock() instead. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:45: UpdateWakeLock(); ditto https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:68: // On Android, additionaly associate the blocker with this WebContents' nit: add empty line before this comment and another after the DCHECK(). https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:78: wake_lock_.reset(); nit: DCHECK(wake_lock_); https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:48: // The actual power save blocker for screen. nit: leave empty line. between two members. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:49: scoped_ptr<PowerSaveBlocker> wake_lock_; You seem to check that scoped_ptr<> isn't null in your test. Maybe worth adding HasWakeLockForTests() ? https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context_unittest.cc (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context_unittest.cc:20: ASSERT_TRUE(GetWakeLockServiceContext()); nit: I don't think you need that. This is a test and the next call will crash anyway. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context_unittest.cc:25: ASSERT_TRUE(GetWakeLockServiceContext()); ditto https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context_unittest.cc:54: EXPECT_FALSE(HasWakeLock()); nit: could you add some empty lines to make this more readable. Something like: ASSERT(...); ASSERT(...); // comment Call() // comment Call() // comment Call() https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context_unittest.cc:68: EXPECT_FALSE(HasWakeLock()); ditto https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context_unittest.cc:82: EXPECT_FALSE(HasWakeLock()); ditto https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_impl.cc:24: void WakeLockServiceImpl::CreateService( Is this method really needed given that it is only calling the ctor? https://codereview.chromium.org/1107333002/diff/120001/content/common/wake_lo... File content/common/wake_lock_service.mojom (right): https://codereview.chromium.org/1107333002/diff/120001/content/common/wake_lo... content/common/wake_lock_service.mojom:7: // WebLockService receives per-frame wake lock preference from its client nit: add "." at the end of the sentence.
mojom lgtm
https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_browsertest.cc (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:71: void LockScreenInMainFrame() { On 2015/08/31 14:59:15, Mounir Lamouri wrote: > nit: rename ScreenWakeLockInMainFrame() (name is confusing otherwise) Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:152: IN_PROC_BROWSER_TEST_F(WakeLockTest, LockScreenInMainFrameAndNestedFrame) { On 2015/08/31 14:59:15, Mounir Lamouri wrote: > nit: rename to "LockInMainFrameandNestedFrame" Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:162: EXPECT_TRUE(HasWakeLock()); On 2015/08/31 14:59:15, Mounir Lamouri wrote: > I'm not sure we want that to work like that by default, see: > https://github.com/w3c/wake-lock/issues/51 > > I don't think this should block this CL but it should be addressed in order to > ship. Acknowledged. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:219: CrashTab(shell()->web_contents()); On 2015/08/31 14:59:15, Mounir Lamouri wrote: > nit: GetWebContents() Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:236: IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterNavigationToSelf) { On 2015/08/31 14:59:15, Mounir Lamouri wrote: > Could you check that in-page navigation keeps the lock? Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:28: registry->AddService<WakeLockService>( On 2015/08/31 14:59:15, Mounir Lamouri wrote: > Could you do that in RenderFrameHostImpl::RegisterMojoServices instead? Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:37: frames_requesting_lock_.erase(render_frame_host); On 2015/08/31 14:59:15, Mounir Lamouri wrote: > Call ::CancelWakeLock() instead. Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:45: UpdateWakeLock(); On 2015/08/31 14:59:15, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:68: // On Android, additionaly associate the blocker with this WebContents' On 2015/08/31 14:59:15, Mounir Lamouri wrote: > nit: add empty line before this comment and another after the DCHECK(). Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:78: wake_lock_.reset(); On 2015/08/31 14:59:15, Mounir Lamouri wrote: > nit: DCHECK(wake_lock_); Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:48: // The actual power save blocker for screen. On 2015/08/31 14:59:15, Mounir Lamouri wrote: > nit: leave empty line. between two members. Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:49: scoped_ptr<PowerSaveBlocker> wake_lock_; On 2015/08/31 14:59:15, Mounir Lamouri wrote: > You seem to check that scoped_ptr<> isn't null in your test. Maybe worth adding > HasWakeLockForTests() ? Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context_unittest.cc (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context_unittest.cc:20: ASSERT_TRUE(GetWakeLockServiceContext()); On 2015/08/31 14:59:15, Mounir Lamouri wrote: > nit: I don't think you need that. This is a test and the next call will crash > anyway. Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context_unittest.cc:25: ASSERT_TRUE(GetWakeLockServiceContext()); On 2015/08/31 14:59:15, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context_unittest.cc:54: EXPECT_FALSE(HasWakeLock()); On 2015/08/31 14:59:15, Mounir Lamouri wrote: > nit: could you add some empty lines to make this more readable. > > Something like: > ASSERT(...); > ASSERT(...); > > // comment > Call() > > // comment > Call() > > // comment > Call() Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context_unittest.cc:68: EXPECT_FALSE(HasWakeLock()); On 2015/08/31 14:59:15, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context_unittest.cc:82: EXPECT_FALSE(HasWakeLock()); On 2015/08/31 14:59:15, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/1107333002/diff/120001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_impl.cc:24: void WakeLockServiceImpl::CreateService( On 2015/08/31 14:59:16, Mounir Lamouri wrote: > Is this method really needed given that it is only calling the ctor? Removed. https://codereview.chromium.org/1107333002/diff/120001/content/common/wake_lo... File content/common/wake_lock_service.mojom (right): https://codereview.chromium.org/1107333002/diff/120001/content/common/wake_lo... content/common/wake_lock_service.mojom:7: // WebLockService receives per-frame wake lock preference from its client On 2015/08/31 14:59:16, Mounir Lamouri wrote: > nit: add "." at the end of the sentence. Done.
Thanks for the thorough test coverage! It is mostly there, but I have a few comments, which are mostly nits. https://codereview.chromium.org/1107333002/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1600: base::Unretained(this))); Can you put a comment why using base::Unretained(this) is safe here? Extra bonus why it is ok for the service context as well. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_browsertest.cc (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:72: base::PlatformThread::Sleep(TestTimeouts::tiny_timeout()); Is it possible to structure this some other way? Waiting arbitrary amount of time usually leads to flaky tests. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:313: IN_PROC_BROWSER_TEST_F(WakeLockTest, OutOfProcessFrame) { Thanks for adding this test case! https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:315: embedded_test_server()->GetURL("/wake_lock/page_with_iframe.html")); There are already a bunch of test files that have out-of-process iframes and there is now content/test/data/cross_site_iframe_factory.html, which allows you to navigate once and setup a full hierarchy of iframes on arbitrary origins. I'd suggest using that, as it cuts down on the amount of boilerplate C++ code. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:85: if (!wake_lock_) { nit: No need for {} for single line if statements. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:89: if (wake_lock_) { Same here. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:40: // Cancels wake lock request for |render_frame_host|. nit: Empty line before the comment. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_impl.cc:24: if (context_) style: indentation uses 2 spaces, not 4. https://codereview.chromium.org/1107333002/diff/140001/content/content_common... File content/content_common_mojo_bindings.gyp (right): https://codereview.chromium.org/1107333002/diff/140001/content/content_common... content/content_common_mojo_bindings.gyp:23: 'common/service_worker/embedded_worker_setup.mojom', Thanks for fixing the order on this one! https://codereview.chromium.org/1107333002/diff/140001/content/test/data/wake... File content/test/data/wake_lock/page_with_iframe.html (right): https://codereview.chromium.org/1107333002/diff/140001/content/test/data/wake... content/test/data/wake_lock/page_with_iframe.html:8: <iframe id="nested_frame" src="../title1.html"></iframe> Why not use content/test/data/frame_tree/page_with_one_frame.html?
https://codereview.chromium.org/1107333002/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/frame_... 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: > Can you put a comment why using base::Unretained(this) is safe here? Extra bonus > why it is ok for the service context as well. Done. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_browsertest.cc (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:72: base::PlatformThread::Sleep(TestTimeouts::tiny_timeout()); On 2015/09/02 22:40:14, nasko (slow to review) wrote: > Is it possible to structure this some other way? Waiting arbitrary amount of > time usually leads to flaky tests. I am aware of that, but I cannot think of a way how this could be done short of injecting test callbacks into WakeLockServiceContext so that we could wait for an update. Failure to receive an update when one is expected would result in a test timeout (yet another arbitrary amount of time) rather than an expectation failure, which is also not great. And even with test callbacks, if no update is expected in response to some action, there is no way to wait for the update "not to happen". Actually, the initial version of this CL employed an update observer and didn't use any timed waiting, but it relied on the implementation detail that every time screen.keepAwake is written to by the script, a Mojo request is sent to the browser regardless of whether the new value is different from the old one, and every such request triggered an observer notification in WakeLockServiceContext. But such behavior (issuing a Mojo request even when there is no change) is not required for correct functioning of the Wake Lock API, so making the test rely on that would make it overly specific. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_browsertest.cc:315: embedded_test_server()->GetURL("/wake_lock/page_with_iframe.html")); On 2015/09/02 22:40:14, nasko (slow to review) wrote: > There are already a bunch of test files that have out-of-process iframes and > there is now content/test/data/cross_site_iframe_factory.html, which allows you > to navigate once and setup a full hierarchy of iframes on arbitrary origins. I'd > suggest using that, as it cuts down on the amount of boilerplate C++ code. Done. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:85: if (!wake_lock_) { On 2015/09/02 22:40:15, nasko (slow to review) wrote: > nit: No need for {} for single line if statements. Done. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:89: if (wake_lock_) { On 2015/09/02 22:40:15, nasko (slow to review) wrote: > Same here. Done. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:40: // Cancels wake lock request for |render_frame_host|. On 2015/09/02 22:40:15, nasko (slow to review) wrote: > nit: Empty line before the comment. Done. https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_impl.cc (right): https://codereview.chromium.org/1107333002/diff/140001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_impl.cc:24: if (context_) On 2015/09/02 22:40:15, nasko (slow to review) wrote: > style: indentation uses 2 spaces, not 4. Done. https://codereview.chromium.org/1107333002/diff/140001/content/test/data/wake... File content/test/data/wake_lock/page_with_iframe.html (right): https://codereview.chromium.org/1107333002/diff/140001/content/test/data/wake... content/test/data/wake_lock/page_with_iframe.html:8: <iframe id="nested_frame" src="../title1.html"></iframe> On 2015/09/02 22:40:15, nasko (slow to review) wrote: > Why not use content/test/data/frame_tree/page_with_one_frame.html? Need an id attribute for renderer-initiated navigation with NavigateIframeToURL, though I can use another suitable page. Done.
https://codereview.chromium.org/1107333002/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1107333002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1602: // RenderFrameHost pointers. Additionally, WakeLockServiceContext observes This comment is good, but the intention doesn't seem right. If we need an unique identifier for a frame, there is already that on FrameTreeNode. If you need unique identifier on the RFH, we also have that - process id, RFH id. I think using one of these will be much more appropriate than using a pointer value, especially when the address can be reallocated based on the memory allocator behavior. https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:32: void WakeLockServiceContext::RenderFrameDeleted( If you end up using FrameTreeNode ID, you can switch to FrameDeleted for handling this case and not needing the RenderFrameHostChanged observer.
https://codereview.chromium.org/1107333002/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1107333002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1602: // RenderFrameHost pointers. Additionally, WakeLockServiceContext observes On 2015/09/03 17:53:37, nasko (slow to review) wrote: > This comment is good, but the intention doesn't seem right. If we need an unique > identifier for a frame, there is already that on FrameTreeNode. If you need > unique identifier on the RFH, we also have that - process id, RFH id. > I think using one of these will be much more appropriate than using a pointer > value, especially when the address can be reallocated based on the memory > allocator behavior. Switched to using FrameTreeNode ids. https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:32: void WakeLockServiceContext::RenderFrameDeleted( On 2015/09/03 17:53:37, nasko (slow to review) wrote: > If you end up using FrameTreeNode ID, you can switch to FrameDeleted for > handling this case and not needing the RenderFrameHostChanged observer. I'm afraid this is not the case. RenderFrameDeleted is additionally fired when renderer process handling a subframe has died, which must remove wake lock request for that frame. FrameDeleted is not fired in this case so it is insufficient. Also, RenderFrameHostChanged is needed to track render process change, i.e. when navigating a subframe from same-process to cross-process. In fact, WebContentsObserver documentation explicitly states that RenderFrameDeleted and RenderFrameHostChanged is an appropriate combination to track the current set of RenderFrameHosts.
https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.cc (right): https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.cc:32: void WakeLockServiceContext::RenderFrameDeleted( On 2015/09/04 15:39:26, alogvinov wrote: > On 2015/09/03 17:53:37, nasko (slow to review) wrote: > > If you end up using FrameTreeNode ID, you can switch to FrameDeleted for > > handling this case and not needing the RenderFrameHostChanged observer. > > I'm afraid this is not the case. RenderFrameDeleted is additionally fired when > renderer process handling a subframe has died, which must remove wake lock > request for that frame. FrameDeleted is not fired in this case so it is > insufficient. For process crash, I totally agree we need RenderFrameDeleted. > Also, RenderFrameHostChanged is needed to track render process change, i.e. when > navigating a subframe from same-process to cross-process. In fact, > WebContentsObserver documentation explicitly states that RenderFrameDeleted and > RenderFrameHostChanged is an appropriate combination to track the current set of > RenderFrameHosts. Yes, while the documentation says that, the fact that tracking FrameTreeNode ids is what the code now does, you don't need to care about cross-process navigations. You conceptually care about frames, which is the FrameTreeNode abstraction. FrameTreeNode ids are stable across process swap, so even when the new RFH sends you a message, it will come from the same FrameTreeNode id. Since you need to handle dropping the lock on navigation, you must already have code that emits the right IPC. I should work in cross-process navigations in addition to same-process ones, without the need for RenderFrameHostChanged. https://codereview.chromium.org/1107333002/diff/180001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/180001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:52: // Set of all frame ids currently requesting wake lock. nit: s/frame/FrameTreeNode/
On 2015/09/04 21:06:45, nasko (out until Sept 11) wrote: > https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_l... > File content/browser/wake_lock/wake_lock_service_context.cc (right): > > https://codereview.chromium.org/1107333002/diff/160001/content/browser/wake_l... > content/browser/wake_lock/wake_lock_service_context.cc:32: void > WakeLockServiceContext::RenderFrameDeleted( > On 2015/09/04 15:39:26, alogvinov wrote: > > On 2015/09/03 17:53:37, nasko (slow to review) wrote: > > > If you end up using FrameTreeNode ID, you can switch to FrameDeleted for > > > handling this case and not needing the RenderFrameHostChanged observer. > > > > I'm afraid this is not the case. RenderFrameDeleted is additionally fired when > > renderer process handling a subframe has died, which must remove wake lock > > request for that frame. FrameDeleted is not fired in this case so it is > > insufficient. > > For process crash, I totally agree we need RenderFrameDeleted. > > > Also, RenderFrameHostChanged is needed to track render process change, i.e. > when > > navigating a subframe from same-process to cross-process. In fact, > > WebContentsObserver documentation explicitly states that RenderFrameDeleted > and > > RenderFrameHostChanged is an appropriate combination to track the current set > of > > RenderFrameHosts. > > Yes, while the documentation says that, the fact that tracking FrameTreeNode ids > is what the code now does, you don't need to care about cross-process > navigations. You conceptually care about frames, which is the FrameTreeNode > abstraction. FrameTreeNode ids are stable across process swap, so even when the > new RFH sends you a message, it will come from the same FrameTreeNode id. > > Since you need to handle dropping the lock on navigation, you must already have > code that emits the right IPC. I should work in cross-process navigations in > addition to same-process ones, without the need for RenderFrameHostChanged. > > https://codereview.chromium.org/1107333002/diff/180001/content/browser/wake_l... > File content/browser/wake_lock/wake_lock_service_context.h (right): > > https://codereview.chromium.org/1107333002/diff/180001/content/browser/wake_l... > content/browser/wake_lock/wake_lock_service_context.h:52: // Set of all frame > ids currently requesting wake lock. > nit: s/frame/FrameTreeNode/ Done, removed RenderFrameHostChanged.
https://codereview.chromium.org/1107333002/diff/180001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/180001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:52: // Set of all frame ids currently requesting wake lock. On 2015/09/04 21:06:45, nasko (out until Sept 11) wrote: > nit: s/frame/FrameTreeNode/ Done.
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
Jochen, can you please take a look at this CL?
nasko already owns all of this
The CQ bit was checked by alogvinov@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1107333002/#ps220001 (title: " ")
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
On 2015/09/14 09:36:51, jochen wrote: > nasko already owns all of this Thank you!
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/334a5a782b578f773f6cd95ee00918300725a2a8 Cr-Commit-Position: refs/heads/master@{#348602}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1345563002/ by rdevlin.cronin@chromium.org. The reason for reverting is: WakeLockTest has been failing on mac 10.9, e.g. WakeLockTest.RendererInitiatedFrameNavigation (run #1): [ RUN ] WakeLockTest.RendererInitiatedFrameNavigation [9365:1287:0914/122522:7769360692956:WARNING:vt_video_decode_accelerator.cc(167)] Failed to create VTDecompressionSession: codecOpenErr (-8973) [9365:1287:0914/122522:7769360765333:WARNING:vt_video_decode_accelerator.cc(209)] Failed to create hardware VideoToolbox session. Hardware accelerated video decoding will be disabled. ../../content/browser/wake_lock/wake_lock_browsertest.cc:302: Failure Value of: HasWakeLock() Actual: false Expected: true https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%2....
The original CL had a problem with cross-site navigation because it used frame node ids for identification of frames requesting wake lock, however when a cross-site navigation occurs, both the Mojo connection from the pre-navigation RenderFrame and the one for post-navigation RenderFrame continue to exist for some time sharing the same frame node id, leading to a race condition. In particular, when a wake lock is requested by the newly navigated page shortly after it is loaded, this request can precede the cancel request issued by the old pre-navigation RenderFrame as it gets destroyed. As they share the same frame node id, the later arriving request from the old RenderFrame cancels wake lock requested by the new RenderFrame. As wake lock request state is fundamentally tied to LocalFrames in Blink, and those are in 1 to 1 relationship with RenderFrames in Chromium renderer, which in turn are unambiguously mapped to RenderFrameHosts in the browser, I think it makes sense to use process id / routing id pairs for RFHs as frame ids in WakeLockServiceContext, which is implemented in this CL. Nasko, can you please take another look at this?
The original CL had a problem with cross-site navigation because it used frame node ids for identification of frames requesting wake lock, however when a cross-site navigation occurs, both the Mojo connection from the pre-navigation RenderFrame and the one for post-navigation RenderFrame continue to exist for some time sharing the same frame node id, leading to a race condition. In particular, when a wake lock is requested by the newly navigated page shortly after it is loaded, this request can precede the cancel request issued by the old pre-navigation RenderFrame as it gets destroyed. As they share the same frame node id, the later arriving request from the old RenderFrame cancels wake lock requested by the new RenderFrame. As wake lock request state is fundamentally tied to LocalFrames in Blink, and those are in 1 to 1 relationship with RenderFrames in Chromium renderer, which in turn are unambiguously mapped to RenderFrameHosts in the browser, I think it makes sense to use process id / routing id pairs for RFHs as frame ids in WakeLockServiceContext, which is implemented in this CL. Nasko, can you please take another look at this?
The original CL had a problem with cross-site navigation because it used frame node ids for identification of frames requesting wake lock, however when a cross-site navigation occurs, both the Mojo connection from the pre-navigation RenderFrame and the one for post-navigation RenderFrame continue to exist for some time sharing the same frame node id, leading to a race condition. In particular, when a wake lock is requested by the newly navigated page shortly after it is loaded, this request can precede the cancel request issued by the old pre-navigation RenderFrame as it gets destroyed. As they share the same frame node id, the later arriving request from the old RenderFrame cancels wake lock requested by the new RenderFrame. As wake lock request state is fundamentally tied to LocalFrames in Blink, and those are in 1 to 1 relationship with RenderFrames in Chromium renderer, which in turn are unambiguously mapped to RenderFrameHosts in the browser, I think it makes sense to use process id / routing id pairs for RFHs as frame ids in WakeLockServiceContext, which is implemented in this CL. Nasko, can you please take another look at this?
On 2015/09/16 14:07:06, alogvinov wrote: > The original CL had a problem with cross-site navigation because it used frame > node ids for identification of frames requesting wake lock, however when a > cross-site navigation occurs, both the Mojo connection from the pre-navigation > RenderFrame and the one for post-navigation RenderFrame continue to exist for > some time sharing the same frame node id, leading to a race condition. In > particular, when a wake lock is requested by the newly navigated page shortly > after it is loaded, this request can precede the cancel request issued by the > old pre-navigation RenderFrame as it gets destroyed. As they share the same > frame node id, the later arriving request from the old RenderFrame cancels wake > lock requested by the new RenderFrame. > > As wake lock request state is fundamentally tied to LocalFrames in Blink, and > those are in 1 to 1 relationship with RenderFrames in Chromium renderer, which > in turn are unambiguously mapped to RenderFrameHosts in the browser, I think it > makes sense to use process id / routing id pairs for RFHs as frame ids in > WakeLockServiceContext, which is implemented in this CL. > > Nasko, can you please take another look at this? I know mlamouri@ asked to continue on this CL, but the usual practice is to create a new issue for relands. To ensure easy review, the first patchset in the new issue should be identical to what was committed in this CL and then any changes are uploaded next. This allows for a clear diff of what has changed to fix the original reason for the revert. Let's stick with this one for now, but wanted to share the efficient way to get new CLs for relands.
LGTM with a few nits. https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:37: // Requests wake lock for render frame identified by |render_process_id| and s/render frame/RenderFrame/ https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:41: // Cancels wake lock request for render frame identified by s/render frame/RenderFrame/ https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:53: // Set of (render_process_id, render_frame_id) pairs indentifying all render s/render frames/RenderFrames/
https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_l... File content/browser/wake_lock/wake_lock_service_context.h (right): https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:37: // Requests wake lock for render frame identified by |render_process_id| and On 2015/09/16 14:17:02, nasko (slow to review) wrote: > s/render frame/RenderFrame/ Done. https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:41: // Cancels wake lock request for render frame identified by On 2015/09/16 14:17:02, nasko (slow to review) wrote: > s/render frame/RenderFrame/ Done. https://codereview.chromium.org/1107333002/diff/240001/content/browser/wake_l... content/browser/wake_lock/wake_lock_service_context.h:53: // Set of (render_process_id, render_frame_id) pairs indentifying all render On 2015/09/16 14:17:02, nasko (slow to review) wrote: > s/render frames/RenderFrames/ Done.
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_... The failures are due to failed HasWakeLock() expectation, and the error is always negative (no wake lock when one is expected). I could not reproduce this problem on a local Mac build. I suspect these failures might be due to incorrectly reported page visibility state, as wake lock is not applied when the page is not visible by design. I've found a couple of page visibility-related tests disabled on Mac due to flakiness in render_frame_host_impl_browsertest.cc which refer to these bugs: http://crbug.com/467670 http://crbug.com/525592 Mounir, can you please advice if these bugs might be related to the problem with this change list?
I've confirmed flakiness of WakeLockTest on Mac to be a visibility issue by adding a frame visibility check to each test: http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_dbg_... http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_dbg_... Value of: GetMainFrame()->GetVisibilityState() Actual: 1 Expected: blink::WebPageVisibilityStateVisible Which is: 0 Is there anything I can do about it?
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/334a5a782b578f773f6cd95ee00918300725a2a8 Cr-Commit-Position: refs/heads/master@{#348602}
Message was sent while issue was closed.
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
Message was sent while issue was closed.
Is there no bug for this feature? Mounir, I thought we had agreed not to support this? Or was that discussion only to say we won't support it on Android?
Message was sent while issue was closed.
On 2015/10/05 at 20:19:34, jdduke wrote: > Is there no bug for this feature? > > Mounir, I thought we had agreed not to support this? Or was that discussion only to say we won't support it on Android? The feature is implemented behind a Blink feature flag which isn't enabled by default. My understanding is that Chrome as a product isn't ready to support this but that doesn't mean that other browsers using the content layer and Blink shouldn't.
Message was sent while issue was closed.
On 2015/10/05 at 22:51:15, Mounir Lamouri wrote: > On 2015/10/05 at 20:19:34, jdduke wrote: > > Is there no bug for this feature? > > > > Mounir, I thought we had agreed not to support this? Or was that discussion only to say we won't support it on Android? > > The feature is implemented behind a Blink feature flag which isn't enabled by default. My understanding is that Chrome as a product isn't ready to support this but that doesn't mean that other browsers using the content layer and Blink shouldn't. bug is https://crbug.com/257511
Message was sent while issue was closed.
On 2015/10/05 22:52:16, Mounir Lamouri wrote: > On 2015/10/05 at 22:51:15, Mounir Lamouri wrote: > > On 2015/10/05 at 20:19:34, jdduke wrote: > > > Is there no bug for this feature? > > > > > > Mounir, I thought we had agreed not to support this? Or was that discussion > only to say we won't support it on Android? > > > > The feature is implemented behind a Blink feature flag which isn't enabled by > default. My understanding is that Chrome as a product isn't ready to support > this but that doesn't mean that other browsers using the content layer and Blink > shouldn't. > > bug is https://crbug.com/257511 Mounir, can you please clarify the status of this issue? Last time I touched it, it was still open and there was still a problem with content_browsertests on mac_chromium_dbg_ng which had to do with unstable page visibility status. Now this issue appears to be closed but as I can see it still hasn't landed. There is a new commit message #43 which refers to a different patch set than #33 (16 vs 12) but the same revision https://crrev.com/334a5a782b578f773f6cd95ee00918300725a2a8. Why is this issue closed and what further steps can I take?
Message was sent while issue was closed.
On 2015/10/12 at 07:36:46, alogvinov wrote: > On 2015/10/05 22:52:16, Mounir Lamouri wrote: > > On 2015/10/05 at 22:51:15, Mounir Lamouri wrote: > > > On 2015/10/05 at 20:19:34, jdduke wrote: > > > > Is there no bug for this feature? > > > > > > > > Mounir, I thought we had agreed not to support this? Or was that discussion > > only to say we won't support it on Android? > > > > > > The feature is implemented behind a Blink feature flag which isn't enabled by > > default. My understanding is that Chrome as a product isn't ready to support > > this but that doesn't mean that other browsers using the content layer and Blink > > shouldn't. > > > > bug is https://crbug.com/257511 > > Mounir, can you please clarify the status of this issue? Last time I touched it, it was still open and there was still a problem with content_browsertests on mac_chromium_dbg_ng which had to do with unstable page visibility status. Now this issue appears to be closed but as I can see it still hasn't landed. There is a new commit message #43 which refers to a different patch set than #33 (16 vs 12) but the same revision https://crrev.com/334a5a782b578f773f6cd95ee00918300725a2a8. > > Why is this issue closed and what further steps can I take? Feel free to reopen this CL or create a new one.
On 2015/10/12 at 10:39:13, mlamouri wrote: > On 2015/10/12 at 07:36:46, alogvinov wrote: > > On 2015/10/05 22:52:16, Mounir Lamouri wrote: > > > On 2015/10/05 at 22:51:15, Mounir Lamouri wrote: > > > > On 2015/10/05 at 20:19:34, jdduke wrote: > > > > > Is there no bug for this feature? > > > > > > > > > > Mounir, I thought we had agreed not to support this? Or was that discussion > > > only to say we won't support it on Android? > > > > > > > > The feature is implemented behind a Blink feature flag which isn't enabled by > > > default. My understanding is that Chrome as a product isn't ready to support > > > this but that doesn't mean that other browsers using the content layer and Blink > > > shouldn't. > > > > > > bug is https://crbug.com/257511 > > > > Mounir, can you please clarify the status of this issue? Last time I touched it, it was still open and there was still a problem with content_browsertests on mac_chromium_dbg_ng which had to do with unstable page visibility status. Now this issue appears to be closed but as I can see it still hasn't landed. There is a new commit message #43 which refers to a different patch set than #33 (16 vs 12) but the same revision https://crrev.com/334a5a782b578f773f6cd95ee00918300725a2a8. > > > > Why is this issue closed and what further steps can I take? > > Feel free to reopen this CL or create a new one. please create a new one
Message was sent while issue was closed.
On 2015/10/12 11:02:48, jochen wrote: > On 2015/10/12 at 10:39:13, mlamouri wrote: > > On 2015/10/12 at 07:36:46, alogvinov wrote: > > > On 2015/10/05 22:52:16, Mounir Lamouri wrote: > > > > On 2015/10/05 at 22:51:15, Mounir Lamouri wrote: > > > > > On 2015/10/05 at 20:19:34, jdduke wrote: > > > > > > Is there no bug for this feature? > > > > > > > > > > > > Mounir, I thought we had agreed not to support this? Or was that > discussion > > > > only to say we won't support it on Android? > > > > > > > > > > The feature is implemented behind a Blink feature flag which isn't > enabled by > > > > default. My understanding is that Chrome as a product isn't ready to > support > > > > this but that doesn't mean that other browsers using the content layer and > Blink > > > > shouldn't. > > > > > > > > bug is https://crbug.com/257511 > > > > > > Mounir, can you please clarify the status of this issue? Last time I touched > it, it was still open and there was still a problem with content_browsertests on > mac_chromium_dbg_ng which had to do with unstable page visibility status. Now > this issue appears to be closed but as I can see it still hasn't landed. There > is a new commit message #43 which refers to a different patch set than #33 (16 > vs 12) but the same revision > https://crrev.com/334a5a782b578f773f6cd95ee00918300725a2a8. > > > > > > Why is this issue closed and what further steps can I take? > > > > Feel free to reopen this CL or create a new one. > > please create a new one Done: https://codereview.chromium.org/1393203004
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. |
