[Sensors] OrientationSensor.populateMatrix() works also with Float64Array and DOMMatrix types

[Sensors] OrientationSensor.populateMatrix() works also with Float64Array and DOMMatrix types

As follows from the updated specification [1] OrientationSensor.populateMatrix() accepts arguments of the `RotationMatrixType` type which is a union type for 'Float32Array', 'Float64Array' and 'DOMMatrix'

BUG=661478
[1] https://w3c.github.io/orientation-sensor/#typedefdef-rotationmatrixtype

Review-Url: https://codereview.chromium.org/2785493002
Cr-Commit-Position: refs/heads/master@{#461092}
Committed: https://chromium.googlesource.com/chromium/src/+/511fde94942ca59c7ac26c0a40e237251be45d8b

[Mikhail, 2017-03-28 15:09:47]

Description was changed from

==========
[Sensors] OrientationSensor.populateMatrix() works also with Float64Array and
DOMMatrix types

As follows from the updated specification [1] OrientationSensor.populateMatrix()
accepts arguments of `RotationMatrixType` which is a union type for
'Float32Array', 'Float64Array' and 'DOMMatrix'

BUG=661478

[1] https://w3c.github.io/orientation-sensor/#typedefdef-rotationmatrixtype
==========

to

==========
[Sensors] OrientationSensor.populateMatrix() works also with Float64Array and
DOMMatrix types

As follows from the updated specification [1] OrientationSensor.populateMatrix()
accepts arguments of the `RotationMatrixType` type which is a union type for
'Float32Array', 'Float64Array' and 'DOMMatrix'

BUG=661478

[1] https://w3c.github.io/orientation-sensor/#typedefdef-rotationmatrixtype
==========

[Mikhail, 2017-03-28 15:09:47]

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

[Mikhail, 2017-03-28 15:10:08]

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

[commit-bot: I haz the power, 2017-03-28 15:10:43]

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-28 15:11:26]

PTAL

[commit-bot: I haz the power, 2017-03-28 17:07:13]

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

