Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2927263002:
Add |notify_client_on_reading_change| flag to sensor configuration (Closed)
|
Created:
3 years, 6 months ago by juncai Modified:
3 years, 6 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, shalamov, viettrungluu+watch_chromium.org, timvolodine, abarth-chromium, Aaron Boodman, yzshen+watch_chromium.org, wanming.lin, darin (slow to review), Mikhail Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd |notify_client_on_reading_change| flag to sensor configuration
This is a CL following the discussion from:
https://codereview.chromium.org/2896583005/diff/180001/content/renderer/device_sensors/device_motion_event_pump.cc
When the sensor client sets its own timer to read the shared memory,
and the sensor reporting mode is ON_CHANGE, the sensor client gets
unnecessary high frequency SensorClient::SensorReadingChanged() mojo
IPC calls. This CL adds a |notify_client_on_reading_change| flag to
sensor configuration to fix it.
BUG=721427
Review-Url: https://codereview.chromium.org/2927263002
Cr-Commit-Position: refs/heads/master@{#479074}
Committed: https://chromium.googlesource.com/chromium/src/+/cfe1d9c8f0f922771e19eb5d2566215b6e47371c
Patch Set 1 : add |notify_client_on_reading_change| flag to sensor configuration #Patch Set 2 : fix browser test #
Total comments: 6
Patch Set 3 : address comments #
Total comments: 6
Patch Set 4 : address more comments #
Total comments: 2
Patch Set 5 : address nit #
Total comments: 3
Messages
Total messages: 51 (31 generated)
The CQ bit was checked by juncai@chromium.org 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...
Description was changed from ========== Add notify_client_on_reading_change flag to sensor configuration BUG=721427 ========== to ========== Add notify_client_on_reading_change flag to sensor configuration This is a CL following the discussion from: https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... When the sensor client sets its own timer to read the shared memory, but the sensor reporting mode is ON_CHANGE, the sensor client gets unnecessary high frequency SensorClient::SensorReadingChanged() mojo IPC calls. This CL adds a notify_client_on_reading_change flag to sensor configuration to fix it. BUG=721427 ==========
Description was changed from ========== Add notify_client_on_reading_change flag to sensor configuration This is a CL following the discussion from: https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... When the sensor client sets its own timer to read the shared memory, but the sensor reporting mode is ON_CHANGE, the sensor client gets unnecessary high frequency SensorClient::SensorReadingChanged() mojo IPC calls. This CL adds a notify_client_on_reading_change flag to sensor configuration to fix it. BUG=721427 ========== to ========== Add |notify_client_on_reading_change| flag to sensor configuration This is a CL following the discussion from: https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... When the sensor client sets its own timer to read the shared memory, but the sensor reporting mode is ON_CHANGE, the sensor client gets unnecessary high frequency SensorClient::SensorReadingChanged() mojo IPC calls. This CL adds a |notify_client_on_reading_change| flag to sensor configuration to fix it. BUG=721427 ==========
Description was changed from ========== Add |notify_client_on_reading_change| flag to sensor configuration This is a CL following the discussion from: https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... When the sensor client sets its own timer to read the shared memory, but the sensor reporting mode is ON_CHANGE, the sensor client gets unnecessary high frequency SensorClient::SensorReadingChanged() mojo IPC calls. This CL adds a |notify_client_on_reading_change| flag to sensor configuration to fix it. BUG=721427 ========== to ========== Add |notify_client_on_reading_change| flag to sensor configuration This is a CL following the discussion from: https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... When the sensor client sets its own timer to read the shared memory, and the sensor reporting mode is ON_CHANGE, the sensor client gets unnecessary high frequency SensorClient::SensorReadingChanged() mojo IPC calls. This CL adds a |notify_client_on_reading_change| flag to sensor configuration to fix it. BUG=721427 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by juncai@chromium.org 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.
juncai@chromium.org changed reviewers: + timvolodine@chromium.org
Please take a look.
great, somewhat surprised this functionality was not there already.. so lgtm, but think it would be a good idea to get a blessing from one of the authors as well, e.g. Mikhail / Alexander. https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/s... File device/generic_sensor/sensor_impl.h (right): https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/s... device/generic_sensor/sensor_impl.h:45: // flag to true, SensorClient::SensorReadingChanged() is called. Is it possible to only notify the client with the configuration which set the flag?
https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/s... File device/generic_sensor/sensor_impl.h (right): https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/s... device/generic_sensor/sensor_impl.h:45: // flag to true, SensorClient::SensorReadingChanged() is called. On 2017/06/09 12:39:09, timvolodine wrote: > Is it possible to only notify the client with the configuration which set the > flag? hmm scratch that, looks like there is only one client anyway..
juncai@chromium.org changed reviewers: + alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com
mikhail.pozdnyakov@intel.com, alexander.shalamov@intel.com: Please take a look
juncai@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@chromium.org: Please do a security review of changes in //device/generic_sensor/public/
reillyg@chromium.org changed reviewers: + reillyg@chromium.org
https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/p... File device/generic_sensor/public/cpp/platform_sensor_configuration.h (right): https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/p... device/generic_sensor/public/cpp/platform_sensor_configuration.h:39: bool notify_client_on_reading_change_ = true; Because only ON_CHANGE sensors generate notifications and the goal of this patch is to suppress them when the client is going to be polling shared memory instead I would name this "suppress_on_change_events" and default to false. https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/s... File device/generic_sensor/sensor_impl.cc (right): https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/s... device/generic_sensor/sensor_impl.cc:32: ++notify_client_on_reading_change_count_; This field should only be updated if the sensor accepts the new configuration by returning true from StartListening.
The CQ bit was checked by juncai@chromium.org 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...
https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/p... File device/generic_sensor/public/cpp/platform_sensor_configuration.h (right): https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/p... device/generic_sensor/public/cpp/platform_sensor_configuration.h:39: bool notify_client_on_reading_change_ = true; On 2017/06/09 18:03:39, Reilly Grant wrote: > Because only ON_CHANGE sensors generate notifications and the goal of this patch > is to suppress them when the client is going to be polling shared memory instead > I would name this "suppress_on_change_events" and default to false. Done. https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/s... File device/generic_sensor/sensor_impl.cc (right): https://codereview.chromium.org/2927263002/diff/20001/device/generic_sensor/s... device/generic_sensor/sensor_impl.cc:32: ++notify_client_on_reading_change_count_; On 2017/06/09 18:03:39, Reilly Grant wrote: > This field should only be updated if the sensor accepts the new configuration by > returning true from StartListening. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/p... File device/generic_sensor/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/p... device/generic_sensor/public/interfaces/sensor.mojom:47: // otherwise it is not. This comment doesn't seem accurate. https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/s... File device/generic_sensor/sensor_impl.cc (right): https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/s... device/generic_sensor/sensor_impl.cc:33: bool flag = sensor_->StartListening(this, configuration); s/flag/success/ https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/s... File device/generic_sensor/sensor_impl.h (right): https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/s... device/generic_sensor/sensor_impl.h:46: int not_suppress_on_change_events_count_; This is getting really weird. Since we only ever set one configuration on a sensor (and should enforce that in a future patch) can we invert this so that if any configuration suppresses notifications then we don't send them?
The CQ bit was checked by juncai@chromium.org 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...
https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/p... File device/generic_sensor/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/p... device/generic_sensor/public/interfaces/sensor.mojom:47: // otherwise it is not. On 2017/06/09 22:10:02, Reilly Grant wrote: > This comment doesn't seem accurate. Done. https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/s... File device/generic_sensor/sensor_impl.cc (right): https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/s... device/generic_sensor/sensor_impl.cc:33: bool flag = sensor_->StartListening(this, configuration); On 2017/06/09 22:10:02, Reilly Grant wrote: > s/flag/success/ Done. https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/s... File device/generic_sensor/sensor_impl.h (right): https://codereview.chromium.org/2927263002/diff/40001/device/generic_sensor/s... device/generic_sensor/sensor_impl.h:46: int not_suppress_on_change_events_count_; On 2017/06/09 22:10:02, Reilly Grant wrote: > This is getting really weird. Since we only ever set one configuration on a > sensor (and should enforce that in a future patch) can we invert this so that if > any configuration suppresses notifications then we don't send them? Done.
lgtm https://codereview.chromium.org/2927263002/diff/60001/device/generic_sensor/s... File device/generic_sensor/sensor_impl.cc (right): https://codereview.chromium.org/2927263002/diff/60001/device/generic_sensor/s... device/generic_sensor/sensor_impl.cc:65: if (client_ && !suppress_on_change_events_count_) nit: this isn't a boolean, I would say suppress_on_change_events_count_ == 0.
The CQ bit was checked by juncai@chromium.org 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...
https://codereview.chromium.org/2927263002/diff/60001/device/generic_sensor/s... File device/generic_sensor/sensor_impl.cc (right): https://codereview.chromium.org/2927263002/diff/60001/device/generic_sensor/s... device/generic_sensor/sensor_impl.cc:65: if (client_ && !suppress_on_change_events_count_) On 2017/06/09 22:57:14, Reilly Grant wrote: > nit: this isn't a boolean, I would say suppress_on_change_events_count_ == 0. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2927263002/diff/80001/device/generic_sensor/p... File device/generic_sensor/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2927263002/diff/80001/device/generic_sensor/p... device/generic_sensor/public/interfaces/sensor.mojom:51: bool suppress_on_change_events = false; Shouldn't it be rather added to SensorInitParams (sensor_provider.mojom)? Think the more elegant way is to modify PlatformSensor::Client API (pls look at https://cs.chromium.org/chromium/src/device/generic_sensor/platform_sensor.cc... ), Client::IsNotificationSuspended() could be renamed to 'IsOnChangeSuppressed()' or smth like that.
https://codereview.chromium.org/2927263002/diff/80001/device/generic_sensor/p... File device/generic_sensor/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2927263002/diff/80001/device/generic_sensor/p... device/generic_sensor/public/interfaces/sensor.mojom:51: bool suppress_on_change_events = false; On 2017/06/12 11:01:10, Mikhail OOO till Jun 12 wrote: > Shouldn't it be rather added to SensorInitParams (sensor_provider.mojom)? > > Think the more elegant way is to modify PlatformSensor::Client API (pls look at > https://cs.chromium.org/chromium/src/device/generic_sensor/platform_sensor.cc... > ), Client::IsNotificationSuspended() could be renamed to > 'IsOnChangeSuppressed()' or smth like that. If I understand it correctly, SensorInitParams is the return value of the mojo IPC function call GetSensor: https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... and the renderer side can't configure or make change of this parameter. The purpose of |suppress_on_change_events| is to let renderer side be able to control if receiving SensorReadingChanged() calls.
https://codereview.chromium.org/2927263002/diff/80001/device/generic_sensor/p... File device/generic_sensor/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2927263002/diff/80001/device/generic_sensor/p... device/generic_sensor/public/interfaces/sensor.mojom:51: bool suppress_on_change_events = false; On 2017/06/12 17:32:10, juncai wrote: > If I understand it correctly, SensorInitParams is the return value of the mojo > IPC function call GetSensor: > https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... > and the renderer side can't configure or make change of this parameter. > > The purpose of |suppress_on_change_events| is to let renderer side be able to > control if receiving SensorReadingChanged() calls. Oh I see. My only concern is that 'SensorConfiguration' is supposed to contain something to be passed to the underlying platform sensor, whereas the newly added "suppress_on_change_events" field only affects the behavior of the MOJO API implementation. The possible alternate solution would be the following API refactoring interface SensorChangeListener { // Signals SensorChangeListener when reading has been changed. SensorReadingChanged(); }; interface Sensor { // The listener is set if the client wants to recieve 'onchange' notification via IPC (and not set otherwise!). SetChangeListener(SensorChangeListener); }; interface SensorClient { // Signals SensorClient when there is an error. RaiseError(); // Does not contain SensorReadingChanged() any more. }; I'm fine with landing this CL as it is for now and making the refactoring in future (I could take this burden).
On 2017/06/13 12:54:56, Mikhail wrote: > https://codereview.chromium.org/2927263002/diff/80001/device/generic_sensor/p... > File device/generic_sensor/public/interfaces/sensor.mojom (right): > > https://codereview.chromium.org/2927263002/diff/80001/device/generic_sensor/p... > device/generic_sensor/public/interfaces/sensor.mojom:51: bool > suppress_on_change_events = false; > On 2017/06/12 17:32:10, juncai wrote: > > If I understand it correctly, SensorInitParams is the return value of the mojo > > IPC function call GetSensor: > > > https://cs.chromium.org/chromium/src/device/generic_sensor/public/interfaces/... > > and the renderer side can't configure or make change of this parameter. > > > > The purpose of |suppress_on_change_events| is to let renderer side be able to > > control if receiving SensorReadingChanged() calls. > > Oh I see. My only concern is that 'SensorConfiguration' is supposed to contain > something to be passed to the underlying platform sensor, > whereas the newly added "suppress_on_change_events" field only affects the > behavior of the MOJO API implementation. > > The possible alternate solution would be the following API refactoring > > interface SensorChangeListener { > // Signals SensorChangeListener when reading has been changed. > SensorReadingChanged(); > }; > > interface Sensor { > // The listener is set if the client wants to recieve 'onchange' notification > via IPC (and not set otherwise!). > SetChangeListener(SensorChangeListener); > }; > > interface SensorClient { > // Signals SensorClient when there is an error. > RaiseError(); > // Does not contain SensorReadingChanged() any more. > }; > > I'm fine with landing this CL as it is for now and making the refactoring in > future (I could take this burden). I don't think that refactor makes sense as it adds extra complexity for a dubious benefit. The external interface for //device/generic_sensor is the Mojo interface so if some configuration parameters are consumed by the Mojo classes and some by the underlying sensor classes then that is fine.
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timvolodine@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2927263002/#ps80001 (title: "address nit")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) 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...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) 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...)
The CQ bit was checked by juncai@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497374512954910,
"parent_rev": "f86ddbbc48abb0d8c2ad6ee10a1728ff7e87e00c", "commit_rev":
"cfe1d9c8f0f922771e19eb5d2566215b6e47371c"}
Message was sent while issue was closed.
Description was changed from ========== Add |notify_client_on_reading_change| flag to sensor configuration This is a CL following the discussion from: https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... When the sensor client sets its own timer to read the shared memory, and the sensor reporting mode is ON_CHANGE, the sensor client gets unnecessary high frequency SensorClient::SensorReadingChanged() mojo IPC calls. This CL adds a |notify_client_on_reading_change| flag to sensor configuration to fix it. BUG=721427 ========== to ========== Add |notify_client_on_reading_change| flag to sensor configuration This is a CL following the discussion from: https://codereview.chromium.org/2896583005/diff/180001/content/renderer/devic... When the sensor client sets its own timer to read the shared memory, and the sensor reporting mode is ON_CHANGE, the sensor client gets unnecessary high frequency SensorClient::SensorReadingChanged() mojo IPC calls. This CL adds a |notify_client_on_reading_change| flag to sensor configuration to fix it. BUG=721427 Review-Url: https://codereview.chromium.org/2927263002 Cr-Commit-Position: refs/heads/master@{#479074} Committed: https://chromium.googlesource.com/chromium/src/+/cfe1d9c8f0f922771e19eb5d2566... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/cfe1d9c8f0f922771e19eb5d2566... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
