Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2121313002:
[sensors] Generic Sensors Framework blink side (Closed)
|
Created:
4 years, 5 months ago by Mikhail Modified:
4 years, 3 months ago CC:
chromium-reviews, blink-reviews, abarth-chromium, shalamov, riju_ Base URL:
https://chromium.googlesource.com/chromium/src.git@sensors_mojo_interfaces Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sensors] Blink-side implementation of mojo interfaces
This patch introduces blink-side implementation of generic
sensor mojo interfaces.
Specification: https://w3c.github.io/sensors
Browser side CL: https://crrev.com/2144623003/
BUG=606766
Committed: https://crrev.com/0c00597deb691b3d180f09f25ee67a494e927311
Cr-Commit-Position: refs/heads/master@{#416920}
Patch Set 1 #Patch Set 2 : Use 'SuspenNotification' API #Patch Set 3 : Updated due to latest device::mojom::blink::Sensor interface #Patch Set 4 : Rebased #
Total comments: 60
Patch Set 5 : Comments from Tim #Patch Set 6 : Stop unneeded class exporting #Patch Set 7 : Align SensorReading update with the spec #Patch Set 8 : Properly handle default sensor options #
Total comments: 8
Patch Set 9 : Comments from Riju #
Total comments: 17
Patch Set 10 : Comments from Tim #
Total comments: 5
Patch Set 11 : Comments from Tim. Removed the default sensor configuration management (to be added with a separate… #
Total comments: 27
Patch Set 12 : Keeping mojo wrappers only + comments from haraken #
Total comments: 4
Patch Set 13 : Comments from haraken #Patch Set 14 : Moved mojo interfaces dep to sensor/BUILD.gn (fixed gn check) #Patch Set 15 : build fix for win #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 94 (60 generated)
Description was changed from ========== [sensors] Generic Sensors Framework blink side This patch introduces blink-side infrastructure for Generic Sensors Framework which includes: implementation of generic sensor mojo interfaces, shared buffer polling strategies and life cycle management. BUG=606766 ========== to ========== [sensors] Generic Sensors Framework blink side This patch introduces blink-side infrastructure for Generic Sensors Framework which includes: implementation of generic sensor mojo interfaces, shared buffer polling strategies and life cycle management. BUG=606766 ==========
mikhail.pozdnyakov@intel.com changed reviewers: + timvolodine@chromium.org
Here is the Blink side of Generic Sensors Framework, this CL depends on https://crrev.com/2078433002/. PTAL.
Rebased and updated the Generic Sensor Framework Blink part (this CL) accordingly to https://codereview.chromium.org/2144623003/#msg15. PTAL.
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Tim, could you pls have a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
mikhail.pozdnyakov@intel.com changed reviewers: + haraken@chromium.org
Just an initial scan, didn't go through the details. Could you add a pointer to the spec in description and maybe also to your browser-side patch for reference? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:39: exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is not idle or errored"); 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 https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:46: exceptionState.throwDOMException(InvalidStateError, "The Sensor is no longer associated to a frame."); when can this happen? and should this ever happen? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:58: exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is either idle or errored"); same here: maybe something like "Ignoring stop() because SensorState is idle or errored." https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:62: if (!m_sensorProxy) { same question https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:130: if (EventTypeNames::change == eventType) hmm, don't we have "change" already, would this be confused with another event? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:182: double frequency = m_sensorOptions.hasFrequency() ? m_sensorOptions.frequency() : 1; 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. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:218: m_sensorProxy->addConfiguration(createSensorOptions(m_sensorOptions), std::move(callback)); can startListening() be invoked multiple times? if so, would that result in addConfiguration call each time that happens? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:246: if (m_state != Sensor::SensorState::ACTIVE) { shouldn't this be a DCHECK. i.e. invariant that pollForData is never called when SensorState is not ACTIVE? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:287: || !hasEventListeners(EventTypeNames::change) // Noone is listening to 'onchange' event. noone -> "no one" or "nobody" ;) actually the comments are pretty obvious https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.h (left): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.h:29: , public PlatformEventController { can we actually re-use the PlatformEventController somehow? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:52: if (m_sensors.contains(sensor)) if think you could just .remove() without contains https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:68: void SensorProviderProxy::handleSensorError() when does this happen? slightly confused with Sensor::onSensorError().. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:89: for (SensorProxy* sensor : m_sensors) but Sensor is also listening to page visibility.. should this go there instead? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:24: // instances within the frame). ->"no SensorProxies left" https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:50: friend class SensorProxy; pls put after "private:" also, why does it need to be a friend class? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:47: if (m_observers.contains(observer)) no need for contains https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:112: if (!buffer) { DCHECK? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:117: if (buffer->offset % SensorReadBuffer::kReadBufferSize != 0) { DCHECK? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.h:20: // JS sensor instances (within a single frame). sensor instances of the same type? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.h:29: // Has valid 'Sensor' binding, add(remove)Configuration() nit: {add,remove}Configuration https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:24: static SensorReadingEvent* create(const AtomicString& eventType, SensorReading* reading) should this also be a const SensorReading&? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:37: SensorReading* reading() { return m_reading.get(); } is this method needed? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:46: SensorReadingEvent(); needed?
Description was changed from ========== [sensors] Generic Sensors Framework blink side This patch introduces blink-side infrastructure for Generic Sensors Framework which includes: implementation of generic sensor mojo interfaces, shared buffer polling strategies and life cycle management. BUG=606766 ========== to ========== [sensors] Generic Sensors Framework blink side This patch introduces blink-side infrastructure for Generic Sensors Framework which includes: implementation of generic sensor mojo interfaces, shared buffer polling strategies and life cycle management. Specification: https://w3c.github.io/sensors BUG=606766 ==========
Description was changed from ========== [sensors] Generic Sensors Framework blink side This patch introduces blink-side infrastructure for Generic Sensors Framework which includes: implementation of generic sensor mojo interfaces, shared buffer polling strategies and life cycle management. Specification: https://w3c.github.io/sensors BUG=606766 ========== to ========== [sensors] Generic Sensors Framework blink side This patch introduces blink-side infrastructure for Generic Sensors Framework which includes: implementation of generic sensor mojo interfaces, shared buffer polling strategies and life cycle management. Specification: https://w3c.github.io/sensors Browser side CL: https://crrev.com/2144623003/ BUG=606766 ==========
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:80001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Thanks for your comments! https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:39: exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is not idle or errored"); On 2016/08/25 17:52:32, timvolodine wrote: > 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 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"? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:46: exceptionState.throwDOMException(InvalidStateError, "The Sensor is no longer associated to a frame."); On 2016/08/25 17:52:32, timvolodine wrote: > when can this happen? and should this ever happen? 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... https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:58: exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is either idle or errored"); On 2016/08/25 17:52:32, timvolodine wrote: > same here: maybe something like "Ignoring stop() because SensorState is idle or > errored." Done. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:130: if (EventTypeNames::change == eventType) On 2016/08/25 17:52:32, timvolodine wrote: > hmm, don't we have "change" already, would this be confused with another event? This method is called on JS call like "sensor.onchange = .." could you explain what "confused with another event" means? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:182: double frequency = m_sensorOptions.hasFrequency() ? m_sensorOptions.frequency() : 1; On 2016/08/25 17:52:32, timvolodine wrote: > why is this needed? i.e. if hasFrequency() is false should be handled > appropriately (not just cap at 1) ? 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 > > 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. Strategies are selected based on reporting mode rather than sensorOptions. I'd rather keep sensorOptions "parsing" here (in the Sensor class). https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:218: m_sensorProxy->addConfiguration(createSensorOptions(m_sensorOptions), std::move(callback)); On 2016/08/25 17:52:32, timvolodine wrote: > can startListening() be invoked multiple times? if so, would that result in > addConfiguration call each time that happens? 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. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:246: if (m_state != Sensor::SensorState::ACTIVE) { On 2016/08/25 17:52:32, timvolodine wrote: > shouldn't this be a DCHECK. i.e. invariant that pollForData is never called when > SensorState is not ACTIVE? It can happen if a scheduled task from 'OnChangeSensorPollingStrategy' arrives after state has been already changed. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:287: || !hasEventListeners(EventTypeNames::change) // Noone is listening to 'onchange' event. On 2016/08/25 17:52:32, timvolodine wrote: > noone -> "no one" or "nobody" ;) > > actually the comments are pretty obvious Done. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.h (left): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.h:29: , public PlatformEventController { On 2016/08/25 17:52:32, timvolodine wrote: > can we actually re-use the PlatformEventController somehow? PageVisibilityObserver is more convenient as we have our own classes wrapping timer (SensorPollingStrategies). https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:52: if (m_sensors.contains(sensor)) On 2016/08/25 17:52:32, timvolodine wrote: > if think you could just .remove() without contains Indeed, thanks for pointing out! https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:68: void SensorProviderProxy::handleSensorError() On 2016/08/25 17:52:32, timvolodine wrote: > when does this happen? on IPC connection problems or if the browser-side counter part is not accessible. > slightly confused with Sensor::onSensorError().. Sensor::onSensorError() will be eventually invoked from this call. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:89: for (SensorProxy* sensor : m_sensors) On 2016/08/25 17:52:32, timvolodine wrote: > but Sensor is also listening to page visibility.. should this go there instead? We could propagate this call from here to sensors having added smth like "SensorProxy::Observer::pageVisibilityChanged" but IMO that would be a bit weird because "SensorProxy::Observer" is supposed to propagate notification from a platform sensor, not from the page.. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:24: // instances within the frame). On 2016/08/25 17:52:32, timvolodine wrote: > ->"no SensorProxies left" Done. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:50: friend class SensorProxy; On 2016/08/25 17:52:32, timvolodine wrote: > pls put after "private:" > also, why does it need to be a friend class? Done. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:47: if (m_observers.contains(observer)) On 2016/08/25 17:52:33, timvolodine wrote: > no need for contains Done. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:112: if (!buffer) { On 2016/08/25 17:52:32, timvolodine wrote: > DCHECK? (!buffer) just means that platform could not create sensor for us, that might happen. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:117: if (buffer->offset % SensorReadBuffer::kReadBufferSize != 0) { On 2016/08/25 17:52:32, timvolodine wrote: > DCHECK? we've NOTREACHED(); below + some exit logic for release code https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.h:20: // JS sensor instances (within a single frame). On 2016/08/25 17:52:33, timvolodine wrote: > sensor instances of the same type? yes (comment is updated in the latest patch) https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.h:29: // Has valid 'Sensor' binding, add(remove)Configuration() On 2016/08/25 17:52:33, timvolodine wrote: > nit: {add,remove}Configuration Done. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:24: static SensorReadingEvent* create(const AtomicString& eventType, SensorReading* reading) On 2016/08/25 17:52:33, timvolodine wrote: > should this also be a const SensorReading&? it might be 'SensorReading&' to give a hint that reading must not be null but since reading is obtained from Sensor as pointer and further stored and returned from this class as pointer too, I would just add DCHECK(reading) to the constructors. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:37: SensorReading* reading() { return m_reading.get(); } On 2016/08/25 17:52:33, timvolodine wrote: > is this method needed? Both are used by V8 bindings.. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:46: SensorReadingEvent(); On 2016/08/25 17:52:33, timvolodine wrote: > needed? actually not :) thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
In the latest patch SensorReading update is fixed so that now all sensors "hold the same SensorReading during a single event loop turn" (https://w3c.github.io/sensors/#sensor-reading).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
mikhail.pozdnyakov@intel.com changed reviewers: + mkwst@chromium.org
gentle ping for review
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/29 19:35:43, Mikhail wrote: > gentle ping for review
rijubrata.bhaumik@intel.com changed reviewers: + rijubrata.bhaumik@intel.com
minor nits https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.cpp:20: Sensor::Sensor(ExecutionContext* executionContext, const SensorOptions& sensorOptions, device::mojom::blink::SensorType type) since you are using "using namespace device::mojom::blink;", maybe it can be shortened ? if this is the only place, maybe remove the "using namespace .." https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/Sensor.h (right): https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.h:62: DECLARE_VIRTUAL_TRACE(); nit: for consistency this can be last statement in public section https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:12: using namespace device::mojom::blink; maybe remove this for 1 usage in getOrCreateSensor() ? https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorReading.h (right): https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.h:11: #include "modules/sensor/SensorProxy.h" forward declaration below is enough
https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.cpp:20: Sensor::Sensor(ExecutionContext* executionContext, const SensorOptions& sensorOptions, device::mojom::blink::SensorType type) On 2016/08/31 10:35:49, riju_ wrote: > since you are using "using namespace device::mojom::blink;", maybe it can be > shortened ? if this is the only place, maybe remove the "using namespace .." Right, that can be shortened. Thanks for noticing https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/Sensor.h (right): https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.h:62: DECLARE_VIRTUAL_TRACE(); On 2016/08/31 10:35:49, riju_ wrote: > nit: for consistency this can be last statement in public section Done. https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:12: using namespace device::mojom::blink; On 2016/08/31 10:35:49, riju_ wrote: > maybe remove this for 1 usage in getOrCreateSensor() ? Done. https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorReading.h (right): https://codereview.chromium.org/2121313002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.h:11: #include "modules/sensor/SensorProxy.h" On 2016/08/31 10:35:49, riju_ wrote: > forward declaration below is enough We're using nested type SensorProxy::Reading, so unfortunately have to keep the include. But forward declaration is not needed and should be omitted.
On 2016/08/31 10:01:59, Mikhail wrote: > On 2016/08/29 19:35:43, Mikhail wrote: > > gentle ping for review Yes, sorry for the delay with the public holidays in the UK. I'll try to have a look tomorrow.
Overall LGTM, but let Tim have a look.
Thanks, some more replies/comments below. Any plans regarding tests for this? How do you know this code works as intended? I assume you did some local testing? It would be good to have some layout tests, at least some basic ones to test basic functionality to start with. It would also be easier to understand how the code is actually supposed to work ;). https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:39: exceptionState.throwDOMException(InvalidStateError, "Invalid State: SensorState is not idle or errored"); On 2016/08/26 16:42:41, Mikhail wrote: > On 2016/08/25 17:52:32, timvolodine wrote: > > 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 > > 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"? yes, that sounds more informative compared to just "Invalid state:.." https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:130: if (EventTypeNames::change == eventType) On 2016/08/26 16:42:41, Mikhail wrote: > On 2016/08/25 17:52:32, timvolodine wrote: > > hmm, don't we have "change" already, would this be confused with another > event? > > This method is called on JS call like "sensor.onchange = .." > could you explain what "confused with another event" means? 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 https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:182: double frequency = m_sensorOptions.hasFrequency() ? m_sensorOptions.frequency() : 1; On 2016/08/26 16:42:41, Mikhail wrote: > On 2016/08/25 17:52:32, timvolodine wrote: > > why is this needed? i.e. if hasFrequency() is false should be handled > > appropriately (not just cap at 1) ? > > 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 > > > > 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. > Strategies are selected based on reporting mode rather than sensorOptions. > I'd rather keep sensorOptions "parsing" here (in the Sensor class). > > hmm don't see the TODO comment in the latest patch.. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.h (left): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.h:29: , public PlatformEventController { On 2016/08/26 16:42:41, Mikhail wrote: > On 2016/08/25 17:52:32, timvolodine wrote: > > can we actually re-use the PlatformEventController somehow? > > PageVisibilityObserver is more convenient as we have our own classes wrapping > timer (SensorPollingStrategies). in the PlatformEventController we have a special mechanism to ensure the sensor values are immediately propagated if they are already available in blink. Is there any similar approach in this code? i.e. if a new Sensor is created with same/similar config can the existing data be reused immediately? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:89: for (SensorProxy* sensor : m_sensors) On 2016/08/26 16:42:42, Mikhail wrote: > On 2016/08/25 17:52:32, timvolodine wrote: > > but Sensor is also listening to page visibility.. should this go there > instead? > We could propagate this call from here to sensors having added smth like > "SensorProxy::Observer::pageVisibilityChanged" > but IMO that would be a bit weird because "SensorProxy::Observer" is supposed > to propagate notification from a platform sensor, not from the page.. what I meant is to just suspend() in the Sensor class itself when page visibility changes (so there would be no need for 'for'-loop). would that work? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:112: if (!buffer) { On 2016/08/26 16:42:42, Mikhail wrote: > On 2016/08/25 17:52:32, timvolodine wrote: > > DCHECK? > > (!buffer) just means that platform could not create sensor for us, that might > happen. Would it be better to go through handleSensorError (or something like that) instead of onSensorCreated in that case? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:117: if (buffer->offset % SensorReadBuffer::kReadBufferSize != 0) { On 2016/08/26 16:42:42, Mikhail wrote: > On 2016/08/25 17:52:32, timvolodine wrote: > > DCHECK? > we've NOTREACHED(); below + some exit logic for release code still looks DCHECK to me.. in other words if the condition is false looks like programmatic error (i.e. this could be an invariant when creating buffer and offset, either true or no buffer at all i.e. buffer==null), this would simplify code. https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:41: m_timer.startRepeating(1 / m_frequency, BLINK_FROM_HERE); what happens if the sensor is notification only, i.e. does not have a frequency like e.g. AmbientLight? https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:41: m_timer.startRepeating(1 / m_frequency, BLINK_FROM_HERE); also maybe DCHECK(frequency>0) ? https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:82: m_polling = true; my understanding was that some sensors do not require polling at all? what is the difference with continuous polling here? https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:56: if (sensor->type() == type) maybe this could be hashed by type for efficiency, but can be considered as optimization for later (e.g. a TODO) https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:147: m_sensor->GetDefaultConfiguration(callback); why is this needed? i.e. wouldn't a sensor always have a config, and if not could it just be returned in onSensorCreated(), like e.g. buffer? https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorReading.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.cpp:38: if (!m_sensorProxy) nit: you could have a singe if with multiple "||" or.. probably better: have an if with multiple conditions without the "!" separated by "&&" and return performance->... and otherwise return 0.0 by default.
Thanks for your comments! https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:130: if (EventTypeNames::change == eventType) On 2016/09/01 19:02:05, timvolodine wrote: > On 2016/08/26 16:42:41, Mikhail wrote: > > On 2016/08/25 17:52:32, timvolodine wrote: > > > hmm, don't we have "change" already, would this be confused with another > > event? > > > > This method is called on JS call like "sensor.onchange = .." > > could you explain what "confused with another event" means? > > 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 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. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:182: double frequency = m_sensorOptions.hasFrequency() ? m_sensorOptions.frequency() : 1; On 2016/09/01 19:02:05, timvolodine wrote: > hmm don't see the TODO comment in the latest patch.. 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. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.h (left): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.h:29: , public PlatformEventController { On 2016/09/01 19:02:05, timvolodine wrote: > On 2016/08/26 16:42:41, Mikhail wrote: > > On 2016/08/25 17:52:32, timvolodine wrote: > > > can we actually re-use the PlatformEventController somehow? > > > > PageVisibilityObserver is more convenient as we have our own classes wrapping > > timer (SensorPollingStrategies). > > in the PlatformEventController we have a special mechanism to ensure the sensor > values are immediately propagated if they are already available in blink. Is > there any similar approach in this code? i.e. if a new Sensor is created with > same/similar config can the existing data be reused immediately? Yes. According to the spec (https://w3c.github.io/sensors/#sensor-reading) all the sensors of same type are sharing reading data (which is owned by SensorProxy). So all the sensors access the same up-to-date data. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:89: for (SensorProxy* sensor : m_sensors) On 2016/09/01 19:02:05, timvolodine wrote: > what I meant is to just suspend() in the Sensor class itself when page > visibility changes (so there would be no need for 'for'-loop). would that work? That would work, but since there can be multiple Sensor instances within a single frame that could cause unneeded IPC calls. An alternative way IMO could be making 'SensorProxy' a PageVisibilityObserver itself (instead on 'SensorProviderProxy') so that there would be no loops too. Do you think that would be better? https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:112: if (!buffer) { On 2016/09/01 19:02:05, timvolodine wrote: > On 2016/08/26 16:42:42, Mikhail wrote: > > On 2016/08/25 17:52:32, timvolodine wrote: > > > DCHECK? > > > > (!buffer) just means that platform could not create sensor for us, that might > > happen. > > Would it be better to go through handleSensorError (or something like that) > instead of onSensorCreated in that case? handleSensorError() is a connection_error_handler for the sensor proxy however at this point the proxy itself is not initialized so it cannot be invoked from the mojo framework code. In the past had had an extra 'result' parameter to show the call result (rather than null checking of returned parameters), here it was decided to move to the current semantics https://codereview.chromium.org/2078433002/#msg11. https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:117: if (buffer->offset % SensorReadBuffer::kReadBufferSize != 0) { On 2016/09/01 19:02:06, timvolodine wrote: > On 2016/08/26 16:42:42, Mikhail wrote: > > On 2016/08/25 17:52:32, timvolodine wrote: > > > DCHECK? > > we've NOTREACHED(); below + some exit logic for release code > > still looks DCHECK to me.. in other words if the condition is false looks like > programmatic error (i.e. this could be an invariant when creating buffer and > offset, either true or no buffer at all i.e. buffer==null), this would simplify > code. Agreed. Done. https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:41: m_timer.startRepeating(1 / m_frequency, BLINK_FROM_HERE); On 2016/09/01 19:02:06, timvolodine wrote: > also maybe DCHECK(frequency>0) ? Done. https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:41: m_timer.startRepeating(1 / m_frequency, BLINK_FROM_HERE); On 2016/09/01 19:02:06, timvolodine wrote: > what happens if the sensor is notification only, i.e. does not have a frequency > like e.g. AmbientLight? Different polling strategy class OnChangeSensorPollingStrategy is to be taken. 'frequency' is the one given by user and AmbientLight sensor consideres it to poll the data on platform side (notification is send however only when the data are changed). https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:82: m_polling = true; On 2016/09/01 19:02:06, timvolodine wrote: > my understanding was that some sensors do not require polling at all? what is > the difference with continuous polling here? Polling here means just refreshing a sensor reading value from the shared buffer. In case of "onchange" sensor type the reading is updated on the signal from the platform side, however its own frequency is considered as well so that reading is not updated more frequently than it was requested by the given 'frequency' option. https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:56: if (sensor->type() == type) On 2016/09/01 19:02:06, timvolodine wrote: > maybe this could be hashed by type for efficiency, but can be considered as > optimization for later (e.g. a TODO) Added TODO comment. https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:147: m_sensor->GetDefaultConfiguration(callback); On 2016/09/01 19:02:06, timvolodine wrote: > why is this needed? i.e. wouldn't a sensor always have a config, and if not > could it just be returned in onSensorCreated(), like e.g. buffer? It is better to know the default config beforehand: if the user gave some parameters but missed others than the sensor implementation could assign the missing parameters to the default ones. https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorReading.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.cpp:38: if (!m_sensorProxy) On 2016/09/01 19:02:06, timvolodine wrote: > nit: you could have a singe if with multiple "||" > or.. probably better: have an if with multiple conditions without the "!" > separated by "&&" and return performance->... and otherwise return 0.0 by > default. Done.
On 2016/09/01 19:02:06, timvolodine wrote: > Thanks, some more replies/comments below. > > Any plans regarding tests for this? How do you know this code works as intended? > I assume you did some local testing? It would be good to have some layout tests, > at least some basic ones to test basic functionality to start with. It would > also be easier to understand how the code is actually supposed to work ;). > Absolutely! Internally we've layout tests but those can be landed only together with concrete sensor implementations (otherwise you cannot construct a sensor instance), so it will be next step after this CL lands (https://codereview.chromium.org/2051083002 contains a bit outdated version of the whole sensors code including tests https://codereview.chromium.org/2051083002/diff/1/third_party/WebKit/LayoutTe...)
On 2016/09/02 10:47:56, Mikhail wrote: > On 2016/09/01 19:02:06, timvolodine wrote: > > Thanks, some more replies/comments below. > > > > Any plans regarding tests for this? How do you know this code works as > intended? > > I assume you did some local testing? It would be good to have some layout > tests, > > at least some basic ones to test basic functionality to start with. It would > > also be easier to understand how the code is actually supposed to work ;). > > > Absolutely! Internally we've layout tests but those can be landed only together > with concrete sensor implementations (otherwise you cannot construct a sensor > instance), so it will be next step after this CL lands > (https://codereview.chromium.org/2051083002 contains a bit outdated version of > the whole sensors code including tests > https://codereview.chromium.org/2051083002/diff/1/third_party/WebKit/LayoutTe...) what do you mean by 'concrete sensor implementations'? layout tests don't need any platform specific sensor implementations, you usually just need some testrunner or other blink testing support..
On 2016/09/02 14:19:32, timvolodine wrote: > On 2016/09/02 10:47:56, Mikhail wrote: > > On 2016/09/01 19:02:06, timvolodine wrote: > > > Thanks, some more replies/comments below. > > > > > > Any plans regarding tests for this? How do you know this code works as > > intended? > > > I assume you did some local testing? It would be good to have some layout > > tests, > > > at least some basic ones to test basic functionality to start with. It would > > > also be easier to understand how the code is actually supposed to work ;). > > > > > Absolutely! Internally we've layout tests but those can be landed only > together > > with concrete sensor implementations (otherwise you cannot construct a sensor > > instance), so it will be next step after this CL lands > > (https://codereview.chromium.org/2051083002 contains a bit outdated version of > > the whole sensors code including tests > > > https://codereview.chromium.org/2051083002/diff/1/third_party/WebKit/LayoutTe...) > > what do you mean by 'concrete sensor implementations'? layout tests don't need > any platform specific sensor implementations, you usually just need some > testrunner or other blink testing support.. by 'concrete sensor implementations' I mean bindings for the concrete sensors APIs which are based on Generic Sensor API (e.g. https://w3c.github.io/ambient-light/, https://w3c.github.io/proximity/, https://w3c.github.io/gyroscope or others) otherwise you cannot create a sensor instance - generic sensor API does not expose constructors. In the following CL we were planning to upload AmbientLight bindings and the layout tests. Would you like it to be merged to the current CL instead?
On 2016/09/02 14:30:08, Mikhail wrote: > On 2016/09/02 14:19:32, timvolodine wrote: > > On 2016/09/02 10:47:56, Mikhail wrote: > > > On 2016/09/01 19:02:06, timvolodine wrote: > > > > Thanks, some more replies/comments below. > > > > > > > > Any plans regarding tests for this? How do you know this code works as > > > intended? > > > > I assume you did some local testing? It would be good to have some layout > > > tests, > > > > at least some basic ones to test basic functionality to start with. It > would > > > > also be easier to understand how the code is actually supposed to work ;). > > > > > > > Absolutely! Internally we've layout tests but those can be landed only > > together > > > with concrete sensor implementations (otherwise you cannot construct a > sensor > > > instance), so it will be next step after this CL lands > > > (https://codereview.chromium.org/2051083002 contains a bit outdated version > of > > > the whole sensors code including tests > > > > > > https://codereview.chromium.org/2051083002/diff/1/third_party/WebKit/LayoutTe...) > > > > what do you mean by 'concrete sensor implementations'? layout tests don't need > > any platform specific sensor implementations, you usually just need some > > testrunner or other blink testing support.. > > by 'concrete sensor implementations' I mean bindings for the concrete sensors > APIs which are based on Generic Sensor API (e.g. > https://w3c.github.io/ambient-light/, https://w3c.github.io/proximity/, > https://w3c.github.io/gyroscope or others) otherwise you cannot create a sensor > instance - generic sensor API does not expose constructors. > In the following CL we were planning to upload AmbientLight bindings and the > layout tests. > Would you like it to be merged to the current CL instead? Oh ok got it, Sensor is a base class, no need to merge. Do you plan to put AmbientLight etc in modules/sensor/? Just considering if it makes sense to have a unit tests for the base sensor functionality (like e.g. DOMFileSystemBaseTest.cpp).. Going through your other replies..
On 2016/09/02 14:52:02, timvolodine wrote: > > Oh ok got it, Sensor is a base class, no need to merge. Do you plan to put > AmbientLight etc in modules/sensor/? Just considering if it makes sense to have > a unit tests for the base sensor functionality (like e.g. > DOMFileSystemBaseTest.cpp).. Going through your other replies.. I would keep AmbientLight and others in the same modules/sensor/ directory as implementation of a concrete sensor will bring just a small amount of code on top of generic sensor framework -- no need to have a dedicated folder.
https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:89: for (SensorProxy* sensor : m_sensors) On 2016/09/02 08:23:43, Mikhail wrote: > On 2016/09/01 19:02:05, timvolodine wrote: > > what I meant is to just suspend() in the Sensor class itself when page > > visibility changes (so there would be no need for 'for'-loop). would that > work? > That would work, but since there can be multiple Sensor instances within a > single frame that could cause unneeded IPC calls. > An alternative way IMO could be making 'SensorProxy' a PageVisibilityObserver > itself (instead on 'SensorProviderProxy') so that there would be no loops too. > Do you think that would be better? > not sure why this would result in more IPCs? would putting "m_sensorProxy->suspend()" in Sensor::pageVisibilityChanged() not do the same thing? https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:82: m_polling = true; On 2016/09/02 08:23:43, Mikhail wrote: > On 2016/09/01 19:02:06, timvolodine wrote: > > my understanding was that some sensors do not require polling at all? what is > > the difference with continuous polling here? > Polling here means just refreshing a sensor reading value from the shared > buffer. > In case of "onchange" sensor type the reading is updated on the signal from the > platform side, however its own frequency is considered as well so that reading > is not updated more frequently than it was requested by the given 'frequency' > option. In that case, could you pls add some comments with explanation about the 'capping' of notification updates depending on the frequency. I actually wonder if this has any use-cases at this point. You could consider making this an 'empty' class and add this more 'advanced' functionality later (with some tests). Up to you though. https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:147: m_sensor->GetDefaultConfiguration(callback); On 2016/09/02 08:23:43, Mikhail wrote: > On 2016/09/01 19:02:06, timvolodine wrote: > > why is this needed? i.e. wouldn't a sensor always have a config, and if not > > could it just be returned in onSensorCreated(), like e.g. buffer? > > It is better to know the default config beforehand: if the user gave some > parameters but missed others than the sensor implementation could assign the > missing parameters to the default ones. In that case I think my question still stands: why not return this in OnSensorCreated()? that way avoiding extra IPC and extra asynchronous callback. How is the default config used in this patch? What happens for example if AddConfiguration() is invoked before onDefaultConfiguration() callback (or some other race condition)? If it's not used anywhere I think it would be better to add it in a separate patch instead. https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:157: m_defaultConfiguration = std::move(config); Is m_defaultConfiguration used anywhere? looks like extra functionality and could be added in a separate patch? https://codereview.chromium.org/2121313002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:48: if (m_observers.contains(observer)) no need for contains (I think I've already commented on this ;)) https://codereview.chromium.org/2121313002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorReading.cpp (right): https://codereview.chromium.org/2121313002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.cpp:38: if (!m_sensorProxy || !m_context) I actually meant this (shorter and more readable): if (m_sensorProxy && m_context && m_context->executingWindow() && DOMWindowPerformance::performance(*window)) { return performance->monotonicTimeToDOMHighResTimeStamp(m_sensorProxy->reading().timestamp); } return 0.0;
https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:89: for (SensorProxy* sensor : m_sensors) On 2016/09/02 19:41:28, timvolodine wrote: > not sure why this would result in more IPCs? > would putting "m_sensorProxy->suspend()" in Sensor::pageVisibilityChanged() not > do the same thing? All the JS Sensor instances of the same type within a frame share the same SensorProxy instance, so "m_sensorProxy->suspend()" will be called N times (assuming N = amount JS Sensor instances within a frame), otherwise (if either SensorProviderProxy or SensorProxy inherit PageVisibilityObserver themselves) it will be a single call. https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:82: m_polling = true; On 2016/09/02 19:41:28, timvolodine wrote: > On 2016/09/02 08:23:43, Mikhail wrote: > > On 2016/09/01 19:02:06, timvolodine wrote: > > > my understanding was that some sensors do not require polling at all? what > is > > > the difference with continuous polling here? > > Polling here means just refreshing a sensor reading value from the shared > > buffer. > > In case of "onchange" sensor type the reading is updated on the signal from > the > > platform side, however its own frequency is considered as well so that reading > > is not updated more frequently than it was requested by the given 'frequency' > > option. > > In that case, could you pls add some comments with explanation about the > 'capping' of notification updates depending on the frequency. > > I actually wonder if this has any use-cases at this point. You could consider > making this an 'empty' class and add this more 'advanced' functionality later > (with some tests). Up to you though. Done. https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:147: m_sensor->GetDefaultConfiguration(callback); On 2016/09/02 19:41:28, timvolodine wrote: > In that case I think my question still stands: why not return this in > OnSensorCreated()? that way avoiding extra IPC and extra asynchronous callback. Oh sorry, I probably misunderstood your comment. It would also work if default configuration is passed in OnSensorCreated(). > > How is the default config used in this patch? What happens for example if > AddConfiguration() is invoked before onDefaultConfiguration() callback (or some > other race condition)? Sensor proxy is initialized only after default configuration is obtained. > > If it's not used anywhere I think it would be better to add it in a separate > patch instead. I agree that it's better to make a separate patch (as it might require mojo interfaces changes). https://codereview.chromium.org/2121313002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:48: if (m_observers.contains(observer)) On 2016/09/02 19:41:28, timvolodine wrote: > no need for contains (I think I've already commented on this ;)) I missed this place, thanks for pointing out! https://codereview.chromium.org/2121313002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorReading.cpp (right): https://codereview.chromium.org/2121313002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.cpp:38: if (!m_sensorProxy || !m_context) On 2016/09/02 19:41:29, timvolodine wrote: > I actually meant this (shorter and more readable): > > if (m_sensorProxy && m_context && m_context->executingWindow() && > DOMWindowPerformance::performance(*window)) { > return > performance->monotonicTimeToDOMHighResTimeStamp(m_sensorProxy->reading().timestamp); > } > > return 0.0; Unfortunately two new variables are created here ('window' and 'performance'), otherwise the call looks too ugly: DOMWindowPerformance::performance(*m_context->executingWindow())->monotonicTimeToDOMHighResTimeStamp(m_sensorProxy->readin g().timestamp);
OK thanks, two more comments below. Looks reasonable in general so lgtm. https://codereview.chromium.org/2121313002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorReading.cpp (right): https://codereview.chromium.org/2121313002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.cpp:38: if (!m_sensorProxy || !m_context) On 2016/09/05 10:26:27, Mikhail wrote: > On 2016/09/02 19:41:29, timvolodine wrote: > > I actually meant this (shorter and more readable): > > > > if (m_sensorProxy && m_context && m_context->executingWindow() && > > DOMWindowPerformance::performance(*window)) { > > return > > > performance->monotonicTimeToDOMHighResTimeStamp(m_sensorProxy->reading().timestamp); > > } > > > > return 0.0; > > Unfortunately two new variables are created here ('window' and 'performance'), > otherwise the call looks too ugly: > > DOMWindowPerformance::performance(*m_context->executingWindow())->monotonicTimeToDOMHighResTimeStamp(m_sensorProxy->readin > g().timestamp); > > yeah sorry my bad (technically I think you could do the assignment in the if condition, but that's not particularly clean either) https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.cpp:151: m_configuration = createSensorConfig(m_sensorOptions, &defaultConfig); would it make sense to drop defaultConfig here and have m_configuration = createSensorConfig(m_sensorOptions); ? I understand createSensorConfig is pure virtual anyway so can decide there later what to return in the 'default' case. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.h (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.h:20: // guarantied not to be called more often than expected. ->guaranteed
timvolodine@chromium.org changed reviewers: + oilpan-reviews@chromium.org
Would be good to have this patch signed off by an oilpan expert (haraken@?) +cc: oilpan-reviews@
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
not LGTM This CL is doing too many things in one go and has a bunch of architectural issues. Would it be possible to split the CL into pieces? Also, where are you going to add tests? https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.cpp:112: if (!getExecutionContext() || getExecutionContext()->activeDOMObjectsAreStopped()) I'd change this check to if(!m_sensorReading) or something. What matters is whether stopListening has been already called or not, rather than the active DOM objects have been stopped. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.cpp:123: if (!document->frame()) document may be nullptr. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.cpp:219: auto callback = WTF::bind(&Sensor::onStartRequestCompleted, wrapWeakPersistent(this)); WeakPersistent means that the callback may not run if the Sensor object gets collected before the callback runs. Is this okay? https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.cpp:240: auto callback = WTF::bind(&Sensor::onStopRequestCompleted, wrapWeakPersistent(this)); Ditto. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/Sensor.h (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.h:108: WeakMember<SensorProxy> m_sensorProxy; Is there any reason you want to make this a weak member? https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.h (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.h:21: class SensorPollingStrategy : public GarbageCollectedFinalized<SensorPollingStrategy> { There's no benefit in putting this class on GarbageCollected. You can put the class on USING_FAST_MALLOC and use std::unique_ptr to hold this object. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:14: const char SensorProviderProxy::s_supplementKey[] = "SensorProvider"; Use supplementName() just like what other supplement objects are doing. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:68: SensorsSet copy(m_sensors); Use copyToVector. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:75: m_frame->removeSupplement(s_supplementKey); We normally don't remove supplement objects from the map. You can leave it as is. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:83: Page* page = m_frame->document()->page(); Just use PageVisibilityObserver::page(). https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:28: , public PageVisibilityObserver { Do you really need to make both Sensor and SensorProviderProxy PageVisibilityObservers? https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:53: using SensorsSet = HeapHashSet<Member<SensorProxy>>; I think this should be HeapHashSet<WeakMember<>>. Then you can stop calling removeObserver() in Sensor's pre-finalizer. I hope we can entirely remove the pre-finalizer from Sensor. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:57: WeakMember<LocalFrame> m_frame; Why weak? https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorReading.cpp (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.cpp:41: LocalDOMWindow* window = m_context->executingWindow(); This is retrieving a wrong window. Alternately you should pass in ScriptState to timeStamp and use scriptState->domWindow(). https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.cpp:46: if (!performance) This shouldn't be nullptr. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorReading.h (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.h:36: WeakMember<ExecutionContext> m_context; Is there any reason they need to be weak members?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tim and haraken@, thanks for your comments https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.cpp:112: if (!getExecutionContext() || getExecutionContext()->activeDOMObjectsAreStopped()) On 2016/09/06 05:04:28, haraken wrote: > > I'd change this check to if(!m_sensorReading) or something. > > What matters is whether stopListening has been already called or not, rather > than the active DOM objects have been stopped. I'll change accordingly, thanks https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.cpp:123: if (!document->frame()) On 2016/09/06 05:04:28, haraken wrote: > > document may be nullptr. we've DCHECK(executionContext->isDocument()); in the ctor https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.cpp:219: auto callback = WTF::bind(&Sensor::onStartRequestCompleted, wrapWeakPersistent(this)); On 2016/09/06 05:04:28, haraken wrote: > > WeakPersistent means that the callback may not run if the Sensor object gets > collected before the callback runs. Is this okay? I think so, if the object is collected there is no reason to provide notification for it. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/Sensor.h (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/Sensor.h:108: WeakMember<SensorProxy> m_sensorProxy; On 2016/09/06 05:04:28, haraken wrote: > > Is there any reason you want to make this a weak member? m_sensorProxy might get deleted if a problem turns up e.g. with the mojo connection or on platform side. Next 'start()' call will try to re-create it. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.h (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.h:21: class SensorPollingStrategy : public GarbageCollectedFinalized<SensorPollingStrategy> { On 2016/09/06 05:04:28, haraken wrote: > > There's no benefit in putting this class on GarbageCollected. You can put the > class on USING_FAST_MALLOC and use std::unique_ptr to hold this object. right, I'll change that https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:83: Page* page = m_frame->document()->page(); On 2016/09/06 05:04:29, haraken wrote: > > Just use PageVisibilityObserver::page(). indeed, thanks https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:28: , public PageVisibilityObserver { On 2016/09/06 05:04:29, haraken wrote: > > Do you really need to make both Sensor and SensorProviderProxy > PageVisibilityObservers? when page goes invisible sensors stop polling from buffer and SensorProviderProxy notifies platform side that it might stop notification to the frame and can reconsider the applied platform sensor configurations. Alternatively SensorProviderProxy could notify Sensors itself but it does not look good IMO since SensorProviderProxy should notify about platform sensor updates, not about page visibility. https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:57: WeakMember<LocalFrame> m_frame; On 2016/09/06 05:04:29, haraken wrote: > > Why weak? Should it be just a member? I thought frame is owning SensorProviderProxy (as a supplement) not the opposite https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorReading.h (right): https://codereview.chromium.org/2121313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorReading.h:36: WeakMember<ExecutionContext> m_context; On 2016/09/06 05:04:29, haraken wrote: > > Is there any reason they need to be weak members? 'm_sensorProxy' might have a different lifetime (same as inside Sensor) Is it fine to keep a strong reference to 'm_context' from here (member)?
On 2016/09/06 05:04:29, haraken wrote: > This CL is doing too many things in one go and has a bunch of architectural > issues. Would it be possible to split the CL into pieces? Sure, I'll split it into several patches. > > Also, where are you going to add tests? > Layout tests will be added in following patches together with concrete sensor implementations (e.g. https://w3c.github.io/ambient-light/, https://w3c.github.io/proximity/, https://w3c.github.io/gyroscope or others) - generic sensor API does not expose constructors so it's impossible to create a new sensor instance. A test example (WIP version) can be found at https://codereview.chromium.org/2051083002/diff/1/third_party/WebKit/LayoutTe...
On 2016/09/06 07:19:04, Mikhail wrote: > On 2016/09/06 05:04:29, haraken wrote: > > This CL is doing too many things in one go and has a bunch of architectural > > issues. Would it be possible to split the CL into pieces? > Sure, I'll split it into several patches. Thanks! > > > > Also, where are you going to add tests? > > > Layout tests will be added in following patches together with concrete sensor > implementations (e.g. https://w3c.github.io/ambient-light/, > https://w3c.github.io/proximity/, https://w3c.github.io/gyroscope or others) - > generic sensor API does not expose constructors so it's impossible to create a > new sensor instance. > A test example (WIP version) can be found at > https://codereview.chromium.org/2051083002/diff/1/third_party/WebKit/LayoutTe... OK, makes sense to add the tests when landing concrete sensor implementations.
Description was changed from ========== [sensors] Generic Sensors Framework blink side This patch introduces blink-side infrastructure for Generic Sensors Framework which includes: implementation of generic sensor mojo interfaces, shared buffer polling strategies and life cycle management. Specification: https://w3c.github.io/sensors Browser side CL: https://crrev.com/2144623003/ BUG=606766 ========== to ========== [sensors] Blink-side implementation of mojo interfaces This patch introduces blink-side implementation of generic sensor mojo interfaces. Specification: https://w3c.github.io/sensors Browser side CL: https://crrev.com/2144623003/ BUG=606766 ==========
I have split the code into 3 parts as following: 1) Mojo wrapper classes - this CL 2) Implementation of Generic Sensor API - https://codereview.chromium.org/2317743002/ 3) Implementation of Sensor polling strategies + {suspend, resume} API calls - to be uploaded soon I simplified platform error handling so that it does not lead to re-creation of mojo wrapper instances and thus got rid of most 'WeakMembers'. Other comments are applied as well. Please take a look.
LGTM https://codereview.chromium.org/2121313002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h (right): https://codereview.chromium.org/2121313002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:44: using SensorsSet = HeapHashSet<Member<SensorProxy>>; I'm guessing that this should be WeakMembers though. If you're expecting that SensorProviderProxy outlives SensorProxy's, they should be WeakMembers. (Also then you don't need to/shouldn't call removeSensorProxy when the SensorProxy dies.) https://codereview.chromium.org/2121313002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:118: m_sensor.set_connection_error_handler(convertToBaseCallback(WTF::bind(&SensorProxy::handleSensorError, wrapWeakPersistent(this)))); I'd call this immediately after setting m_sensor; i.e., at the end of SensorProxy::initialize.
https://codereview.chromium.org/2121313002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h (right): https://codereview.chromium.org/2121313002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:44: using SensorsSet = HeapHashSet<Member<SensorProxy>>; On 2016/09/07 02:21:56, haraken wrote: > > I'm guessing that this should be WeakMembers though. > > If you're expecting that SensorProviderProxy outlives SensorProxy's, they should > be WeakMembers. (Also then you don't need to/shouldn't call removeSensorProxy > when the SensorProxy dies.) Indeed, thanks for pointing that out! https://codereview.chromium.org/2121313002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2121313002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:118: m_sensor.set_connection_error_handler(convertToBaseCallback(WTF::bind(&SensorProxy::handleSensorError, wrapWeakPersistent(this)))); On 2016/09/07 02:21:56, haraken wrote: > > I'd call this immediately after setting m_sensor; i.e., at the end of > SensorProxy::initialize. Done.
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mikhail.pozdnyakov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from rijubrata.bhaumik@intel.com, timvolodine@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2121313002/#ps320001 (title: "build fix for win")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [sensors] Blink-side implementation of mojo interfaces This patch introduces blink-side implementation of generic sensor mojo interfaces. Specification: https://w3c.github.io/sensors Browser side CL: https://crrev.com/2144623003/ BUG=606766 ========== to ========== [sensors] Blink-side implementation of mojo interfaces This patch introduces blink-side implementation of generic sensor mojo interfaces. Specification: https://w3c.github.io/sensors Browser side CL: https://crrev.com/2144623003/ BUG=606766 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== [sensors] Blink-side implementation of mojo interfaces This patch introduces blink-side implementation of generic sensor mojo interfaces. Specification: https://w3c.github.io/sensors Browser side CL: https://crrev.com/2144623003/ BUG=606766 ========== to ========== [sensors] Blink-side implementation of mojo interfaces This patch introduces blink-side implementation of generic sensor mojo interfaces. Specification: https://w3c.github.io/sensors Browser side CL: https://crrev.com/2144623003/ BUG=606766 Committed: https://crrev.com/0c00597deb691b3d180f09f25ee67a494e927311 Cr-Commit-Position: refs/heads/master@{#416920} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/0c00597deb691b3d180f09f25ee67a494e927311 Cr-Commit-Position: refs/heads/master@{#416920} |
