[Sensors] Implement bindings for AbsoluteOrientationSensor

[Sensors] Implement bindings for AbsoluteOrientationSensor

Specification: https://w3c.github.io/orientation-sensor/

BUG=661478
Review-Url: https://codereview.chromium.org/2746573002
Cr-Commit-Position: refs/heads/master@{#459759}
Committed: https://chromium.googlesource.com/chromium/src/+/e3e5baac82ee84437b0de0de0e208f7503ae2de3

[Mikhail, 2017-03-16 12:16:24]

Description was changed from

==========
[Sensors] Implement bindings for OrientationSensor

Specification editor's draft: https://w3c.github.io/orientation-sensor/

BUG=661478
==========

to

==========
[Sensors] Implement bindings for AbsoluteOrientationSensor

Specification: https://w3c.github.io/orientation-sensor/

BUG=661478
==========

[Mikhail, 2017-03-16 13:49:58]

Patchset #1 (id:1) has been deleted

[Mikhail, 2017-03-22 14:25:56]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 14:26:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 14:38:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 14:38:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mikhail, 2017-03-22 14:43:04]

Patchset #1 (id:20001) has been deleted

[Mikhail, 2017-03-22 14:44:36]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 14:44:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 14:45:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 14:45:41]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Mikhail, 2017-03-22 14:48:07]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[Mikhail, 2017-03-22 14:48:19]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-22 14:48:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mikhail, 2017-03-22 14:51:30]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[Mikhail, 2017-03-22 14:51:48]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-22 14:52:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 16:12:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 16:12:45]

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_...)

[shalamov, 2017-03-23 08:41:05]

alexander.shalamov@intel.com changed reviewers:
+ alexander.shalamov@intel.com

[shalamov, 2017-03-23 08:41:06]

https://codereview.chromium.org/2746573002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html
(right):

https://codereview.chromium.org/2746573002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html:15: const
kIdentityMatrix = [1, 0, 0, 0,
Would be cool if we provide predefined quats and verify that conversion to
rotation matrix is correct.

https://codereview.chromium.org/2746573002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html:39:
return rotationMatrix.toString() == kIdentityMatrix.toString();
I think there were array comparison helpers, maybe we can use those.

https://codereview.chromium.org/2746573002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp (right):

https://codereview.chromium.org/2746573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:14: isNull =
!canReturnReadings();
Can we return [SameObject] unless reading is changed? We can compare current
timestamp with timestamp in cached Vector and return new object when they are
different.

[Mikhail, 2017-03-24 14:44:49]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 14:45:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mikhail, 2017-03-24 14:47:09]

Thanks for your comments!

https://codereview.chromium.org/2746573002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html
(right):

