Updated VRService to match the latest Blink WebVR interface.

Updated VRService to match the latest Blink WebVR interface.

The Blink interface now exposes values that the previous version of the service
did not supply. This patch updates the service and cardboard backend to supply
all the necessary values for WebVR v1. It also takes the opportunity to do some
renaming for consistency. Mainly "device" -> "display" to match the WebVR spec
verbiage.

BUG=389343
Committed: https://crrev.com/2f845c7547a8ea5d1edad21be0719f6b6b55f06b
Cr-Commit-Position: refs/heads/master@{#395010}

[bajones, 2016-05-10 21:35:17]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-10 21:35:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1967633002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1967633002/20001

[bajones, 2016-05-10 21:42:52]

bajones@chromium.org changed reviewers:
+ esprehn@chromium.org, mkwst@chromium.org, rockot@chromium.org

[bajones, 2016-05-10 21:42:53]

The biggest question mark I have personally about this code is the conversion of
mojo::WTFArray<float> into DOMFloat32Array (See an example in VRPose.cpp). Not
sure if there's a safer or more efficient way.

Otherwise I'd like any general advice on the changes to vr_service.mojo. I'm
generally trying to mimic the Blink interfaces as directly as possible but that
may not follow Mojo best practices.

[commit-bot: I haz the power, 2016-05-11 01:15:41]

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

[commit-bot: I haz the power, 2016-05-11 01:15:42]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-05-11 01:17:07]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, yzshen@chromium.org

[haraken, 2016-05-11 01:17:08]

On 2016/05/10 21:42:53, bajones wrote:
> The biggest question mark I have personally about this code is the conversion
of
> mojo::WTFArray<float> into DOMFloat32Array (See an example in VRPose.cpp). Not
> sure if there's a safer or more efficient way.

Regarding safety, would you help me understand your concern?

Regarding efficiency, it's definitely not efficient to copy data twice: Mojo's
array => WTFArray => DOMFloat32Array.

yzshen@: Would it be possible to support the Mojo's array => DOMFloat32Array
conversion?

That said, this CL is not making anything worse because vecToFloat32Array is
already copying data twice, right?

[esprehn, 2016-05-11 01:52:38]

You should just std::move() the Mojo array into a WTF Array and then again into
a DOMFloatArray, that should avoid all the copies I think? Is the DOM array
stored somewhere else?

[esprehn, 2016-05-18 00:35:41]

What's the status here? Should I review?

[bajones, 2016-05-18 03:06:52]

On 2016/05/18 00:35:41, esprehn wrote:
> What's the status here? Should I review?

Apologies, I've been out on vacation and haven't had time to address the
std::move() suggestion. That said, after a cursory glance at the WTFTypedArray
class it's not clear to me where I should move into in order to avoid
unnecessary allocations or copies. Will this require a new :create() variant?

[yzshen1, 2016-05-18 15:58:18]

On 2016/05/11 01:17:08, haraken wrote:
> On 2016/05/10 21:42:53, bajones wrote:
> > The biggest question mark I have personally about this code is the
conversion
> of
> > mojo::WTFArray<float> into DOMFloat32Array (See an example in VRPose.cpp).
Not
> > sure if there's a safer or more efficient way.
> 
> Regarding safety, would you help me understand your concern?
> 
> Regarding efficiency, it's definitely not efficient to copy data twice: Mojo's
> array => WTFArray => DOMFloat32Array.
> 
> yzshen@: Would it be possible to support the Mojo's array => DOMFloat32Array
> conversion?

