WebVR: lock focus while presenting to presenting window

WebVR: lock focus while presenting to presenting window

This CL is kept intentionally simple for merging back to M59 to fix the regression, but hides some complexity.

This won't violate the assumption of one VRDisplay having a VSync provider at a time because VrDisplayImpl::GetVRVSyncProvider drops requests from displays that aren't presenting while presentation is active.

However, if a non-presenting VrDisplay actually gets focus while a different VrDisplay is presenting (possibly from directing the controller click to a different display) then that non-presenting VrDisplay will fail to get a VSync provider even after presentation ends because its focused state won't change and the original request will have failed. This means that a page with multiple VrDisplays will under certain degenerate cases fail to resume magic window mode after presenting until the focus changes again.

BUG=710863
Review-Url: https://codereview.chromium.org/2859533003
Cr-Commit-Position: refs/heads/master@{#469804}
Committed: https://chromium.googlesource.com/chromium/src/+/89079d08e53122fc1921517942e259ec1e674d2d

[mthiesse, 2017-05-02 23:17:35]

mthiesse@chromium.org changed reviewers:
+ bajones@chromium.org, klausw@chromium.org

[mthiesse, 2017-05-02 23:17:36]

PTAL. Will add a test tomorrow before submitting but wanted to get some eyeballs
on this.

[klausw, 2017-05-03 16:20:43]

LGTM with nitpicks.

Consider removing the first two files from this CL? The reformatting and added
namespace comment are unrelated to this change.

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

https://codereview.chromium.org/2859533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:562: if (!image_ref.Get() ||
!image_ref->IsTextureBacked()) {
Is this out of paranoia, or have you run into cases where this was null? If the
latter, may be preferable to make a separate change for this to avoid losing
that fix in case this one gets rolled back.

[mthiesse, 2017-05-04 17:29:01]

Added test. PTAL bajones

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

https://codereview.chromium.org/2859533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:562: if (!image_ref.Get() ||
!image_ref->IsTextureBacked()) {
On 2017/05/03 16:20:42, klausw wrote:
> Is this out of paranoia, or have you run into cases where this was null? If
the
> latter, may be preferable to make a separate change for this to avoid losing
> that fix in case this one gets rolled back.

I was running into this occasionally when using the under neon lights webVR
demo, like 30 seconds into running it. Possibly a memory issue, possibly not,
but either way we shouldn't crash. Will fix this in a separate CL.

[mthiesse, 2017-05-04 17:29:27]

mthiesse@chromium.org changed reviewers:
+ dtrainor@chromium.org

[mthiesse, 2017-05-04 17:29:28]

dtrainor@chromium.org: Please review javatest changes.

[mthiesse, 2017-05-04 20:08:34]

dtrainor, would you like us to add an OWNERS file for the vr_shell javatests
directory, or are you happy to keep reviewing these CLs?

[bsheedy, 2017-05-04 20:17:45]

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

[bsheedy, 2017-05-04 20:17:46]

https://codereview.chromium.org/2859533003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java
(right):

https://codereview.chromium.org/2859533003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:254:
* Tests that Daydream controller clicks are registered as screen taps when
Wrong test description?

https://codereview.chromium.org/2859533003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:260:
loadUrl(getHtmlTestFile(testName), PAGE_LOAD_TIMEOUT_S);
Since you're presenting in this test, probably good to assert that a VR display
is found like in other WebVR tests.

[mthiesse, 2017-05-04 20:24:20]

https://codereview.chromium.org/2859533003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java
(right):

https://codereview.chromium.org/2859533003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:254:
* Tests that Daydream controller clicks are registered as screen taps when
On 2017/05/04 20:17:45, bsheedy wrote:
> Wrong test description?

Done.

https://codereview.chromium.org/2859533003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:260:
loadUrl(getHtmlTestFile(testName), PAGE_LOAD_TIMEOUT_S);
On 2017/05/04 20:17:45, bsheedy wrote:
> Since you're presenting in this test, probably good to assert that a VR
display
> is found like in other WebVR tests.

I think tests should be as simple as possible. Another test is already covering
the check that a VR display is found, so we don't need to check that again here.

[bajones, 2017-05-04 20:40:37]

lgtm

[mthiesse, 2017-05-04 21:17:35]

Description was changed from

==========
WebVR: lock focus while presenting to presenting window

This CL is kept intentionally simple for merging back to M59 to fix the
regression, but hides some complexity.

This won't violate the assumption of one VRDisplay having a VSync provider at a
time because VrDisplayImpl::GetVRVSyncProvider drops requests from display that
aren't presenting while presentation is active.

However, if a non-presenting VrDisplay actually gets focus while a different
VrDisplay is presenting (possibly from directing the controller click to a
different display) then that non-presenting VrDisplay will fail to get a VSync
provider even after presentation ends becaues its focused state won't change and
the original request will have failed. This means that a page with multiple
VrDisplays will under certain degenerate cases fail to resume magic window mode
after presenting until the focus changes again.

BUG=710863
==========

to

==========
WebVR: lock focus while presenting to presenting window

This CL is kept intentionally simple for merging back to M59 to fix the
regression, but hides some complexity.

This won't violate the assumption of one VRDisplay having a VSync provider at a
time because VrDisplayImpl::GetVRVSyncProvider drops requests from displays that
aren't presenting while presentation is active.

However, if a non-presenting VrDisplay actually gets focus while a different
VrDisplay is presenting (possibly from directing the controller click to a
different display) then that non-presenting VrDisplay will fail to get a VSync
provider even after presentation ends because its focused state won't change and
the original request will have failed. This means that a page with multiple
VrDisplays will under certain degenerate cases fail to resume magic window mode
after presenting until the focus changes again.

BUG=710863
==========

[David Trainor- moved to gerrit, 2017-05-05 20:14:24]

javatests lgtm.  Let me talk to some of the other owners and see if this is
something we want to start allowing in general.  I'm fine with it but I don't
want to unilaterally decide that.

[mthiesse, 2017-05-05 20:18:33]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-05-05 20:18:33]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org
Link to the patchset: https://codereview.chromium.org/2859533003/#ps60001
(title: "Update comment + fix comment line lengths")

