Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2144623003:
[sensors] Introduce Generic Sensor API interfaces (Closed)
|
Created:
4 years, 5 months ago by Mikhail Modified:
4 years, 4 months ago Reviewers:
Ken Rockot(use gerrit already), Reilly Grant (use Gerrit), timvolodine, kinuko, Sam McNally, dcheng, Tom Sepez, jochen (gone - plz use gerrit), pfeldman CC:
Aaron Boodman, abarth-chromium, shalamov, ben+mojo_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, jam, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sensors] Introduce Generic Sensor API interfaces
Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com
New generic sensor mojo interfaces are introduced together with platform independent implementations.
Intent To implement mailing thread containing design description: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/4J7Z088MBAAJ
BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/66ac3d4e8a635db673ff6dd47cc59976ee0b956c
Cr-Commit-Position: refs/heads/master@{#413792}
Patch Set 1 : Initial patch from crrev.com/2078433002 #Patch Set 2 : Check that frequency is > 0 #
Total comments: 1
Patch Set 3 : Moved frequency check to StructTraits #
Total comments: 1
Patch Set 4 : dcheng comments #
Total comments: 21
Patch Set 5 : Comments from Tim and Reilly #
Total comments: 2
Patch Set 6 : Fixed gn headers check #Patch Set 7 : Comments from Reilly #
Total comments: 29
Patch Set 8 : Comments from Reilly #
Total comments: 35
Patch Set 9 : Comments from Tim and Ken #
Total comments: 17
Patch Set 10 : Comments from Tim #Patch Set 11 : Introduced PlatformSensorProviderBase #
Total comments: 1
Patch Set 12 : Definition for PlatformSensorProvider ctor/dtor #Dependent Patchsets: Messages
Total messages: 118 (67 generated)
Description was changed from ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. BUG=606766 ========== to ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. BUG=606766 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
tsepez@chromium.org changed reviewers: + tsepez@chromium.org
Ok, I can give a LGTM on my part of it. Adding the other reviewers as reviewers of this CL is likely to get more attention than just linking from the previous CL.
Description was changed from ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. BUG=606766 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. BUG=606766 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
mikhail.pozdnyakov@intel.com changed reviewers: + dcheng@chromium.org
On 2016/07/13 19:01:48, Tom Sepez wrote: > Ok, I can give a LGTM on my part of it. > > Adding the other reviewers as reviewers of this CL is likely to get more > attention than just linking from the previous CL. Done. Thanks for having a look!
https://codereview.chromium.org/2144623003/diff/40001/device/sensors/platform... File device/sensors/platform_sensor.cc (right): https://codereview.chromium.org/2144623003/diff/40001/device/sensors/platform... device/sensors/platform_sensor.cc:18: return config.frequency() <= 60.0 && config.frequency() > 0.0; As mentioned in the other CL, can we move this check to the StructTraits for reading in this configuration? It's better if we don't need to remember to call this outside of the bindings layer.
On 2016/07/14 02:50:27, dcheng wrote: > https://codereview.chromium.org/2144623003/diff/40001/device/sensors/platform... > File device/sensors/platform_sensor.cc (right): > > https://codereview.chromium.org/2144623003/diff/40001/device/sensors/platform... > device/sensors/platform_sensor.cc:18: return config.frequency() <= 60.0 && > config.frequency() > 0.0; > As mentioned in the other CL, can we move this check to the StructTraits for > reading in this configuration? It's better if we don't need to remember to call > this outside of the bindings layer. Done in the latest patch.
lgtm https://codereview.chromium.org/2144623003/diff/60001/device/sensors/public/i... File device/sensors/public/interfaces/sensor_struct_traits.cc (right): https://codereview.chromium.org/2144623003/diff/60001/device/sensors/public/i... device/sensors/public/interfaces/sensor_struct_traits.cc:20: NOTREACHED(); Nit: this shouldn't be NOTREACHED(), as it's something that can be reached by a compromised process purposely sending bad messages, or the ipc fuzzer. I might also recommend DCHECKing that the frequency is in the right range (0 - 60) in the getter, so we'll know if we start violating this condition somehow by accident on the serialization side.
On 2016/07/14 09:24:10, dcheng wrote: > lgtm > > https://codereview.chromium.org/2144623003/diff/60001/device/sensors/public/i... > File device/sensors/public/interfaces/sensor_struct_traits.cc (right): > > https://codereview.chromium.org/2144623003/diff/60001/device/sensors/public/i... > device/sensors/public/interfaces/sensor_struct_traits.cc:20: NOTREACHED(); > Nit: this shouldn't be NOTREACHED(), as it's something that can be reached by a > compromised process purposely sending bad messages, or the ipc fuzzer. > > I might also recommend DCHECKing that the frequency is in the right range (0 - > 60) in the getter, so we'll know if we start violating this condition somehow by > accident on the serialization side. Done, thanks for the proposal.
Thanks for uploading the renderer/blink side, easier to see how it's used. A couple more comments below, not sure which issue to use (this one/old one), so went with the newer one. in general: - we already have device/sensors/ directory now, so probably need to find a different name - if you plan to land soon, will need gyp build files as well, but the try bots will show that if needed. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... File device/sensors/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... device/sensors/public/interfaces/sensor.mojom:31: // Frequency in Hz add a comment that this is the requested frequency but not always the guaranteed? maybe also something about max frequency https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... device/sensors/public/interfaces/sensor.mojom:44: AddConfiguration(SensorConfiguration configuration) => (bool success); Does the browser really need to know all 'javascript' configurations? Looking ahead at the blink code we typically stop the sensors when page goes invisible (I think your blink patch does that as well). So if you have 1000 sensors in javascript when the page goes invisible you'll get 1000 RemoveConfigurations calls and similarly 1000 AddConfigurations when it becomes visible again (if I understand correctly). To avoid this issue how about doing the 'multiplexing' in the renderer and using a mojo interface something like: Sensor { // can be invoked multiple times with 'override' semantics; pass null to stop run(SensorConfiguration?) } https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... File device/sensors/public/interfaces/sensor_provider.mojom (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... device/sensors/public/interfaces/sensor_provider.mojom:36: GetSensor(SensorType type, Sensor& sensor_request) => ( is there a reason for not having something like GetSensor(SensorType, SensorClient&) => Sensor? (with or without SensorReadBuffer as return, which then can be obtained via Sensor)
My apologies for taking so long to review //device/sensor. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_e... File device/sensors/sensor_export.h (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_e... device/sensors/sensor_export.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: We don't put "(c)" in the copyright header anymore. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_i... File device/sensors/sensor_impl.h (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_i... device/sensors/sensor_impl.h:14: namespace mojom { In this file and those below: Only the generated mojo sources should be in the mojom namespace. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_p... File device/sensors/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_p... device/sensors/sensor_provider_impl.cc:28: if (s_provider_) { nit: No braces for single-line if. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_p... device/sensors/sensor_provider_impl.cc:39: namespace { nit: Keep all the functions defined in anonymous namespaces together at the top of the file. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_p... device/sensors/sensor_provider_impl.cc:56: auto cloned_handle = s_provider_->GetClonedSharedBufferHandle(); The use of auto in this function is counter to my expectations. I don't know what the return value of GetClonedSharedBufferHandle() is here so I'd rather have the type spelled out. In contrast, below when allocating a new SensorImpl and SensorReadBuffer auto can be used because the type is already spelled out once on that line. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_p... device/sensors/sensor_provider_impl.cc:63: if (!sensor) nit: Braces are required around the body of a multi-line if. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_s... File device/sensors/sensor_service.cc (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_s... device/sensors/sensor_service.cc:20: task_runner->PostTask(FROM_HERE, base::Bind(mojom::SensorProviderImpl::Create, What is the purpose of posting a task here? https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_s... File device/sensors/sensor_service.h (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_s... device/sensors/sensor_service.h:20: // internal implementation of SensorProvider. "The SensorService binds a request for the SensorProvider interface to an internal implementation of SensorProvider."
On 2016/07/22 19:31:45, timvolodine wrote: > Thanks for uploading the renderer/blink side, easier to see how it's used. A > couple more comments below, not sure which issue to use (this one/old one), so > went with the newer one. > > in general: > - we already have device/sensors/ directory now, so probably need to find a > different name Sorry for the delay in response due to vacations time. I am preparing a new patch now and I'm putting the sources to the directory named 'generic_sensor' (based on the API name). > - if you plan to land soon, will need gyp build files as well, but the try bots > will show that if needed. > > https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... > File device/sensors/public/interfaces/sensor.mojom (right): > > https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... > device/sensors/public/interfaces/sensor.mojom:31: // Frequency in Hz > add a comment that this is the requested frequency but not always the > guaranteed? maybe also something about max frequency > > https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... > device/sensors/public/interfaces/sensor.mojom:44: > AddConfiguration(SensorConfiguration configuration) => (bool success); > Does the browser really need to know all 'javascript' configurations? Looking > ahead at the blink code we typically stop the sensors when page goes invisible > (I think your blink patch does that as well). So if you have 1000 sensors in > javascript when the page goes invisible you'll get 1000 RemoveConfigurations > calls and similarly 1000 AddConfigurations when it becomes visible again (if I > understand correctly). > > To avoid this issue how about doing the 'multiplexing' in the renderer and using > a mojo interface something like: > > Sensor { > // can be invoked multiple times with 'override' semantics; pass null to stop > run(SensorConfiguration?) > } Thanks for pointing this out, however we'd rather introduce Suspend/Resume methods to Sensor interface (called once per frame when page goes invisible). Rational: PlatformSensor on browser side will anyway have to do the 'multiplexing' (for multiple renders) and we'd prefer keeping this logic in one place. PlatformSensor already holds the map: client <-> its list of configurations, so "suspended" client's configurations can just be ignored.
Thanks for your comments! https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... File device/sensors/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... device/sensors/public/interfaces/sensor.mojom:31: // Frequency in Hz On 2016/07/22 19:31:45, timvolodine wrote: > add a comment that this is the requested frequency but not always the > guaranteed? maybe also something about max frequency Done. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... File device/sensors/public/interfaces/sensor_provider.mojom (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... device/sensors/public/interfaces/sensor_provider.mojom:36: GetSensor(SensorType type, Sensor& sensor_request) => ( On 2016/07/22 19:31:45, timvolodine wrote: > is there a reason for not having something like > GetSensor(SensorType, SensorClient&) => Sensor? > > (with or without SensorReadBuffer as return, which then can be obtained via > Sensor) Wouldn't it lead to an extra async call (to obtain SensorReadBuffer from Sensor)? https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_e... File device/sensors/sensor_export.h (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_e... device/sensors/sensor_export.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/07/27 22:07:58, Reilly Grant wrote: > nit: We don't put "(c)" in the copyright header anymore. Done. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_i... File device/sensors/sensor_impl.h (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_i... device/sensors/sensor_impl.h:14: namespace mojom { On 2016/07/27 22:07:58, Reilly Grant wrote: > In this file and those below: Only the generated mojo sources should be in the > mojom namespace. Done. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_p... File device/sensors/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_p... device/sensors/sensor_provider_impl.cc:28: if (s_provider_) { On 2016/07/27 22:07:58, Reilly Grant wrote: > nit: No braces for single-line if. Done. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_p... device/sensors/sensor_provider_impl.cc:39: namespace { On 2016/07/27 22:07:58, Reilly Grant wrote: > nit: Keep all the functions defined in anonymous namespaces together at the top > of the file. Done. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_p... device/sensors/sensor_provider_impl.cc:56: auto cloned_handle = s_provider_->GetClonedSharedBufferHandle(); On 2016/07/27 22:07:58, Reilly Grant wrote: > The use of auto in this function is counter to my expectations. I don't know > what the return value of GetClonedSharedBufferHandle() is here so I'd rather > have the type spelled out. In contrast, below when allocating a new SensorImpl > and SensorReadBuffer auto can be used because the type is already spelled out > once on that line. Done. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_p... device/sensors/sensor_provider_impl.cc:63: if (!sensor) On 2016/07/27 22:07:58, Reilly Grant wrote: > nit: Braces are required around the body of a multi-line if. Done. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_s... File device/sensors/sensor_service.cc (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_s... device/sensors/sensor_service.cc:20: task_runner->PostTask(FROM_HERE, base::Bind(mojom::SensorProviderImpl::Create, On 2016/07/27 22:07:58, Reilly Grant wrote: > What is the purpose of posting a task here? We want to move the notification from the sensors out of the main thread. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_s... File device/sensors/sensor_service.h (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_s... device/sensors/sensor_service.h:20: // internal implementation of SensorProvider. On 2016/07/27 22:07:58, Reilly Grant wrote: > "The SensorService binds a request for the SensorProvider interface to an > internal implementation of SensorProvider." 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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_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 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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2144623003/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2144623003/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2149: base::RetainedRef( base::RetainedRef() is not necessary here as GetTaskRunnerForThread() already returns a scoped_refptr.
https://codereview.chromium.org/2144623003/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2144623003/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2149: base::RetainedRef( On 2016/08/10 18:00:19, Reilly Grant wrote: > base::RetainedRef() is not necessary here as GetTaskRunnerForThread() already > returns a scoped_refptr. Done.
https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor.cc (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor.cc:44: config_list.erase(iterator); Can this be replaced by config_list.remove(config)? Do you expect multiple entries equal to config to be present in the list? Maybe you can just use pop_back() to remove it? https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensor(const ConfigMap& configurations) = 0; Consider naming this UpdateSensorInternal() as overloading the public function name may be confusing. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor_configuration.h (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:23: DCHECK(frequency_ <= 60.0 && frequency_ > 0.0); Do this check in set_frequency so that the call stack shows anyone debugging this were the bad value came from. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:28: double frequency_; Provide a safe default for frequency_ so that the default constructor doesn't leave it uninitialized. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.cc (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:38: // Create new platform sensor instance. This comment is unnecessary. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:46: DCHECK(add_result.second); It's a little shorter/clearer to write: DCHECK(!ContainsKey(sensor_map_, type)); sensor_map_[type] = new_sensor.get(); https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:74: sensor_map_.erase(it); sensor_map_.erase(type) is simpler. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.h (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:29: mojo::ScopedSharedBufferHandle GetClonedSharedBufferHandle(); CloneSharedBufferHandle() would be a shorter/clearer name. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:39: uint64_t buffer_size) = 0; This overload is definitely confusing. Please name it CreateSensorInternal(). https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:44: friend class PlatformSensor; // To call RemoveSensor(); Move this line to the top of the private: section. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:48: using SensorMap = std::map<mojom::SensorType, PlatformSensor*>; This typedef is used only once here. Don't worry about the line below being long. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/public/interfaces/sensor.typemap (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.typemap:16: [ "device.mojom.SensorConfiguration=device::PlatformSensorConfiguration" ] Unless an automatic formatting tool did this I wouldn't wrap single-item lists this way. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/public/interfaces/sensor_provider.mojom (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor_provider.mojom:30: // |request| the Sensor interface instance to be initialized. s/request/sensor_request/ Also extra blank line above. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:21: return (static_cast<uint64_t>(mojom::SensorType::LAST) - Why arrange offsets in the buffer in reverse order like this?
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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor.cc (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor.cc:44: config_list.erase(iterator); On 2016/08/11 20:59:37, Reilly Grant wrote: > Can this be replaced by config_list.remove(config)? Do you expect multiple > entries equal to config to be present in the list? Maybe you can just use > pop_back() to remove it? Absolutely, thanks for the proposal! https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensor(const ConfigMap& configurations) = 0; On 2016/08/11 20:59:37, Reilly Grant wrote: > Consider naming this UpdateSensorInternal() as overloading the public function > name may be confusing. Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor_configuration.h (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:23: DCHECK(frequency_ <= 60.0 && frequency_ > 0.0); On 2016/08/11 20:59:37, Reilly Grant wrote: > Do this check in set_frequency so that the call stack shows anyone debugging > this were the bad value came from. Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:28: double frequency_; On 2016/08/11 20:59:37, Reilly Grant wrote: > Provide a safe default for frequency_ so that the default constructor doesn't > leave it uninitialized. Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.cc (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:38: // Create new platform sensor instance. On 2016/08/11 20:59:37, Reilly Grant wrote: > This comment is unnecessary. Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:46: DCHECK(add_result.second); On 2016/08/11 20:59:37, Reilly Grant wrote: > It's a little shorter/clearer to write: > > DCHECK(!ContainsKey(sensor_map_, type)); > sensor_map_[type] = new_sensor.get(); Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:74: sensor_map_.erase(it); On 2016/08/11 20:59:37, Reilly Grant wrote: > sensor_map_.erase(type) is simpler. Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.h (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:29: mojo::ScopedSharedBufferHandle GetClonedSharedBufferHandle(); On 2016/08/11 20:59:37, Reilly Grant wrote: > CloneSharedBufferHandle() would be a shorter/clearer name. Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:39: uint64_t buffer_size) = 0; On 2016/08/11 20:59:37, Reilly Grant wrote: > This overload is definitely confusing. Please name it CreateSensorInternal(). Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:44: friend class PlatformSensor; // To call RemoveSensor(); On 2016/08/11 20:59:37, Reilly Grant wrote: > Move this line to the top of the private: section. Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:48: using SensorMap = std::map<mojom::SensorType, PlatformSensor*>; On 2016/08/11 20:59:37, Reilly Grant wrote: > This typedef is used only once here. Don't worry about the line below being > long. Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/public/interfaces/sensor.typemap (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.typemap:16: [ "device.mojom.SensorConfiguration=device::PlatformSensorConfiguration" ] On 2016/08/11 20:59:37, Reilly Grant wrote: > Unless an automatic formatting tool did this I wouldn't wrap single-item lists > this way. this comes from 'git cl format' https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/public/interfaces/sensor_provider.mojom (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor_provider.mojom:30: // |request| the Sensor interface instance to be initialized. On 2016/08/11 20:59:37, Reilly Grant wrote: > s/request/sensor_request/ > > Also extra blank line above. Done. https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:21: return (static_cast<uint64_t>(mojom::SensorType::LAST) - On 2016/08/11 20:59:37, Reilly Grant wrote: > Why arrange offsets in the buffer in reverse order like this? In practice it has no effect but IMO just looks a bit better than: (static_cast<uint64_t>(type) - 1) * mojom::SensorReadBuffer::kReadBufferSize; // mojom::SensorType::FIRST==1, as we wanted the sensor types count to come from mojo (mojom::SensorType::LAST == types count)
lgtm https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:21: return (static_cast<uint64_t>(mojom::SensorType::LAST) - On 2016/08/12 at 10:21:48, Mikhail wrote: > On 2016/08/11 20:59:37, Reilly Grant wrote: > > Why arrange offsets in the buffer in reverse order like this? > In practice it has no effect but IMO just looks a bit better than: > (static_cast<uint64_t>(type) - 1) * mojom::SensorReadBuffer::kReadBufferSize; // mojom::SensorType::FIRST==1, as we wanted the sensor types count to come from mojo (mojom::SensorType::LAST == types count) Why does mojom::SensorType start at 1 anyways?
On 2016/08/12 18:01:59, Reilly Grant wrote: > lgtm > > https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... > File device/generic_sensor/sensor_provider_impl.cc (right): > > https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/... > device/generic_sensor/sensor_provider_impl.cc:21: return > (static_cast<uint64_t>(mojom::SensorType::LAST) - > On 2016/08/12 at 10:21:48, Mikhail wrote: > > On 2016/08/11 20:59:37, Reilly Grant wrote: > > > Why arrange offsets in the buffer in reverse order like this? > > In practice it has no effect but IMO just looks a bit better than: > > (static_cast<uint64_t>(type) - 1) * mojom::SensorReadBuffer::kReadBufferSize; > // mojom::SensorType::FIRST==1, as we wanted the sensor types count to come from > mojo (mojom::SensorType::LAST == types count) > > Why does mojom::SensorType start at 1 anyways? So that mojom::SensorType::LAST == types count, otherwise it would be (mojom::SensorType::LAST + 1) == types count
mikhail.pozdnyakov@intel.com changed reviewers: + kinuko@chromium.org, pfeldman@chromium.org
@pfeldman, @kinuko could you pls have a look at content/ changes (since jochen is OOO)?
> https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... > > device/sensors/public/interfaces/sensor.mojom:44: > > AddConfiguration(SensorConfiguration configuration) => (bool success); > > Does the browser really need to know all 'javascript' configurations? Looking > > ahead at the blink code we typically stop the sensors when page goes invisible > > (I think your blink patch does that as well). So if you have 1000 sensors in > > javascript when the page goes invisible you'll get 1000 RemoveConfigurations > > calls and similarly 1000 AddConfigurations when it becomes visible again (if I > > understand correctly). > > > > To avoid this issue how about doing the 'multiplexing' in the renderer and > using > > a mojo interface something like: > > > > Sensor { > > // can be invoked multiple times with 'override' semantics; pass null to > stop > > run(SensorConfiguration?) > > } > > Thanks for pointing this out, however we'd rather introduce Suspend/Resume > methods to Sensor interface (called once per frame when page goes invisible). > Rational: PlatformSensor on browser side will anyway have to do the > 'multiplexing' (for multiple renders) and we'd prefer keeping this logic in one > place. > PlatformSensor already holds the map: client <-> its list of configurations, so > "suspended" client's configurations can just be ignored. Sorry for the delay, it's not my intention to be nagging on this, however I am still not clear why the browser should know about all configs in the renderer. To have suspend/resume I think you still need some kind of 'multiplexing' on the blink side, since the current code is not really structured on a 'frame' basis but basically each javascript sensor now calls browser from the blink side, with potentially a lot of calls when page visibility changes. Page-visibility seems to be a pretty common use case so probably worth taking into account. Basically I think the mojo interface could be somewhat simpler, with a 'multiplexer' on the blink side, and the eventual shared code could potentially live somewhere in the common/ directory.
On 2016/08/15 19:35:22, timvolodine wrote: > > > https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... > > > device/sensors/public/interfaces/sensor.mojom:44: > > > AddConfiguration(SensorConfiguration configuration) => (bool success); > > > Does the browser really need to know all 'javascript' configurations? > Looking > > > ahead at the blink code we typically stop the sensors when page goes > invisible > > > (I think your blink patch does that as well). So if you have 1000 sensors in > > > javascript when the page goes invisible you'll get 1000 RemoveConfigurations > > > calls and similarly 1000 AddConfigurations when it becomes visible again (if > I > > > understand correctly). > > > > > > To avoid this issue how about doing the 'multiplexing' in the renderer and > > using > > > a mojo interface something like: > > > > > > Sensor { > > > // can be invoked multiple times with 'override' semantics; pass null to > > stop > > > run(SensorConfiguration?) > > > } > > > > Thanks for pointing this out, however we'd rather introduce Suspend/Resume > > methods to Sensor interface (called once per frame when page goes invisible). > > Rational: PlatformSensor on browser side will anyway have to do the > > 'multiplexing' (for multiple renders) and we'd prefer keeping this logic in > one > > place. > > PlatformSensor already holds the map: client <-> its list of configurations, > so > > "suspended" client's configurations can just be ignored. > > Sorry for the delay, it's not my intention to be nagging on this, however I am > still not clear why the browser should know about all configs in the renderer. > To have suspend/resume I think you still need some kind of 'multiplexing' on the > blink side, since the current code is not really structured on a 'frame' basis > but basically each javascript sensor now calls browser from the blink side, with > potentially a lot of calls when page visibility changes. Page-visibility seems > to be a pretty common use case so probably worth taking into account. Basically > I think the mojo interface could be somewhat simpler, with a 'multiplexer' on > the blink side, and the eventual shared code could potentially live somewhere in > the common/ directory. The resulting configuration for the 'real' sensor will be calculated considering underlying SW and HW platform constraints/capabilities which IMO better to be encapsulated on browser side (not shared with renderer) for both design and security reasons. All JS sensors within a single frame share one mojo proxy, 'SuspendNotification' call will be called once for each frame when the page goes invisible. In this case JS sensors are not invoking any methods on mojo interface but rather simply stop reading from the shared memory buffer (pls see https://codereview.chromium.org/2121313002/)
content/ 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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
On 2016/08/15 22:16:46, Mikhail wrote: > On 2016/08/15 19:35:22, timvolodine wrote: > > > > > > https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/i... > > > > device/sensors/public/interfaces/sensor.mojom:44: > > > > AddConfiguration(SensorConfiguration configuration) => (bool success); > > > > Does the browser really need to know all 'javascript' configurations? > > Looking > > > > ahead at the blink code we typically stop the sensors when page goes > > invisible > > > > (I think your blink patch does that as well). So if you have 1000 sensors > in > > > > javascript when the page goes invisible you'll get 1000 > RemoveConfigurations > > > > calls and similarly 1000 AddConfigurations when it becomes visible again > (if > > I > > > > understand correctly). > > > > > > > > To avoid this issue how about doing the 'multiplexing' in the renderer and > > > using > > > > a mojo interface something like: > > > > > > > > Sensor { > > > > // can be invoked multiple times with 'override' semantics; pass null to > > > stop > > > > run(SensorConfiguration?) > > > > } > > > > > > Thanks for pointing this out, however we'd rather introduce Suspend/Resume > > > methods to Sensor interface (called once per frame when page goes > invisible). > > > Rational: PlatformSensor on browser side will anyway have to do the > > > 'multiplexing' (for multiple renders) and we'd prefer keeping this logic in > > one > > > place. > > > PlatformSensor already holds the map: client <-> its list of configurations, > > so > > > "suspended" client's configurations can just be ignored. > > > > Sorry for the delay, it's not my intention to be nagging on this, however I am > > still not clear why the browser should know about all configs in the renderer. > > To have suspend/resume I think you still need some kind of 'multiplexing' on > the > > blink side, since the current code is not really structured on a 'frame' basis > > but basically each javascript sensor now calls browser from the blink side, > with > > potentially a lot of calls when page visibility changes. Page-visibility seems > > to be a pretty common use case so probably worth taking into account. > Basically > > I think the mojo interface could be somewhat simpler, with a 'multiplexer' on > > the blink side, and the eventual shared code could potentially live somewhere > in > > the common/ directory. > > The resulting configuration for the 'real' sensor will be calculated considering > underlying SW and HW platform constraints/capabilities which IMO better to be > encapsulated on browser side (not shared with renderer) for both design and > security reasons. > What kind of security reasons? generally speaking the simpler the interface the easier it is to analyse any potential security issues. > All JS sensors within a single frame share one mojo proxy, 'SuspendNotification' > call will be called once for each frame when the page goes invisible. > In this case JS sensors are not invoking any methods on mojo interface but > rather simply stop reading from the shared memory buffer > (pls see https://codereview.chromium.org/2121313002/) In the blink patch I see you are not inheriting from PlatformEventController but instead overriding pageVisibility change. Does this result in sensors being switched off when page goes invisible? This was an important issue which was addressed specifically for other sensor-related apis in order to not drain battery/cpu. It probably would have been nice to have some kind a 'design doc' before coding so that these issues could be noted early on.
On 2016/08/16 12:52:11, timvolodine wrote: > > What kind of security reasons? generally speaking the simpler the interface the > easier it is to analyse any potential security issues. By 'security reasons' I meant that we would reduce amount of platform information exposed to the render process (e.g. allowed frequency ranges, current platform sensor configuration that might be shared by multiple renderers, etc) > > > All JS sensors within a single frame share one mojo proxy, > 'SuspendNotification' > > call will be called once for each frame when the page goes invisible. > > In this case JS sensors are not invoking any methods on mojo interface but > > rather simply stop reading from the shared memory buffer > > (pls see https://codereview.chromium.org/2121313002/) > > In the blink patch I see you are not inheriting from PlatformEventController but > instead overriding pageVisibility change. Does this result in sensors being > switched off when page goes invisible? This was an important issue which was > addressed specifically for other sensor-related apis in order to not drain > battery/cpu. > Sensor is switched off if the page goes invisible (https://codereview.chromium.org/2121313002/diff/20001/third_party/WebKit/Sour...) > It probably would have been nice to have some kind a 'design doc' before coding > so that these issues could be noted early on. We sent design proposals to the intent-to-implement mailing thread, and made few improvements based on Reilly's and Ken's feedback. https://groups.google.com/a/chromium.org/d/msg/blink-dev/TkfdVqYAYiE/4J7Z088M...
The CQ bit was checked by mikhail.pozdnyakov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2144623003/#ps160001 (title: "Comments from Reilly")
The CQ bit was unchecked by mikhail.pozdnyakov@intel.com
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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
On 2016/08/16 13:28:30, Mikhail wrote: > On 2016/08/16 12:52:11, timvolodine wrote: > > > > What kind of security reasons? generally speaking the simpler the interface > the > > easier it is to analyse any potential security issues. > > By 'security reasons' I meant that we would reduce amount of platform > information exposed to the render process > (e.g. allowed frequency ranges, current platform sensor configuration that might > be shared by multiple renderers, etc) > > > > > > All JS sensors within a single frame share one mojo proxy, > > 'SuspendNotification' > > > call will be called once for each frame when the page goes invisible. > > > In this case JS sensors are not invoking any methods on mojo interface but > > > rather simply stop reading from the shared memory buffer > > > (pls see https://codereview.chromium.org/2121313002/) > > > > In the blink patch I see you are not inheriting from PlatformEventController > but > > instead overriding pageVisibility change. Does this result in sensors being > > switched off when page goes invisible? This was an important issue which was > > addressed specifically for other sensor-related apis in order to not drain > > battery/cpu. > > > > Sensor is switched off if the page goes invisible > (https://codereview.chromium.org/2121313002/diff/20001/third_party/WebKit/Sour...) > > > It probably would have been nice to have some kind a 'design doc' before > coding > > so that these issues could be noted early on. > > We sent design proposals to the intent-to-implement mailing thread, and made few > improvements based on Reilly's and Ken's feedback. > https://groups.google.com/a/chromium.org/d/msg/blink-dev/TkfdVqYAYiE/4J7Z088M... for some reason there was no notification from my reply, so reposting it.
On 2016/08/16 13:28:30, Mikhail wrote: > On 2016/08/16 12:52:11, timvolodine wrote: > > > > What kind of security reasons? generally speaking the simpler the interface > the > > easier it is to analyse any potential security issues. > > By 'security reasons' I meant that we would reduce amount of platform > information exposed to the render process > (e.g. allowed frequency ranges, current platform sensor configuration that might > be shared by multiple renderers, etc) > Thanks for the clarifications Mikhail. Actually, regarding security I think the simpler interface (with multiplexer in blink) would provide more control and potentially less IPCs. Imagine a rogue javascript (not even a compromised renderer) doing a 'denial of service' type attack on the whole browser by creating/deleting many sensors.. > > > > > All JS sensors within a single frame share one mojo proxy, > > 'SuspendNotification' > > > call will be called once for each frame when the page goes invisible. > > > In this case JS sensors are not invoking any methods on mojo interface but > > > rather simply stop reading from the shared memory buffer > > > (pls see https://codereview.chromium.org/2121313002/) > > > > In the blink patch I see you are not inheriting from PlatformEventController > but > > instead overriding pageVisibility change. Does this result in sensors being > > switched off when page goes invisible? This was an important issue which was > > addressed specifically for other sensor-related apis in order to not drain > > battery/cpu. > > > > Sensor is switched off if the page goes invisible > (https://codereview.chromium.org/2121313002/diff/20001/third_party/WebKit/Sour...) I see, both Sensor and SensorProviderProxy listen to page visibility. I was initially confused since we normally use PlatformEventController in these cases. I guess my thinking is that resume/suspend/add/remove/ mojo calls can all be replaced my a single method with the same effect. > > > It probably would have been nice to have some kind a 'design doc' before > coding > > so that these issues could be noted early on. > > We sent design proposals to the intent-to-implement mailing thread, and made few > improvements based on Reilly's and Ken's feedback. > https://groups.google.com/a/chromium.org/d/msg/blink-dev/TkfdVqYAYiE/4J7Z088M... Sorry must have missed the diagrams since it was some time ago.. They look somewhat different to the actual code though, anyway maybe worth including a pointer to that thread in description.
Description was changed from ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. BUG=606766 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. Intent To implement mailing thread containing design description: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/4J... BUG=606766 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Thanks for your comments and proposals.
On 2016/08/16 15:51:20, timvolodine wrote:
> Actually, regarding security I think the simpler interface (with multiplexer
in
> blink) would provide more control and potentially less IPCs. Imagine a rogue
> javascript (not even a compromised renderer) doing a 'denial of service' type
> attack on the whole browser by creating/deleting many sensors..
>
I think multiplexing on Blink side wouldn't actually decrease IPC, pls. consider
function generateNewSensorConfiguration() // Returns a new configuration on each
call.
for (i = 0; i < ...; ++i {
var sensor = new AwesomeSensor(generateNewSensorConfiguration());
sensor.start(); // an IPC call anyway to update sensor configuration on
platfrom side.
}
What can be improved indeed on Blink side is filtering out the JS sensor
instances sharing the same configurations and obviating extra IPC calls for them
(i.e. if there is already a sensor with the same configuration we don't call
'Add/RemoveConfiguration' methods for the new one).
This won't require any extra system data to be exported to a renderer.
Would it look good for you? If yes we can shortly update
https://crrev.org/2121313002 to add this optimization.
>
> Sorry must have missed the diagrams since it was some time ago.. They look
> somewhat different to the actual code though, anyway maybe worth including a
> pointer to that thread in description.
Done.
On 2016/08/17 11:09:19, Mikhail wrote:
> Thanks for your comments and proposals.
> On 2016/08/16 15:51:20, timvolodine wrote:
> > Actually, regarding security I think the simpler interface (with multiplexer
> in
> > blink) would provide more control and potentially less IPCs. Imagine a rogue
> > javascript (not even a compromised renderer) doing a 'denial of service'
type
> > attack on the whole browser by creating/deleting many sensors..
> >
> I think multiplexing on Blink side wouldn't actually decrease IPC, pls.
consider
>
> function generateNewSensorConfiguration() // Returns a new configuration on
each
> call.
> for (i = 0; i < ...; ++i {
> var sensor = new AwesomeSensor(generateNewSensorConfiguration());
> sensor.start(); // an IPC call anyway to update sensor configuration on
> platfrom side.
> }
>
> What can be improved indeed on Blink side is filtering out the JS sensor
> instances sharing the same configurations and obviating extra IPC calls for
them
> (i.e. if there is already a sensor with the same configuration we don't call
> 'Add/RemoveConfiguration' methods for the new one).
Yes the blink multiplexer would not always result in strictly less IPC, but in
general would have less IPC depending on configs of the sensors and the order in
which they are created. It's probably debatable but
"generateNewSensorConfiguration()" looks like a bit of a stretch as a use-case
anyway..
What I am more concerned about is the creation/deletion of large quantities of
sensors on purpose as mentioned earlier. In the case of multiplexer this would
be easy to prevent by e.g. limiting the number of sensors with different configs
or something like that.
>
> This won't require any extra system data to be exported to a renderer.
>
not sure why we should be worried about exporting bit of extra logic to
renderer, we do that in other blink apis.
> Would it look good for you? If yes we can shortly update
> https://crrev.org/2121313002 to add this optimization.
not sure exactly how you plan to filter the configs in blink, but sounds like
you would be writing (a part of) blink multiplexer already ;)
On 2016/08/17 13:13:39, timvolodine wrote: > What I am more concerned about is the creation/deletion of large quantities of > sensors on purpose as mentioned earlier. In the case of multiplexer this would > be easy to prevent by e.g. limiting the number of sensors with different configs > or something like that. > Construction of a sensor does not lead to an IPC call, the first IPC call happens on 'start()' JS method call. Multiplexing on Blink side IMO would not help much here (as still need to update the platform sensor config) unless different JS sensor instances share the same configuration which is indeed worth optimizing like proposed above. > > > Would it look good for you? If yes we can shortly update > > https://crrev.org/2121313002 to add this optimization. > > not sure exactly how you plan to filter the configs in blink, but sounds like > you would be writing (a part of) blink multiplexer already ;) Yeah, that is actually multiplexing as well :)
On 2016/08/17 13:13:39, timvolodine wrote: > What I am more concerned about is the creation/deletion of large quantities of > sensors on purpose as mentioned earlier. In the case of multiplexer this would > be easy to prevent by e.g. limiting the number of sensors with different configs > or something like that. > Construction of a sensor does not lead to an IPC call, the first IPC call happens on 'start()' JS method call. Multiplexing on Blink side IMO would not help much here (as still need to update the platform sensor config) unless different JS sensor instances share the same configuration which is indeed worth optimizing like proposed above. > > > Would it look good for you? If yes we can shortly update > > https://crrev.org/2121313002 to add this optimization. > > not sure exactly how you plan to filter the configs in blink, but sounds like > you would be writing (a part of) blink multiplexer already ;) Yeah, that is actually multiplexing as well :)
On 2016/08/17 13:13:39, timvolodine wrote: > not sure why we should be worried about exporting bit of extra logic to > renderer, we do that in other blink apis. It's not just a piece of logic -- what I'm concerned is platform sensor data and constrains (e.g. frequency range, maximum delay and others) which has to be considered when we define the resulting platform device configuration. If we want to implement complete multiplexing within renderer (not just filtering out the same configurations), all of these platform data would have to be serialized in generic cross-platform way and shared with renderers -- this increases complexity. Furthermore platform data can change in time (https://developer.android.com/reference/android/hardware/SensorManager.Dynami...) so there should be mojo API to update it in renderers, this will also result in extra complexity and possible raise conditions (platform has been updated but not all renderers are yet notified).
lgtm https://codereview.chromium.org/2144623003/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2144623003/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2147: GetInterfaceRegistry()->AddInterface(base::Bind( AddInterface() itself can take a task runner as a second argument: GetInterfaceRegistry()->AddInterface( base::Bind(&SensorProviderImpl::Create), BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)); https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/sensor_service.h (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/sensor_service.h:21: class SensorService { I don't think we need SensorService or this h/cc file. Please just use SensorProviderImpl directly in content
On 2016/08/18 14:28:24, Mikhail wrote: > On 2016/08/17 13:13:39, timvolodine wrote: > > not sure why we should be worried about exporting bit of extra logic to > > renderer, we do that in other blink apis. > It's not just a piece of logic -- what I'm concerned is platform sensor data and > constrains (e.g. frequency range, maximum delay and others) which has to be > considered when we define the resulting platform device configuration. > If we want to implement complete multiplexing within renderer (not just > filtering out the same configurations), all of these platform data would have to > be serialized in generic cross-platform way and shared with renderers -- this > increases complexity. > Furthermore platform data can change in time > (https://developer.android.com/reference/android/hardware/SensorManager.Dynami...) > so there should be mojo API to update it in renderers, this will also result in > extra complexity and possible raise conditions (platform has been updated but > not all renderers are yet notified). FYI, for the record: Chatted with Mikhail today (thanks Mikhail!). I understood that adding each sensorConfig to the browser is to allow for platform-side validation of each config. I.e. config with frequency=10000 can fail if it's not supported by the platform. If we want to fail based on frequency then multiplexing something like that would be difficult on the blink side. Other potential parameter for platform validation could be 'precision' or 'accuracy'. Note that JS sensors like Accelerometer(includeGravity=true/false) (as in the spec example) would actually need to use two different 'internal' sensorTypes with separate shared memory buffers depending on the includeGravity setting.
A few comments below. Would be nice to have some unit testing at some stage. + run trybots? https://codereview.chromium.org/2144623003/diff/160001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2144623003/diff/160001/content/browser/BUILD.... content/browser/BUILD.gn:65: "//device/generic_sensor", we may want to have "enable_generic_sensor" for this (as e.g. enable_webrtc)? (to allow control for smaller chrome/webview footprint) https://codereview.chromium.org/2144623003/diff/160001/content/public/common/... File content/public/common/content_switches.h (right): https://codereview.chromium.org/2144623003/diff/160001/content/public/common/... content/public/common/content_switches.h:128: CONTENT_EXPORT extern const char kEnableGenericSensors[]; maybe #ifdef this with "ENABLE_GENERIC_SENSORS"? https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/platform_sensor_configuration.h (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:5: #ifndef DEVICE_SENSORS_PLATFORM_SENSOR_CONFIGURATION_H_ -> DEVICE_GENERIC_SENSOR_PLATFORM.. ? https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:15: PlatformSensorConfiguration(); is this constructor needed? just keep the one with explicit frequency? https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:22: DCHECK(frequency_ <= 60.0 && frequency_ > 0.0); probably worth having some kind of constant for 60Hz as a max cut-off frequency (also used in sensor_struct_traits.cc and maybe elsewhere) https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:29: double frequency_ = 1.0; // 1 Hz by default. 1 Hz seems a bit arbitrary, no need for this is only allow explicit constructor https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.cc (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:17: // implementations here. NOTREACHED()? https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.h (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:5: #ifndef DEVICE_SENSORS_PLATFORM_SENSOR_PROVIDER_H_ -> DEVICE_GENERIC_SENSOR_PLATFORM.. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:18: static PlatformSensorProvider* Create(uint64_t shared_buffer_size); is this a singleton? then seems better to have a GetInstance() method with base::Singleton semantics (without parameter instead using kSharedBufferSizeInBytes directly) https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.mojom:12: ACCELEROMETER, maybe distinguish accelerometer with including/excluding gravity, ok to do in separate patch.. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.mojom:44: AddConfiguration(SensorConfiguration configuration) => (bool success); maybe mention that this can fail if the config is not supported. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.mojom:52: RemoveConfiguration(SensorConfiguration configuration) => (bool success); not sure if a return value (=> bool success) is useful here. what would the implementation do if the result is false? https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.mojom:55: SuspendNotification(); maybe just "Suspend()"? I understand this will stop sensor updates (and potentially stop the platform sensor) https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/sensor_impl.cc (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/sensor_impl.cc:30: callback.Run(sensor_->StartListening(this, configuration)); add TODO to avoid overflowing browser by repeated AddConfigs (maybe limit the number of configs per client) https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/sensor_impl.cc:51: if (client_) is client_ set anywhere? should this be a DCHECK? https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.h (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.h:35: static PlatformSensorProvider* s_provider_; no need for this pointer if we have PlatformSensorProvider::GetInstance
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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
https://codereview.chromium.org/2144623003/diff/160001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2144623003/diff/160001/content/browser/BUILD.... content/browser/BUILD.gn:65: "//device/generic_sensor", On 2016/08/18 22:52:13, timvolodine wrote: > we may want to have "enable_generic_sensor" for this (as e.g. enable_webrtc)? > (to allow control for smaller chrome/webview footprint) We'd like to keep it as a runtime-enabled feature, so that try bots can compile the code, and developers can easily enable it and provide early feedback for the new API.. https://codereview.chromium.org/2144623003/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2144623003/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2147: GetInterfaceRegistry()->AddInterface(base::Bind( On 2016/08/18 19:36:00, Ken Rockot wrote: > AddInterface() itself can take a task runner as a second argument: > > GetInterfaceRegistry()->AddInterface( > base::Bind(&SensorProviderImpl::Create), > BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)); > Done. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/platform_sensor_configuration.h (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:5: #ifndef DEVICE_SENSORS_PLATFORM_SENSOR_CONFIGURATION_H_ On 2016/08/18 22:52:14, timvolodine wrote: > -> DEVICE_GENERIC_SENSOR_PLATFORM.. ? Done, thanks for noticing https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:15: PlatformSensorConfiguration(); On 2016/08/18 22:52:14, timvolodine wrote: > is this constructor needed? just keep the one with explicit frequency? It is required by mojo generated binding, have to provide a default constructor https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_configuration.h:22: DCHECK(frequency_ <= 60.0 && frequency_ > 0.0); On 2016/08/18 22:52:14, timvolodine wrote: > probably worth having some kind of constant for 60Hz as a max cut-off frequency > (also used in sensor_struct_traits.cc and maybe elsewhere) Done. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.cc (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:17: // implementations here. On 2016/08/18 22:52:14, timvolodine wrote: > NOTREACHED()? 'nullptr' just means that there is no available implementation for the current platform. Clients should/do anticipate this. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.h (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:5: #ifndef DEVICE_SENSORS_PLATFORM_SENSOR_PROVIDER_H_ On 2016/08/18 22:52:14, timvolodine wrote: > -> DEVICE_GENERIC_SENSOR_PLATFORM.. Done. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:18: static PlatformSensorProvider* Create(uint64_t shared_buffer_size); On 2016/08/18 22:52:14, timvolodine wrote: > is this a singleton? then seems better to have a GetInstance() method with > base::Singleton semantics (without parameter instead using > kSharedBufferSizeInBytes directly) Done. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.mojom:12: ACCELEROMETER, On 2016/08/18 22:52:14, timvolodine wrote: > maybe distinguish accelerometer with including/excluding gravity, ok to do in > separate patch.. yeah, this list will be updated when implementations of concrete sensors turn up. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.mojom:44: AddConfiguration(SensorConfiguration configuration) => (bool success); On 2016/08/18 22:52:14, timvolodine wrote: > maybe mention that this can fail if the config is not supported. Done. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.mojom:52: RemoveConfiguration(SensorConfiguration configuration) => (bool success); On 2016/08/18 22:52:14, timvolodine wrote: > not sure if a return value (=> bool success) is useful here. what would the > implementation do if the result is false? Go to 'errored' state. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.mojom:55: SuspendNotification(); On 2016/08/18 22:52:14, timvolodine wrote: > maybe just "Suspend()"? I understand this will stop sensor updates (and > potentially stop the platform sensor) Agree 'Suspend' is better as this call would affect platform sensor settings (not just notification mechanisms). Thanks for proposal. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/sensor_impl.cc (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/sensor_impl.cc:30: callback.Run(sensor_->StartListening(this, configuration)); On 2016/08/18 22:52:14, timvolodine wrote: > add TODO to avoid overflowing browser by repeated AddConfigs (maybe limit the > number of configs per client) Done. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/sensor_impl.cc:51: if (client_) On 2016/08/18 22:52:14, timvolodine wrote: > is client_ set anywhere? should this be a DCHECK? It might be null if hasn't been actually bound to a client implementation. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.h (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.h:35: static PlatformSensorProvider* s_provider_; On 2016/08/18 22:52:14, timvolodine wrote: > no need for this pointer if we have PlatformSensorProvider::GetInstance Good point. For some reason I did not realize that moving the static pointer inside the 'PlatformSensorProvider' class itself would make things simpler. Thanks for the proposal. https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... File device/generic_sensor/sensor_service.h (right): https://codereview.chromium.org/2144623003/diff/160001/device/generic_sensor/... device/generic_sensor/sensor_service.h:21: class SensorService { On 2016/08/18 19:36:00, Ken Rockot wrote: > I don't think we need SensorService or this h/cc file. Please just use > SensorProviderImpl directly in content 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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Description was changed from ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. Intent To implement mailing thread containing design description: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/4J... BUG=606766 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. Intent To implement mailing thread containing design description: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/4J... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
thanks! just a few more comments below https://codereview.chromium.org/2144623003/diff/160001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2144623003/diff/160001/content/browser/BUILD.... content/browser/BUILD.gn:65: "//device/generic_sensor", On 2016/08/19 09:29:55, Mikhail wrote: > On 2016/08/18 22:52:13, timvolodine wrote: > > we may want to have "enable_generic_sensor" for this (as e.g. enable_webrtc)? > > (to allow control for smaller chrome/webview footprint) > > We'd like to keep it as a runtime-enabled feature, so that try bots can compile > the code, and developers can easily enable it and provide early feedback for the > new API.. This would allow to enable on platform by platform basis when actual platform implementations are available. There is not much to feedback on if there is no implementation for a particular platform ;) I don't feel strongly about this though, guess android would be the most footprint-sensistive. https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensorInternal(const ConfigMap& configurations) = 0; is this platform dependent? would it make sense to move to platform provider to keep platform dependent implementations in one place (and have PlatformSensor as platform-independent) ? https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:28: return s_instance_; we usually use return base::Singleton<.... instead of having a member s_instance in chromium you could have a platform_sensors_provider_base.{h,cc} with shared code and then platform_sensor_provider.h deriving from it with platform-specific implementations in platform_sensor_provider_*.cc files. The one in this patch could be called platform_sensor_provider_default.cc. https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.mojom:57: // (e.g. |configuration| is not present in the Sensor's list). nit: |configuration| not present in the list seems like a programmatic DCHECK issue though (i.e. should never occur)? https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:28: if (provider) so what happens if provider is null? maybe better to always return a non-null provider that would fail to CreateSensor() in this case so that error callback is invoked?
https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensorInternal(const ConfigMap& configurations) = 0; On 2016/08/19 16:02:30, timvolodine wrote: > is this platform dependent? would it make sense to move to platform provider to > keep platform dependent implementations in one place (and have PlatformSensor as > platform-independent) ? I think now it happens like that already. PlatformSensor is generic class and all the platform-specific stuff is encapsulated in corresponding inherited classes, i.e. PlatformSensorAndroid. Everything within PlatformSensor itself is generic. https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:28: return s_instance_; On 2016/08/19 16:02:30, timvolodine wrote: > we usually use return base::Singleton<.... instead of having a member s_instance > in chromium > > you could have a platform_sensors_provider_base.{h,cc} with shared code and then > platform_sensor_provider.h deriving from it with platform-specific > implementations in platform_sensor_provider_*.cc files. The one in this patch > could be called platform_sensor_provider_default.cc. I was considering using of Singleton, but here I beleive it is not beneficial as we have singleton and factory at the same time, now it's gonna be PlatformSensorProvider* PlatformSensorProvider::GetInstance() { if (!s_instance_) { #if PLATFORM(ANDROID) s_instance_ = new PlatformSensorProviderAndroid(); #elif PLATFORM(WIN) s_instance_ = new PlatformSensorProviderWin(); #endif } return s_instance_; } with Singleton this requires custom traits class which IMO is an overkill. https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/public/interfaces/sensor.mojom:57: // (e.g. |configuration| is not present in the Sensor's list). On 2016/08/19 16:02:30, timvolodine wrote: > nit: |configuration| not present in the list seems like a programmatic DCHECK > issue though (i.e. should never occur)? Agree, but returning extra status will make it safer on release builds https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:28: if (provider) On 2016/08/19 16:02:30, timvolodine wrote: > so what happens if provider is null? maybe better to always return a non-null > provider that would fail to CreateSensor() in this case so that error callback > is invoked? blink part will be notified on very early stage via mojo mechanisms (connection_error_handler)
On 2016/08/19 19:10:03, Mikhail wrote: > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > File device/generic_sensor/platform_sensor.h (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > device/generic_sensor/platform_sensor.h:59: virtual bool > UpdateSensorInternal(const ConfigMap& configurations) = 0; > On 2016/08/19 16:02:30, timvolodine wrote: > > is this platform dependent? would it make sense to move to platform provider > to > > keep platform dependent implementations in one place (and have PlatformSensor > as > > platform-independent) ? > I think now it happens like that already. PlatformSensor is generic class and > all the platform-specific stuff is encapsulated in corresponding inherited > classes, i.e. PlatformSensorAndroid. > Everything within PlatformSensor itself is generic. > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_provider.cc (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > device/generic_sensor/platform_sensor_provider.cc:28: return s_instance_; > On 2016/08/19 16:02:30, timvolodine wrote: > > we usually use return base::Singleton<.... instead of having a member > s_instance > > in chromium > > > > you could have a platform_sensors_provider_base.{h,cc} with shared code and > then > > platform_sensor_provider.h deriving from it with platform-specific > > implementations in platform_sensor_provider_*.cc files. The one in this patch > > could be called platform_sensor_provider_default.cc. > > I was considering using of Singleton, but here I beleive it is not beneficial as > we have singleton and factory at the same time, now it's gonna be > > PlatformSensorProvider* PlatformSensorProvider::GetInstance() { > if (!s_instance_) { > #if PLATFORM(ANDROID) > s_instance_ = new PlatformSensorProviderAndroid(); > #elif PLATFORM(WIN) > s_instance_ = new PlatformSensorProviderWin(); > #endif > } > return s_instance_; > } > > with Singleton this requires custom traits class which IMO is an overkill. > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > File device/generic_sensor/public/interfaces/sensor.mojom (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > device/generic_sensor/public/interfaces/sensor.mojom:57: // (e.g. > |configuration| is not present in the Sensor's list). > On 2016/08/19 16:02:30, timvolodine wrote: > > nit: |configuration| not present in the list seems like a programmatic DCHECK > > issue though (i.e. should never occur)? > Agree, but returning extra status will make it safer on release builds > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > File device/generic_sensor/sensor_provider_impl.cc (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > device/generic_sensor/sensor_provider_impl.cc:28: if (provider) > On 2016/08/19 16:02:30, timvolodine wrote: > > so what happens if provider is null? maybe better to always return a non-null > > provider that would fail to CreateSensor() in this case so that error callback > > is invoked? > > blink part will be notified on very early stage via mojo mechanisms > (connection_error_handler)
The CQ bit was checked by mikhail.pozdnyakov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, dcheng@chromium.org, rockot@chromium.org, reillyg@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2144623003/#ps180001 (title: "Comments from Tim and Ken")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/19 19:10:03, Mikhail wrote: > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > File device/generic_sensor/platform_sensor.h (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > device/generic_sensor/platform_sensor.h:59: virtual bool > UpdateSensorInternal(const ConfigMap& configurations) = 0; > On 2016/08/19 16:02:30, timvolodine wrote: > > is this platform dependent? would it make sense to move to platform provider > to > > keep platform dependent implementations in one place (and have PlatformSensor > as > > platform-independent) ? > I think now it happens like that already. PlatformSensor is generic class and > all the platform-specific stuff is encapsulated in corresponding inherited > classes, i.e. PlatformSensorAndroid. > Everything within PlatformSensor itself is generic. > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_provider.cc (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > device/generic_sensor/platform_sensor_provider.cc:28: return s_instance_; > On 2016/08/19 16:02:30, timvolodine wrote: > > we usually use return base::Singleton<.... instead of having a member > s_instance > > in chromium > > > > you could have a platform_sensors_provider_base.{h,cc} with shared code and > then > > platform_sensor_provider.h deriving from it with platform-specific > > implementations in platform_sensor_provider_*.cc files. The one in this patch > > could be called platform_sensor_provider_default.cc. > > I was considering using of Singleton, but here I beleive it is not beneficial as > we have singleton and factory at the same time, now it's gonna be > > PlatformSensorProvider* PlatformSensorProvider::GetInstance() { > if (!s_instance_) { > #if PLATFORM(ANDROID) > s_instance_ = new PlatformSensorProviderAndroid(); > #elif PLATFORM(WIN) > s_instance_ = new PlatformSensorProviderWin(); > #endif > } > return s_instance_; > } > > with Singleton this requires custom traits class which IMO is an overkill. > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > File device/generic_sensor/public/interfaces/sensor.mojom (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > device/generic_sensor/public/interfaces/sensor.mojom:57: // (e.g. > |configuration| is not present in the Sensor's list). > On 2016/08/19 16:02:30, timvolodine wrote: > > nit: |configuration| not present in the list seems like a programmatic DCHECK > > issue though (i.e. should never occur)? > Agree, but returning extra status will make it safer on release builds > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > File device/generic_sensor/sensor_provider_impl.cc (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > device/generic_sensor/sensor_provider_impl.cc:28: if (provider) > On 2016/08/19 16:02:30, timvolodine wrote: > > so what happens if provider is null? maybe better to always return a non-null > > provider that would fail to CreateSensor() in this case so that error callback > > is invoked? > > blink part will be notified on very early stage via mojo mechanisms > (connection_error_handler) Thanks for comments, hopefully they are addressed. Asuming these are not blockers I dare to land the CL so that we can progress further. If needed we can include fixes to the following CLs.
https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensorInternal(const ConfigMap& configurations) = 0; On 2016/08/19 19:10:02, Mikhail wrote: > On 2016/08/19 16:02:30, timvolodine wrote: > > is this platform dependent? would it make sense to move to platform provider > to > > keep platform dependent implementations in one place (and have PlatformSensor > as > > platform-independent) ? > I think now it happens like that already. PlatformSensor is generic class and > all the platform-specific stuff is encapsulated in corresponding inherited > classes, i.e. PlatformSensorAndroid. > Everything within PlatformSensor itself is generic. I was actually suggesting to try avoid PlatformSensorAndroid completely if possible, and having platform specific functionality encapsulated in PlatformSensorProviderAndroid etc.. https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:28: return s_instance_; On 2016/08/19 19:10:02, Mikhail wrote: > On 2016/08/19 16:02:30, timvolodine wrote: > > we usually use return base::Singleton<.... instead of having a member > s_instance > > in chromium > > > > you could have a platform_sensors_provider_base.{h,cc} with shared code and > then > > platform_sensor_provider.h deriving from it with platform-specific > > implementations in platform_sensor_provider_*.cc files. The one in this patch > > could be called platform_sensor_provider_default.cc. > > I was considering using of Singleton, but here I beleive it is not beneficial as > we have singleton and factory at the same time, now it's gonna be > > PlatformSensorProvider* PlatformSensorProvider::GetInstance() { > if (!s_instance_) { > #if PLATFORM(ANDROID) > s_instance_ = new PlatformSensorProviderAndroid(); > #elif PLATFORM(WIN) > s_instance_ = new PlatformSensorProviderWin(); > #endif > } > return s_instance_; > } > > with Singleton this requires custom traits class which IMO is an overkill. > There is no need for #ifdefs when you follow the standard practice for multi-platform implementations in chromium using "split the implementation" approach described in https://www.chromium.org/developers/design-documents/conventions-and-patterns... There are many examples in the code base as well (e.g. device/sensors, device/battery to name a few). https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:28: if (provider) On 2016/08/19 19:10:02, Mikhail wrote: > On 2016/08/19 16:02:30, timvolodine wrote: > > so what happens if provider is null? maybe better to always return a non-null > > provider that would fail to CreateSensor() in this case so that error callback > > is invoked? > > blink part will be notified on very early stage via mojo mechanisms > (connection_error_handler) wouldn't that result in duplicate code though? empty provider implementation would simply use the !sensor code path.. also easier to distinguish between empty platform or something wrong with mojo connection, just saying.
The CQ bit was unchecked by mikhail.pozdnyakov@intel.com
https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensorInternal(const ConfigMap& configurations) = 0; On 2016/08/22 18:42:46, timvolodine wrote: > On 2016/08/19 19:10:02, Mikhail wrote: > > On 2016/08/19 16:02:30, timvolodine wrote: > > > is this platform dependent? would it make sense to move to platform provider > > to > > > keep platform dependent implementations in one place (and have > PlatformSensor > > as > > > platform-independent) ? > > I think now it happens like that already. PlatformSensor is generic class and > > all the platform-specific stuff is encapsulated in corresponding inherited > > classes, i.e. PlatformSensorAndroid. > > Everything within PlatformSensor itself is generic. > > I was actually suggesting to try avoid PlatformSensorAndroid completely if > possible, and having platform specific functionality encapsulated in > PlatformSensorProviderAndroid etc.. These two classes have different roles and each is supposed to have platform-specific implementation: PlatformSensorProviderAndroid is a factory-like class that creates sensors by given type -- it is wrapping SensorManager (https://developer.android.com/reference/android/hardware/SensorManager.html); PlatformSensorAndroid is responsible for managing a concrete device sensor (of a specific type) so it contains all the sensor platform data for this sensor -- it is wrapping Sensor (https://developer.android.com/reference/android/hardware/Sensor.html) What would be the place for this functionality in case we don't have 'PlatformSensorAndroid'? https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:28: if (provider) On 2016/08/22 18:42:46, timvolodine wrote: > On 2016/08/19 19:10:02, Mikhail wrote: > > On 2016/08/19 16:02:30, timvolodine wrote: > > > so what happens if provider is null? maybe better to always return a > non-null > > > provider that would fail to CreateSensor() in this case so that error > callback > > > is invoked? > > > > blink part will be notified on very early stage via mojo mechanisms > > (connection_error_handler) > > wouldn't that result in duplicate code though? mm.. why would it cause code duplication? > would simply use the !sensor code path.. also easier to distinguish between > empty platform or something wrong with mojo connection, just saying. From API POV we do not need to distinguish, Sensor will switch to 'errored' state in any of these cases. My point here is to return/notify error as soon as we know that we cannot proceed (no service available).
https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:28: return s_instance_; On 2016/08/22 18:42:46, timvolodine wrote: > On 2016/08/19 19:10:02, Mikhail wrote: > > On 2016/08/19 16:02:30, timvolodine wrote: > > > we usually use return base::Singleton<.... instead of having a member > > s_instance > > > in chromium > > > > > > you could have a platform_sensors_provider_base.{h,cc} with shared code and > > then > > > platform_sensor_provider.h deriving from it with platform-specific > > > implementations in platform_sensor_provider_*.cc files. The one in this > patch > > > could be called platform_sensor_provider_default.cc. > > > > I was considering using of Singleton, but here I beleive it is not beneficial > as > > we have singleton and factory at the same time, now it's gonna be > > > > PlatformSensorProvider* PlatformSensorProvider::GetInstance() { > > if (!s_instance_) { > > #if PLATFORM(ANDROID) > > s_instance_ = new PlatformSensorProviderAndroid(); > > #elif PLATFORM(WIN) > > s_instance_ = new PlatformSensorProviderWin(); > > #endif > > } > > return s_instance_; > > } > > > > with Singleton this requires custom traits class which IMO is an overkill. > > > > There is no need for #ifdefs when you follow the standard practice for > multi-platform implementations in chromium using "split the implementation" > approach described in > https://www.chromium.org/developers/design-documents/conventions-and-patterns... > > There are many examples in the code base as well (e.g. device/sensors, > device/battery to name a few). Thanks for the links! so the solution could be to add to each of the platform specific files its own "PlatformSensorProvider::GetInstance()" definition, this is indeed more elegant and allows to get rid of #ifdefs. I'll update the patch accordingly.
https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensorInternal(const ConfigMap& configurations) = 0; On 2016/08/22 19:07:50, Mikhail wrote: > On 2016/08/22 18:42:46, timvolodine wrote: > > On 2016/08/19 19:10:02, Mikhail wrote: > > > On 2016/08/19 16:02:30, timvolodine wrote: > > > > is this platform dependent? would it make sense to move to platform > provider > > > to > > > > keep platform dependent implementations in one place (and have > > PlatformSensor > > > as > > > > platform-independent) ? > > > I think now it happens like that already. PlatformSensor is generic class > and > > > all the platform-specific stuff is encapsulated in corresponding inherited > > > classes, i.e. PlatformSensorAndroid. > > > Everything within PlatformSensor itself is generic. > > > > I was actually suggesting to try avoid PlatformSensorAndroid completely if > > possible, and having platform specific functionality encapsulated in > > PlatformSensorProviderAndroid etc.. > These two classes have different roles and each is supposed to have > platform-specific implementation: > PlatformSensorProviderAndroid is a factory-like class that creates sensors by > given type -- it is wrapping SensorManager > (https://developer.android.com/reference/android/hardware/SensorManager.html); > > PlatformSensorAndroid is responsible for managing a concrete device sensor (of a > specific type) so it contains all the sensor platform data for this sensor -- it > is wrapping Sensor > (https://developer.android.com/reference/android/hardware/Sensor.html) > What would be the place for this functionality in case we don't have > 'PlatformSensorAndroid'? I was thinking letting the PlatformSensorProvider handle some platform-specific sensor functionality. But it's up to you, whatever you think is best. https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.cc:28: return s_instance_; On 2016/08/22 19:16:15, Mikhail wrote: > On 2016/08/22 18:42:46, timvolodine wrote: > > On 2016/08/19 19:10:02, Mikhail wrote: > > > On 2016/08/19 16:02:30, timvolodine wrote: > > > > we usually use return base::Singleton<.... instead of having a member > > > s_instance > > > > in chromium > > > > > > > > you could have a platform_sensors_provider_base.{h,cc} with shared code > and > > > then > > > > platform_sensor_provider.h deriving from it with platform-specific > > > > implementations in platform_sensor_provider_*.cc files. The one in this > > patch > > > > could be called platform_sensor_provider_default.cc. > > > > > > I was considering using of Singleton, but here I beleive it is not > beneficial > > as > > > we have singleton and factory at the same time, now it's gonna be > > > > > > PlatformSensorProvider* PlatformSensorProvider::GetInstance() { > > > if (!s_instance_) { > > > #if PLATFORM(ANDROID) > > > s_instance_ = new PlatformSensorProviderAndroid(); > > > #elif PLATFORM(WIN) > > > s_instance_ = new PlatformSensorProviderWin(); > > > #endif > > > } > > > return s_instance_; > > > } > > > > > > with Singleton this requires custom traits class which IMO is an overkill. > > > > > > > There is no need for #ifdefs when you follow the standard practice for > > multi-platform implementations in chromium using "split the implementation" > > approach described in > > > https://www.chromium.org/developers/design-documents/conventions-and-patterns... > > > > There are many examples in the code base as well (e.g. device/sensors, > > device/battery to name a few). > > Thanks for the links! so the solution could be to add to each of the platform > specific files its own "PlatformSensorProvider::GetInstance()" definition, this > is indeed more elegant and allows to get rid of #ifdefs. I'll update the patch > accordingly. yes please, thanks ;) https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:28: if (provider) On 2016/08/22 19:07:50, Mikhail wrote: > On 2016/08/22 18:42:46, timvolodine wrote: > > On 2016/08/19 19:10:02, Mikhail wrote: > > > On 2016/08/19 16:02:30, timvolodine wrote: > > > > so what happens if provider is null? maybe better to always return a > > non-null > > > > provider that would fail to CreateSensor() in this case so that error > > callback > > > > is invoked? > > > > > > blink part will be notified on very early stage via mojo mechanisms > > > (connection_error_handler) > > > > wouldn't that result in duplicate code though? > mm.. why would it cause code duplication? > > > would simply use the !sensor code path.. also easier to distinguish between > > empty platform or something wrong with mojo connection, just saying. > From API POV we do not need to distinguish, Sensor will switch to 'errored' > state in any of these cases. > My point here is to return/notify error as soon as we know that we cannot > proceed (no service available). I guess it's a question of semantics, so up to you to decide. (having something with potential null value usually results in more 'special' cases though)
On 2016/08/22 19:16:15, Mikhail wrote: > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_provider.cc (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > device/generic_sensor/platform_sensor_provider.cc:28: return s_instance_; > On 2016/08/22 18:42:46, timvolodine wrote: > > There is no need for #ifdefs when you follow the standard practice for > > multi-platform implementations in chromium using "split the implementation" > > approach described in > > > https://www.chromium.org/developers/design-documents/conventions-and-patterns... > > > > There are many examples in the code base as well (e.g. device/sensors, > > device/battery to name a few). > > Thanks for the links! so the solution could be to add to each of the platform > specific files its own "PlatformSensorProvider::GetInstance()" definition, this > is indeed more elegant and allows to get rid of #ifdefs. I'll update the patch > accordingly. In the latest patch platform_sensor_provider_default.cc has a definition for PlatformSensorProvider::GetInstance() that just returns a nullptr. platform_sensor_provider_android.cc will have one returning 'base::Singleton<PlatformSensorProviderAndroid, base::LeakySingletonTraits<PlatformSensorProviderAndroid>>::get();' so no #ifdefs and static pointer member in 'PlatformSensorProvider'.
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/22 19:51:12, Mikhail wrote: > On 2016/08/22 19:16:15, Mikhail wrote: > > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > > File device/generic_sensor/platform_sensor_provider.cc (right): > > > > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_provider.cc:28: return s_instance_; > > On 2016/08/22 18:42:46, timvolodine wrote: > > > There is no need for #ifdefs when you follow the standard practice for > > > multi-platform implementations in chromium using "split the implementation" > > > approach described in > > > > > > https://www.chromium.org/developers/design-documents/conventions-and-patterns... > > > > > > There are many examples in the code base as well (e.g. device/sensors, > > > device/battery to name a few). > > > > Thanks for the links! so the solution could be to add to each of the platform > > specific files its own "PlatformSensorProvider::GetInstance()" definition, > this > > is indeed more elegant and allows to get rid of #ifdefs. I'll update the patch > > accordingly. > > In the latest patch platform_sensor_provider_default.cc has a definition for > PlatformSensorProvider::GetInstance() that just returns a nullptr. > platform_sensor_provider_android.cc will have one returning > 'base::Singleton<PlatformSensorProviderAndroid, > base::LeakySingletonTraits<PlatformSensorProviderAndroid>>::get();' so no > #ifdefs and static pointer member in 'PlatformSensorProvider'. thanks, there should be no platform_sensor_provider.cc if you have platform_sensor_provider_default.cc (otherwise it's confusing). (you could put the code in a common base class as I already mentioned)
On 2016/08/23 14:40:46, timvolodine wrote: > thanks, there should be no platform_sensor_provider.cc if you have > platform_sensor_provider_default.cc (otherwise it's confusing). (you could put > the code in a common base class as I already mentioned) Right, I should have added common base class. Fixed now, thanks!
thanks for following through with the changes! lgtm https://codereview.chromium.org/2144623003/diff/220001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider.h (right): https://codereview.chromium.org/2144623003/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider.h:22: PlatformSensorProvider(); think you'll need implementations for these
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 rockot@chromium.org, reillyg@chromium.org, kinuko@chromium.org, dcheng@chromium.org, tsepez@chromium.org, timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/2144623003/#ps240001 (title: "Definition for PlatformSensorProvider ctor/dtor")
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] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. Intent To implement mailing thread containing design description: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/4J... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. Intent To implement mailing thread containing design description: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/4J... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. Intent To implement mailing thread containing design description: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/4J... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [sensors] Introduce Generic Sensor API interfaces Authors: alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com New generic sensor mojo interfaces are introduced together with platform independent implementations. Intent To implement mailing thread containing design description: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/4J... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/66ac3d4e8a635db673ff6dd47cc59976ee0b956c Cr-Commit-Position: refs/heads/master@{#413792} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/66ac3d4e8a635db673ff6dd47cc59976ee0b956c Cr-Commit-Position: refs/heads/master@{#413792} |