(sorry I didn't see this earlier)

Imagine we have the following mojom definition:
struct Foo {
  array<float> bar;
};

The standard generated Foo C++ class is:

class Foo {
  ...
  // It is always mapped to WTFArray. And you will need to manually convert to
your desired type.
  mojo::WTFArray<float> bar;
};

However, if you want, you can use custom typemapping. It is possible to use a
CustomFoo struct, and defined StructTraits<> for it:

template<>
struct CustomFoo {
  static const YourOwnArrayType& foo(const CustomFoo& input) {
    // As long as there is an ArrayTraits defined for YourOwnArrayType.
  }
};

(Please read more here:
http://www.chromium.org/developers/design-documents/mojo/type-mapping)

That is more coding work, but avoids one extra copy. It really depends on how
costly it is in your use case to have one more copy.







> 
> That said, this CL is not making anything worse because vecToFloat32Array is
> already copying data twice, right?

[bajones, 2016-05-18 21:21:08]

I'm all for avoiding copies and I think that's the right direction to go
eventually, but after doing some more investigation I'm going to ask that the CL
be able to land as-is with the intent to follow up with a more efficient
conversion to DOMFloat32Array for a couple of reasons:

 * I cannot currently see a way to initialize a DOMTypedArray with data moved
from another source. This may be because the memory needs to be owned by V8? In
any case, if such a constructor can be added it shouldn't be in the context of a
CL like this one.
 * Converting directly to a DOMTypedArray instead of a mojo::WTFArray sounds
like a good path, but it seems like support for that would be useful outside of
this VR interface and as such should probably be done outside this CL.
 * As haraken@ pointed out: this isn't any additional copies than was previously
done, so it's not a regression.

[esprehn, 2016-05-18 21:55:29]

Please wrap your change description to 80 or 100 cols. :)

https://codereview.chromium.org/1967633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRPose.cpp (right):

https://codereview.chromium.org/1967633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRPose.cpp:14: return
DOMFloat32Array::create(&(vec.front()), vec.size());
This will crash if the vector is of size 0. Should the vector always be 4
elements?

[bajones, 2016-05-18 23:24:20]

Description was changed from

==========
Updated VRService to match the latest Blink WebVR interface.

The Blink interface now exposes values that the previous version of the service
did not supply. This patch updates the service and cardboard backend to supply
all the necessary values for WebVR v1. It also takes the opportunity to do some
renaming for consistency. Mainly "device" -> "display" to match the WebVR spec
verbiage.

BUG=389343
==========

to

==========
Updated VRService to match the latest Blink WebVR interface.

The Blink interface now exposes values that the previous version of the service
did not supply. This patch updates the service and cardboard backend to supply
all the necessary values for WebVR v1. It also takes the opportunity to do some
renaming for consistency. Mainly "device" -> "display" to match the WebVR spec
verbiage.

BUG=389343
==========

[bajones, 2016-05-18 23:25:19]

On 2016/05/18 21:55:29, esprehn wrote:
> Please wrap your change description to 80 or 100 cols. :)

Done!

> third_party/WebKit/Source/modules/vr/VRPose.cpp:14: return
> DOMFloat32Array::create(&(vec.front()), vec.size());
> This will crash if the vector is of size 0. Should the vector always be 4
> elements?

3 or 4, but that's a good point. I'll add a check for that.

[haraken, 2016-05-18 23:50:01]

LGTM on my side.

https://codereview.chromium.org/1967633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRPose.cpp (right):

https://codereview.chromium.org/1967633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRPose.cpp:11: DOMFloat32Array*
mojoArrayToFloat32Array(const mojo::WTFArray<float>& vec)

Shall we:

- move this function to platform/mojo/MojoHelper.h
- file a bug about the extra copy
- add a TODO

?

[bajones, 2016-05-19 20:41:11]

On 2016/05/18 23:50:01, haraken wrote:
> Shall we:
> 
> - move this function to platform/mojo/MojoHelper.h
> - file a bug about the extra copy
> - add a TODO
> 
> ?

Done, Done, and Done. I'd appreciate a sanity check on the new converter in
MojoHelper.h, though.

[bajones, 2016-05-19 20:42:15]

On 2016/05/19 20:41:11, bajones wrote:
> On 2016/05/18 23:50:01, haraken wrote:
> > Shall we:
> > 
> > - move this function to platform/mojo/MojoHelper.h
> > - file a bug about the extra copy
> > - add a TODO
> > 
> > ?
> 
> Done, Done, and Done. I'd appreciate a sanity check on the new converter in
> MojoHelper.h, though.
Oh, and the bug in question is: http://crbug.com/613290

[dcheng, 2016-05-19 20:53:48]

On 2016/05/19 at 20:41:11, bajones wrote:
> On 2016/05/18 23:50:01, haraken wrote:
> > Shall we:
> > 
> > - move this function to platform/mojo/MojoHelper.h
> > - file a bug about the extra copy
> > - add a TODO
> > 
> > ?
> 
> Done, Done, and Done. I'd appreciate a sanity check on the new converter in
MojoHelper.h, though.

Sorry, but I think we should leave this as a one-off in VRPose if we're not
going to use StructTraits. Several reasons for this:
1) We're encouraging use of StructTraits instead of TypeConverters.
TypeConverters have no way to handle/signal conversion failure. Adding another
one makes it easier/possible for someone else to start depending on something
we're actively trying to remove.
2) It's not clear to me the semantics of the current TypeConverter generalize
well: I'd expect it to be perfectly reasonable to have a 0-length DOMTypedArray
rather than returning nullptr in that case.

[bajones, 2016-05-19 21:44:07]