https://codereview.chromium.org/2746573002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html:15: const
kIdentityMatrix = [1, 0, 0, 0,
On 2017/03/23 08:41:06, shalamov wrote:
> Would be cool if we provide predefined quats and verify that conversion to
> rotation matrix is correct.

Done.

https://codereview.chromium.org/2746573002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html:39:
return rotationMatrix.toString() == kIdentityMatrix.toString();
On 2017/03/23 08:41:05, shalamov wrote:
> I think there were array comparison helpers, maybe we can use those.

I found only asserts being exposed

https://codereview.chromium.org/2746573002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp (right):

https://codereview.chromium.org/2746573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:14: isNull =
!canReturnReadings();
On 2017/03/23 08:41:06, shalamov wrote:
> Can we return [SameObject] unless reading is changed? We can compare current
> timestamp with timestamp in cached Vector and return new object when they are
> different.

Cached using [CachedAttribute]

[Mikhail, 2017-03-24 14:48:21]

mikhail.pozdnyakov@intel.com changed reviewers:
+ haraken@chromium.org, reillyg@chromium.org

[Mikhail, 2017-03-24 14:48:43]

PTAL

[Reilly Grant (use Gerrit), 2017-03-24 15:10:33]

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/sensor/idl-AbsoluteOrientationSensor.html
(right):

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/sensor/idl-AbsoluteOrientationSensor.html:1:
<!DOCTYPE html>
In a separate patch can you replace this with an idlharness.js test in the
upstream Web Platform Tests repository? We import them automatically so there's
no point testing in two places.

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp (right):

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:27: "Target
buffer must have minimal size of 16.");
"Target buffer must have at least 16 elements."

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:32:
InvalidStateError, "The Sensor must be in 'connected' state.");
Sensor -> sensor

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:53: out[15] =
1.0;
If callers are passing in arbitrary buffers we should probably set all 16
elements.

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/OrientationSensor.h (right):

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/OrientationSensor.h:8: #include
"core/dom/DOMTypedArray.h"
DOMFloat32Array can be forward declared here.

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right):

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:34: class Client : public
GarbageCollectedMixin {
Why rename this? It is an observer.

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:39: // Platfrom sensort
reading has changed.
sensort -> sensor

[commit-bot: I haz the power, 2017-03-24 15:44:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 15:44:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Shalamov, 2017-03-24 16:16:46]

shalamov@chromium.org changed reviewers:
+ shalamov@chromium.org

[Alexander Shalamov, 2017-03-24 16:16:47]

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp (right):

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:53: out[15] =
1.0;
On 2017/03/24 15:10:33, Reilly Grant wrote:
> If callers are passing in arbitrary buffers we should probably set all 16
> elements.

Mikhail@ I think Reilly is right. If I remember correctly, we wanted to support
use-cases where translation values are not overwritten, e.g., user sets
translation (m41,m42,m43) and then populateMatrix will fill rotation values.
Then, using webgl you can do both transformations in one call. We forgot that
matrix can contain, e.g. orto or perspective projection, then after
populateMatrix is called, we will have broken transform. We need to update spec
and impl., wdyt?

[Mikhail, 2017-03-24 16:27:44]

On 2017/03/24 16:16:47, shalamov1 wrote:
>
https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp (right):
> 
>
https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:53: out[15] =
> 1.0;
> On 2017/03/24 15:10:33, Reilly Grant wrote:
> > If callers are passing in arbitrary buffers we should probably set all 16
> > elements.
> 
> Mikhail@ I think Reilly is right. If I remember correctly, we wanted to
support
> use-cases where translation values are not overwritten, e.g., user sets
> translation (m41,m42,m43) and then populateMatrix will fill rotation values.
> Then, using webgl you can do both transformations in one call. We forgot that
> matrix can contain, e.g. orto or perspective projection, then after
> populateMatrix is called, we will have broken transform. We need to update
spec
> and impl., wdyt?

It can but position matrix has usually translation + rotation, think it's worth
creating a spec issue, but so far I'd keep what is currently specified.

[Mikhail, 2017-03-24 16:29:56]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[Mikhail, 2017-03-24 16:30:31]

Thanks for taking a look!

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/sensor/idl-AbsoluteOrientationSensor.html
(right):

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/sensor/idl-AbsoluteOrientationSensor.html:1:
<!DOCTYPE html>
On 2017/03/24 15:10:33, Reilly Grant wrote:
> In a separate patch can you replace this with an idlharness.js test in the
> upstream Web Platform Tests repository? We import them automatically so
there's
> no point testing in two places.

Sure. I'll make it in a following patch.

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp (right):

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:27: "Target
buffer must have minimal size of 16.");
On 2017/03/24 15:10:33, Reilly Grant wrote:
> "Target buffer must have at least 16 elements."

Done.

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:32:
InvalidStateError, "The Sensor must be in 'connected' state.");
On 2017/03/24 15:10:33, Reilly Grant wrote:
> Sensor -> sensor

Done.

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:53: out[15] =
1.0;
On 2017/03/24 15:10:33, Reilly Grant wrote:
> If callers are passing in arbitrary buffers we should probably set all 16
> elements.

Other elements are not updated on purpose: matrix might be preinitialized with
translation data, so after this call this translation data will remain.

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/OrientationSensor.h (right):

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/OrientationSensor.h:8: #include
"core/dom/DOMTypedArray.h"
On 2017/03/24 15:10:33, Reilly Grant wrote:
> DOMFloat32Array can be forward declared here.

Unfortunately not, as it's a typedef

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right):

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:34: class Client : public
GarbageCollectedMixin {
On 2017/03/24 15:10:33, Reilly Grant wrote:
> Why rename this? It is an observer.

Agree, the newly added method is actually notification too, renamed it back :)
thanks!

https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:39: // Platfrom sensort
reading has changed.
On 2017/03/24 15:10:33, Reilly Grant wrote:
> sensort -> sensor

Done.

[Alexander Shalamov, 2017-03-24 16:30:36]

