Wake Lock API implementation (Blink part)

Wake 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

[alogvinov, 2015-04-22 08:51:09]

Can you please take a look at this patch?

[mlamouri (slow - plz ping), 2015-04-28 08:00:26]

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.

[alogvinov, 2015-04-28 10:32:12]

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

[alogvinov, 2015-04-28 16:34:25]

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.

[alogvinov, 2015-04-28 16:49:10]

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.

[mlamouri (slow - plz ping), 2015-04-29 10:07:08]

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.

[alogvinov, 2015-04-29 16:50:59]

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 (slow - plz ping), 2015-04-30 13:15:37]

mlamouri@chromium.org changed reviewers:
+ jl@opera.com, jochen@chromium.org

[mlamouri (slow - plz ping), 2015-04-30 13:15:38]

+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.

[Jens Widell, 2015-04-30 14:07:51]

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.)

[jochen (gone - plz use gerrit), 2015-05-04 07:01:59]

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?

[mlamouri (slow - plz ping), 2015-05-04 13:19:14]

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.

[alogvinov, 2015-05-05 11:29:57]

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.

[Jens Widell, 2015-05-05 11:34:15]

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.

[mlamouri (slow - plz ping), 2015-05-05 13:45:44]

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.

[jochen (gone - plz use gerrit), 2015-05-05 14:16:22]

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

[alogvinov, 2015-05-05 16:22:27]

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).

[alogvinov, 2015-05-06 09:12:34]

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.

[mlamouri (slow - plz ping), 2015-05-06 10:03:42]

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.

[alogvinov, 2015-05-07 12:07:36]

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.

[mlamouri (slow - plz ping), 2015-05-07 12:54:09]

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.

[alogvinov, 2015-05-18 17:06:34]

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?

[jochen (gone - plz use gerrit), 2015-05-19 11:07:55]

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.

[mlamouri (slow - plz ping), 2015-05-19 11:35:35]

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.

[jochen (gone - plz use gerrit), 2015-05-19 11:37:54]

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

[alogvinov, 2015-05-22 14:06:17]

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?

[jochen (gone - plz use gerrit), 2015-05-27 14:45:32]

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)

[alogvinov, 2015-07-02 09:26:23]

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.

[mlamouri (slow - plz ping), 2015-07-02 10:24:22]

The recent discussions in the spec repository were moving to have the property
on the Screen interface, right?

[alogvinov, 2015-07-02 10:44:41]

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.

[alogvinov, 2015-07-03 16:51:19]

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).

[mlamouri (slow - plz ping), 2015-07-05 14:50:15]

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;" ?

[alogvinov, 2015-07-06 09:26:29]

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?

[mlamouri (slow - plz ping), 2015-07-06 10:13:01]

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?

[alogvinov, 2015-07-06 10:18:35]

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.

[alogvinov, 2015-07-09 16:22:17]

Now that the spec is changed to use Screen interface instead of Document, the
implementation conforms to the spec.

[mlamouri (slow - plz ping), 2015-07-24 10:47:58]

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 (gone - plz use gerrit), 2015-07-24 11:25:42]

jochen@chromium.org changed reviewers:
+ haraken@chromium.org

[jochen (gone - plz use gerrit), 2015-07-24 11:25:43]

+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?

[sof, 2015-07-24 12:01:50]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-07-24 12:01:51]

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.

[alogvinov, 2015-07-24 12:39:23]

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.

[alogvinov, 2015-07-24 13:36:01]

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.

[haraken, 2015-07-24 18:39:28]

Oilpan part LGTM

[sof, 2015-07-27 06:39:01]

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.

[alogvinov, 2015-07-29 08:30:00]

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?

[jochen (gone - plz use gerrit), 2015-07-29 11:44:50]

lgtm from my side, but please wait for sigbjornf to ack that he's happy with the
lifetime here

[sof, 2015-07-29 11:52:45]

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.

[alogvinov, 2015-07-29 12:13:37]

The CQ bit was checked by alogvinov@yandex-team.ru

[alogvinov, 2015-07-29 12:13:37]

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: " ")

[commit-bot: I haz the power, 2015-07-29 12:13:46]

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

[commit-bot: I haz the power, 2015-07-29 13:21:30]

Committed patchset #13 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199659

[commit-bot: I haz the power, 2015-09-23 11:53:33]

Patchset 13 (id:??) landed as
https://crrev.com/2f87b1b5d7ce34064a9f1302a9a19683abf592a8