Return same promise consistently when using navigator.getBattery()

Return same promise consistently when using navigator.getBattery()

Use ScriptPromiseProperty to return same battery manager promise
every time, even when it has already been resolved. This is in
line with the specification. It also simplifies the code logic a bit
since there is no need to keep an enum for the state of the battery
manager.

BUG=385025
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197048

[timvolodine, 2015-03-05 17:09:14]

timvolodine@chromium.org changed reviewers:
+ mlamouri@chromium.org

[timvolodine, 2015-03-05 17:09:14]

Mounir would you be able to take a look?

[mlamouri (slow - plz ping), 2015-03-07 14:54:42]

Very nice patch! :)

Could you have a look at the comments below?

https://codereview.chromium.org/976373002/diff/1/Source/modules/battery/Batte...
File Source/modules/battery/BatteryManager.cpp (right):

https://codereview.chromium.org/976373002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryManager.cpp:83: if (!m_isResolved) {
Instead of m_isResolved, could you use m_batteryProperty.state()? (from
ScriptPromisePropertyBase)

https://codereview.chromium.org/976373002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryManager.cpp:145: return m_isResolved &&
hasEventListeners();
Why |m_isResolved|? instead of |m_batteryProperty|? Actually, why just not only
hasEventListeners()?

https://codereview.chromium.org/976373002/diff/1/Source/modules/battery/Batte...
File Source/modules/battery/BatteryManager.h (right):

https://codereview.chromium.org/976373002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryManager.h:62: typedef
ScriptPromiseProperty<Member<BatteryManager>, Member<BatteryManager>,
RefPtrWillBeMember<DOMException>> BatteryProperty;
nit: you can use C++11 form which I think is more readable (using
BatteryProperty = ScriptPromiseProperty<...>;)

[timvolodine, 2015-03-12 13:53:11]

thanks for the comments, ptal

https://codereview.chromium.org/976373002/diff/1/Source/modules/battery/Batte...
File Source/modules/battery/BatteryManager.cpp (right):

https://codereview.chromium.org/976373002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryManager.cpp:83: if (!m_isResolved) {
On 2015/03/07 14:54:42, Mounir Lamouri wrote:
> Instead of m_isResolved, could you use m_batteryProperty.state()? (from
> ScriptPromisePropertyBase)

Done.

https://codereview.chromium.org/976373002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryManager.cpp:145: return m_isResolved &&
hasEventListeners();
On 2015/03/07 14:54:42, Mounir Lamouri wrote:
> Why |m_isResolved|? instead of |m_batteryProperty|? Actually, why just not
only
> hasEventListeners()?

yes changed to hasEventListeners() seems sufficient.

https://codereview.chromium.org/976373002/diff/1/Source/modules/battery/Batte...
File Source/modules/battery/BatteryManager.h (right):

https://codereview.chromium.org/976373002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryManager.h:62: typedef
ScriptPromiseProperty<Member<BatteryManager>, Member<BatteryManager>,
RefPtrWillBeMember<DOMException>> BatteryProperty;
On 2015/03/07 14:54:42, Mounir Lamouri wrote:
> nit: you can use C++11 form which I think is more readable (using
> BatteryProperty = ScriptPromiseProperty<...>;)

Done.

[mlamouri (slow - plz ping), 2015-03-13 16:20:00]

lgtm with the comment below addressed

Thanks! :)

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

https://codereview.chromium.org/976373002/diff/20001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:43: if (m_batteryProperty->state() ==
ScriptPromisePropertyBase::Resolved || !executionContext() ||
executionContext()->activeDOMObjectsAreStopped()) {
|m_batteryProperty->state() == ScriptPromisePropertyBase::Resolved|
doesn't sound look useful here given that you create it just above. Is it
possible that the object is resolved at creation time?

[timvolodine, 2015-06-12 17:29:04]

revisiting this patch..

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

https://codereview.chromium.org/976373002/diff/20001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:43: if (m_batteryProperty->state() ==
ScriptPromisePropertyBase::Resolved || !executionContext() ||
executionContext()->activeDOMObjectsAreStopped()) {
On 2015/03/13 16:20:00, Mounir Lamouri wrote:
> |m_batteryProperty->state() == ScriptPromisePropertyBase::Resolved|
> doesn't sound look useful here given that you create it just above. Is it
> possible that the object is resolved at creation time?

right, removed this test

[timvolodine, 2015-06-12 17:29:17]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2015-06-12 17:29:17]

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/976373002/#ps40001 (title:
"rebase, make compile, fix comment")

[commit-bot: I haz the power, 2015-06-12 17:29:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/976373002/40001

[commit-bot: I haz the power, 2015-06-12 17:33:04]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197048