On 2017/03/24 16:27:44, Mikhail wrote:
> On 2017/03/24 16:16:47, shalamov1 wrote:
> >
>
https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:53: out[15] =
> > 1.0;
> > On 2017/03/24 15:10:33, Reilly Grant wrote:
> > > If callers are passing in arbitrary buffers we should probably set all 16
> > > elements.
> > 
> > Mikhail@ I think Reilly is right. If I remember correctly, we wanted to
> support
> > use-cases where translation values are not overwritten, e.g., user sets
> > translation (m41,m42,m43) and then populateMatrix will fill rotation values.
> > Then, using webgl you can do both transformations in one call. We forgot
that
> > matrix can contain, e.g. orto or perspective projection, then after
> > populateMatrix is called, we will have broken transform. We need to update
> spec
> > and impl., wdyt?
> 
> It can but position matrix has usually translation + rotation, think it's
worth
> creating a spec issue, but so far I'd keep what is currently specified.

What if there are leftovers from projection matrix? m34 and m43 would contain
something that will produce invalid transformation matrix.

[commit-bot: I haz the power, 2017-03-24 16:30:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexander Shalamov, 2017-03-24 16:45:47]

On 2017/03/24 16:30:36, Alexander Shalamov wrote:
> On 2017/03/24 16:27:44, Mikhail wrote:
> > On 2017/03/24 16:16:47, shalamov1 wrote:
> > >
> >
>
https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2746573002/diff/100001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:53: out[15]
=
> > > 1.0;
> > > On 2017/03/24 15:10:33, Reilly Grant wrote:
> > > > If callers are passing in arbitrary buffers we should probably set all
16
> > > > elements.
> > > 
> > > Mikhail@ I think Reilly is right. If I remember correctly, we wanted to
> > support
> > > use-cases where translation values are not overwritten, e.g., user sets
> > > translation (m41,m42,m43) and then populateMatrix will fill rotation
values.
> > > Then, using webgl you can do both transformations in one call. We forgot
> that
> > > matrix can contain, e.g. orto or perspective projection, then after
> > > populateMatrix is called, we will have broken transform. We need to update
> > spec
> > > and impl., wdyt?
> > 
> > It can but position matrix has usually translation + rotation, think it's
> worth
> > creating a spec issue, but so far I'd keep what is currently specified.
> 
> What if there are leftovers from projection matrix? m34 and m43 would contain
> something that will produce invalid transformation matrix.

Created issue https://github.com/w3c/orientation-sensor/issues/24

[commit-bot: I haz the power, 2017-03-24 17:37:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 17:37:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Reilly Grant (use Gerrit), 2017-03-24 20:49:46]

lgtm

[shalamov, 2017-03-27 07:37:21]

lgtm

[Mikhail, 2017-03-27 08:50:09]

haraken@, could you PTAL? thanks!

[haraken, 2017-03-27 09:59:38]

Implementation-wise LGTM

https://codereview.chromium.org/2746573002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2746573002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:134: for (Observer*
observer : m_observers)

Don't you need to copy the observers before starting iterating it? See line 142.

[Mikhail, 2017-03-27 10:04:01]

Thanks for taking a look!

https://codereview.chromium.org/2746573002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2746573002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:134: for (Observer*
observer : m_observers)
On 2017/03/27 09:59:38, haraken wrote:
> 
> Don't you need to copy the observers before starting iterating it? See line
142.

This callback will not cause invokes of JS handlers so `m_observers` container
won't be modified in the middle. So this is safe.

[Mikhail, 2017-03-27 10:28:46]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 10:29:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 11:58:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 11:58:25]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2017-03-27 12:01:07]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2017-03-27 12:01:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, reillyg@chromium.org, alexander.shalamov@intel.com
Link to the patchset: https://codereview.chromium.org/2746573002/#ps140001
(title: "webexpose")

[commit-bot: I haz the power, 2017-03-27 12:01:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 12:05:59]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490616067293120,
"parent_rev": "6990ce31a20867d7c0ba05b93d3f3bee7fc56144", "commit_rev":
"e3e5baac82ee84437b0de0de0e208f7503ae2de3"}

[commit-bot: I haz the power, 2017-03-27 12:06:40]

Description was changed from

==========
[Sensors] Implement bindings for AbsoluteOrientationSensor

Specification: https://w3c.github.io/orientation-sensor/

BUG=661478
==========

to

==========
[Sensors] Implement bindings for AbsoluteOrientationSensor

Specification: https://w3c.github.io/orientation-sensor/

BUG=661478

Review-Url: https://codereview.chromium.org/2746573002
Cr-Commit-Position: refs/heads/master@{#459759}
Committed:
https://chromium.googlesource.com/chromium/src/+/e3e5baac82ee84437b0de0de0e20...
==========

[commit-bot: I haz the power, 2017-03-27 12:06:41]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/e3e5baac82ee84437b0de0de0e20...