Ensure navigator.getVRDisplays always resolves.

Ensure navigator.getVRDisplays always resolves.

In the case that the backing VR service is not available the call will now
resolve to an empty array.

BUG=678283
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2614873002
Cr-Commit-Position: refs/heads/master@{#442117}
Committed: https://chromium.googlesource.com/chromium/src/+/d83aedbf719e5346d2de69a79b5a1d7e9b6c8462

[bajones, 2017-01-05 00:12:01]

Description was changed from

==========
Ensure navigator.getVRDisplays always resolves.

In the case that the backing VR service is not available the call will now
resolve to an empty array.

BUG=678283
==========

to

==========
Ensure navigator.getVRDisplays always resolves.

In the case that the backing VR service is not available the call will now
resolve to an empty array.

BUG=678283
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[bajones, 2017-01-05 18:44:53]

bajones@chromium.org changed reviewers:
+ alexmos@chromium.org, rockot@chromium.org

[bajones, 2017-01-05 18:44:54]

alexmos@: Could you review the frame_host change? Per rockot@'s recommendation
we're providing an empty handler for the VR mojo interface on platforms where VR
features aren't enabled yet (desktop, at this point) so that the Blink end
explicitly sees the message pipe close and we can handle it appropriately.

rockot@: Sanity check to ensure this is indeed what you recommended?

[alexmos, 2017-01-05 19:06:00]

On 2017/01/05 18:44:54, bajones wrote:
> alexmos@: Could you review the frame_host change? Per rockot@'s recommendation
> we're providing an empty handler for the VR mojo interface on platforms where
VR
> features aren't enabled yet (desktop, at this point) so that the Blink end
> explicitly sees the message pipe close and we can handle it appropriately.
> 
> rockot@: Sanity check to ensure this is indeed what you recommended?

Looks good, happy to stamp this if rockot@ is happy with it.  Would it be
possible to add a test for this, per https://crbug.com/678283#c3?

[Ken Rockot(use gerrit already), 2017-01-05 19:08:16]

lgtm

[bajones, 2017-01-05 19:30:35]

On 2017/01/05 19:06:00, alexmos wrote:
> Looks good, happy to stamp this if rockot@ is happy with it.  Would it be
> possible to add a test for this, per https://crbug.com/678283#c3?

I have a test that works when run standalone, but times out when run as part of
the layout tests harness. Apparently the mojo mocking interface has a fairly
long timeout for closing interfaces that aren't resolved, which is longer than
the timeout for layout tests to give up on the test as being frozen. I was going
to pursue how to resolve that separately.

[Ken Rockot(use gerrit already), 2017-01-05 20:03:45]

On Thu, Jan 5, 2017 at 11:30 AM, <bajones@chromium.org> wrote:

> On 2017/01/05 19:06:00, alexmos wrote:
> > Looks good, happy to stamp this if rockot@ is happy with it. Would it be
> > possible to add a test for this, per https://crbug.com/678283#c3?
>
> I have a test that works when run standalone, but times out when run as
> part of
> the layout tests harness. Apparently the mojo mocking interface has a
> fairly
> long timeout for closing interfaces that aren't resolved, which is longer
> than
> the timeout for layout tests to give up on the test as being frozen. I was
> going
> to pursue how to resolve that separately.
>

I'm not sure what this means, as I'm not aware of any kind of timeout
associated with closing unbound pipes. I'd be happy to look at the test.


>
>
> https://codereview.chromium.org/2614873002/
>

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

[Ken Rockot(use gerrit already), 2017-01-05 20:03:46]

On Thu, Jan 5, 2017 at 11:30 AM, <bajones@chromium.org> wrote:

> On 2017/01/05 19:06:00, alexmos wrote:
> > Looks good, happy to stamp this if rockot@ is happy with it. Would it be
> > possible to add a test for this, per https://crbug.com/678283#c3?
>
> I have a test that works when run standalone, but times out when run as
> part of
> the layout tests harness. Apparently the mojo mocking interface has a
> fairly
> long timeout for closing interfaces that aren't resolved, which is longer
> than
> the timeout for layout tests to give up on the test as being frozen. I was
> going
> to pursue how to resolve that separately.
>

I'm not sure what this means, as I'm not aware of any kind of timeout
associated with closing unbound pipes. I'd be happy to look at the test.


>
>
> https://codereview.chromium.org/2614873002/
>

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

[bajones, 2017-01-05 20:10:57]

On 2017/01/05 20:03:46, Ken Rockot wrote:
> I'm not sure what this means, as I'm not aware of any kind of timeout
> associated with closing unbound pipes. I'd be happy to look at the test.

It's the mojo mock interface in the layout tests that I'm referring to. I
suspect they work differently.

In any case, after talking it over offline with ddorwin@ seems like we can
accomplish our goals for testing this patch by simply adding a tests that
ensures the native implementation always resolves successfully, even if it's
just returning an empty array. Added that test above.

[ddorwin, 2017-01-05 20:33:02]

test nits

https://codereview.chromium.org/2614873002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/getVRDisplays_native_impl.html (right):

https://codereview.chromium.org/2614873002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getVRDisplays_native_impl.html:1: <!DOCTYPE
html>
Regarding the file name:

It seems that mocking should be the exception. I realize that's not the case
now. Also, this seems like a good WPT so the naming should be generic.

It would be nice if the name provided more information too. For example,
getVRDisplays_always_resolves.

Why do the vr/ test filenames have underscores and mixed case? All/most other
tests seem to use dashes and all lowercase letters.

https://codereview.chromium.org/2614873002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getVRDisplays_native_impl.html:7:
assert_true(devices != null);
Can we check the type?

https://codereview.chromium.org/2614873002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getVRDisplays_native_impl.html:9: }), "Test
that getVRDisplays always resolves to at least an empty array.");
s/to/with/ ?
s/array/sequence/

[bajones, 2017-01-05 21:31:27]

On 2017/01/05 20:33:02, ddorwin wrote:
> It would be nice if the name provided more information too. For example,
> getVRDisplays_always_resolves.

Done.
 
> Why do the vr/ test filenames have underscores and mixed case? All/most other
> tests seem to use dashes and all lowercase letters.

If we're going to address this it feels like material for a separate CL.
 
>
https://codereview.chromium.org/2614873002/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vr/getVRDisplays_native_impl.html:7:
> assert_true(devices != null);
> Can we check the type?

Added a check, but sequences just report back the ubiquitous JS "object" when
asked typeof.
 
>
https://codereview.chromium.org/2614873002/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vr/getVRDisplays_native_impl.html:9: }), "Test
> that getVRDisplays always resolves to at least an empty array.");
> s/to/with/ ?
> s/array/sequence/

Done.

[alexmos, 2017-01-05 21:43:22]

LGTM

[ddorwin, 2017-01-06 02:26:09]

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

[ddorwin, 2017-01-06 02:26:10]

Thanks. Some minor suggestions for improved type checks.

https://codereview.chromium.org/2614873002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/getVRDisplays_always_resolves.html
(right):

https://codereview.chromium.org/2614873002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getVRDisplays_always_resolves.html:8:
assert_equals(typeof devices, "object");
`devices instanceof Array` should work and be a little better.

https://codereview.chromium.org/2614873002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getVRDisplays_always_resolves.html:9:
assert_greater_than_equal(0, devices.length);
You could add:
if (0 < devices.length)
  assert_true(devices[0] instanceof VRDisplay);

This would check the type on devices that have a display.

(If we don't already, we should check this in the mocked tests too.)

https://codereview.chromium.org/2614873002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getVRDisplays_always_resolves.html:10: }),
"Test that getVRDisplays always resolves to at least an empty sequence.");
Should "to" be "with"? (Not sure if you missed this comment previously or
decided not ot change it.)

[bajones, 2017-01-06 22:00:25]

Thanks for the suggestions! Updated.

[bajones, 2017-01-06 22:00:29]

The CQ bit was checked by bajones@chromium.org

[bajones, 2017-01-06 22:00:30]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2614873002/#ps100001
(title: "Addition test nits")

[commit-bot: I haz the power, 2017-01-06 22:00:51]

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-01-07 00:54:22]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483740029854700,
"parent_rev": "fa7637300e305fc861b257729e88da3cde2feaf1", "commit_rev":
"d83aedbf719e5346d2de69a79b5a1d7e9b6c8462"}

[commit-bot: I haz the power, 2017-01-07 00:54:49]

Description was changed from

==========
Ensure navigator.getVRDisplays always resolves.

In the case that the backing VR service is not available the call will now
resolve to an empty array.

BUG=678283
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure navigator.getVRDisplays always resolves.

In the case that the backing VR service is not available the call will now
resolve to an empty array.

BUG=678283
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2017-01-07 00:54:52]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d83aedbf719e5346d2de69a79b5a...

[Tobias Sargeant, 2017-01-09 12:02:07]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2614353002/ by tobiasjs@chromium.org.

The reason for reverting is: Caused builder failures(e.g.
https://uberchromegw.corp.google.com/i/internal.client.clank/builders/x64-bui...)
when building blimp.

undefined reference to `device::mojom::VRService::Name_'.