Add more WebVR layout tests, adjust test format

Add more WebVR layout tests, adjust test format

Adds a handful of new WebVR layout tests using mocking.

Adjusts the vr_test to pass the provided function an async_test handle. This
was done due to how easy it was for the previous promise_test implementation
to resolve earlier than expected, causing the test to pass before all code was
run with no visible difference in the output. The use of the async_test makes
the tests slightly more verbose, but ensures that we're only ending the test
when we expect to.

Add a fix for VRDisplay not initializing layer bounds to defaults correctly.

BUG=675325, 670490, 650703
Review-Url: https://codereview.chromium.org/2617183002
Cr-Commit-Position: refs/heads/master@{#442135}
Committed: https://chromium.googlesource.com/chromium/src/+/ed03ff2faec8339e93a7c48ef5213d4d73fc4f41

[bsheedy, 2017-01-06 18:38:16]

bsheedy@chromium.org changed reviewers:
+ bajones@chromium.org, dglazkov@chromium.org

[bsheedy, 2017-01-06 18:38:17]

bajones@ for making sure the tests add proper coverage

dglazkov@ for OWNERS

I'll likely go back and move some of the repeated code (e.g. the check for
getVRDisplay's return values) to a common place in the near future, but I want
to get these working versions committed before then.

[bsheedy, 2017-01-06 19:19:53]

Description was changed from

==========
Add more WebVR layout tests, adjust test format

Adds a handful of new WebVR layout tests using mocking.

Adjusts the vr_test to pass the provided function an async_test handle. This
was done due to how easy it was for the previous promise_test implementation
to resolve earlier than expected, causing the test to pass before all code was
run with no visible difference in the output. The use of the async_test makes
the tests slightly more verbose, but ensures that we're only ending the test
when we expect to.

Add a fix for VRDisplay not initializing layer bounds to defaults correctly.

BUG=675325,670490
==========

to

==========
Add more WebVR layout tests, adjust test format

Adds a handful of new WebVR layout tests using mocking.

Adjusts the vr_test to pass the provided function an async_test handle. This
was done due to how easy it was for the previous promise_test implementation
to resolve earlier than expected, causing the test to pass before all code was
run with no visible difference in the output. The use of the async_test makes
the tests slightly more verbose, but ensures that we're only ending the test
when we expect to.

Add a fix for VRDisplay not initializing layer bounds to defaults correctly.

BUG=675325,670490,650703
==========

[bajones, 2017-01-06 20:20:25]

Generally looking good, with some nits:

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/exitPresent_reject_notpresenting.html
(right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/exitPresent_reject_notpresenting.html:23: },
"requestPresent succeeded unexpectedly");
Should be exitPresent

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/getEyeParameters_match.html (right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getEyeParameters_match.html:34:
assert_equals(givenFov.upDegrees, returnedFov.upDegrees);
It seems inconsistent to use approx_equals above but equals here, given that
these are also floats.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/getLayers_presenting.html (right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getLayers_presenting.html:16: assert_equals(1,
displays.length);
For consistency let's make sure the assert's arguments are consistently in the
order (actual, expected) throughout the tests.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getLayers_presenting.html:17: },
"getVRDisplays returned correct results");
Seems redundant to have this check in every single test.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getLayers_presenting.html:31:
assert_equals(layer.source, webglCanvas);
For consistency let's swap the order of these three assert's arguments. (so
assert_equals(4, layer.leftBounds.length), like the displays.length assert
above)

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/getVRDisplays_one_display.html (right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getVRDisplays_one_display.html:18:
assert_equals(display.depthNear, 0.01);
Again, let's try to consistently use approx_equals for floats.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/vr/requestPresent_resolve_repeatwithgesture.html
(right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/requestPresent_resolve_repeatwithgesture.html:1:
<!DOCTYPE html>
Not sure this test is useful. We test elsewhere that requestPresent can run
within a gesture, and then we test to make sure the edge case (repeated calls
without a gesture) also works. As such this doesn't feel like it add any new
coverage.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/resources/fake-vr-displays.js (right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/resources/fake-vr-displays.js:44:
hasExternalDisplay : false,
Room scale devices currently all have external displays (Vive, Rift). Probably
worth setting this to true just so we can check that it comes through correctly
in one of the tests.

[bsheedy, 2017-01-06 22:14:19]

Addressed comments.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/exitPresent_reject_notpresenting.html
(right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/exitPresent_reject_notpresenting.html:23: },
"requestPresent succeeded unexpectedly");
On 2017/01/06 20:20:25, bajones wrote:
> Should be exitPresent

Done.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/getEyeParameters_match.html (right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getEyeParameters_match.html:34:
assert_equals(givenFov.upDegrees, returnedFov.upDegrees);
On 2017/01/06 20:20:25, bajones wrote:
> It seems inconsistent to use approx_equals above but equals here, given that
> these are also floats.

Done.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/getLayers_presenting.html (right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getLayers_presenting.html:16: assert_equals(1,
displays.length);
On 2017/01/06 20:20:25, bajones wrote:
> For consistency let's make sure the assert's arguments are consistently in the
> order (actual, expected) throughout the tests.

Done.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getLayers_presenting.html:17: },
"getVRDisplays returned correct results");
On 2017/01/06 20:20:25, bajones wrote:
> Seems redundant to have this check in every single test.

Done, removed from tests except for ones specifically testing getVRDisplays().

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getLayers_presenting.html:31:
assert_equals(layer.source, webglCanvas);
On 2017/01/06 20:20:25, bajones wrote:
> For consistency let's swap the order of these three assert's arguments. (so
> assert_equals(4, layer.leftBounds.length), like the displays.length assert
> above)

If we want assert_equals(actual, expected), then these should be in the correct
order already.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/getVRDisplays_one_display.html (right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/getVRDisplays_one_display.html:18:
assert_equals(display.depthNear, 0.01);
On 2017/01/06 20:20:25, bajones wrote:
> Again, let's try to consistently use approx_equals for floats.

Done.

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/vr/requestPresent_resolve_repeatwithgesture.html
(right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/requestPresent_resolve_repeatwithgesture.html:1:
<!DOCTYPE html>
On 2017/01/06 20:20:25, bajones wrote:
> Not sure this test is useful. We test elsewhere that requestPresent can run
> within a gesture, and then we test to make sure the edge case (repeated calls
> without a gesture) also works. As such this doesn't feel like it add any new
> coverage.

This was to address a specific bug that popped up a while back. If you called
requestPresent with a gesture, then called requestPresent again without a user
gesture, rendering got messed up
(https://bugs.chromium.org/p/chromium/issues/detail?id=654907). If you called
requestPresent twice with a gesture, then the browser crashed
(https://bugs.chromium.org/p/chromium/issues/detail?id=654909).

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/resources/fake-vr-displays.js (right):

https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/resources/fake-vr-displays.js:44:
hasExternalDisplay : false,
On 2017/01/06 20:20:25, bajones wrote:
> Room scale devices currently all have external displays (Vive, Rift). Probably
> worth setting this to true just so we can check that it comes through
correctly
> in one of the tests.

Done.

[bajones, 2017-01-06 23:16:16]

On 2017/01/06 22:14:19, bsheedy wrote:
> Addressed comments.
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/vr/exitPresent_reject_notpresenting.html
> (right):
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vr/exitPresent_reject_notpresenting.html:23: },
> "requestPresent succeeded unexpectedly");
> On 2017/01/06 20:20:25, bajones wrote:
> > Should be exitPresent
> 
> Done.
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/vr/getEyeParameters_match.html (right):
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vr/getEyeParameters_match.html:34:
> assert_equals(givenFov.upDegrees, returnedFov.upDegrees);
> On 2017/01/06 20:20:25, bajones wrote:
> > It seems inconsistent to use approx_equals above but equals here, given that
> > these are also floats.
> 
> Done.
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/vr/getLayers_presenting.html (right):
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vr/getLayers_presenting.html:16:
assert_equals(1,
> displays.length);
> On 2017/01/06 20:20:25, bajones wrote:
> > For consistency let's make sure the assert's arguments are consistently in
the
> > order (actual, expected) throughout the tests.
> 
> Done.
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vr/getLayers_presenting.html:17: },
> "getVRDisplays returned correct results");
> On 2017/01/06 20:20:25, bajones wrote:
> > Seems redundant to have this check in every single test.
> 
> Done, removed from tests except for ones specifically testing getVRDisplays().
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vr/getLayers_presenting.html:31:
> assert_equals(layer.source, webglCanvas);
> On 2017/01/06 20:20:25, bajones wrote:
> > For consistency let's swap the order of these three assert's arguments. (so
> > assert_equals(4, layer.leftBounds.length), like the displays.length assert
> > above)
> 
> If we want assert_equals(actual, expected), then these should be in the
correct
> order already.
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/vr/getVRDisplays_one_display.html (right):
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vr/getVRDisplays_one_display.html:18:
> assert_equals(display.depthNear, 0.01);
> On 2017/01/06 20:20:25, bajones wrote:
> > Again, let's try to consistently use approx_equals for floats.
> 
> Done.
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/vr/requestPresent_resolve_repeatwithgesture.html
> (right):
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/vr/requestPresent_resolve_repeatwithgesture.html:1:
> <!DOCTYPE html>
> On 2017/01/06 20:20:25, bajones wrote:
> > Not sure this test is useful. We test elsewhere that requestPresent can run
> > within a gesture, and then we test to make sure the edge case (repeated
calls
> > without a gesture) also works. As such this doesn't feel like it add any new
> > coverage.
> 
> This was to address a specific bug that popped up a while back. If you called
> requestPresent with a gesture, then called requestPresent again without a user
> gesture, rendering got messed up
> (https://bugs.chromium.org/p/chromium/issues/detail?id=654907). If you called
> requestPresent twice with a gesture, then the browser crashed
> (https://bugs.chromium.org/p/chromium/issues/detail?id=654909).
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/vr/resources/fake-vr-displays.js (right):
> 
>
https://codereview.chromium.org/2617183002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vr/resources/fake-vr-displays.js:44:
> hasExternalDisplay : false,
> On 2017/01/06 20:20:25, bajones wrote:
> > Room scale devices currently all have external displays (Vive, Rift).
Probably
> > worth setting this to true just so we can check that it comes through
> correctly
> > in one of the tests.
> 
> Done.

LGTM, thanks!

[bsheedy, 2017-01-06 23:18:00]

On 2017/01/06 23:16:16, bajones wrote:
> LGTM, thanks!

Oh, I guess you count as an OWNER for VR stuff - that makes this review simpler.

[bsheedy, 2017-01-06 23:18:07]

The CQ bit was checked by bsheedy@chromium.org

[commit-bot: I haz the power, 2017-01-06 23:18:32]

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

[bsheedy, 2017-01-07 00:27:30]

The CQ bit was checked by bsheedy@chromium.org

[bsheedy, 2017-01-07 00:27:31]

The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2617183002/#ps60001
(title: "Add missing constants file")

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

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 01:55:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483748850992370,
"parent_rev": "e21b84f9d52d63c3a5e4d89d8316534669c756af", "commit_rev":
"ed03ff2faec8339e93a7c48ef5213d4d73fc4f41"}

[commit-bot: I haz the power, 2017-01-07 01:55:59]

Description was changed from

==========
Add more WebVR layout tests, adjust test format

Adds a handful of new WebVR layout tests using mocking.

Adjusts the vr_test to pass the provided function an async_test handle. This
was done due to how easy it was for the previous promise_test implementation
to resolve earlier than expected, causing the test to pass before all code was
run with no visible difference in the output. The use of the async_test makes
the tests slightly more verbose, but ensures that we're only ending the test
when we expect to.

Add a fix for VRDisplay not initializing layer bounds to defaults correctly.

BUG=675325,670490,650703
==========

to

==========
Add more WebVR layout tests, adjust test format

Adds a handful of new WebVR layout tests using mocking.

Adjusts the vr_test to pass the provided function an async_test handle. This
was done due to how easy it was for the previous promise_test implementation
to resolve earlier than expected, causing the test to pass before all code was
run with no visible difference in the output. The use of the async_test makes
the tests slightly more verbose, but ensures that we're only ending the test
when we expect to.

Add a fix for VRDisplay not initializing layer bounds to defaults correctly.

BUG=675325,670490,650703

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

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

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

[vitaliii, 2017-01-09 10:58:41]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2616403002/ by vitaliii@chromium.org.

The reason for reverting is: Two of the added tests crash:
vr/requestAnimationFrame_invalidhandle.html
vr/requestAnimationFrame_unregister.html

BUG=679288.