Initial implementation of API WakeLock.

Initial implementation of API WakeLock.

BUG=257511
R=darin

[abarth-chromium, 2014-07-18 03:07:20]

This path is too large.  can you divide it into smaller pieces, each of which
has a test?

[kenneth.christiansen, 2014-07-18 09:54:00]

The wake lock only affects the screen right? then why is it associated with the
navigator object and not screen?

[mlamouri (slow - plz ping), 2014-07-18 09:59:25]

On 2014/07/18 09:54:00, kenneth.christiansen wrote:
> The wake lock only affects the screen right? then why is it associated with
the
> navigator object and not screen?

As you can see in WakeLock.idl, there are two types of WakeLockType, "screen"
and "system". With "system", you can ask the system to stay at full power (for
example, if you do heavy computation). There are discussions to split "system"
in different parts including "cpu".

[Ilya Bogdanovich, 2014-07-18 16:20:01]

On 2014/07/18 03:07:20, abarth wrote:
> This path is too large.  can you divide it into smaller pieces, each of which
> has a test?

Ok, we'll divide it into a number of patches with tests. May you have at least a
brief look at this patch - maybe there's something we doing completely wrong
here?

[abarth-chromium, 2014-07-18 18:02:40]

On 2014/07/18 at 16:20:01, bogdanovichiy wrote:
> Ok, we'll divide it into a number of patches with tests. May you have at least
a brief look at this patch - maybe there's something we doing completely wrong
here?

You shouldn't need to modify public/web.  All the interfaces with Chromium
should be in public/platform.  That will greatly reduce the amount of
boilerplate code in your CL.

[Ilya Bogdanovich, 2014-07-21 13:01:17]

On 2014/07/18 18:02:40, abarth wrote:
> On 2014/07/18 at 16:20:01, bogdanovichiy wrote:
> > Ok, we'll divide it into a number of patches with tests. May you have at
least
> a brief look at this patch - maybe there's something we doing completely wrong
> here?
> 
> You shouldn't need to modify public/web.  All the interfaces with Chromium
> should be in public/platform.  That will greatly reduce the amount of
> boilerplate code in your CL.

Thanks, got it!

[redchenko, 2014-08-07 14:43:58]

Please, take a look to Patch 2. This is the base implementation of WakeLock API.
Binding with blink is planned in the next commit.

[redchenko, 2014-08-11 17:06:38]

Please, take a look at the Patch 3.

[darin (slow to review), 2014-08-11 19:47:16]

This CL seems to be missing a way to get (or set) the WebWakeLockClient
interface. I only see it being done in a unit testing context. How will Chromium
use WebWakeLockClient?

[redchenko, 2014-08-12 08:37:22]

On 2014/08/11 19:47:16, darin wrote:
> This CL seems to be missing a way to get (or set) the WebWakeLockClient
> interface. I only see it being done in a unit testing context. How will
Chromium
> use WebWakeLockClient?

Using of WebWakeLockClient was implemented in Patch 1 (look at
public/web/WebViewClient.h and Source/web/WebViewImpl.h/cpp). But reviewers
(abarth) said that patch is too large and asked to divide it into smaller CLs.
We will commit WebWakeLockClient usage in one of the next CLs, it will be almost
like as in Patch 1.

[mlamouri (slow - plz ping), 2014-08-18 12:15:53]

(sorry for the late review, I was on vacation)

