Return same promise consistently when using navigator.getBattery()

Source/modules/battery/BatteryManager.cpp

Index: Source/modules/battery/BatteryManager.cpp
diff --git a/Source/modules/battery/BatteryManager.cpp b/Source/modules/battery/BatteryManager.cpp
index 79d8e262aa58b9069bc3427b93a94a5305a103cb..23bc84c2e694c546e37f74fdce897b50c7f1bf56 100644
--- a/Source/modules/battery/BatteryManager.cpp
+++ b/Source/modules/battery/BatteryManager.cpp
@@ -31,30 +31,25 @@ BatteryManager::BatteryManager(ExecutionContext* context)
     : ActiveDOMObject(context)
     , PlatformEventController(toDocument(context)->page())
     , m_batteryStatus(BatteryStatus::create())
-    , m_state(NotStarted)
+    , m_isResolved(false)
 {
 }
 
 ScriptPromise BatteryManager::startRequest(ScriptState* scriptState)
 {
-    if (m_state == Pending)
-        return m_resolver->promise();
-
-    m_resolver = ScriptPromiseResolver::create(scriptState);
-    ScriptPromise promise = m_resolver->promise();
-
-    // If the context is in a stopped state already, do not start updating.
-    if (m_state == Resolved || !executionContext() || executionContext()->activeDOMObjectsAreStopped()) {
-        // FIXME: Consider returning the same promise in this case. See crbug.com/385025.
-        m_state = Resolved;
-        m_resolver->resolve(this);
-    } else if (m_state == NotStarted) {
-        m_state = Pending;
-        m_hasEventListener = true;
-        startUpdating();
+    if (!m_batteryProperty) {
+        m_batteryProperty = new BatteryProperty(scriptState->executionContext(), this, BatteryProperty::Ready);
+
+        // If the context is in a stopped state already, do not start updating.
+        if (m_isResolved || !executionContext() || executionContext()->activeDOMObjectsAreStopped()) {
+            m_batteryProperty->resolve(this);
+        } else {
+            m_hasEventListener = true;
+            startUpdating();
+        }
     }
 
-    return promise;
+    return m_batteryProperty->promise(scriptState->world());
 }
 
 bool BatteryManager::charging()
@@ -80,15 +75,14 @@ double BatteryManager::level()
 void BatteryManager::didUpdateData()
 {
     ASSERT(RuntimeEnabledFeatures::batteryStatusEnabled());
-    ASSERT(m_state != NotStarted);
+    ASSERT(m_batteryProperty);
 
     BatteryStatus* oldStatus = m_batteryStatus;
     m_batteryStatus = BatteryDispatcher::instance().latestData();
 
-    if (m_state == Pending) {
-        ASSERT(m_resolver);
-        m_state = Resolved;
-        m_resolver->resolve(this);
+    if (!m_isResolved) {

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

Instead of m_isResolved, could you use m_batteryProperty.state()? (from
ScriptPromisePropertyBase)

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

Done.

+        m_isResolved = true;
+        m_batteryProperty->resolve(this);
         return;
     }
 
@@ -139,7 +133,8 @@ void BatteryManager::resume()
 void BatteryManager::stop()
 {
     m_hasEventListener = false;
-    m_state = NotStarted;
+    m_isResolved = false;
+    m_batteryProperty.clear();
     stopUpdating();
 }
 
@@ -147,12 +142,12 @@ bool BatteryManager::hasPendingActivity() const
 {
     // Prevent V8 from garbage collecting the wrapper object if there are
     // event listeners attached to it.
-    return m_state == Resolved && hasEventListeners();
+    return m_isResolved && hasEventListeners();

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

Why |m_isResolved|? instead of |m_batteryProperty|? Actually, why just not only
hasEventListeners()?

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

yes changed to hasEventListeners() seems sufficient.

 }
 
 DEFINE_TRACE(BatteryManager)
 {
-    visitor->trace(m_resolver);
+    visitor->trace(m_batteryProperty);
     visitor->trace(m_batteryStatus);
     PlatformEventController::trace(visitor);
     RefCountedGarbageCollectedEventTargetWithInlineData<BatteryManager>::trace(visitor);