[sensors] Generic Sensors Framework blink side

third_party/WebKit/Source/modules/sensor/Sensor.cpp

Index: third_party/WebKit/Source/modules/sensor/Sensor.cpp
diff --git a/third_party/WebKit/Source/modules/sensor/Sensor.cpp b/third_party/WebKit/Source/modules/sensor/Sensor.cpp
index 9d716ca5e02e56ebec1fc95b3088d3d2ec82e2a2..0696a9f0556a690a37b2895070ec74d979206553 100644
--- a/third_party/WebKit/Source/modules/sensor/Sensor.cpp
+++ b/third_party/WebKit/Source/modules/sensor/Sensor.cpp
@@ -6,130 +6,290 @@
 
 #include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
-#include "core/dom/ExecutionContextTask.h"
-#include "core/events/Event.h"
-
+#include "device/generic_sensor/public/interfaces/sensor.mojom-blink.h"
+#include "modules/sensor/SensorErrorEvent.h"
+#include "modules/sensor/SensorPollingStrategy.h"
+#include "modules/sensor/SensorProviderProxy.h"
 #include "modules/sensor/SensorReading.h"
+#include "modules/sensor/SensorReadingEvent.h"
 
-namespace blink {
+using namespace device::mojom::blink;
 
-Sensor::~Sensor()
-{
-}
+namespace blink {
 
-Sensor::Sensor(ExecutionContext* executionContext, const SensorOptions& sensorOptions)
+Sensor::Sensor(ExecutionContext* executionContext, const SensorOptions& sensorOptions, device::mojom::blink::SensorType type)
     : ActiveScriptWrappable(this)
     , ActiveDOMObject(executionContext)
-    , PlatformEventController(toDocument(executionContext)->page())
-    , m_sensorState(SensorState::Idle)
-    , m_sensorReading(nullptr)
+    , PageVisibilityObserver(toDocument(executionContext)->page())
     , m_sensorOptions(sensorOptions)
+    , m_type(type)
+    , m_state(Sensor::SensorState::IDLE)
 {
+    DCHECK(executionContext->isDocument());
 }
 
-// Getters
-String Sensor::state() const
+void Sensor::dispose()
+{
+    stopListening();
+}
+
+void Sensor::start(ScriptState* scriptState, ExceptionState& exceptionState)
+{
+    if (m_state != Sensor::SensorState::IDLE && m_state != Sensor::SensorState::ERRORED) {
+        exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is not idle or errored");

[timvolodine, 2016/08/25 17:52:32]

error message not very specific, maybe better something like "cannot start
sensor because it is either active or activating"..
and maybe also change the if condition accordingly

[Mikhail, 2016/08/26 16:42:41]

This if condition is coming from spec
https://w3c.github.io/sensors/#sensor-start

so maybe "Cannot start because SensorState is not idle or errored"?

[timvolodine, 2016/09/01 19:02:05]

yes, that sounds more informative compared to just "Invalid state:.."

+        return;
+    }
+
+    initSensorProxyIfNeeded();
+
+    if (!m_sensorProxy) {
+        exceptionState.throwDOMException(InvalidStateError, "The Sensor is no longer associated to a frame.");

[timvolodine, 2016/08/25 17:52:32]

when can this happen? and should this ever happen?

[Mikhail, 2016/08/26 16:42:41]

document might not be always associated with a frame during its lifetime, in
these cases m_sensorProxy will remain 'nullptr'. There are other example of the
same check, e.g. at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/presen...

+        return;
+    }
+
+    updateState(Sensor::SensorState::ACTIVATING);
+
+    startListening();
+}
+
+void Sensor::stop(ScriptState*, ExceptionState& exceptionState)
 {
-    // TODO(riju): Validate the transitions.
-    switch (m_sensorState) {
-    case SensorState::Idle:
+    if (m_state == Sensor::SensorState::IDLE || m_state == Sensor::SensorState::ERRORED) {
+        exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is either idle or errored");

[timvolodine, 2016/08/25 17:52:32]

same here: maybe something like "Ignoring stop() because SensorState is idle or
errored."

[Mikhail, 2016/08/26 16:42:41]

Done.

+        return;
+    }
+
+    if (!m_sensorProxy) {

[timvolodine, 2016/08/25 17:52:32]

same question

+        exceptionState.throwDOMException(InvalidStateError, "The Sensor is no longer associated to a frame.");
+        return;
+    }
+
+    stopListening();
+}
+
+static String ToString(Sensor::SensorState state)
+{
+    switch (state) {
+    case Sensor::SensorState::IDLE:
         return "idle";
-    case SensorState::Activating:
+    case Sensor::SensorState::ACTIVATING:
         return "activating";
-    case SensorState::Active:
+    case Sensor::SensorState::ACTIVE:
         return "active";
-    case SensorState::Errored:
+    case Sensor::SensorState::ERRORED:
         return "errored";
+    default:
+        NOTREACHED();
     }
-    NOTREACHED();
     return "idle";
 }
 
+// Getters
+String Sensor::state() const
+{
+    return ToString(m_state);
+}
+
 SensorReading* Sensor::reading() const
 {
     return m_sensorReading.get();
 }
 
-void Sensor::start(ScriptState* scriptState, ExceptionState& exceptionState)
+DEFINE_TRACE(Sensor)
 {
+    visitor->trace(m_polling);
+    visitor->trace(m_sensorProxy);
+    visitor->trace(m_sensorReading);
+    ActiveScriptWrappable::trace(visitor);
+    ActiveDOMObject::trace(visitor);
+    PageVisibilityObserver::trace(visitor);
+    EventTargetWithInlineData::trace(visitor);
+}
 
-    if (m_sensorState != SensorState::Idle && m_sensorState != SensorState::Errored) {
-        exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is not idle or errored");
+bool Sensor::hasPendingActivity() const
+{
+    if (!getExecutionContext() || getExecutionContext()->activeDOMObjectsAreStopped())
+        return false;
+    return hasEventListeners();
+}
+
+void Sensor::initSensorProxyIfNeeded()
+{
+    if (m_sensorProxy)
+        return;
+
+    Document* document = toDocument(getExecutionContext());
+    if (!document->frame())
+        return;
+
+    m_sensorProxy = SensorProviderProxy::getOrCreateForFrame(document->frame())->getOrCreateSensor(m_type);
+}
+
+void Sensor::addedEventListener(const AtomicString& eventType, RegisteredEventListener&)
+{
+    if (EventTypeNames::change == eventType)

[timvolodine, 2016/08/25 17:52:32]

hmm, don't we have "change" already, would this be confused with another event?

[Mikhail, 2016/08/26 16:42:41]

This method is called on JS call like "sensor.onchange = .."
could you explain what "confused with another event" means?

[timvolodine, 2016/09/01 19:02:05]

I was looking at
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/EventTypeNames....
so there is already a 'change' defined in blink so my question was about if
'sensor.onchange' would be confused with something else here

[Mikhail, 2016/09/02 08:23:43]

This event is local to sensor object (does not bubble and is not cancelable), so
it does participate in DOM event dispatch mechanism. The 'onchange' events are
also defined in other modules, e.g. netinfo, permissions, presentation,
remoteplayback, screen_orientation.

+        updatePollingStatus();
+}
+
+void Sensor::removedEventListener(const AtomicString& eventType, const RegisteredEventListener&)
+{
+    if (EventTypeNames::change == eventType)
+        updatePollingStatus();
+}
+
+void Sensor::stop()
+{
+    stopListening();
+}
+
+void Sensor::onSensorInitialized()
+{
+    if (m_state != Sensor::SensorState::ACTIVATING)
         return;
-    }
 
-    updateState(SensorState::Activating);
+    if (!m_sensorProxy) {
+        reportError();
+        return;
+    }
 
-    // TODO(riju) : Add Permissions stuff later.
+    auto startCallback = WTF::bind(&Sensor::onStartRequestCompleted, wrapWeakPersistent(this));
+    m_sensorProxy->addConfiguration(createSensorOptions(m_sensorOptions), std::move(startCallback));
+}
 
-    m_hasEventListener = true;
+void Sensor::onSensorReadingChanged()
+{
+    if (m_polling)
+        m_polling->onSensorReadingChanged();
+}
 
-    // TODO(riju): verify the correct order of onstatechange(active) and the first onchange(event).
-    startUpdating();
+void Sensor::onSensorError()
+{
+    reportError();
 }
 
-void Sensor::stop(ScriptState* scriptState, ExceptionState& exceptionState)
+void Sensor::onStartRequestCompleted(bool result)
 {
-    if (m_sensorState == SensorState::Idle || m_sensorState == SensorState::Errored) {
-        exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is either idle or errored");
+    if (m_state != Sensor::SensorState::ACTIVATING)
+        return;
+
+    if (!result || !m_sensorProxy) {
+        reportError();
         return;
     }
 
-    m_hasEventListener = false;
-    stopUpdating();
+    updateState(Sensor::SensorState::ACTIVE);
 
-    m_sensorReading.clear();
-    updateState(SensorState::Idle);
+    double frequency = m_sensorOptions.hasFrequency() ? m_sensorOptions.frequency() : 1;

[timvolodine, 2016/08/25 17:52:32]

why is this needed? i.e. if hasFrequency() is false should be handled
appropriately (not just cap at 1) ?

also, looking at SensorPollingStrategy::create below, why not just pass the
sensorOptions directly? it seems any particular polling decisions should be made
by the "strategy" class.

[Mikhail, 2016/08/26 16:42:41]

We need to slightly modify the mojo interface to handle it properly and
@shalamov is preparing a CL for it.. Meanwhile I've added a // TODO comment 
Strategies are selected based on reporting mode rather than sensorOptions.
I'd rather keep sensorOptions "parsing" here (in the Sensor class).

[timvolodine, 2016/09/01 19:02:05]

hmm don't see the TODO comment in the latest patch..

[Mikhail, 2016/09/02 08:23:42]

The latest patch contains already the solution for that. Default Sensor
parameters are handled using the default sensor configuration obtained from the
platform. It became possible after 'GetDefaultConfiguration()' method was added
to the mojo interface (https://codereview.chromium.org/2288103002/). So no need
in "TODO" comment any more. 

+    auto pollCallback = WTF::bind(&Sensor::pollForData, wrapWeakPersistent(this));
+    m_polling = SensorPollingStrategy::create(frequency, std::move(pollCallback), m_sensorProxy->reportingMode());
+    updatePollingStatus();
 }
 
-void Sensor::updateState(SensorState newState)
+void Sensor::onStopRequestCompleted(bool result)
 {
-    DCHECK(isMainThread());
-    if (m_sensorState == newState)
+    if (m_state == Sensor::SensorState::IDLE)
         return;
 
-    m_sensorState = newState;
-    // Notify context that state changed.
-    if (getExecutionContext())
-        getExecutionContext()->postTask(BLINK_FROM_HERE, createSameThreadTask(&Sensor::notifyStateChange, wrapPersistent(this)));
+    if (!result)
+        reportError();
+
+    if (m_sensorProxy)
+        m_sensorProxy->removeObserver(this);
 }
 
-void Sensor::notifyStateChange()
+void Sensor::pageVisibilityChanged()
 {
-    dispatchEvent(Event::create(EventTypeNames::statechange));
+    updatePollingStatus();
 }
 
-void Sensor::suspend()
+void Sensor::startListening()
 {
-    m_hasEventListener = false;
-    stopUpdating();
+    if (!m_sensorProxy)
+        return;
+
+    if (!m_sensorReading) {
+        m_sensorReading = createSensorReading(getExecutionContext());
+        m_sensorReading->attach(m_sensorProxy);
+    }
+
+    m_sensorProxy->addObserver(this);
+    if (m_sensorProxy->isInitialized()) {
+        auto callback = WTF::bind(&Sensor::onStartRequestCompleted, wrapWeakPersistent(this));
+        m_sensorProxy->addConfiguration(createSensorOptions(m_sensorOptions), std::move(callback));

[timvolodine, 2016/08/25 17:52:32]

can startListening() be invoked multiple times? if so, would that result in
addConfiguration call each time that happens?

[Mikhail, 2016/08/26 16:42:41]

It is called only from 'Sensor::start' and only if sensor state was 'idle' or
'errored' (as per spec), so it is called only once for each "activating -
active" sensor states cycle.

+    } else {
+        m_sensorProxy->initialize();
+    }
 }
 
-void Sensor::resume()
+void Sensor::stopListening()
 {
-    m_hasEventListener = true;
-    startUpdating();
+    if (m_sensorReading) {
+        m_sensorReading->detach();
+        m_sensorReading = nullptr;
+    }
+
+    updateState(Sensor::SensorState::IDLE);
+
+    if (!m_sensorProxy)
+        return;
+
+    if (m_sensorProxy->isInitialized()) {
+        auto callback = WTF::bind(&Sensor::onStopRequestCompleted, wrapWeakPersistent(this));
+        m_sensorProxy->removeConfiguration(createSensorOptions(m_sensorOptions), std::move(callback));
+    } else {
+        m_sensorProxy->removeObserver(this);
+    }
 }
 
-void Sensor::stop()
+void Sensor::pollForData()
 {
-    m_hasEventListener = false;
-    stopUpdating();
+    if (m_state != Sensor::SensorState::ACTIVE) {

[timvolodine, 2016/08/25 17:52:32]

shouldn't this be a DCHECK. i.e. invariant that pollForData is never called when
SensorState is not ACTIVE?

[Mikhail, 2016/08/26 16:42:41]

It can happen if a scheduled task from 'OnChangeSensorPollingStrategy' arrives
after state has been already changed.

+        DCHECK(m_polling);
+        m_polling->stopPolling();
+        return;
+    }
+
+    switch (m_sensorReading->updateInternalData()) {
+    case SensorReading::Updated:
+        dispatchEvent(SensorReadingEvent::create(EventTypeNames::change, m_sensorReading));
+        break;
+    case SensorReading::Error:
+        reportError();
+        break;
+    case SensorReading::Same:
+        break;
+    default:
+        NOTREACHED();
+    }
 }
 
-bool Sensor::hasPendingActivity() const
+void Sensor::updateState(Sensor::SensorState newState)
 {
-    // Prevent V8 from garbage collecting the wrapper object if there are
-    // event listeners attached to it.
-    return hasEventListeners();
+    if (newState == m_state)
+        return;
+    m_state = newState;
+    dispatchEvent(Event::create(EventTypeNames::statechange));
 }
 
-DEFINE_TRACE(Sensor)
+void Sensor::reportError()
 {
-    ActiveDOMObject::trace(visitor);
-    EventTargetWithInlineData::trace(visitor);
-    PlatformEventController::trace(visitor);
-    visitor->trace(m_sensorReading);
+    updateState(Sensor::SensorState::ERRORED);
+    updatePollingStatus();
+    // TODO(Mikhail) : Dispatch Sensor Error event.
+}
+
+void Sensor::updatePollingStatus()
+{
+    if (!m_polling)
+        return;
+
+    if (m_state != Sensor::SensorState::ACTIVE // Sensor is not active.
+        || !hasEventListeners(EventTypeNames::change) // Noone is listening to 'onchange' event.

[timvolodine, 2016/08/25 17:52:32]

noone -> "no one" or "nobody" ;)

actually the comments are pretty obvious

[Mikhail, 2016/08/26 16:42:41]

Done.

+        || page()->visibilityState() != PageVisibilityStateVisible) { // Page is not visible.
+        m_polling->stopPolling();
+    } else {
+        m_polling->startPolling();
+    }
 }
 
 } // namespace blink