Battery Status API: blink promise implementation and layout tests.

Battery Status API: blink promise implementation and layout tests.

Implement Battery Status API using promises in blink as
according to the (modified) spec:
https://dvcs.w3.org/hg/dap/raw-file/tip/battery/Overview.html.
Using this patch the Battery Status API is accessible using:
navigator.getBattery().then(...) syntax.

Also added layout tests for the various use-cases.

BUG=122593, 360068
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176222

[abarth-chromium, 2014-06-11 19:44:35]

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/B...
File Source/modules/battery/BatteryManager.cpp (right):

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:68: }
Why not just return m_resolver->promise(); at this point?

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:72:
promise.then(ThenFunction::create(this));
Why do we need to create a ThenFunction instead of just setting m_state =
Resolved near line 116?

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/B...
File Source/modules/battery/BatteryManager.h (right):

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.h:66: void onPromiseResolved();
didResolvePromise?

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/N...
File Source/modules/battery/NavigatorBattery.cpp (right):

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.cpp:31:
toV8NoInline(m_batteryManager.get(), scriptState->context()->Global(),
scriptState->isolate());
Why is this necessary?  We're already holding m_batteryManager...

[timvolodine, 2014-06-12 16:38:46]

thanks for the comments, please take another look.

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/B...
File Source/modules/battery/BatteryManager.cpp (right):

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:68: }
On 2014/06/11 19:44:34, abarth wrote:
> Why not just return m_resolver->promise(); at this point?

slightly refactored this code. Done.

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:72:
promise.then(ThenFunction::create(this));
On 2014/06/11 19:44:34, abarth wrote:
> Why do we need to create a ThenFunction instead of just setting m_state =
> Resolved near line 116?

yeah on second thought the "ThenFunction" seems like overkill. Done.

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/B...
File Source/modules/battery/BatteryManager.h (right):

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.h:66: void onPromiseResolved();
On 2014/06/11 19:44:34, abarth wrote:
> didResolvePromise?

Done.

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/N...
File Source/modules/battery/NavigatorBattery.cpp (right):

https://codereview.chromium.org/329723005/diff/20001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.cpp:31:
toV8NoInline(m_batteryManager.get(), scriptState->context()->Global(),
scriptState->isolate());
On 2014/06/11 19:44:34, abarth wrote:
> Why is this necessary?  We're already holding m_batteryManager...

Done.

[abarth-chromium, 2014-06-13 04:36:30]

LGTM w/ comment below

https://codereview.chromium.org/329723005/diff/40001/Source/modules/battery/B...
File Source/modules/battery/BatteryManager.cpp (right):

https://codereview.chromium.org/329723005/diff/40001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:43: m_resolver->resolve(this);
It's strange that we return a the same promise while m_state == Pending but that
we start returning new promises once the state becomes Resolved.  Why not always
return m_resolver->promise() when m_resolver is non-zero?

if (!m_resolver) {
    ASSERT(m_state == NotStarted);
    m_state = Pending;
    m_resolver = ScriptPromiseResolverWithContext::create(scriptState);
    m_hasEventListener = true;
    startUpdating();
}
return m_resolver->promise();

[timvolodine, 2014-06-13 14:42:20]

https://codereview.chromium.org/329723005/diff/40001/Source/modules/battery/B...
File Source/modules/battery/BatteryManager.cpp (right):

https://codereview.chromium.org/329723005/diff/40001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:43: m_resolver->resolve(this);
On 2014/06/13 04:36:30, abarth wrote:
> It's strange that we return a the same promise while m_state == Pending but
that
> we start returning new promises once the state becomes Resolved.  Why not
always
> return m_resolver->promise() when m_resolver is non-zero?
> 
> if (!m_resolver) {
>     ASSERT(m_state == NotStarted);
>     m_state = Pending;
>     m_resolver = ScriptPromiseResolverWithContext::create(scriptState);
>     m_hasEventListener = true;
>     startUpdating();
> }
> return m_resolver->promise();

I've tried this code +
if (m_state == Resolved) { m_resolver->resolve(this); }
and it doesn't seem to work as expected for the case when m_state==Resolved.
Apparently the same resolver cannot resolve twice.. (e.g. it fails on
multiple-promises-after-resolve.html.)

