[sensors] Generic Sensors Framework blink side

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

 // Copyright 2016 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 "modules/sensor/Sensor.h"
 
 #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"
+
+using namespace device::mojom::blink;
 
 namespace blink {
 
-Sensor::~Sensor()
-{
-}
-
-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());
+}
+
+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)
+{
+    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 Sensor::SensorState::ACTIVATING:
+        return "activating";
+    case Sensor::SensorState::ACTIVE:
+        return "active";
+    case Sensor::SensorState::ERRORED:
+        return "errored";
+    default:
+        NOTREACHED();
+    }
+    return "idle";
 }
 
 // Getters
 String Sensor::state() const
 {
-    // TODO(riju): Validate the transitions.
-    switch (m_sensorState) {
-    case SensorState::Idle:
-        return "idle";
-    case SensorState::Activating:
-        return "activating";
-    case SensorState::Active:
-        return "active";
-    case SensorState::Errored:
-        return "errored";
-    }
-    NOTREACHED();
-    return "idle";
+    return ToString(m_state);
 }
 
 SensorReading* Sensor::reading() const
 {
     return m_sensorReading.get();
 }
 
-void Sensor::start(ScriptState* scriptState, ExceptionState& exceptionState)
-{
-
-    if (m_sensorState != SensorState::Idle && m_sensorState != SensorState::Errored) {
-        exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is not idle or errored");
-        return;
-    }
-
-    updateState(SensorState::Activating);
-
-    // TODO(riju) : Add Permissions stuff later.
-
-    m_hasEventListener = true;
-
-    // TODO(riju): verify the correct order of onstatechange(active) and the first onchange(event).
-    startUpdating();
-}
-
-void Sensor::stop(ScriptState* scriptState, ExceptionState& exceptionState)
-{
-    if (m_sensorState == SensorState::Idle || m_sensorState == SensorState::Errored) {
-        exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is either idle or errored");
-        return;
-    }
-
-    m_hasEventListener = false;
-    stopUpdating();
-
-    m_sensorReading.clear();
-    updateState(SensorState::Idle);
-}
-
-void Sensor::updateState(SensorState newState)
-{
-    DCHECK(isMainThread());
-    if (m_sensorState == newState)
-        return;
-
-    m_sensorState = newState;
-    // Notify context that state changed.
-    if (getExecutionContext())
-        getExecutionContext()->postTask(BLINK_FROM_HERE, createSameThreadTask(&Sensor::notifyStateChange, wrapPersistent(this)));
-}
-
-void Sensor::notifyStateChange()
-{
+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);
+}
+
+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;
+
+    if (!m_sensorProxy) {
+        reportError();
+        return;
+    }
+
+    auto startCallback = WTF::bind(&Sensor::onStartRequestCompleted, wrapWeakPersistent(this));
+    m_sensorProxy->addConfiguration(createSensorOptions(m_sensorOptions), std::move(startCallback));
+}
+
+void Sensor::onSensorReadingChanged()
+{
+    if (m_polling)
+        m_polling->onSensorReadingChanged();
+}
+
+void Sensor::onSensorError()
+{
+    reportError();
+}
+
+void Sensor::onStartRequestCompleted(bool result)
+{
+    if (m_state != Sensor::SensorState::ACTIVATING)
+        return;
+
+    if (!result || !m_sensorProxy) {
+        reportError();
+        return;
+    }
+
+    updateState(Sensor::SensorState::ACTIVE);
+
+    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::onStopRequestCompleted(bool result)
+{
+    if (m_state == Sensor::SensorState::IDLE)
+        return;
+
+    if (!result)
+        reportError();
+
+    if (m_sensorProxy)
+        m_sensorProxy->removeObserver(this);
+}
+
+void Sensor::pageVisibilityChanged()
+{
+    updatePollingStatus();
+}
+
+void Sensor::startListening()
+{
+    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::stopListening()
+{
+    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::pollForData()
+{
+    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();
+    }
+}
+
+void Sensor::updateState(Sensor::SensorState newState)
+{
+    if (newState == m_state)
+        return;
+    m_state = newState;
     dispatchEvent(Event::create(EventTypeNames::statechange));
 }
 
-void Sensor::suspend()
-{
-    m_hasEventListener = false;
-    stopUpdating();
-}
-
-void Sensor::resume()
-{
-    m_hasEventListener = true;
-    startUpdating();
-}
-
-void Sensor::stop()
-{
-    m_hasEventListener = false;
-    stopUpdating();
-}
-
-bool Sensor::hasPendingActivity() const
-{
-    // Prevent V8 from garbage collecting the wrapper object if there are
-    // event listeners attached to it.
-    return hasEventListeners();
-}
-
-DEFINE_TRACE(Sensor)
-{
-    ActiveDOMObject::trace(visitor);
-    EventTargetWithInlineData::trace(visitor);
-    PlatformEventController::trace(visitor);
-    visitor->trace(m_sensorReading);
+void Sensor::reportError()
+{
+    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