Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 8 months ago by alogvinov Modified:
5 years, 3 months ago Reviewers:
haraken, Jens Widell, sof, jochen (gone - plz use gerrit), mlamouri (slow - plz ping) CC:
arv+blink, blink-reviews, blink-reviews-dom_chromium.org, Inactive, dglazkov+blink, eae+blinkwatch, jpmedley, rwlbuis, vivekg_samsung, vivekg Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionWake Lock API implementation (Blink part)
This is Blink part of Wake Lock API implementation as per specification:
http://w3c.github.io/wake-lock/
The corresponding Chromium part is ready and will be submitted after
these changes land, as it is dependent on them.
Tracking bug: https://code.google.com/p/chromium/issues/detail?id=257511
Design document:
https://docs.google.com/document/d/1KbIENP0wgxtSXDQFn9PbHZ_tAKZfR1Y8u4Hst8LpeaA/edit?usp=sharing
R=mlamouri@chromium.org
Committed: https://crrev.com/2f87b1b5d7ce34064a9f1302a9a19683abf592a8
git-svn-id: svn://svn.chromium.org/blink/trunk@199659 bbb929c8-8fbe-4397-9dbb-9b2b20218538
Patch Set 1 #
Total comments: 22
Patch Set 2 : Applied review comments #
Total comments: 18
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 15
Patch Set 11 : #
Total comments: 22
Patch Set 12 : #Patch Set 13 : #
Created: 5 years, 5 months ago
Messages
Total messages: 54 (5 generated)
Can you please take a look at this patch?
Thanks for the CL! and sorry for the delay. I have left a few comments below. I think you should move this to a modules/. I am not entirely sure if the specification is entirely stable. It's nicely simple but maybe too simple. Regardless, I think it's fine to start implementing while discussing the specification. Also, could you upload the other CLs you have and CC me on them it is always good to have a look at the big picture when reviewing one CL. https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... File LayoutTests/wake_lock/wakelock-api.html (right): https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... LayoutTests/wake_lock/wakelock-api.html:9: assert_true('keepScreenAwake' in window.document); There are methods that are meant to check if something is present in an object. Look at similar tests. https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... LayoutTests/wake_lock/wakelock-api.html:11: assert_true(typeof document.keepScreenAwake === 'boolean'); No need to test !== undefined and === boolean. https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... LayoutTests/wake_lock/wakelock-api.html:25: }, 'Test that document.keepScreenAwake doesn\'t throw when set the value'); You can remove that test. If it were to throw, the next test will fail anyway. https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... File LayoutTests/wake_lock/wakelock-defined.html (right): https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... LayoutTests/wake_lock/wakelock-defined.html:4: <script src="../resources/js-test.js"></script> Could you use testharness.js? https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... LayoutTests/wake_lock/wakelock-defined.html:11: shouldBeTrue("'keepScreenAwake' in document"); Isn't all of that already tested in wakelock-api.html? https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... File LayoutTests/wake_lock/wakelock-in-nested-frame.html (right): https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... LayoutTests/wake_lock/wakelock-in-nested-frame.html:4: <script src="../resources/js-test.js"></script> Please use test-harness.js https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/DocumentWak... File Source/core/dom/DocumentWakeLock.idl (right): https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/DocumentWak... Source/core/dom/DocumentWakeLock.idl:1: // Copyright 2015 The Chromium Authors. All rights reserved. Could you move that to a modules instead? Like Sources/modules/wake-lock/ https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.cpp File Source/core/dom/WakeLock.cpp (right): https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.cp... Source/core/dom/WakeLock.cpp:53: if (LocalFrame* frame = document.frame()) { if (!document.frame()) return; [...] https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h File Source/core/dom/WakeLock.h (right): https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h#... Source/core/dom/WakeLock.h:20: , public DocumentSupplement Hmmm, why do you have a Document supplement and a LocalFrame supplement. It seems that only one of these would suffice, wouldn't it? https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h#... Source/core/dom/WakeLock.h:50: // it will be destroyed only after Document's destructor has finished. I'm not sure I follow. How is HTMLCanvasElement doing it then? https://codereview.chromium.org/1084923002/diff/1/Source/web/tests/data/wake_... File Source/web/tests/data/wake_lock_test.html (right): https://codereview.chromium.org/1084923002/diff/1/Source/web/tests/data/wake_... Source/web/tests/data/wake_lock_test.html:6: </html> This file is basically empty. Is there another dummy file you could use in web/tests/data/? or maybe you could simply pass a string in the test? https://codereview.chromium.org/1084923002/diff/1/public/platform/WebWakeLock... File public/platform/WebWakeLockClient.h (right): https://codereview.chromium.org/1084923002/diff/1/public/platform/WebWakeLock... public/platform/WebWakeLockClient.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Could you have that in public/platform/modules/wake-lock/ ? https://codereview.chromium.org/1084923002/diff/1/public/platform/WebWakeLock... public/platform/WebWakeLockClient.h:14: virtual void requestKeepScreenAwake(bool) = 0; Usually, Blink interfaces are not using pure virtual methods because if the Blink interface changes, it would break Chromium.
On 2015/04/28 08:00:26, Mounir Lamouri wrote: > Thanks for the CL! and sorry for the delay. > > I have left a few comments below. I think you should move this to a modules/. > > I am not entirely sure if the specification is entirely stable. It's nicely > simple but maybe too simple. Regardless, I think it's fine to start implementing > while discussing the specification. > > Also, could you upload the other CLs you have and CC me on them it is always > good to have a look at the big picture when reviewing one CL. > > https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... > File LayoutTests/wake_lock/wakelock-api.html (right): > > https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... > LayoutTests/wake_lock/wakelock-api.html:9: assert_true('keepScreenAwake' in > window.document); > There are methods that are meant to check if something is present in an object. > Look at similar tests. > > https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... > LayoutTests/wake_lock/wakelock-api.html:11: assert_true(typeof > document.keepScreenAwake === 'boolean'); > No need to test !== undefined and === boolean. > > https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... > LayoutTests/wake_lock/wakelock-api.html:25: }, 'Test that > document.keepScreenAwake doesn\'t throw when set the value'); > You can remove that test. If it were to throw, the next test will fail anyway. > > https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... > File LayoutTests/wake_lock/wakelock-defined.html (right): > > https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... > LayoutTests/wake_lock/wakelock-defined.html:4: <script > src="../resources/js-test.js"></script> > Could you use testharness.js? > > https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... > LayoutTests/wake_lock/wakelock-defined.html:11: shouldBeTrue("'keepScreenAwake' > in document"); > Isn't all of that already tested in wakelock-api.html? > > https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... > File LayoutTests/wake_lock/wakelock-in-nested-frame.html (right): > > https://codereview.chromium.org/1084923002/diff/1/LayoutTests/wake_lock/wakel... > LayoutTests/wake_lock/wakelock-in-nested-frame.html:4: <script > src="../resources/js-test.js"></script> > Please use test-harness.js > > https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/DocumentWak... > File Source/core/dom/DocumentWakeLock.idl (right): > > https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/DocumentWak... > Source/core/dom/DocumentWakeLock.idl:1: // Copyright 2015 The Chromium Authors. > All rights reserved. > Could you move that to a modules instead? Like Sources/modules/wake-lock/ > > https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.cpp > File Source/core/dom/WakeLock.cpp (right): > > https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.cp... > Source/core/dom/WakeLock.cpp:53: if (LocalFrame* frame = document.frame()) { > if (!document.frame()) > return; > > [...] > > https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h > File Source/core/dom/WakeLock.h (right): > > https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h#... > Source/core/dom/WakeLock.h:20: , public DocumentSupplement > Hmmm, why do you have a Document supplement and a LocalFrame supplement. It > seems that only one of these would suffice, wouldn't it? > > https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h#... > Source/core/dom/WakeLock.h:50: // it will be destroyed only after Document's > destructor has finished. > I'm not sure I follow. How is HTMLCanvasElement doing it then? > > https://codereview.chromium.org/1084923002/diff/1/Source/web/tests/data/wake_... > File Source/web/tests/data/wake_lock_test.html (right): > > https://codereview.chromium.org/1084923002/diff/1/Source/web/tests/data/wake_... > Source/web/tests/data/wake_lock_test.html:6: </html> > This file is basically empty. Is there another dummy file you could use in > web/tests/data/? or maybe you could simply pass a string in the test? > > https://codereview.chromium.org/1084923002/diff/1/public/platform/WebWakeLock... > File public/platform/WebWakeLockClient.h (right): > > https://codereview.chromium.org/1084923002/diff/1/public/platform/WebWakeLock... > public/platform/WebWakeLockClient.h:1: // Copyright 2015 The Chromium Authors. > All rights reserved. > Could you have that in public/platform/modules/wake-lock/ ? > > https://codereview.chromium.org/1084923002/diff/1/public/platform/WebWakeLock... > public/platform/WebWakeLockClient.h:14: virtual void > requestKeepScreenAwake(bool) = 0; > Usually, Blink interfaces are not using pure virtual methods because if the > Blink interface changes, it would break Chromium. Sure, I've created a CL for the Chromium part: https://codereview.chromium.org/1107333002
https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h File Source/core/dom/WakeLock.h (right): https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h#... Source/core/dom/WakeLock.h:20: , public DocumentSupplement WakeLock stores the state of keepScreenAwake flag. It has to be Document supplement, not LocalFrame supplement as a Document can have no frame. In the latter case, keepScreenAwake state will obviously be ignored, but the variable itself still exists so it needs to be stored somewhere. On the other hand, WakeLockController is a bridge between WakeLock and WebWakeLockClient which is obtained from WebFrameClient. In Chromium, WebWakeLockClient is implemented by WakeLockDispatcher which sends wake lock requests to the corresponding RenderFrameHost. It is thus reasonable to have WakeLockController supplement LocalFrame. https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h#... Source/core/dom/WakeLock.h:50: // it will be destroyed only after Document's destructor has finished. HTMLCanvasElement is an HTMLElement, i.e. it is part of the DOM tree. It is destroyed when the parent Document is detached, if I understand it correctly. So when Oilpan is disabled, all elements are guaranteed to be deleted when Document's dtor runs. WakeLock is, on the other hand, a DocumentSupplement, so it owned not by Document but by DocumentSupplementable, a Document's base. When Document's dtor runs, DocumentSupplementable's dtor hasn't yet started (as it is a base), and WakeLock is still alive. Had WakeLock implemented DocumentVisibilityObserver itself, it would have been unregistered from Document in DocumentVisibilityObserver's dtor. But as WakeLock is still alive, it would have not been unregistered at this point, failing the assert in the Document's dtor. unergisterObserver() is private in DocumentVisibilityObserver, so it cannot be invoked separately from dtor either.
https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/DocumentWak... File Source/core/dom/DocumentWakeLock.idl (right): https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/DocumentWak... Source/core/dom/DocumentWakeLock.idl:1: // Copyright 2015 The Chromium Authors. All rights reserved. We actually tried doing that, but got a strange runtime assert in generated file "gen/blink/bindings/core/v8/V8Document.cpp": ASSERT(V8Document::installV8DocumentTemplateFunction != V8Document::installV8DocumentTemplate); Can you perhaps point me at a possible cause? There is no existing case of extending Document interface in modules, so we have no working example to learn from.
https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/DocumentWak... File Source/core/dom/DocumentWakeLock.idl (right): https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/DocumentWak... Source/core/dom/DocumentWakeLock.idl:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/04/28 at 16:49:10, alogvinov wrote: > We actually tried doing that, but got a strange runtime assert in generated file "gen/blink/bindings/core/v8/V8Document.cpp": > > ASSERT(V8Document::installV8DocumentTemplateFunction != V8Document::installV8DocumentTemplate); > > Can you perhaps point me at a possible cause? > > There is no existing case of extending Document interface in modules, so we have no working example to learn from. Hmm, I'm not sure where that's coming from. Did you try to move the RuntimeEnabled=WakeLock to the attribute? https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h File Source/core/dom/WakeLock.h (right): https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h#... Source/core/dom/WakeLock.h:20: , public DocumentSupplement On 2015/04/28 at 16:34:25, alogvinov wrote: > WakeLock stores the state of keepScreenAwake flag. It has to be Document supplement, not LocalFrame supplement as a Document can have no frame. In the latter case, keepScreenAwake state will obviously be ignored, but the variable itself still exists so it needs to be stored somewhere. On the other hand, WakeLockController is a bridge between WakeLock and WebWakeLockClient which is obtained from WebFrameClient. In Chromium, WebWakeLockClient is implemented by WakeLockDispatcher which sends wake lock requests to the corresponding RenderFrameHost. It is thus reasonable to have WakeLockController supplement LocalFrame. Couldn't WakeLockDocument rely on the value stored in the frame and return a default value if there are no frames? I believe that's a common pattern in Document. For example, ::devicePixelRatio() does that or ::parentDocument(). https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h#... Source/core/dom/WakeLock.h:50: // it will be destroyed only after Document's destructor has finished. On 2015/04/28 at 16:34:25, alogvinov wrote: > HTMLCanvasElement is an HTMLElement, i.e. it is part of the DOM tree. It is destroyed when the parent Document is detached, if I understand it correctly. So when Oilpan is disabled, all elements are guaranteed to be deleted when Document's dtor runs. > > WakeLock is, on the other hand, a DocumentSupplement, so it owned not by Document but by DocumentSupplementable, a Document's base. When Document's dtor runs, DocumentSupplementable's dtor hasn't yet started (as it is a base), and WakeLock is still alive. Had WakeLock implemented DocumentVisibilityObserver itself, it would have been unregistered from Document in DocumentVisibilityObserver's dtor. But as WakeLock is still alive, it would have not been unregistered at this point, failing the assert in the Document's dtor. unergisterObserver() is private in DocumentVisibilityObserver, so it cannot be invoked separately from dtor either. Ok. I see. Maybe we should have a way to clear the observing in the dtor. It seems unfortunate to have to create this different object and add this complexity.
https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/DocumentWak... File Source/core/dom/DocumentWakeLock.idl (right): https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/DocumentWak... Source/core/dom/DocumentWakeLock.idl:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/04/29 10:07:08, Mounir Lamouri wrote: > On 2015/04/28 at 16:49:10, alogvinov wrote: > > We actually tried doing that, but got a strange runtime assert in generated > file "gen/blink/bindings/core/v8/V8Document.cpp": > > > > ASSERT(V8Document::installV8DocumentTemplateFunction != > V8Document::installV8DocumentTemplate); > > > > Can you perhaps point me at a possible cause? > > > > There is no existing case of extending Document interface in modules, so we > have no working example to learn from. > > Hmm, I'm not sure where that's coming from. Did you try to move the > RuntimeEnabled=WakeLock to the attribute? Tried that, makes no difference. I also tried to moving dom/Document.idl from 'core_idl_files' to 'core_idl_with_modules_dependency_files' in core.gypi, but then I got a even stranger code generation problem. In gen/blink/bindings/modules/v8/V8DocumentPartial.cpp I got function definition for V8Document::PrivateScript::transformDocumentToTreeViewMethod, the same one as in gen/blink/bindings/core/v8/V8Document.cpp, so linker complains about duplicate function definition. "transformDocumentToTreeView" is actually defined in core/xml/DocumentXMLTreeViewer.idl, and that file is listed in 'core_dependency_idl_files' in core.gypi. DocumentWakeLock.idl is listed in 'modules_dependency_idl_files' in modules.gypi. Could it be a v8 bindings code generator bug, or am I doing something wrong? I can update this CL with what I currently have if that helps. https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h File Source/core/dom/WakeLock.h (right): https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h#... Source/core/dom/WakeLock.h:20: , public DocumentSupplement > Couldn't WakeLockDocument rely on the value stored in the frame and return a > default value if there are no frames? I believe that's a common pattern in > Document. For example, ::devicePixelRatio() does that or ::parentDocument(). Those properties are read-only. keepScreenAwake is writable, so it would be confusing if a script writes one value and reads back another (the default one).
mlamouri@chromium.org changed reviewers: + jl@opera.com, jochen@chromium.org
+jochen@ for the document/frame issue. +jl@ for the bindings problem (partial Document interfaces in modules/). https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h File Source/core/dom/WakeLock.h (right): https://codereview.chromium.org/1084923002/diff/1/Source/core/dom/WakeLock.h#... Source/core/dom/WakeLock.h:20: , public DocumentSupplement On 2015/04/29 at 16:50:59, alogvinov wrote: > > Couldn't WakeLockDocument rely on the value stored in the frame and return a > > default value if there are no frames? I believe that's a common pattern in > > Document. For example, ::devicePixelRatio() does that or ::parentDocument(). > > Those properties are read-only. keepScreenAwake is writable, so it would be confusing if a script writes one value and reads back another (the default one). Fair point. Though, I wonder if we would be okay with that if it meant simpler code. My understanding is that the behaviour of detached frames isn't really important as long as it doesn't crash. I might be wrong though.
On 2015/04/30 13:15:38, Mounir Lamouri wrote: > +jl@ for the bindings problem (partial Document interfaces in modules/). With https://codereview.chromium.org/1113023002/ I think adding a partial Document interface in modules/ should work, provided Document.idl is moved to core_idl_with_modules_dependency_files in core.gypi. (I didn't confirm by patching in this CL due to conflicts and compilation problems, but I did test it by moving NavigatorBeacon onto Document, and that seemed to work.)
can you please file a tracking bug and explain how you intend to implement this? I don't understand how this whole feature is supposed to work. From the spec it looks like you'd just want to set a flag on the document and have content read the flag whenever it decides to make another document active?
On 2015/05/04 at 07:01:59, jochen wrote: > can you please file a tracking bug and explain how you intend to implement this? https://crbug.com/257511 could be used.
https://codereview.chromium.org/1084923002/diff/20001/Source/bindings/templat... File Source/bindings/templates/interface_base.cpp (right): https://codereview.chromium.org/1084923002/diff/20001/Source/bindings/templat... Source/bindings/templates/interface_base.cpp:496: {% for method in methods if method.is_implemented_in_private_script and method.visible %} I had to make this change to get rid of duplicate definition of V8Document::PrivateScript::transformDocumentToTreeViewMethod in gen/blink/bindings/core/v8/V8Document.cpp and gen/blink/bindings/modules/v8/V8DocumentPartial.cpp. Though I'm not sure if this is an appropriate things to do and if perhaps a separate issue is needed for that. Also, run-bindings-tests no longer passes with this change as code generation result is different.
On 2015/05/05 11:29:57, alogvinov wrote: > https://codereview.chromium.org/1084923002/diff/20001/Source/bindings/templat... > File Source/bindings/templates/interface_base.cpp (right): > > https://codereview.chromium.org/1084923002/diff/20001/Source/bindings/templat... > Source/bindings/templates/interface_base.cpp:496: {% for method in methods if > method.is_implemented_in_private_script and method.visible %} > I had to make this change to get rid of duplicate definition of > V8Document::PrivateScript::transformDocumentToTreeViewMethod in > gen/blink/bindings/core/v8/V8Document.cpp and > gen/blink/bindings/modules/v8/V8DocumentPartial.cpp. Though I'm not sure if this > is an appropriate things to do and if perhaps a separate issue is needed for > that. Also, run-bindings-tests no longer passes with this change as code > generation result is different. I've landed that change in https://codereview.chromium.org/1113023002/ already, so you just need to rebase. About run-bindings-tests: when you change the code generator, you just run 'run-bindings-tests --reset-results' and include the changed test output files in the patch. This allows reviewers to see how code generation changes.
Thanks for moving things to modules/. I'm still interested to hear what jochen@ has to say about returning a dummy value when the frame is detached. I think it would make a lot of things simpler. https://codereview.chromium.org/1084923002/diff/20001/LayoutTests/wake_lock/r... File LayoutTests/wake_lock/resources/subframe.html (right): https://codereview.chromium.org/1084923002/diff/20001/LayoutTests/wake_lock/r... LayoutTests/wake_lock/resources/subframe.html:7: if(e.data.tag === 'getKeepScreenAwake') { nit: space between 'if' and '(' https://codereview.chromium.org/1084923002/diff/20001/LayoutTests/wake_lock/w... File LayoutTests/wake_lock/wakelock-in-nested-frame.html (right): https://codereview.chromium.org/1084923002/diff/20001/LayoutTests/wake_lock/w... LayoutTests/wake_lock/wakelock-in-nested-frame.html:29: document.keepScreenAwake = true; Hmm, what about that: var nestedFrameTest = async_test('Test that keepScreenAwake state is independent in main and nested frame'); (at the top of the file) then at the bottom: window.onload = nestedFrameTest.step_func(function() { // the test }); https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... File Source/modules/wake_lock/DocumentWakeLock.cpp (right): https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/DocumentWakeLock.cpp:14: return WakeLock::keepScreenAwake(document); Would it make sense to return false if document.frame() is null? https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/DocumentWakeLock.cpp:19: WakeLock::setKeepScreenAwake(document, keepScreenAwake); Would it make sense to make it a no-op if document.frame() is null? https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... File Source/modules/wake_lock/WakeLock.cpp (right): https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/WakeLock.cpp:15: WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(WakeLockDocumentVisibilityObserver); WTF_MAKE_NONCOPYABLE? https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/WakeLock.cpp:100: setKeepScreenAwake(false); You might want to set m_controller to false at that point to prevent calling a dangling pointer? https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... File Source/modules/wake_lock/WakeLock.h (right): https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/WakeLock.h:22: WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(WakeLock); WTF_MAKE_NONCOPYABLE? https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... File Source/modules/wake_lock/WakeLockController.cpp (right): https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/WakeLockController.cpp:18: PassOwnPtrWillBeRawPtr<WakeLockController> WakeLockController::create(LocalFrame& frame, WebWakeLockClient* client) You only use this method in ::provideTo(). It seems like you could simply move that line there. https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/WakeLockController.cpp:38: WillBeHeapSupplement<LocalFrame>::provideTo(frame, WakeLockController::supplementName(), WakeLockController::create(frame, client)); Could you add an ASSERT() checking that RuntimeEnabledFeatures::wakeLockEnabled() is true? https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/WakeLockController.cpp:44: m_client->requestKeepScreenAwake(keepScreenAwake); It seems that you will have problems when the frame is detached... https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/WakeLockController.cpp:45: } nit: no need for {} https://codereview.chromium.org/1084923002/diff/20001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1084923002/diff/20001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1646: WakeLockController::provideTo(*m_frame, m_client ? m_client->wakeLockClient() : nullptr); nit: could you append that to the block instead of adding it at the beginning? https://codereview.chromium.org/1084923002/diff/20001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1646: WakeLockController::provideTo(*m_frame, m_client ? m_client->wakeLockClient() : nullptr); nit: could you append that to the block instead of adding it at the beginning? https://codereview.chromium.org/1084923002/diff/20001/Source/web/tests/WakeLo... File Source/web/tests/WakeLockTest.cpp (right): https://codereview.chromium.org/1084923002/diff/20001/Source/web/tests/WakeLo... Source/web/tests/WakeLockTest.cpp:3: // found in the LICENSE file. It's a bit odd to have unit tests in Source/web/ for a module but I guess more tests is better than less.
On 2015/05/05 at 13:45:44, mlamouri wrote: > Thanks for moving things to modules/. I'm still interested to hear what jochen@ has to say about returning a dummy value when the frame is detached. I think it would make a lot of things simpler. > I still wait for some kind of design doc
https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... File Source/modules/wake_lock/DocumentWakeLock.cpp (right): https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/DocumentWakeLock.cpp:14: return WakeLock::keepScreenAwake(document); On 2015/05/05 13:45:43, Mounir Lamouri wrote: > Would it make sense to return false if document.frame() is null? A web page could potentially create a detached document via document.implementation.createHTMLDocument() and set keepScreenAwake flag on it, then read it back, which works for the implementation presented here. This is hardly useful, of course, but nevertheless possible. I will add a layout test to the next CL to demonstrate it (and if we decide to use default value for keepScreenAwake, we can modify this test accordingly to specify the behavior).
https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... File Source/modules/wake_lock/WakeLockController.cpp (right): https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/WakeLockController.cpp:44: m_client->requestKeepScreenAwake(keepScreenAwake); On 2015/05/05 13:45:44, Mounir Lamouri wrote: > It seems that you will have problems when the frame is detached... Could you please elaborate? It seems that e.g. GeolocationContoller does the same thing.
https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... File Source/modules/wake_lock/WakeLockController.cpp (right): https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... Source/modules/wake_lock/WakeLockController.cpp:44: m_client->requestKeepScreenAwake(keepScreenAwake); On 2015/05/06 at 09:12:34, alogvinov wrote: > On 2015/05/05 13:45:44, Mounir Lamouri wrote: > > It seems that you will have problems when the frame is detached... > > Could you please elaborate? It seems that e.g. GeolocationContoller does the same thing. Geolocation.cpp will do early returns if there is no frame. Note that there are tests that should catch if there are obvious issues with detached frames. Generally speaking, you might want to run try jobs for this CL if you haven't yet.
On 2015/05/06 10:03:42, Mounir Lamouri wrote: > https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... > File Source/modules/wake_lock/WakeLockController.cpp (right): > > https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... > Source/modules/wake_lock/WakeLockController.cpp:44: > m_client->requestKeepScreenAwake(keepScreenAwake); > On 2015/05/06 at 09:12:34, alogvinov wrote: > > On 2015/05/05 13:45:44, Mounir Lamouri wrote: > > > It seems that you will have problems when the frame is detached... > > > > Could you please elaborate? It seems that e.g. GeolocationContoller does the > same thing. > > Geolocation.cpp will do early returns if there is no frame. Note that there are > tests that should catch if there are obvious issues with detached frames. > Generally speaking, you might want to run try jobs for this CL if you haven't > yet. How do I run try jobs? It seems that I don't have try job access, but when I try to go to https://chromium-access.appspot.com/request as per instruction, I only get blank page.
On 2015/05/07 at 12:07:36, alogvinov wrote: > On 2015/05/06 10:03:42, Mounir Lamouri wrote: > > https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... > > File Source/modules/wake_lock/WakeLockController.cpp (right): > > > > https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... > > Source/modules/wake_lock/WakeLockController.cpp:44: > > m_client->requestKeepScreenAwake(keepScreenAwake); > > On 2015/05/06 at 09:12:34, alogvinov wrote: > > > On 2015/05/05 13:45:44, Mounir Lamouri wrote: > > > > It seems that you will have problems when the frame is detached... > > > > > > Could you please elaborate? It seems that e.g. GeolocationContoller does the > > same thing. > > > > Geolocation.cpp will do early returns if there is no frame. Note that there are > > tests that should catch if there are obvious issues with detached frames. > > Generally speaking, you might want to run try jobs for this CL if you haven't > > yet. > > How do I run try jobs? It seems that I don't have try job access, but when I try to go to https://chromium-access.appspot.com/request as per instruction, I only get blank page. I just made a request for you to get the try jobs access.
On 2015/05/07 12:54:09, Mounir Lamouri wrote: > On 2015/05/07 at 12:07:36, alogvinov wrote: > > On 2015/05/06 10:03:42, Mounir Lamouri wrote: > > > > https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... > > > File Source/modules/wake_lock/WakeLockController.cpp (right): > > > > > > > https://codereview.chromium.org/1084923002/diff/20001/Source/modules/wake_loc... > > > Source/modules/wake_lock/WakeLockController.cpp:44: > > > m_client->requestKeepScreenAwake(keepScreenAwake); > > > On 2015/05/06 at 09:12:34, alogvinov wrote: > > > > On 2015/05/05 13:45:44, Mounir Lamouri wrote: > > > > > It seems that you will have problems when the frame is detached... > > > > > > > > Could you please elaborate? It seems that e.g. GeolocationContoller does > the > > > same thing. > > > > > > Geolocation.cpp will do early returns if there is no frame. Note that there > are > > > tests that should catch if there are obvious issues with detached frames. > > > Generally speaking, you might want to run try jobs for this CL if you > haven't > > > yet. > > > > How do I run try jobs? It seems that I don't have try job access, but when I > try to go to https://chromium-access.appspot.com/request as per instruction, I > only get blank page. > > I just made a request for you to get the try jobs access. All try jobs are green (except the first one), does it mean that there are no detached frame related problems or should I look into it further?
not lgtm sorry to be a pest about this, but there's no design doc, no tracking bug, and no intent to implement as far as I can see.
On 2015/05/19 at 11:07:55, jochen wrote: > not lgtm > > sorry to be a pest about this, but there's no design doc, no tracking bug, and no intent to implement as far as I can see. FWIW, this is the intent to implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/SzVuAi2KRhA/OaNDiHOK... Work on this project has been on pause for a while and the name of the API doesn't match the one in the intent to implement anymore. I guess sending a refreshed intent to implement wouldn't hurt.
On 2015/05/19 at 11:35:35, mlamouri wrote: > On 2015/05/19 at 11:07:55, jochen wrote: > > not lgtm > > > > sorry to be a pest about this, but there's no design doc, no tracking bug, and no intent to implement as far as I can see. > > > FWIW, this is the intent to implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/SzVuAi2KRhA/OaNDiHOK... > > Work on this project has been on pause for a while and the name of the API doesn't match the one in the intent to implement anymore. I guess sending a refreshed intent to implement wouldn't hurt. ah, maybe just an FYI that it's now called wake lock API to the old thread? anyway, bug and design doc are still missing
On 2015/05/19 11:37:54, jochen wrote: > On 2015/05/19 at 11:35:35, mlamouri wrote: > > On 2015/05/19 at 11:07:55, jochen wrote: > > > not lgtm > > > > > > sorry to be a pest about this, but there's no design doc, no tracking bug, > and no intent to implement as far as I can see. > > > > > > FWIW, this is the intent to implement: > https://groups.google.com/a/chromium.org/d/msg/blink-dev/SzVuAi2KRhA/OaNDiHOK... > > > > Work on this project has been on pause for a while and the name of the API > doesn't match the one in the intent to implement anymore. I guess sending a > refreshed intent to implement wouldn't hurt. > > ah, maybe just an FYI that it's now called wake lock API to the old thread? > > anyway, bug and design doc are still missing I've posted an updated Intent To Implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Yujx917y6-Y Though I used the same tracking bug as for the old one because I think it is still relevant as the proposed API solves the problem specified in the bug. Is this not correct? As for the design doc, are there any specific guidelines how to write one?
On 2015/05/22 at 14:06:17, alogvinov wrote: > On 2015/05/19 11:37:54, jochen wrote: > > On 2015/05/19 at 11:35:35, mlamouri wrote: > > > On 2015/05/19 at 11:07:55, jochen wrote: > > > > not lgtm > > > > > > > > sorry to be a pest about this, but there's no design doc, no tracking bug, > > and no intent to implement as far as I can see. > > > > > > > > > FWIW, this is the intent to implement: > > https://groups.google.com/a/chromium.org/d/msg/blink-dev/SzVuAi2KRhA/OaNDiHOK... > > > > > > Work on this project has been on pause for a while and the name of the API > > doesn't match the one in the intent to implement anymore. I guess sending a > > refreshed intent to implement wouldn't hurt. > > > > ah, maybe just an FYI that it's now called wake lock API to the old thread? > > > > anyway, bug and design doc are still missing > > I've posted an updated Intent To Implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Yujx917y6-Y cool, thanks > > Though I used the same tracking bug as for the old one because I think it is still relevant as the proposed API solves the problem specified in the bug. Is this not correct? Can you modify this CL's description to reference the bug? > > As for the design doc, are there any specific guidelines how to write one? there are no formal requirements. Basically, it should address questions such as the ones that came up on the thread, as well as how you intend to implement this on the browser side (e.g. when is the wake lock actually requested by chrome and when is it released)
On 2015/05/27 14:45:32, jochen wrote: > On 2015/05/22 at 14:06:17, alogvinov wrote: > > On 2015/05/19 11:37:54, jochen wrote: > > > On 2015/05/19 at 11:35:35, mlamouri wrote: > > > > On 2015/05/19 at 11:07:55, jochen wrote: > > > > > not lgtm > > > > > > > > > > sorry to be a pest about this, but there's no design doc, no tracking > bug, > > > and no intent to implement as far as I can see. > > > > > > > > > > > > FWIW, this is the intent to implement: > > > > https://groups.google.com/a/chromium.org/d/msg/blink-dev/SzVuAi2KRhA/OaNDiHOK... > > > > > > > > Work on this project has been on pause for a while and the name of the API > > > doesn't match the one in the intent to implement anymore. I guess sending a > > > refreshed intent to implement wouldn't hurt. > > > > > > ah, maybe just an FYI that it's now called wake lock API to the old thread? > > > > > > anyway, bug and design doc are still missing > > > > I've posted an updated Intent To Implement: > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Yujx917y6-Y > > cool, thanks > > > > > Though I used the same tracking bug as for the old one because I think it is > still relevant as the proposed API solves the problem specified in the bug. Is > this not correct? > > Can you modify this CL's description to reference the bug? > > > > > As for the design doc, are there any specific guidelines how to write one? > > there are no formal requirements. Basically, it should address questions such as > the ones that came up on the thread, as well as how you intend to implement this > on the browser side (e.g. when is the wake lock actually requested by chrome and > when is it released) I've created a design document (https://docs.google.com/document/d/1KbIENP0wgxtSXDQFn9PbHZ_tAKZfR1Y8u4Hst8Lpe...), also added link to the tracking bug.
The recent discussions in the spec repository were moving to have the property on the Screen interface, right?
On 2015/07/02 10:24:22, Mounir Lamouri wrote: > The recent discussions in the spec repository were moving to have the property > on the Screen interface, right? It seems that at least the majority is pro moving it to Screen, and I don't object to it either. I'll check if there are any possible implementation obstacles specific to Chromium and if there are none, update the docs and the CL accordingly.
On 2015/07/02 10:24:22, Mounir Lamouri wrote: > The recent discussions in the spec repository were moving to have the property > on the Screen interface, right? I've updated the implementation so that now the property is in the Screen interface and prepared the corresponding changes to the spec: https://github.com/w3c/wake-lock/pull/54. Also I've refactored the class structure to make it less complicated (hopefully).
It looks good pretty good. I still have a few comments though, see below. https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... File Source/modules/wake_lock/DEPS (right): https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... Source/modules/wake_lock/DEPS:2: "+bindings", do you need that? https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... Source/modules/wake_lock/DEPS:4: "+heap", ditto? https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... File Source/modules/wake_lock/ScreenWakeLock.cpp (right): https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.cpp:16: ScreenWakeLock::ScreenWakeLock(LocalFrame& frame, WebWakeLockClient* client) nit: could you move around the definitions to match the ordering from the header file? https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.cpp:34: nit: remove empty line https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.cpp:45: ditto https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.cpp:84: } Couldn't that be handled by the content layer? https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.cpp:105: void ScreenWakeLock::updateClient() nit: could you rename updateClient() to notificyClient()? https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.cpp:107: if (m_client) { if (!m_client) return; https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... File Source/modules/wake_lock/ScreenWakeLock.idl (right): https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.idl:6: [RuntimeEnabled=WakeLock] attribute boolean keepAwake; nit: could you add the RuntimeEnabled=Wakelock on the partial interface instead? https://codereview.chromium.org/1084923002/diff/180001/Source/web/tests/Scree... File Source/web/tests/ScreenWakeLockTest.cpp (right): https://codereview.chromium.org/1084923002/diff/180001/Source/web/tests/Scree... Source/web/tests/ScreenWakeLockTest.cpp:100: // } nit: remove? https://codereview.chromium.org/1084923002/diff/180001/public/platform/module... File public/platform/modules/wake_lock/WebWakeLockClient.h (right): https://codereview.chromium.org/1084923002/diff/180001/public/platform/module... public/platform/modules/wake_lock/WebWakeLockClient.h:12: virtual ~WebWakeLockClient() { } nit: use "= default;" https://codereview.chromium.org/1084923002/diff/180001/public/platform/module... public/platform/modules/wake_lock/WebWakeLockClient.h:14: virtual void requestKeepScreenAwake(bool) { }; nit: why not "= 0;" ?
https://codereview.chromium.org/1084923002/diff/180001/public/platform/module... File public/platform/modules/wake_lock/WebWakeLockClient.h (right): https://codereview.chromium.org/1084923002/diff/180001/public/platform/module... public/platform/modules/wake_lock/WebWakeLockClient.h:14: virtual void requestKeepScreenAwake(bool) { }; On 2015/07/05 14:50:15, Mounir Lamouri wrote: > nit: why not "= 0;" ? You suggested not using pure virtual functions in this very place in this patch: https://codereview.chromium.org/1084923002/patch/1/10020 "Usually, Blink interfaces are not using pure virtual methods because if the Blink interface changes, it would break Chromium." Has the situation changed since then?
https://codereview.chromium.org/1084923002/diff/180001/public/platform/module... File public/platform/modules/wake_lock/WebWakeLockClient.h (right): https://codereview.chromium.org/1084923002/diff/180001/public/platform/module... public/platform/modules/wake_lock/WebWakeLockClient.h:14: virtual void requestKeepScreenAwake(bool) { }; On 2015/07/06 at 09:26:29, alogvinov wrote: > On 2015/07/05 14:50:15, Mounir Lamouri wrote: > > nit: why not "= 0;" ? > > You suggested not using pure virtual functions in this very place in this patch: https://codereview.chromium.org/1084923002/patch/1/10020 > > "Usually, Blink interfaces are not using pure virtual methods because if the Blink interface changes, it would break Chromium." > > Has the situation changed since then? Eh :) I would say that is obviously a problem for classes that already exist because it would break Chromium's build. I'm not sure if that's a problem for new classes actually. I guess I'm confused :) Jochen, do you know what's the expected practice?
https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... File Source/modules/wake_lock/ScreenWakeLock.cpp (right): https://codereview.chromium.org/1084923002/diff/180001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.cpp:84: } On 2015/07/05 14:50:14, Mounir Lamouri wrote: > Couldn't that be handled by the content layer? Probably it could, but we're going to need to reset m_keepAwake here anyway, or else it would retain the value from the previous loaded page, so some form of notification would still be needed.
Now that the spec is changed to use Screen interface instead of Document, the implementation conforms to the spec.
lgtm with comments applied. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... File Source/modules/wake_lock/ScreenWakeLock.cpp (right): https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.cpp:71: // Reset wake lock flag for this frame if it is the one being navigated nit: period at the end of the sentence. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... File Source/modules/wake_lock/ScreenWakeLock.h (right): https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:27: // IDL-bound functions nit: we usually don't add that kind of comments. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:38: // Inherited from PageLifecycleObserver nit: period at the end of the sentence. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:42: // Inherited from LocalFrameLifecycleObserver ditto. https://codereview.chromium.org/1084923002/diff/200001/Source/platform/Runtim... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1084923002/diff/200001/Source/platform/Runtim... Source/platform/RuntimeEnabledFeatures.in:179: WakeLock status=experimental test only maybe for now
jochen@chromium.org changed reviewers: + haraken@chromium.org
+haraken for oilpan https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... File Source/modules/wake_lock/ScreenWakeLock.h (right): https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:19: class MODULES_EXPORT ScreenWakeLock final all the inherited stuff on one line please, there's no line length limit https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:22: , PageLifecycleObserver always add public https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:24: WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(ScreenWakeLock); why WILL_BE? modules are already in oilpan https://codereview.chromium.org/1084923002/diff/200001/public/platform/module... File public/platform/modules/wake_lock/WebWakeLockClient.h (right): https://codereview.chromium.org/1084923002/diff/200001/public/platform/module... public/platform/modules/wake_lock/WebWakeLockClient.h:14: virtual void requestKeepScreenAwake(bool) { }; nit. should this be pure virtual?
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Oilpan fine. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... File Source/modules/wake_lock/ScreenWakeLock.h (right): https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:21: , public WillBeHeapSupplement<LocalFrame> Question: ScreenWakeLock.idl is defined to be a Screen partial interface, but this is a supplement of LocalFrame (and not Screen); is that intentional? https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:24: WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(ScreenWakeLock); On 2015/07/24 11:25:43, jochen wrote: > why WILL_BE? modules are already in oilpan A bit confusing, but WILL_BE is relative to LocalFrame&Page here, and being supplement/observers of either. Those two core objects aren't yet on the Oilpan heap by default. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:40: virtual void didCommitLoad(LocalFrame*) override; Blink is now aligned with http://code.google.com/p/chromium/issues/detail?id=417463 , so leave out 'virtual' from these overrides. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:54: WebWakeLockClient* m_client; nit: have m_keepAwake last.
On 2015/07/24 12:01:51, sof wrote: > Oilpan fine. > > https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... > File Source/modules/wake_lock/ScreenWakeLock.h (right): > > https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... > Source/modules/wake_lock/ScreenWakeLock.h:21: , public > WillBeHeapSupplement<LocalFrame> > Question: ScreenWakeLock.idl is defined to be a Screen partial interface, but > this is a supplement of LocalFrame (and not Screen); is that intentional? > > https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... > Source/modules/wake_lock/ScreenWakeLock.h:24: > WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(ScreenWakeLock); > On 2015/07/24 11:25:43, jochen wrote: > > why WILL_BE? modules are already in oilpan > > A bit confusing, but WILL_BE is relative to LocalFrame&Page here, and being > supplement/observers of either. Those two core objects aren't yet on the Oilpan > heap by default. > > https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... > Source/modules/wake_lock/ScreenWakeLock.h:40: virtual void > didCommitLoad(LocalFrame*) override; > Blink is now aligned with > http://code.google.com/p/chromium/issues/detail?id=417463 , so leave out > 'virtual' from these overrides. > > https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... > Source/modules/wake_lock/ScreenWakeLock.h:54: WebWakeLockClient* m_client; > nit: have m_keepAwake last. Yes, ScreenWakeLock intentionally supplements LocalFrame and not Screen because it is tied to LocalFrame-related facilities such as LocalFrameLifeycleObserver. It could have been divided into two separate classes (one for Screen IDL static functions and one for LocalFrame-related code) but as the logic is very simple, I don't see a point in such subdivision.
https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... File Source/modules/wake_lock/ScreenWakeLock.cpp (right): https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.cpp:71: // Reset wake lock flag for this frame if it is the one being navigated On 2015/07/24 10:47:57, Mounir Lamouri wrote: > nit: period at the end of the sentence. Acknowledged. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... File Source/modules/wake_lock/ScreenWakeLock.h (right): https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:19: class MODULES_EXPORT ScreenWakeLock final On 2015/07/24 11:25:43, jochen wrote: > all the inherited stuff on one line please, there's no line length limit Acknowledged. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:22: , PageLifecycleObserver On 2015/07/24 11:25:43, jochen wrote: > always add public Acknowledged. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:27: // IDL-bound functions On 2015/07/24 10:47:57, Mounir Lamouri wrote: > nit: we usually don't add that kind of comments. Acknowledged. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:38: // Inherited from PageLifecycleObserver On 2015/07/24 10:47:57, Mounir Lamouri wrote: > nit: period at the end of the sentence. Acknowledged. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:40: virtual void didCommitLoad(LocalFrame*) override; On 2015/07/24 12:01:51, sof wrote: > Blink is now aligned with > http://code.google.com/p/chromium/issues/detail?id=417463 , so leave out > 'virtual' from these overrides. Acknowledged. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:42: // Inherited from LocalFrameLifecycleObserver On 2015/07/24 10:47:57, Mounir Lamouri wrote: > ditto. Acknowledged. https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... Source/modules/wake_lock/ScreenWakeLock.h:54: WebWakeLockClient* m_client; On 2015/07/24 12:01:51, sof wrote: > nit: have m_keepAwake last. Acknowledged. https://codereview.chromium.org/1084923002/diff/200001/public/platform/module... File public/platform/modules/wake_lock/WebWakeLockClient.h (right): https://codereview.chromium.org/1084923002/diff/200001/public/platform/module... public/platform/modules/wake_lock/WebWakeLockClient.h:14: virtual void requestKeepScreenAwake(bool) { }; On 2015/07/24 11:25:43, jochen wrote: > nit. should this be pure virtual? It used to be in the beginning. I'll make it pure virtual again.
Oilpan part LGTM
On 2015/07/24 12:39:23, alogvinov wrote: > On 2015/07/24 12:01:51, sof wrote: > ... > > https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... > > Source/modules/wake_lock/ScreenWakeLock.h:21: , public > > WillBeHeapSupplement<LocalFrame> > > Question: ScreenWakeLock.idl is defined to be a Screen partial interface, but > > this is a supplement of LocalFrame (and not Screen); is that intentional? > > ... > > Yes, ScreenWakeLock intentionally supplements LocalFrame and not Screen because > it is tied to LocalFrame-related facilities such as LocalFrameLifeycleObserver. > It could have been divided into two separate classes (one for Screen IDL static > functions and one for LocalFrame-related code) but as the logic is very simple, > I don't see a point in such subdivision. You have to be careful about non-overlapping lifetimes in this area, avoiding one supplementable from freeing an owned supplement before it becomes inaccessible by other routes. The implementation here though redirects Screen-relative accesses of the partial interface back to the LocalFrame supplement, and checks for the Screen being detached, so this looks fine.
On 2015/07/24 11:25:43, jochen wrote: > +haraken for oilpan > > https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... > File Source/modules/wake_lock/ScreenWakeLock.h (right): > > https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... > Source/modules/wake_lock/ScreenWakeLock.h:19: class MODULES_EXPORT > ScreenWakeLock final > all the inherited stuff on one line please, there's no line length limit > > https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... > Source/modules/wake_lock/ScreenWakeLock.h:22: , PageLifecycleObserver > always add public > > https://codereview.chromium.org/1084923002/diff/200001/Source/modules/wake_lo... > Source/modules/wake_lock/ScreenWakeLock.h:24: > WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(ScreenWakeLock); > why WILL_BE? modules are already in oilpan > > https://codereview.chromium.org/1084923002/diff/200001/public/platform/module... > File public/platform/modules/wake_lock/WebWakeLockClient.h (right): > > https://codereview.chromium.org/1084923002/diff/200001/public/platform/module... > public/platform/modules/wake_lock/WebWakeLockClient.h:14: virtual void > requestKeepScreenAwake(bool) { }; > nit. should this be pure virtual? Can you please take another look?
lgtm from my side, but please wait for sigbjornf to ack that he's happy with the lifetime here
On 2015/07/29 11:44:50, jochen wrote: > lgtm from my side, but please wait for sigbjornf to ack that he's happy with the > lifetime here it's fine, i see no detached use problems.
The CQ bit was checked by alogvinov@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1084923002/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1084923002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1084923002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199659
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/2f87b1b5d7ce34064a9f1302a9a19683abf592a8 |
