Clean up WebVR RequestPresent and make callback asynchronous.

Clean up WebVR RequestPresent and make callback asynchronous.

BUG=665006
Committed: https://crrev.com/f97e23678abe358ec7c7241fe5511708be863625
Cr-Commit-Position: refs/heads/master@{#433063}

[mthiesse, 2016-11-16 21:21:04]

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

[mthiesse, 2016-11-16 21:21:05]

PTAL

[bajones, 2016-11-16 21:44:18]

device/vr and chrome/browser/android/vr_shell/ LGTM with one change.

https://codereview.chromium.org/2510873003/diff/1/device/vr/vr_display_impl.cc
File device/vr/vr_display_impl.cc (right):

https://codereview.chromium.org/2510873003/diff/1/device/vr/vr_display_impl.c...
device/vr/vr_display_impl.cc:48: if (!device_->IsAccessAllowed(service_)) {
Realizing now that it's possible that multiple pages could requestPresent
simultaneously, and since vr_shell only handles one callback at a time that'll
leave one of them "stranded". Let's set a flag on device_ that indicates it's
got a pending requestPresent callback, clear the flag in RequestPresentResult,
and add a check for that flag to this if statement.

[mthiesse, 2016-11-16 21:57:53]

https://codereview.chromium.org/2510873003/diff/1/device/vr/vr_display_impl.cc
File device/vr/vr_display_impl.cc (right):

https://codereview.chromium.org/2510873003/diff/1/device/vr/vr_display_impl.c...
device/vr/vr_display_impl.cc:48: if (!device_->IsAccessAllowed(service_)) {
On 2016/11/16 21:44:17, bajones wrote:
> Realizing now that it's possible that multiple pages could requestPresent
> simultaneously, and since vr_shell only handles one callback at a time that'll
> leave one of them "stranded". Let's set a flag on device_ that indicates it's
> got a pending requestPresent callback, clear the flag in RequestPresentResult,
> and add a check for that flag to this if statement.

Probably cleaner if I handle this in vr_shell_delegate, and call the
callback.Run(false) if another callback is pending.

[bajones, 2016-11-16 22:00:30]

On 2016/11/16 21:57:53, mthiesse wrote:
> Probably cleaner if I handle this in vr_shell_delegate, and call the
> callback.Run(false) if another callback is pending.

Agreed, that sounds like the better route. Thanks!

[mthiesse, 2016-11-16 22:03:20]

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

[mthiesse, 2016-11-16 22:03:20]

+tedchoc@chromium.org for java side OWNERS.

[Ted C, 2016-11-16 22:20:51]

lgtm

https://codereview.chromium.org/2510873003/diff/10001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2510873003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:43:
public static final int ENTER_VR_NOT_NECESSARY = 0;
For these, take a look at IntDef's as a way of making a pseudo enum that you
would then annotate your methods with to ensure the return/passed in values are
correct.

https://cs.chromium.org/search/?q=@IntDef+lang:java&sq=package:chromium&type=cs

[mthiesse, 2016-11-17 02:50:26]

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

[mthiesse, 2016-11-17 02:50:26]

bshe@chromium.org PTAL

[bshe, 2016-11-17 02:51:22]

On 2016/11/17 02:50:26, mthiesse wrote:
> mailto:bshe@chromium.org PTAL

lgtm

[mthiesse, 2016-11-17 18:31:17]

Brandon, PTAL at new vr_shell_delegate.cc, and VRDisplay.cpp changes.

[bajones, 2016-11-17 19:54:45]

On 2016/11/17 18:31:17, mthiesse wrote:
> Brandon, PTAL at new vr_shell_delegate.cc, and VRDisplay.cpp changes.

LGTM

[mthiesse, 2016-11-17 19:54:56]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-11-17 19:54:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2510873003/#ps90001
(title: "Fix webVR when VR Shell is disabled")

[commit-bot: I haz the power, 2016-11-17 19:56:32]

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, 2016-11-17 20:31:05]

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

[commit-bot: I haz the power, 2016-11-17 20:31:06]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[mthiesse, 2016-11-17 20:33:47]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-11-17 20:33:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, bajones@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2510873003/#ps110001
(title: "Fix FakeVRDevice")

[commit-bot: I haz the power, 2016-11-17 20:35:36]

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, 2016-11-17 22:05:19]

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

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

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mthiesse, 2016-11-17 22:05:47]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2016-11-17 22:07:09]

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, 2016-11-17 23:14:50]

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

[commit-bot: I haz the power, 2016-11-17 23:14:51]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mthiesse, 2016-11-17 23:15:56]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2016-11-17 23:17: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, 2016-11-18 00:22:04]

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

[commit-bot: I haz the power, 2016-11-18 00:22:06]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mthiesse, 2016-11-18 00:26:15]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2016-11-18 00:28:40]

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, 2016-11-18 03:31:29]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2016-11-18 03:35:05]

Description was changed from

==========
Clean up WebVR RequestPresent and make callback asynchronous.

BUG=665006
==========

to

==========
Clean up WebVR RequestPresent and make callback asynchronous.

BUG=665006

Committed: https://crrev.com/f97e23678abe358ec7c7241fe5511708be863625
Cr-Commit-Position: refs/heads/master@{#433063}
==========

[commit-bot: I haz the power, 2016-11-18 03:35:07]

Patchset 7 (id:??) landed as
https://crrev.com/f97e23678abe358ec7c7241fe5511708be863625
Cr-Commit-Position: refs/heads/master@{#433063}