Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/254443)
https://codereview.chromium.org/2471003002/diff/1/third_party/WebKit/LayoutTests/sensor/accelerometer.html File third_party/WebKit/LayoutTests/sensor/accelerometer.html (right): https://codereview.chromium.org/2471003002/diff/1/third_party/WebKit/LayoutTests/sensor/accelerometer.html#newcode1 third_party/WebKit/LayoutTests/sensor/accelerometer.html:1: <!DOCTYPE html> We had a plan to make generic ...
On 2016/11/02 at 14:25:14, alexander.shalamov wrote:
>
https://codereview.chromium.org/2471003002/diff/1/third_party/WebKit/LayoutTe...
> File third_party/WebKit/LayoutTests/sensor/accelerometer.html (right):
>
>
https://codereview.chromium.org/2471003002/diff/1/third_party/WebKit/LayoutTe...
> third_party/WebKit/LayoutTests/sensor/accelerometer.html:1: <!DOCTYPE html>
> We had a plan to make generic list of tests when second binding is added.
Basically, all tests from ambient-light-sensor.html can be moved to e.g.
generic-tests.js, then, ambient-light-sensor.html or accelerometer.html will use
it and provide:
>
> 1. function that constructs sensor
> 2. function that populates buffer
> 3. function that compares received reading values
>
> Then, it would be easy to add / remove tests for all GS API based sensors.
>
>
https://codereview.chromium.org/2471003002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/sensor/Accelerometer.cpp (right):
>
>
https://codereview.chromium.org/2471003002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/sensor/Accelerometer.cpp:30: :
Sensor(scriptState, options, exceptionState, SensorType::ACCELEROMETER),
> Sensor(scriptState, options, exceptionState, options.includeGravity() ?
SensorType::ACCELEROMETER : SensorType::LINEAR_ACCELERATION)
New version uploaded.
darktears
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Description was changed from ========== [sensors] Accelerometer sensor bindings implementation This patch implements Accelerometer [1] ...
4 years, 1 month ago
(2016-11-02 22:48:31 UTC)
#11
Description was changed from
==========
[sensors] Accelerometer sensor bindings implementation
This patch implements Accelerometer [1] blink bindings..
Following layout tests added to test new functionality:
IDL tests.
- third_party/WebKit/LayoutTests/sensor/idl-Accelerometer.html
Accelerometer tests.
- third_party/WebKit/LayoutTests/sensor/accelerometer.html
Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_IReUkNKF6o
[1] ED specification for Accelerometer https://w3c.github.io/accelerometer/
BUG=606766
==========
to
==========
[sensors] Accelerometer sensor bindings implementation
This patch implements Accelerometer [1] blink bindings.
Following layout tests added to test new functionality:
IDL tests.
- third_party/WebKit/LayoutTests/sensor/idl-Accelerometer.html
Accelerometer tests.
- third_party/WebKit/LayoutTests/sensor/accelerometer.html
Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_IReUkNKF6o
[1] ED specification for Accelerometer https://w3c.github.io/accelerometer/
Based on patch from Alexander Shalamov <alexander.shalamov@intel.com>
BUG=661478
==========
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 1 month ago
(2016-11-02 23:12:50 UTC)
#12
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_ng/builds/324082)
4 years, 1 month ago
(2016-11-02 23:12:50 UTC)
#13
4 years, 1 month ago
(2016-11-03 17:11:41 UTC)
#20
Dry run: This issue passed the CQ dry run.
shalamov
Alexis, did you check why accelerometer tests are failing?
4 years, 1 month ago
(2016-11-04 09:20:23 UTC)
#21
Alexis, did you check why accelerometer tests are failing?
darktears
On 2016/11/04 at 09:20:23, alexander.shalamov wrote: > Alexis, did you check why accelerometer tests are ...
4 years, 1 month ago
(2016-11-04 15:13:19 UTC)
#22
On 2016/11/04 at 09:20:23, alexander.shalamov wrote:
> Alexis, did you check why accelerometer tests are failing?
what do you mean? If you're talking about the new test, it passes here.
On 2016/11/05 at 12:30:38, haraken wrote: > ... > > esprehn: Not directly related to ...
4 years, 1 month ago
(2016-11-05 22:06:46 UTC)
#27
On 2016/11/05 at 12:30:38, haraken wrote:
> ...
>
> esprehn: Not directly related to this CL, is it wrong to add a direct
dependency on cpp files in //device? i.e., should we use generated mojo bindings
instead?
Yeah, we shouldn't include anything from device that isn't a wtf mojo binding.
I'm not sure I understand the purpose of these cpp files, they look like structs
for IPC which is what the mojoms are for?
In general though we shouldn't depend on these external headers and should use
the mojo bindings unless something automated is enforcing that they never
contain std::string, vector, map, anything from base, etc.
You could depend on them inside platform if you want to build wrappers around
them like in the past, though that's a bit silly since the mojo wtf bindings are
designed to avoid that duplication. :)
The other places we use device/ all use mojo bindings today though, what's
different about this api?
Mikhail
On 2016/11/05 22:06:46, esprehn wrote: > On 2016/11/05 at 12:30:38, haraken wrote: > > ...
4 years, 1 month ago
(2016-11-07 07:33:27 UTC)
#28
On 2016/11/05 22:06:46, esprehn wrote:
> On 2016/11/05 at 12:30:38, haraken wrote:
> > ...
> >
> > esprehn: Not directly related to this CL, is it wrong to add a direct
> dependency on cpp files in //device? i.e., should we use generated mojo
bindings
> instead?
>
>
> Yeah, we shouldn't include anything from device that isn't a wtf mojo binding.
>
> I'm not sure I understand the purpose of these cpp files, they look like
structs
> for IPC which is what the mojoms are for?
>
> In general though we shouldn't depend on these external headers and should use
> the mojo bindings unless something automated is enforcing that they never
> contain std::string, vector, map, anything from base, etc.
>
> You could depend on them inside platform if you want to build wrappers around
> them like in the past, though that's a bit silly since the mojo wtf bindings
are
> designed to avoid that duplication. :)
>
> The other places we use device/ all use mojo bindings today though, what's
> different about this api?
The reason for having this dependency is the classes that describe shared buffer
layout for a sensor reading
(https://cs.chromium.org/chromium/src/device/generic_sensor/public/cpp/sensor_...)
and that are used on both sides: platform and blink.
The shared buffer layout for a single sensor type on every platform contains 5
64-bits values as following { seqlock, timestamp, values[3] }.
haraken
On 2016/11/07 07:33:27, Mikhail wrote: > On 2016/11/05 22:06:46, esprehn wrote: > > On 2016/11/05 ...
4 years, 1 month ago
(2016-11-07 10:02:22 UTC)
#29
On 2016/11/07 07:33:27, Mikhail wrote:
> On 2016/11/05 22:06:46, esprehn wrote:
> > On 2016/11/05 at 12:30:38, haraken wrote:
> > > ...
> > >
> > > esprehn: Not directly related to this CL, is it wrong to add a direct
> > dependency on cpp files in //device? i.e., should we use generated mojo
> bindings
> > instead?
> >
> >
> > Yeah, we shouldn't include anything from device that isn't a wtf mojo
binding.
> >
> > I'm not sure I understand the purpose of these cpp files, they look like
> structs
> > for IPC which is what the mojoms are for?
> >
> > In general though we shouldn't depend on these external headers and should
use
> > the mojo bindings unless something automated is enforcing that they never
> > contain std::string, vector, map, anything from base, etc.
> >
> > You could depend on them inside platform if you want to build wrappers
around
> > them like in the past, though that's a bit silly since the mojo wtf bindings
> are
> > designed to avoid that duplication. :)
> >
> > The other places we use device/ all use mojo bindings today though, what's
> > different about this api?
>
> The reason for having this dependency is the classes that describe shared
buffer
> layout for a sensor reading
>
(https://cs.chromium.org/chromium/src/device/generic_sensor/public/cpp/sensor_...)
> and that are used on both sides: platform and blink.
> The shared buffer layout for a single sensor type on every platform contains 5
> 64-bits values as following { seqlock, timestamp, values[3] }.
Why
haraken
On 2016/11/07 10:02:22, haraken wrote: > On 2016/11/07 07:33:27, Mikhail wrote: > > On 2016/11/05 ...
4 years, 1 month ago
(2016-11-07 10:02:53 UTC)
#30
On 2016/11/07 10:02:22, haraken wrote:
> On 2016/11/07 07:33:27, Mikhail wrote:
> > On 2016/11/05 22:06:46, esprehn wrote:
> > > On 2016/11/05 at 12:30:38, haraken wrote:
> > > > ...
> > > >
> > > > esprehn: Not directly related to this CL, is it wrong to add a direct
> > > dependency on cpp files in //device? i.e., should we use generated mojo
> > bindings
> > > instead?
> > >
> > >
> > > Yeah, we shouldn't include anything from device that isn't a wtf mojo
> binding.
> > >
> > > I'm not sure I understand the purpose of these cpp files, they look like
> > structs
> > > for IPC which is what the mojoms are for?
> > >
> > > In general though we shouldn't depend on these external headers and should
> use
> > > the mojo bindings unless something automated is enforcing that they never
> > > contain std::string, vector, map, anything from base, etc.
> > >
> > > You could depend on them inside platform if you want to build wrappers
> around
> > > them like in the past, though that's a bit silly since the mojo wtf
bindings
> > are
> > > designed to avoid that duplication. :)
> > >
> > > The other places we use device/ all use mojo bindings today though, what's
> > > different about this api?
> >
> > The reason for having this dependency is the classes that describe shared
> buffer
> > layout for a sensor reading
> >
>
(https://cs.chromium.org/chromium/src/device/generic_sensor/public/cpp/sensor_...)
> > and that are used on both sides: platform and blink.
> > The shared buffer layout for a single sensor type on every platform contains
5
> > 64-bits values as following { seqlock, timestamp, values[3] }.
>
> Why
Why can't we use mojo-blink bindings for blink?
Mikhail
On 2016/11/07 10:02:53, haraken wrote: > Why can't we use mojo-blink bindings for blink? To ...
4 years, 1 month ago
(2016-11-07 11:02:32 UTC)
#31
On 2016/11/07 10:02:53, haraken wrote:
> Why can't we use mojo-blink bindings for blink?
To reduce code duplication, as there is no way to achieve the same layout with
just mojo bindings:
1) in mojo there is no abstraction for seqlock
(/src/device/base/synchronization/one_writer_seqlock.h)
2) 'double' has different size on diffrent platforms but we need fixed size
everywhrere so that our JS mock objects in layout tests work properly.
3) arrays in .mojom result in std::vector which does not fit for putting into
shared buffer.
what could be done in .mojom is smth like:
struct SensorReadingSharedBuffer {
uint64 seqlock;
uint64 timestamp;
uint64 value1;
uint64 value2;
uint64 value3;
};
and then a wrapping code should reinterpret cast struct fields to actual types
(OneWriterSeqLock and double), but the problem is that such code would be
repeated on both platform and blink side..
Maybe we just found the wrong place to put the shared code, do you see better
options?
darktears
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
4 years, 1 month ago
(2016-11-08 17:55:36 UTC)
#32
4 years, 1 month ago
(2016-11-08 19:28:33 UTC)
#35
Dry run: This issue passed the CQ dry run.
Mikhail
darktears@, could I ask you to split this CL and make a separate one containing ...
4 years, 1 month ago
(2016-11-11 12:45:50 UTC)
#36
darktears@, could I ask you to split this CL and make a separate one containing
only layout tests infra improvements so that other sensor bindings can reuse it?
darktears
On 2016/11/11 at 12:45:50, mikhail.pozdnyakov wrote: > darktears@, could I ask you to split this ...
4 years, 1 month ago
(2016-11-11 22:39:07 UTC)
#37
On 2016/11/11 at 12:45:50, mikhail.pozdnyakov wrote:
> darktears@, could I ask you to split this CL and make a separate one
containing only layout tests infra improvements so that other sensor bindings
can reuse it?
Sure.
jochen (gone - plz use gerrit)
idl files lgtm
4 years, 1 month ago
(2016-11-12 21:08:50 UTC)
#38
idl files lgtm
shalamov
lgtm
4 years, 1 month ago
(2016-11-15 15:28:21 UTC)
#39
lgtm
darktears
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
4 years, 1 month ago
(2016-11-15 16:37:26 UTC)
#40
On 2016/11/15 at 15:28:21, alexander.shalamov wrote: > lgtm @esprehn, @haraken : any other concerns after ...
4 years, 1 month ago
(2016-11-15 17:02:23 UTC)
#42
On 2016/11/15 at 15:28:21, alexander.shalamov wrote:
> lgtm
@esprehn, @haraken : any other concerns after Mikhail answer?
haraken
On 2016/11/15 17:02:23, darktears wrote: > On 2016/11/15 at 15:28:21, alexander.shalamov wrote: > > lgtm ...
4 years, 1 month ago
(2016-11-15 17:06:31 UTC)
#43
On 2016/11/15 17:02:23, darktears wrote:
> On 2016/11/15 at 15:28:21, alexander.shalamov wrote:
> > lgtm
>
> @esprehn, @haraken : any other concerns after Mikhail answer?
Regarding the //device dependency, I'll delegate the decision to Elliott.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 1 month ago
(2016-11-15 17:59:36 UTC)
#44
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/337610)
4 years, 1 month ago
(2016-11-15 17:59:37 UTC)
#45
The most recent patch doesn't seem to include any device headers?
4 years, 1 month ago
(2016-11-16 04:43:55 UTC)
#46
The most recent patch doesn't seem to include any device headers?
haraken
On 2016/11/16 04:43:55, esprehn wrote: > The most recent patch doesn't seem to include any ...
4 years, 1 month ago
(2016-11-16 05:50:18 UTC)
#47
On 2016/11/16 04:43:55, esprehn wrote:
> The most recent patch doesn't seem to include any device headers?
We were talking about the //device dependency written already written in
BUILD.gn.
Either way, this CL doesn't add any new dependency, so LGTM.
darktears
The CQ bit was checked by alexis.menard@intel.com
4 years, 1 month ago
(2016-11-16 16:08:28 UTC)
#48
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/68951)
4 years, 1 month ago
(2016-11-16 19:38:21 UTC)
#52
Issue 2471003002: [sensors] Accelerometer sensor bindings implementation
(Closed)
Created 4 years, 1 month ago by darktears
Modified 4 years, 1 month ago
Reviewers: jochen (gone - plz use gerrit), shalamov, esprehn, haraken, Mikhail, timvolodine
Base URL:
Comments: 7
Chromium Code Reviews
Issue
2471003002:
[sensors] Accelerometer sensor bindings implementation (Closed)