That patch looks great. It's very exciting to see so much progress happening on
that API! I have a few minor comments below, they are numerous but really
nothing major.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/NavigatorWakeLock.cpp (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.cpp:16: : DOMWindowProperty(frame)
I would keep this ctor as a default ctor (ie. no LocalFrame*).

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.cpp:47: m_wakeLock =
WakeLock::create(frame()->document());
It seems that what you really want here is not a frame() or a document() but the
ExecutionContext. You can get that from the ScriptState. If you add
[CallWith=ScriptState] in the idl file, you should get the bindings to generate
static NavigatorWakeLock::wakeLock(ScriptState*, Navigator&), then you can have
NavigatorWakeLock::wakeLock(ScriptState*).

Doing that would prevent NavigatorWakeLock from inheriting from
DOMWindowProperty so you will not need to keep track of the window and frame
here while you actually don't need to.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.cpp:53: ASSERT(visitor);
I don't think you need that.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/NavigatorWakeLock.h (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.h:21: , public DOMWindowProperty {
I think you should be able to not depend on DOMWindowProperty, see below.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.h:23: 
nit: remove this empty line.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.h:34: NavigatorWakeLock(LocalFrame*);
I think this should be NavigatorWakeLock(). But if it is not, it should probably
be marked as explicit.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.h:37: mutable
RefPtrWillBeMember<WakeLock> m_wakeLock;
Why is m_wakeLock mutable? If you have a good reason for that, you might want to
add a comment explaining it here.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/NavigatorWakeLock.idl (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.idl:8: readonly attribute WakeLock
wakeLock;
You might want to upload this CL with:
git cl upload --no-find-copies
to prevent git from assuming you copied NavigatorPushMessage.idl.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/WakeLock.h (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLock.h:28: Document* document() const;
Could that be private like page()?

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLock.h:29: LocalFrame* frame() const;
You do not seem to use frame().

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/WakeLock.idl (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLock.idl:7: "system"
Do you intend to implement "system"? Should we stick with "screen" for the
moment? We should consider how to behave if we support "screen" but not
"system": should we reject "system" as an unknown keyword or reject it as known
but unsupported?

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLock.idl:13: NoInterfaceObject,
Why is that NoInterfaceObject? It's unlikely what we want.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLock.idl:17: boolean isHeld(WakeLockType type);
Should you add an event handler for when the lock changes?

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/WakeLockController.cpp (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockController.cpp:17:
m_lockCounter.resize(WebWakeLockNone + 1);
I'm not sure you need the +1, assuming you will not lock on "None". Also, given
the usage you are making of it, why not name this "WebWakeLockGuard"?

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockController.cpp:98: ++m_lockCounter[type];
Does that mean that we expect to handle multiple locks for the same frame? I
would expect that we would release when .release() is called regardless of the
number of times .request() was called before. In other words, .request() should
be a no-op if the lock is already held. We could take this into account and not
go all the way to the embedder in that case. The spec doesn't specify whether
the request should be rejected or resolved in that case. I believe it should be
resolved because the lock will happen FWIW.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockController.cpp:123:
WakeLockPromiseResolverOwnPtr& resolver = (*it);
nit: no need for the parenthesis.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/WakeLockController.h (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockController.h:40: virtual void
onCreatedWakeLockSuccessful(int);
nit: specify the interface you are implementing, like:
// Implements WebWakeLockRequestCallback.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/WakeLockPromiseResolver.cpp (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockPromiseResolver.cpp:37: m_resolver->resolve(0);
Maybe you want:
m_resolver->resolve();

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockPromiseResolver.cpp:42: m_resolver->reject(0);
and:
m_resolver->reject();

https://codereview.chromium.org/399313003/diff/80001/Source/web/tests/WakeLoc...
File Source/web/tests/WakeLockTest.cpp (right):

https://codereview.chromium.org/399313003/diff/80001/Source/web/tests/WakeLoc...
Source/web/tests/WakeLockTest.cpp:263: wakeLockController->resetClient();
I did not review those tests but I was wondering why not taking the route of
some LayoutTests with a mock client set up in the content shell? You can look at
how mock_web_push_client or mock_screen_orientation do that
(content/shell/test_runner/).

https://codereview.chromium.org/399313003/diff/80001/public/platform/WebWakeL...
File public/platform/WebWakeLockClient.h (right):

https://codereview.chromium.org/399313003/diff/80001/public/platform/WebWakeL...
public/platform/WebWakeLockClient.h:17: ~WebWakeLockClient() { }
virtual

https://codereview.chromium.org/399313003/diff/80001/public/platform/WebWakeL...
public/platform/WebWakeLockClient.h:19: virtual void
requestWakeLock(WebWakeLockType, int requestId, WebSecurityOrigin) = 0;
Couldn't you set this client in WebFrameClient and pass the WebFrame instead of
WebSecurityOrigin? According to your previous comment, you seem to want this to
be hooked to the WebView but it seems that it could be better in the WebFrame
given that you might want the lock to be in the browser context scope (ie. each
frame would have independant locks). Also, passing a WebFrame would give you
access to the WebSecurityOrigin and much more if needed.

https://codereview.chromium.org/399313003/diff/80001/public/platform/WebWakeL...
public/platform/WebWakeLockClient.h:23: virtual void
setRequestCallback(WebWakeLockRequestCallback*) = 0;
It's hard to review those without the WebWakeLockClient usage. Do you have a
link for the other CLs?

[redchenko, 2014-08-19 16:42:21]

Mounir, thank you for review!

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/NavigatorWakeLock.cpp (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.cpp:16: : DOMWindowProperty(frame)
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> I would keep this ctor as a default ctor (ie. no LocalFrame*).

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.cpp:47: m_wakeLock =
WakeLock::create(frame()->document());
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> It seems that what you really want here is not a frame() or a document() but
the
> ExecutionContext. You can get that from the ScriptState. If you add
> [CallWith=ScriptState] in the idl file, you should get the bindings to
generate
> static NavigatorWakeLock::wakeLock(ScriptState*, Navigator&), then you can
have
> NavigatorWakeLock::wakeLock(ScriptState*).
> 
> Doing that would prevent NavigatorWakeLock from inheriting from
> DOMWindowProperty so you will not need to keep track of the window and frame
> here while you actually don't need to.

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.cpp:53: ASSERT(visitor);
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> I don't think you need that.

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/NavigatorWakeLock.h (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.h:21: , public DOMWindowProperty {
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> I think you should be able to not depend on DOMWindowProperty, see below.

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.h:23: 
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> nit: remove this empty line.

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.h:34: NavigatorWakeLock(LocalFrame*);
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> I think this should be NavigatorWakeLock(). But if it is not, it should
probably
> be marked as explicit.

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.h:37: mutable
RefPtrWillBeMember<WakeLock> m_wakeLock;
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> Why is m_wakeLock mutable? If you have a good reason for that, you might want
to
> add a comment explaining it here.

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/NavigatorWakeLock.idl (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/NavigatorWakeLock.idl:8: readonly attribute WakeLock
wakeLock;
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> You might want to upload this CL with:
> git cl upload --no-find-copies
> to prevent git from assuming you copied NavigatorPushMessage.idl.

Thanks!

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/WakeLock.h (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLock.h:28: Document* document() const;
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> Could that be private like page()?

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLock.h:29: LocalFrame* frame() const;
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> You do not seem to use frame().

Done. frame() was removed.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/WakeLock.idl (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLock.idl:7: "system"
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> Do you intend to implement "system"? Should we stick with "screen" for the
> moment? We should consider how to behave if we support "screen" but not
> "system": should we reject "system" as an unknown keyword or reject it as
known
> but unsupported?

Yes we do, as Chromium's PowerSaveBlocker already has the code for that.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLock.idl:13: NoInterfaceObject,
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> Why is that NoInterfaceObject? It's unlikely what we want.

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLock.idl:17: boolean isHeld(WakeLockType type);
On 2014/08/18 12:15:52, Mounir Lamouri wrote:
> Should you add an event handler for when the lock changes?

Right, I'm planning it in the next CLs. Also there are questions about event
types we need.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/WakeLockController.cpp (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockController.cpp:17:
m_lockCounter.resize(WebWakeLockNone + 1);
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> I'm not sure you need the +1, assuming you will not lock on "None". Also,
given
> the usage you are making of it, why not name this "WebWakeLockGuard"?

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockController.cpp:98: ++m_lockCounter[type];
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> Does that mean that we expect to handle multiple locks for the same frame? I
> would expect that we would release when .release() is called regardless of the
> number of times .request() was called before. In other words, .request()
should
> be a no-op if the lock is already held. We could take this into account and
not
> go all the way to the embedder in that case. The spec doesn't specify whether
> the request should be rejected or resolved in that case. I believe it should
be
> resolved because the lock will happen FWIW.

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockController.cpp:123:
WakeLockPromiseResolverOwnPtr& resolver = (*it);
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> nit: no need for the parenthesis.

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/WakeLockController.h (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockController.h:40: virtual void
onCreatedWakeLockSuccessful(int);
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> nit: specify the interface you are implementing, like:
> // Implements WebWakeLockRequestCallback.

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
File Source/modules/wake_lock/WakeLockPromiseResolver.cpp (right):

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockPromiseResolver.cpp:37: m_resolver->resolve(0);
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> Maybe you want:
> m_resolver->resolve();

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/modules/wake_lock...
Source/modules/wake_lock/WakeLockPromiseResolver.cpp:42: m_resolver->reject(0);
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> and:
> m_resolver->reject();

Done.

https://codereview.chromium.org/399313003/diff/80001/Source/web/tests/WakeLoc...
File Source/web/tests/WakeLockTest.cpp (right):

https://codereview.chromium.org/399313003/diff/80001/Source/web/tests/WakeLoc...
Source/web/tests/WakeLockTest.cpp:263: wakeLockController->resetClient();
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> I did not review those tests but I was wondering why not taking the route of
> some LayoutTests with a mock client set up in the content shell? You can look
at
> how mock_web_push_client or mock_screen_orientation do that
> (content/shell/test_runner/).

Yes, we'll add layout tests later.

https://codereview.chromium.org/399313003/diff/80001/public/platform/WebWakeL...
File public/platform/WebWakeLockClient.h (right):

https://codereview.chromium.org/399313003/diff/80001/public/platform/WebWakeL...
public/platform/WebWakeLockClient.h:17: ~WebWakeLockClient() { }
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> virtual

Done.

https://codereview.chromium.org/399313003/diff/80001/public/platform/WebWakeL...
public/platform/WebWakeLockClient.h:23: virtual void
setRequestCallback(WebWakeLockRequestCallback*) = 0;
On 2014/08/18 12:15:53, Mounir Lamouri wrote:
> It's hard to review those without the WebWakeLockClient usage. Do you have a
> link for the other CLs?

I'll send link to CL later.

[redchenko, 2014-08-19 21:26:07]

IPC messages for WakeLock API
https://codereview.chromium.org/471763006/

[redchenko, 2014-08-19 21:59:34]

Mounir, please take a look at next CLs:
1) IPC messages for WakeLock API https://codereview.chromium.org/471763006
2) Created client and IPC at renderer side
https://codereview.chromium.org/486383003
3) IPC at browser side https://codereview.chromium.org/474913003/

[redchenko, 2014-08-20 09:56:57]

On 2014/07/18 18:02:40, abarth wrote:
> On 2014/07/18 at 16:20:01, bogdanovichiy wrote:
> > Ok, we'll divide it into a number of patches with tests. May you have at
least
> a brief look at this patch - maybe there's something we doing completely wrong
> here?
> 
> You shouldn't need to modify public/web.  All the interfaces with Chromium
> should be in public/platform.  That will greatly reduce the amount of
> boilerplate code in your CL.

Is it okay to modify public/web/WebFrameClient.h and add an abstract method
(create WebWakeLockClient)?

[mlamouri (slow - plz ping), 2014-09-03 13:12:58]

On 2014/08/20 09:56:57, redchenko wrote:
> On 2014/07/18 18:02:40, abarth wrote:
> > On 2014/07/18 at 16:20:01, bogdanovichiy wrote:
> > > Ok, we'll divide it into a number of patches with tests. May you have at
> least
> > a brief look at this patch - maybe there's something we doing completely
wrong
> > here?
> > 
> > You shouldn't need to modify public/web.  All the interfaces with Chromium
> > should be in public/platform.  That will greatly reduce the amount of
> > boilerplate code in your CL.
> 
> Is it okay to modify public/web/WebFrameClient.h and add an abstract method
> (create WebWakeLockClient)?

Yes, it is okay to add this in WebFrameClient.h:
virtual WebWakeLockClient* webWakeLockClient() { return 0; }

It will then be implemented by content/renderer/render_frame_impl.cc

Redchenko, my understanding is that the API is still in heavy flux. Do you still
want me to review this CL or are you planning to update it to match the latest
discussions?