[commit-bot: I haz the power, 2017-05-05 20:19:15]

CQ is trying da patch.

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

[David Trainor- moved to gerrit, 2017-05-05 23:05:02]

Do you have an owners file in your other vr_shell directory?  If so you could
just add an owners in the test directory and point to that one.  We're ok with
you adding an owners file under javatests/.../vr_shell.  Thanks!

[David Trainor- moved to gerrit, 2017-05-05 23:07:06]

On 2017/05/05 23:05:02, David Trainor-ping if over 24h wrote:
> Do you have an owners file in your other vr_shell directory?  If so you could
> just add an owners in the test directory and point to that one.  We're ok with
> you adding an owners file under javatests/.../vr_shell.  Thanks!

Leave the noparent off though please!

[commit-bot: I haz the power, 2017-05-05 23:40:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494015513198060,
"parent_rev": "58af3bbef01008fd3af4481892e19c5da5bc2d74", "commit_rev":
"89079d08e53122fc1921517942e259ec1e674d2d"}

[commit-bot: I haz the power, 2017-05-05 23:41:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494015513198060,
"parent_rev": "58af3bbef01008fd3af4481892e19c5da5bc2d74", "commit_rev":
"89079d08e53122fc1921517942e259ec1e674d2d"}

[commit-bot: I haz the power, 2017-05-05 23:42:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494015513198060,
"parent_rev": "58af3bbef01008fd3af4481892e19c5da5bc2d74", "commit_rev":
"89079d08e53122fc1921517942e259ec1e674d2d"}

[commit-bot: I haz the power, 2017-05-05 23:42:58]

Description was changed from

==========
WebVR: lock focus while presenting to presenting window

This CL is kept intentionally simple for merging back to M59 to fix the
regression, but hides some complexity.

This won't violate the assumption of one VRDisplay having a VSync provider at a
time because VrDisplayImpl::GetVRVSyncProvider drops requests from displays that
aren't presenting while presentation is active.

However, if a non-presenting VrDisplay actually gets focus while a different
VrDisplay is presenting (possibly from directing the controller click to a
different display) then that non-presenting VrDisplay will fail to get a VSync
provider even after presentation ends because its focused state won't change and
the original request will have failed. This means that a page with multiple
VrDisplays will under certain degenerate cases fail to resume magic window mode
after presenting until the focus changes again.

BUG=710863
==========

to

==========
WebVR: lock focus while presenting to presenting window

This CL is kept intentionally simple for merging back to M59 to fix the
regression, but hides some complexity.

This won't violate the assumption of one VRDisplay having a VSync provider at a
time because VrDisplayImpl::GetVRVSyncProvider drops requests from displays that
aren't presenting while presentation is active.

However, if a non-presenting VrDisplay actually gets focus while a different
VrDisplay is presenting (possibly from directing the controller click to a
different display) then that non-presenting VrDisplay will fail to get a VSync
provider even after presentation ends because its focused state won't change and
the original request will have failed. This means that a page with multiple
VrDisplays will under certain degenerate cases fail to resume magic window mode
after presenting until the focus changes again.

BUG=710863

Review-Url: https://codereview.chromium.org/2859533003
Cr-Commit-Position: refs/heads/master@{#469804}
Committed:
https://chromium.googlesource.com/chromium/src/+/89079d08e53122fc1921517942e2...
==========

[commit-bot: I haz the power, 2017-05-05 23:42:59]

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