Instead I've now added a Vector of resolvers to handle the special case when we
get multiple startRequests() while the battery has not resolved yet.

[abarth-chromium, 2014-06-13 17:44:36]

On 2014/06/13 at 14:42:20, timvolodine wrote:
> I've tried this code +
> if (m_state == Resolved) { m_resolver->resolve(this); }
> and it doesn't seem to work as expected for the case when m_state==Resolved.
Apparently the same resolver cannot resolve twice.. (e.g. it fails on
multiple-promises-after-resolve.html.)

Yes, you can only resolve each promise once.

> Instead I've now added a Vector of resolvers to handle the special case when
we get multiple startRequests() while the battery has not resolved yet.

That's not correct.  Instead, you can just keep returning the same promise all
the time.  It's fine to return an already resolved promise.  When the client
calls .then() on the promise, the callback will be called at the end of the
microtask.

[timvolodine, 2014-06-13 17:56:38]

On 2014/06/13 17:44:36, abarth wrote:
> On 2014/06/13 at 14:42:20, timvolodine wrote:
> > I've tried this code +
> > if (m_state == Resolved) { m_resolver->resolve(this); }
> > and it doesn't seem to work as expected for the case when m_state==Resolved.
> Apparently the same resolver cannot resolve twice.. (e.g. it fails on
> multiple-promises-after-resolve.html.)
> 
> Yes, you can only resolve each promise once.
> 
> > Instead I've now added a Vector of resolvers to handle the special case when
> we get multiple startRequests() while the battery has not resolved yet.
> 
> That's not correct.  Instead, you can just keep returning the same promise all
> the time.  It's fine to return an already resolved promise.  When the client
> calls .then() on the promise, the callback will be called at the end of the
> microtask.

when I tried that, the test in multiple-promises-after-resolve.html was timing
out. Looks like the second resolution was not happening. Should it happen if I
just return a resolved promise without explicitly calling "resolve"?

[timvolodine, 2014-06-13 18:54:50]

On 2014/06/13 17:56:38, timvolodine wrote:
> On 2014/06/13 17:44:36, abarth wrote:
> > On 2014/06/13 at 14:42:20, timvolodine wrote:
> > > I've tried this code +
> > > if (m_state == Resolved) { m_resolver->resolve(this); }
> > > and it doesn't seem to work as expected for the case when
m_state==Resolved.
> > Apparently the same resolver cannot resolve twice.. (e.g. it fails on
> > multiple-promises-after-resolve.html.)
> > 
> > Yes, you can only resolve each promise once.
> > 
> > > Instead I've now added a Vector of resolvers to handle the special case
when
> > we get multiple startRequests() while the battery has not resolved yet.
> > 
> > That's not correct.  Instead, you can just keep returning the same promise
all
> > the time.  It's fine to return an already resolved promise.  When the client
> > calls .then() on the promise, the callback will be called at the end of the
> > microtask.
> 
> when I tried that, the test in multiple-promises-after-resolve.html was timing
> out. Looks like the second resolution was not happening. Should it happen if I
> just return a resolved promise without explicitly calling "resolve"?

If you strongly believe this is incorrect, I'll probably revert to your
suggestion with one resolver, but it looks like there could be an issue in the
nested-resolution case..

[abarth-chromium, 2014-06-13 22:01:51]

On 2014/06/13 at 18:54:50, timvolodine wrote:
> > when I tried that, the test in multiple-promises-after-resolve.html was
timing
> > out. Looks like the second resolution was not happening. Should it happen if
I
> > just return a resolved promise without explicitly calling "resolve"?

Yes, I would think so.  Try it:

http://jsfiddle.net/GGWMe/

> If you strongly believe this is incorrect, I'll probably revert to your
suggestion with one resolver, but it looks like there could be an issue in the
nested-resolution case..

Maybe there's an issue with the PromiseResolver dropping the promise after it
resolves?  I see ScriptPromiseResolverWithContext::resolveOrRejectImmediately
calls clear(), which drops the promise.

[abarth-chromium, 2014-06-13 22:03:09]

+dominicc, because he was mentioning a similar problem with
ScriptPromiseResolverWithContext he was having for service worker.