[commit-bot: I haz the power, 2017-03-28 17:07:16]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2017-03-28 18:23:03]

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html
(right):

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html:58:
assert_equals(error.name, 'InvalidStateError');
assert_throws can be used for this one.

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html:63: let
testPromise = sensor.mockSensorProvider.getCreatedSensor()
return ...

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html:67:
.then((mockSensor) => {
nit: unnecessary parens

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp (right):

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:51:
targetMatrix->setM11(1.0 - 2 * (y * y + z * z));
It would be nice if DOMMatrix provided a way to call m_matrix->setMatrix() so
you could set all these values in a single call.

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:80: void
OrientationSensor::populateMatrixInternal(Matrix* targetMatrix,
I would merge this function into populateMatrix because otherwise you are
duplicating a bunch of code that is identical regardless of the matrix type.

[Mikhail, 2017-03-29 08:27:58]

Thanks for your comments!

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html
(right):

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html:58:
assert_equals(error.name, 'InvalidStateError');
On 2017/03/28 18:23:02, Reilly Grant wrote:
> assert_throws can be used for this one.

Indeed, thanks!

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html:63: let
testPromise = sensor.mockSensorProvider.getCreatedSensor()
On 2017/03/28 18:23:02, Reilly Grant wrote:
> return ...

Done.

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/absolute-orientation-sensor.html:67:
.then((mockSensor) => {
On 2017/03/28 18:23:02, Reilly Grant wrote:
> nit: unnecessary parens

Done.

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp (right):

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:80: void
OrientationSensor::populateMatrixInternal(Matrix* targetMatrix,
On 2017/03/28 18:23:03, Reilly Grant wrote:
> I would merge this function into populateMatrix because otherwise you are
> duplicating a bunch of code that is identical regardless of the matrix type.

Merging looks a bit problematic as there are two template calls in this
function: in the beginning (checkBufferLength) and in the end
(doPopulateMatrix), so extracting somehow the common code in the middle would
IMO affect readability.
Given that all these template functions are not exported and only used inside
this .cpp I do not think binary size will be an issue here, wdyt?

[haraken, 2017-03-29 08:34:22]

bindings LGTM

[shalamov, 2017-03-29 08:39:20]

lgtm

[Reilly Grant (use Gerrit), 2017-03-29 15:31:22]

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp (right):

https://codereview.chromium.org/2785493002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp:80: void
OrientationSensor::populateMatrixInternal(Matrix* targetMatrix,
On 2017/03/29 08:27:58, Mikhail wrote:
> On 2017/03/28 18:23:03, Reilly Grant wrote:
> > I would merge this function into populateMatrix because otherwise you are
> > duplicating a bunch of code that is identical regardless of the matrix type.
> 
> Merging looks a bit problematic as there are two template calls in this
> function: in the beginning (checkBufferLength) and in the end
> (doPopulateMatrix), so extracting somehow the common code in the middle would
> IMO affect readability.
> Given that all these template functions are not exported and only used inside
> this .cpp I do not think binary size will be an issue here, wdyt?

I would write populateMatrix like this, which only duplicates one of the error
checks:

void OrientationSensor::populateMatrix(
    Float32ArrayOrFloat64ArrayOrDOMMatrix& matrix,
    ExceptionState& exceptionState) {
  if (!isActivated()) {
    exceptionState.throwDOMException(
        InvalidStateError, "The sensor must be in 'connected' state.");
    return;
  }
  if (!canReturnReadings())
    return;

  double x = readingValueUnchecked(0);
  double y = readingValueUnchecked(1);
  double z = readingValueUnchecked(2);
  double w = readingValueUnchecked(3);

  // You may even pre-compute the 3x3 matrix here.

  if (matrix.isFloat32Array()) {
    if (matrix.getAsFloat32Array()->length() < 16) {
      exceptionState.throwTypeError(
          "Target buffer must have at least 16 elements.");
      return;
    }
    populateMatrixInternal(matrix.getAsFloat32Array(), x, y, z, w);
  } else if (matrix.isFloat64Array()) {
    if (matrix.getAsFloat64Array()->length() < 16) {
      exceptionState.throwTypeError(
          "Target buffer must have at least 16 elements.");
      return;
    }
    populateMatrixInternal(matrix.getAsFloat64Array(), x, y, z, w);
  } else if (matrix.isDOMMatrix()) {
    populateMatrixInternal(matrix.getAsDOMMatrix(), x, y, z, w);
  } else {
    NOTREACHED() << "Unexpected rotation matrix type."
  }
}

[Mikhail, 2017-03-30 08:26:07]

On 2017/03/29 15:31:22, Reilly Grant wrote:
> I would write populateMatrix like this, which only duplicates one of the error
> checks:
> 
> void OrientationSensor::populateMatrix(
>     Float32ArrayOrFloat64ArrayOrDOMMatrix& matrix,
>     ExceptionState& exceptionState) {
>   if (!isActivated()) {
>     exceptionState.throwDOMException(
>         InvalidStateError, "The sensor must be in 'connected' state.");
>     return;
>   }
>   if (!canReturnReadings())
>     return;
> 
>   double x = readingValueUnchecked(0);
>   double y = readingValueUnchecked(1);
>   double z = readingValueUnchecked(2);
>   double w = readingValueUnchecked(3);
> 
>   // You may even pre-compute the 3x3 matrix here.
> 
>   if (matrix.isFloat32Array()) {
>     if (matrix.getAsFloat32Array()->length() < 16) {
>       exceptionState.throwTypeError(
>           "Target buffer must have at least 16 elements.");
>       return;
>     }
>     populateMatrixInternal(matrix.getAsFloat32Array(), x, y, z, w);
>   } else if (matrix.isFloat64Array()) {
>     if (matrix.getAsFloat64Array()->length() < 16) {
>       exceptionState.throwTypeError(
>           "Target buffer must have at least 16 elements.");
>       return;
>     }
>     populateMatrixInternal(matrix.getAsFloat64Array(), x, y, z, w);
>   } else if (matrix.isDOMMatrix()) {
>     populateMatrixInternal(matrix.getAsDOMMatrix(), x, y, z, w);
>   } else {
>     NOTREACHED() << "Unexpected rotation matrix type."
>   }
> }

Thanks for your proposal! this is something I considered as well :) but in this
case the implementation diverges from what is specified: we must first check the
given argument (both type and buffer length) and after that check the sensor's
state.

[Reilly Grant (use Gerrit), 2017-03-30 14:24:27]

lgtm because it doesn't matter whether this bike shed is blue.

[Mikhail, 2017-03-31 08:54:09]

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

[commit-bot: I haz the power, 2017-03-31 08:54:24]

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-31 10:34:50]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490950449490150,
"parent_rev": "6883fe184564665aee120504f2d8b23310093eea", "commit_rev":
"511fde94942ca59c7ac26c0a40e237251be45d8b"}

[commit-bot: I haz the power, 2017-03-31 10:35:40]

Description was changed from

==========
[Sensors] OrientationSensor.populateMatrix() works also with Float64Array and
DOMMatrix types

As follows from the updated specification [1] OrientationSensor.populateMatrix()
accepts arguments of the `RotationMatrixType` type which is a union type for
'Float32Array', 'Float64Array' and 'DOMMatrix'

BUG=661478

[1] https://w3c.github.io/orientation-sensor/#typedefdef-rotationmatrixtype
==========

to

==========
[Sensors] OrientationSensor.populateMatrix() works also with Float64Array and
DOMMatrix types

As follows from the updated specification [1] OrientationSensor.populateMatrix()
accepts arguments of the `RotationMatrixType` type which is a union type for
'Float32Array', 'Float64Array' and 'DOMMatrix'

BUG=661478

[1] https://w3c.github.io/orientation-sensor/#typedefdef-rotationmatrixtype

Review-Url: https://codereview.chromium.org/2785493002
Cr-Commit-Position: refs/heads/master@{#461092}
Committed:
https://chromium.googlesource.com/chromium/src/+/511fde94942ca59c7ac26c0a40e2...
==========

[commit-bot: I haz the power, 2017-03-31 10:35:41]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/511fde94942ca59c7ac26c0a40e2...