Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(638)

Issue 2144623003: [sensors] Introduce Generic Sensor API interfaces (Closed)

Created:
4 years, 5 months ago by Mikhail
Modified:
4 years, 4 months ago
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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+903 lines, -8 lines) Patch
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/DEPS View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -0 lines 0 comments Download
M content/public/common/content_switches.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_switches.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
A device/generic_sensor/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +28 lines, -0 lines 0 comments Download
A + device/generic_sensor/OWNERS View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
A device/generic_sensor/platform_sensor.h View 1 2 3 4 5 6 7 1 chunk +81 lines, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor.cc View 1 2 3 4 5 6 7 1 chunk +91 lines, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor_configuration.h View 1 2 3 4 5 6 7 8 1 chunk +36 lines, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor_configuration.cc View 1 2 3 4 5 6 7 8 1 chunk +22 lines, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor_provider.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +30 lines, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor_provider_base.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +60 lines, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor_provider_base.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +86 lines, -0 lines 0 comments Download
A + device/generic_sensor/platform_sensor_provider_default.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -4 lines 0 comments Download
A + device/generic_sensor/public/interfaces/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
A + device/generic_sensor/public/interfaces/OWNERS View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
A device/generic_sensor/public/interfaces/sensor.mojom View 1 2 3 4 5 6 7 8 1 chunk +78 lines, -0 lines 0 comments Download
A device/generic_sensor/public/interfaces/sensor.typemap View 1 2 3 4 5 1 chunk +16 lines, -0 lines 0 comments Download
A device/generic_sensor/public/interfaces/sensor_provider.mojom View 1 2 3 4 5 6 7 1 chunk +40 lines, -0 lines 0 comments Download
A device/generic_sensor/public/interfaces/sensor_struct_traits.h View 1 2 3 4 5 1 chunk +26 lines, -0 lines 0 comments Download
A device/generic_sensor/public/interfaces/sensor_struct_traits.cc View 1 2 3 4 5 6 7 8 1 chunk +25 lines, -0 lines 0 comments Download
A + device/generic_sensor/public/interfaces/typemaps.gni View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
A device/generic_sensor/sensor_export.h View 1 2 3 4 1 chunk +29 lines, -0 lines 0 comments Download
A device/generic_sensor/sensor_impl.h View 1 2 3 4 5 6 7 8 1 chunk +50 lines, -0 lines 0 comments Download
A device/generic_sensor/sensor_impl.cc View 1 2 3 4 5 6 7 8 1 chunk +67 lines, -0 lines 0 comments Download
A device/generic_sensor/sensor_provider_impl.h View 1 2 3 4 5 6 7 8 1 chunk +43 lines, -0 lines 0 comments Download
A device/generic_sensor/sensor_provider_impl.cc View 1 2 3 4 5 6 7 8 1 chunk +71 lines, -0 lines 0 comments Download
M mojo/public/tools/bindings/chromium_bindings_configuration.gni View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 118 (67 generated)
Tom Sepez
Ok, I can give a LGTM on my part of it. Adding the other reviewers ...
4 years, 5 months ago (2016-07-13 19:01:48 UTC) #4
Mikhail
On 2016/07/13 19:01:48, Tom Sepez wrote: > Ok, I can give a LGTM on my ...
4 years, 5 months ago (2016-07-13 19:16:06 UTC) #8
dcheng
https://codereview.chromium.org/2144623003/diff/40001/device/sensors/platform_sensor.cc File device/sensors/platform_sensor.cc (right): https://codereview.chromium.org/2144623003/diff/40001/device/sensors/platform_sensor.cc#newcode18 device/sensors/platform_sensor.cc:18: return config.frequency() <= 60.0 && config.frequency() > 0.0; As ...
4 years, 5 months ago (2016-07-14 02:50:27 UTC) #9
Mikhail
On 2016/07/14 02:50:27, dcheng wrote: > https://codereview.chromium.org/2144623003/diff/40001/device/sensors/platform_sensor.cc > File device/sensors/platform_sensor.cc (right): > > https://codereview.chromium.org/2144623003/diff/40001/device/sensors/platform_sensor.cc#newcode18 > ...
4 years, 5 months ago (2016-07-14 08:40:35 UTC) #10
dcheng
lgtm https://codereview.chromium.org/2144623003/diff/60001/device/sensors/public/interfaces/sensor_struct_traits.cc File device/sensors/public/interfaces/sensor_struct_traits.cc (right): https://codereview.chromium.org/2144623003/diff/60001/device/sensors/public/interfaces/sensor_struct_traits.cc#newcode20 device/sensors/public/interfaces/sensor_struct_traits.cc:20: NOTREACHED(); Nit: this shouldn't be NOTREACHED(), as it's ...
4 years, 5 months ago (2016-07-14 09:24:10 UTC) #11
Mikhail
On 2016/07/14 09:24:10, dcheng wrote: > lgtm > > https://codereview.chromium.org/2144623003/diff/60001/device/sensors/public/interfaces/sensor_struct_traits.cc > File device/sensors/public/interfaces/sensor_struct_traits.cc (right): > ...
4 years, 5 months ago (2016-07-14 10:09:57 UTC) #12
timvolodine
Thanks for uploading the renderer/blink side, easier to see how it's used. A couple more ...
4 years, 5 months ago (2016-07-22 19:31:45 UTC) #13
Reilly Grant (use Gerrit)
My apologies for taking so long to review //device/sensor. https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_export.h File device/sensors/sensor_export.h (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/sensor_export.h#newcode1 device/sensors/sensor_export.h:1: ...
4 years, 4 months ago (2016-07-27 22:07:58 UTC) #14
Mikhail
On 2016/07/22 19:31:45, timvolodine wrote: > Thanks for uploading the renderer/blink side, easier to see ...
4 years, 4 months ago (2016-08-08 14:02:15 UTC) #15
Mikhail
Thanks for your comments! https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/interfaces/sensor.mojom File device/sensors/public/interfaces/sensor.mojom (right): https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/interfaces/sensor.mojom#newcode31 device/sensors/public/interfaces/sensor.mojom:31: // Frequency in Hz On ...
4 years, 4 months ago (2016-08-09 19:38:28 UTC) #16
commit-bot: I haz the power
Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...
4 years, 4 months ago (2016-08-10 06:30:25 UTC) #19
commit-bot: I haz the power
Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...
4 years, 4 months ago (2016-08-10 07:11:05 UTC) #24
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2144623003/diff/100001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2144623003/diff/100001/content/browser/frame_host/render_frame_host_impl.cc#newcode2149 content/browser/frame_host/render_frame_host_impl.cc:2149: base::RetainedRef( base::RetainedRef() is not necessary here as GetTaskRunnerForThread() already ...
4 years, 4 months ago (2016-08-10 18:00:19 UTC) #27
Mikhail
https://codereview.chromium.org/2144623003/diff/100001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2144623003/diff/100001/content/browser/frame_host/render_frame_host_impl.cc#newcode2149 content/browser/frame_host/render_frame_host_impl.cc:2149: base::RetainedRef( On 2016/08/10 18:00:19, Reilly Grant wrote: > base::RetainedRef() ...
4 years, 4 months ago (2016-08-10 18:16:51 UTC) #28
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/platform_sensor.cc File device/generic_sensor/platform_sensor.cc (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/platform_sensor.cc#newcode44 device/generic_sensor/platform_sensor.cc:44: config_list.erase(iterator); Can this be replaced by config_list.remove(config)? Do you ...
4 years, 4 months ago (2016-08-11 20:59:37 UTC) #29
Mikhail
https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/platform_sensor.cc File device/generic_sensor/platform_sensor.cc (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/platform_sensor.cc#newcode44 device/generic_sensor/platform_sensor.cc:44: config_list.erase(iterator); On 2016/08/11 20:59:37, Reilly Grant wrote: > Can ...
4 years, 4 months ago (2016-08-12 10:21:49 UTC) #34
Reilly Grant (use Gerrit)
lgtm https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/sensor_provider_impl.cc File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/sensor_provider_impl.cc#newcode21 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 ...
4 years, 4 months ago (2016-08-12 18:01:59 UTC) #35
Mikhail
On 2016/08/12 18:01:59, Reilly Grant wrote: > lgtm > > https://codereview.chromium.org/2144623003/diff/140001/device/generic_sensor/sensor_provider_impl.cc > File device/generic_sensor/sensor_provider_impl.cc (right): ...
4 years, 4 months ago (2016-08-15 06:44:24 UTC) #36
Mikhail
@pfeldman, @kinuko could you pls have a look at content/ changes (since jochen is OOO)?
4 years, 4 months ago (2016-08-15 07:33:46 UTC) #38
timvolodine
> https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/interfaces/sensor.mojom#newcode44 > > device/sensors/public/interfaces/sensor.mojom:44: > > AddConfiguration(SensorConfiguration configuration) => (bool success); > > Does ...
4 years, 4 months ago (2016-08-15 19:35:22 UTC) #39
Mikhail
On 2016/08/15 19:35:22, timvolodine wrote: > > > https://codereview.chromium.org/2144623003/diff/80001/device/sensors/public/interfaces/sensor.mojom#newcode44 > > > device/sensors/public/interfaces/sensor.mojom:44: > > ...
4 years, 4 months ago (2016-08-15 22:16:46 UTC) #40
kinuko
content/ lgtm
4 years, 4 months ago (2016-08-15 23:08:01 UTC) #41
timvolodine
On 2016/08/15 22:16:46, Mikhail wrote: > On 2016/08/15 19:35:22, timvolodine wrote: > > > > ...
4 years, 4 months ago (2016-08-16 12:52:11 UTC) #50
Mikhail
On 2016/08/16 12:52:11, timvolodine wrote: > > What kind of security reasons? generally speaking the ...
4 years, 4 months ago (2016-08-16 13:28:30 UTC) #51
Mikhail
On 2016/08/16 13:28:30, Mikhail wrote: > On 2016/08/16 12:52:11, timvolodine wrote: > > > > ...
4 years, 4 months ago (2016-08-16 14:45:48 UTC) #59
timvolodine
On 2016/08/16 13:28:30, Mikhail wrote: > On 2016/08/16 12:52:11, timvolodine wrote: > > > > ...
4 years, 4 months ago (2016-08-16 15:51:20 UTC) #60
Mikhail
Thanks for your comments and proposals. On 2016/08/16 15:51:20, timvolodine wrote: > Actually, regarding security ...
4 years, 4 months ago (2016-08-17 11:09:19 UTC) #62
timvolodine
On 2016/08/17 11:09:19, Mikhail wrote: > Thanks for your comments and proposals. > On 2016/08/16 ...
4 years, 4 months ago (2016-08-17 13:13:39 UTC) #63
Mikhail
On 2016/08/17 13:13:39, timvolodine wrote: > What I am more concerned about is the creation/deletion ...
4 years, 4 months ago (2016-08-17 14:34:25 UTC) #64
Mikhail
On 2016/08/17 13:13:39, timvolodine wrote: > What I am more concerned about is the creation/deletion ...
4 years, 4 months ago (2016-08-17 14:36:54 UTC) #65
Mikhail
On 2016/08/17 13:13:39, timvolodine wrote: > not sure why we should be worried about exporting ...
4 years, 4 months ago (2016-08-18 14:28:24 UTC) #66
Ken Rockot(use gerrit already)
lgtm https://codereview.chromium.org/2144623003/diff/160001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2144623003/diff/160001/content/browser/frame_host/render_frame_host_impl.cc#newcode2147 content/browser/frame_host/render_frame_host_impl.cc:2147: GetInterfaceRegistry()->AddInterface(base::Bind( AddInterface() itself can take a task runner ...
4 years, 4 months ago (2016-08-18 19:36:00 UTC) #67
timvolodine
On 2016/08/18 14:28:24, Mikhail wrote: > On 2016/08/17 13:13:39, timvolodine wrote: > > not sure ...
4 years, 4 months ago (2016-08-18 22:48:52 UTC) #68
timvolodine
A few comments below. Would be nice to have some unit testing at some stage. ...
4 years, 4 months ago (2016-08-18 22:52:14 UTC) #69
Mikhail
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.gn#newcode65 content/browser/BUILD.gn:65: "//device/generic_sensor", On 2016/08/18 22:52:13, timvolodine wrote: > we may ...
4 years, 4 months ago (2016-08-19 09:29:56 UTC) #74
timvolodine
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.gn#newcode65 content/browser/BUILD.gn:65: "//device/generic_sensor", On ...
4 years, 4 months ago (2016-08-19 16:02:31 UTC) #88
Mikhail
https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h#newcode59 device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensorInternal(const ConfigMap& configurations) = 0; On 2016/08/19 ...
4 years, 4 months ago (2016-08-19 19:10:03 UTC) #89
Mikhail
On 2016/08/19 19:10:03, Mikhail wrote: > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h > File device/generic_sensor/platform_sensor.h (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h#newcode59 > ...
4 years, 4 months ago (2016-08-21 12:05:51 UTC) #90
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2144623003/180001
4 years, 4 months ago (2016-08-22 18:41:20 UTC) #93
Mikhail
On 2016/08/19 19:10:03, Mikhail wrote: > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h > File device/generic_sensor/platform_sensor.h (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h#newcode59 > ...
4 years, 4 months ago (2016-08-22 18:41:34 UTC) #94
timvolodine
https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h#newcode59 device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensorInternal(const ConfigMap& configurations) = 0; On 2016/08/19 ...
4 years, 4 months ago (2016-08-22 18:42:46 UTC) #95
Mikhail
https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h#newcode59 device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensorInternal(const ConfigMap& configurations) = 0; On 2016/08/22 ...
4 years, 4 months ago (2016-08-22 19:07:50 UTC) #97
Mikhail
https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor_provider.cc File device/generic_sensor/platform_sensor_provider.cc (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor_provider.cc#newcode28 device/generic_sensor/platform_sensor_provider.cc:28: return s_instance_; On 2016/08/22 18:42:46, timvolodine wrote: > On ...
4 years, 4 months ago (2016-08-22 19:16:15 UTC) #98
timvolodine
https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor.h#newcode59 device/generic_sensor/platform_sensor.h:59: virtual bool UpdateSensorInternal(const ConfigMap& configurations) = 0; On 2016/08/22 ...
4 years, 4 months ago (2016-08-22 19:42:25 UTC) #99
Mikhail
On 2016/08/22 19:16:15, Mikhail wrote: > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor_provider.cc > File device/generic_sensor/platform_sensor_provider.cc (right): > > https://codereview.chromium.org/2144623003/diff/180001/device/generic_sensor/platform_sensor_provider.cc#newcode28 > ...
4 years, 4 months ago (2016-08-22 19:51:12 UTC) #100
timvolodine
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/platform_sensor_provider.cc ...
4 years, 4 months ago (2016-08-23 14:40:46 UTC) #105
Mikhail
On 2016/08/23 14:40:46, timvolodine wrote: > thanks, there should be no platform_sensor_provider.cc if you have ...
4 years, 4 months ago (2016-08-23 15:35:20 UTC) #106
timvolodine
thanks for following through with the changes! lgtm https://codereview.chromium.org/2144623003/diff/220001/device/generic_sensor/platform_sensor_provider.h File device/generic_sensor/platform_sensor_provider.h (right): https://codereview.chromium.org/2144623003/diff/220001/device/generic_sensor/platform_sensor_provider.h#newcode22 device/generic_sensor/platform_sensor_provider.h:22: PlatformSensorProvider(); ...
4 years, 4 months ago (2016-08-23 15:48:33 UTC) #107
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2144623003/240001
4 years, 4 months ago (2016-08-23 18:21:51 UTC) #114
commit-bot: I haz the power
Committed patchset #12 (id:240001)
4 years, 4 months ago (2016-08-23 18:27:36 UTC) #116
commit-bot: I haz the power
4 years, 4 months ago (2016-08-23 18:29:57 UTC) #118
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/66ac3d4e8a635db673ff6dd47cc59976ee0b956c
Cr-Commit-Position: refs/heads/master@{#413792}

Powered by Google App Engine
This is Rietveld 408576698