For now, I think it's fine to create a new promise resolver and resolve it right
away if you're in the resolved state.  In the future, it would be better to keep
returning the same promise, but that's a fine point.

[abarth-chromium, 2014-06-13 22:04:00]

In summary, can we go back to the earlier version where you have one promise
resolver at a time?  Sorry for the run-around.

[timvolodine, 2014-06-13 23:35:43]

On 2014/06/13 22:01:51, abarth wrote:
> On 2014/06/13 at 18:54:50, timvolodine wrote:
> > > when I tried that, the test in multiple-promises-after-resolve.html was
> timing
> > > out. Looks like the second resolution was not happening. Should it happen
if
> I
> > > just return a resolved promise without explicitly calling "resolve"?
> 
> Yes, I would think so.  Try it:
> 
> http://jsfiddle.net/GGWMe/
> 
> > If you strongly believe this is incorrect, I'll probably revert to your
> suggestion with one resolver, but it looks like there could be an issue in the
> nested-resolution case..
> 
> Maybe there's an issue with the PromiseResolver dropping the promise after it
> resolves?  I see ScriptPromiseResolverWithContext::resolveOrRejectImmediately
> calls clear(), which drops the promise.

Yes the comment in ScriptPromiseResolver.h also mentions that the
resolver->promise() returns an "empty" promise after it is resolved.
The code below does appear to work as expected, so it seems to be a resolver
issue.

var promise = new Promise(function(resolve, reject) {
   resolve(3);
});
promise.then(function(val) {
    alert('First time ' + val);
    promise.then(function(val) {
       alert('Second time '+val); 
    });
});

[timvolodine, 2014-06-13 23:37:48]

On 2014/06/13 22:04:00, abarth wrote:
> In summary, can we go back to the earlier version where you have one promise
> resolver at a time?  Sorry for the run-around.

sure, will do that, no problem.

[dominicc (has gone to gerrit), 2014-06-14 21:51:04]

Don't want to hijack your code review, but I was aware of
ScriptPromiseResolver clearing the promise and am already carrying around
two things: the promise and its resolver.

For a property that returns a promise that it makes sense to "fire"
multiple times due to successive state changes (obviously with a new
promise each time) having these as separate members is reasonably
convenient.

I did more digging and I think my problem is specific to when a promise
from one frame is joined (".then"ed) from a different frame. Spelunking is
not complete but I think native promises may be buggy in this situation,
sometimes not resolving and sometimes hitting assertions.

On Fri, Jun 13, 2014 at 4:37 PM, <timvolodine@chromium.org> wrote:

> On 2014/06/13 22:04:00, abarth wrote:
>
>> In summary, can we go back to the earlier version where you have one
>> promise
>> resolver at a time?  Sorry for the run-around.
>>
>
> sure, will do that, no problem.
>
>
> https://codereview.chromium.org/329723005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[timvolodine, 2014-06-16 16:53:09]

On 2014/06/14 21:51:04, dominicc wrote:
> Don't want to hijack your code review, but I was aware of
> ScriptPromiseResolver clearing the promise and am already carrying around
> two things: the promise and its resolver.
> 
> For a property that returns a promise that it makes sense to "fire"
> multiple times due to successive state changes (obviously with a new
> promise each time) having these as separate members is reasonably
> convenient.
> 
> I did more digging and I think my problem is specific to when a promise
> from one frame is joined (".then"ed) from a different frame. Spelunking is
> not complete but I think native promises may be buggy in this situation,
> sometimes not resolving and sometimes hitting assertions.
> 

thanks for the comments, I guess we could store the promise as well. I've added
a 'fixme' referring to crbug.com/385025 for future reference.

[timvolodine, 2014-06-16 16:55:28]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-06-16 16:56:08]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/timvolodine@chromium.org/329723005/120001

[commit-bot: I haz the power, 2014-06-16 17:00:20]

Change committed as 176222

[jamesr, 2014-06-18 19:58:45]

The multiple-windows.html test is incredibly flaky:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

I'm going to disable it, please fix and re-enable.

[timvolodine, 2014-07-03 12:07:48]

On 2014/06/18 19:58:45, jamesr wrote:
> The multiple-windows.html test is incredibly flaky:
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
> 
> I'm going to disable it, please fix and re-enable.

FYI, fixed and re-enabled:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...