On 2016/05/19 20:53:48, dcheng wrote:
> On 2016/05/19 at 20:41:11, bajones wrote:
> > On 2016/05/18 23:50:01, haraken wrote:
> > > Shall we:
> > > 
> > > - move this function to platform/mojo/MojoHelper.h
> > > - file a bug about the extra copy
> > > - add a TODO
> > > 
> > > ?
> > 
> > Done, Done, and Done. I'd appreciate a sanity check on the new converter in
> MojoHelper.h, though.
> 
> Sorry, but I think we should leave this as a one-off in VRPose if we're not
> going to use StructTraits. Several reasons for this:
> 1) We're encouraging use of StructTraits instead of TypeConverters.
> TypeConverters have no way to handle/signal conversion failure. Adding another
> one makes it easier/possible for someone else to start depending on something
> we're actively trying to remove.
> 2) It's not clear to me the semantics of the current TypeConverter generalize
> well: I'd expect it to be perfectly reasonable to have a 0-length
DOMTypedArray
> rather than returning nullptr in that case.

Thanks for the feedback. Reverted back to the older code, though it now includes
some size checks.

[dcheng, 2016-05-19 22:17:47]

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

[dcheng, 2016-05-19 22:17:49]

https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRPose.cpp (right):

https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRPose.cpp:13: if (!vec.is_null() &&
vec.size()) {
We don't actually expect size == 0 if the vector is non-null right? I think this
could/should be an assert.

https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRStageParameters.cpp (right):

https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRStageParameters.cpp:6: #include
"platform/mojo/MojoHelper.h"
Not needed now, I think.

https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRStageParameters.cpp:24: if
(!stage->standingTransform.is_null() && stage->standingTransform.size() == 16) {
Ditto: will Mojo actually deserialize the message if the array size isn't 16?

https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/vr/vr_service.mojom (right):

https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/vr/vr_service.mojom:19: array<float,
4>? orientation;
Out of curiosity, does the compiler or mojo enforce that the sender side writes
an array of 4 floats?

[Ken Rockot(use gerrit already), 2016-05-19 22:34:31]

On 2016/05/19 at 22:17:49, dcheng wrote:
>
https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/vr/VRPose.cpp (right):
> 
>
https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/vr/VRPose.cpp:13: if (!vec.is_null() &&
vec.size()) {
> We don't actually expect size == 0 if the vector is non-null right? I think
this could/should be an assert.
> 
>
https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/vr/VRStageParameters.cpp (right):
> 
>
https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/vr/VRStageParameters.cpp:6: #include
"platform/mojo/MojoHelper.h"
> Not needed now, I think.
> 
>
https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/vr/VRStageParameters.cpp:24: if
(!stage->standingTransform.is_null() && stage->standingTransform.size() == 16) {
> Ditto: will Mojo actually deserialize the message if the array size isn't 16?

Correct, it will not. It'll close the pipe instead of dispatching the message.

> 
>
https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/publ...
> File third_party/WebKit/public/platform/modules/vr/vr_service.mojom (right):
> 
>
https://codereview.chromium.org/1967633002/diff/60001/third_party/WebKit/publ...
> third_party/WebKit/public/platform/modules/vr/vr_service.mojom:19:
array<float, 4>? orientation;
> Out of curiosity, does the compiler or mojo enforce that the sender side
writes an array of 4 floats?

Generated bindings will only log if the sender writes the wrong size for a fixed
array. Maybe we should change it to DCHECK.

The receiver of course also validates the size, and will close the pipe on
mismatch like it would with any other kind of validation error.

[bajones, 2016-05-19 22:45:46]

On 2016/05/19 22:34:31, Ken Rockot wrote:
> Correct, it will not. It'll close the pipe instead of dispatching the message.

So to make sure we're all on the same page: The arrays will either come through
as null (since these ones are declared as nullable) or with the number of
elements specified in the mojom, but never anything else? Can I safely remove
the size checks, then? (Effectively go back to Patch Set 2)

[Ken Rockot(use gerrit already), 2016-05-19 22:58:35]

On 2016/05/19 at 22:45:46, bajones wrote:
> On 2016/05/19 22:34:31, Ken Rockot wrote:
> > Correct, it will not. It'll close the pipe instead of dispatching the
message.
> 
> So to make sure we're all on the same page: The arrays will either come
through as null (since these ones are declared as nullable) or with the number
of elements specified in the mojom, but never anything else? Can I safely remove
the size checks, then? (Effectively go back to Patch Set 2)

Yes, any size checks on fixed-size arrays are redundant. I think DCHECKs are
reasonable as a form of local documentation, but I am ambivalent about using
them in this case.

[esprehn, 2016-05-19 23:02:19]

So vector<T, N> in mojom produces a vector that is always size N or is it *at
least* size N?

[Ken Rockot(use gerrit already), 2016-05-19 23:02:52]

Exactly size N. If the size is anything other than N, the message is
rejected and the pipe is closed.

On Thu, May 19, 2016 at 4:02 PM, <esprehn@chromium.org> wrote:

> So vector<T, N> in mojom produces a vector that is always size N or is it
> *at
> least* size N?
>
> https://codereview.chromium.org/1967633002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-05-19 23:02:55]

Exactly size N. If the size is anything other than N, the message is
rejected and the pipe is closed.

On Thu, May 19, 2016 at 4:02 PM, <esprehn@chromium.org> wrote:

> So vector<T, N> in mojom produces a vector that is always size N or is it
> *at
> least* size N?
>
> https://codereview.chromium.org/1967633002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[bajones, 2016-05-20 03:11:50]

Removed the size checks once again. One last question for Ken: I presume that if
a member is not declared optional in the mojom it will similarly reject the
message if no value is provided?

[Ken Rockot(use gerrit already), 2016-05-20 03:15:05]

On 2016/05/20 at 03:11:50, bajones wrote:
> Removed the size checks once again. One last question for Ken: I presume that
if a member is not declared optional in the mojom it will similarly reject the
message if no value is provided?

Correct

[bajones, 2016-05-20 03:35:57]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 03:36:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1967633002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1967633002/80001

[commit-bot: I haz the power, 2016-05-20 04:52:39]

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

[commit-bot: I haz the power, 2016-05-20 04:52:41]

Dry run: This issue passed the CQ dry run.

[bajones, 2016-05-20 05:11:17]

bajones@chromium.org changed reviewers:
+ kenrb@chromium.org

[bajones, 2016-05-20 05:11:18]

dcheng@ or kenrb@, could I get a review on the changes to vr_service.mojom?

[dcheng, 2016-05-20 05:13:27]

lgtm

[bajones, 2016-05-20 05:20:56]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-05-20 05:20:57]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1967633002/#ps80001
(title: "And we're back to removing all the array size checks")

[commit-bot: I haz the power, 2016-05-20 05:22:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1967633002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1967633002/80001

[commit-bot: I haz the power, 2016-05-20 05:59:40]

Description was changed from

==========
Updated VRService to match the latest Blink WebVR interface.

The Blink interface now exposes values that the previous version of the service
did not supply. This patch updates the service and cardboard backend to supply
all the necessary values for WebVR v1. It also takes the opportunity to do some
renaming for consistency. Mainly "device" -> "display" to match the WebVR spec
verbiage.

BUG=389343
==========

to

==========
Updated VRService to match the latest Blink WebVR interface.

The Blink interface now exposes values that the previous version of the service
did not supply. This patch updates the service and cardboard backend to supply
all the necessary values for WebVR v1. It also takes the opportunity to do some
renaming for consistency. Mainly "device" -> "display" to match the WebVR spec
verbiage.

BUG=389343
==========

[commit-bot: I haz the power, 2016-05-20 05:59:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-20 06:00:56]

Description was changed from

==========
Updated VRService to match the latest Blink WebVR interface.

The Blink interface now exposes values that the previous version of the service
did not supply. This patch updates the service and cardboard backend to supply
all the necessary values for WebVR v1. It also takes the opportunity to do some
renaming for consistency. Mainly "device" -> "display" to match the WebVR spec
verbiage.

BUG=389343
==========

to

==========
Updated VRService to match the latest Blink WebVR interface.

The Blink interface now exposes values that the previous version of the service
did not supply. This patch updates the service and cardboard backend to supply
all the necessary values for WebVR v1. It also takes the opportunity to do some
renaming for consistency. Mainly "device" -> "display" to match the WebVR spec
verbiage.

BUG=389343

Committed: https://crrev.com/2f845c7547a8ea5d1edad21be0719f6b6b55f06b
Cr-Commit-Position: refs/heads/master@{#395010}
==========

[commit-bot: I haz the power, 2016-05-20 06:00:58]

Patchset 5 (id:??) landed as
https://crrev.com/2f845c7547a8ea5d1edad21be0719f6b6b55f06b
Cr-Commit-Position: refs/heads/master@{#395010}

[yzshen1, 2016-05-23 15:40:07]

> Generated bindings will only log if the sender writes the wrong size for a
fixed
> array. Maybe we should change it to DCHECK.

Currently we use DLOG_IF(FATAL, ...). It should cause the program to terminate
in debug build.