Return same promise consistently when using navigator.getBattery()

Source/modules/battery/BatteryManager.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "modules/battery/BatteryManager.h"
 
 #include "core/dom/Document.h"
 #include "core/events/Event.h"
 #include "modules/battery/BatteryDispatcher.h"
@@ skipping 13 matching lines @@
 {
 #if !ENABLE(OILPAN)
     stopUpdating();
 #endif
 }
 
 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();
+    if (!m_batteryProperty) {
+        m_batteryProperty = new BatteryProperty(scriptState->executionContext(), this, BatteryProperty::Ready);
 
-    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 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()
 {
     return m_batteryStatus->charging();
 }
 
 double BatteryManager::chargingTime()
 {
     return m_batteryStatus->chargingTime();
 }
 
 double BatteryManager::dischargingTime()
 {
     return m_batteryStatus->dischargingTime();
 }
 
 double BatteryManager::level()
 {
     return m_batteryStatus->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;
     }
 
     Document* document = toDocument(executionContext());
     ASSERT(document);
     if (document->activeDOMObjectsAreSuspended() || document->activeDOMObjectsAreStopped())
         return;
 
     ASSERT(oldStatus);
 
@@ skipping 30 matching lines @@
 
 void BatteryManager::resume()
 {
     m_hasEventListener = true;
     startUpdating();
 }
 
 void BatteryManager::stop()
 {
     m_hasEventListener = false;
-    m_state = NotStarted;
+    m_isResolved = false;
+    m_batteryProperty.clear();
     stopUpdating();
 }
 
 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);
     ActiveDOMObject::trace(visitor);
 }
 
 } // namespace blink