Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2701523008:
Intercept WebVR api calls and check for VrCore compatibility. (Closed)
DescriptionIntercept WebVR api calls and check for VrCore compatibility.
Proof of concept only, not ready for submitting.
BUG=670166
Review-Url: https://codereview.chromium.org/2701523008
Cr-Commit-Position: refs/heads/master@{#454482}
Committed: https://chromium.googlesource.com/chromium/src/+/24b50378a13f0099e231bcbfefaea67eec005ab3
Patch Set 1 #
Total comments: 12
Patch Set 2 : refactor things around a bit, try not to prompt when there is no vr support #Patch Set 3 : rebase on delegate lifecycle changes and simplify #Patch Set 4 : Fix branches so dependencies are shown #Patch Set 5 : Small fix #Patch Set 6 : rebase #Patch Set 7 : rebase on latest patch #
Depends on Patchset: Messages
Total messages: 31 (17 generated)
Description was changed from ========== Intercept WebVR api calls and check for VrCore compatibility. Proof of concept only, not ready for full review. BUG=670166 ========== to ========== Intercept WebVR api calls and check for VrCore compatibility. Proof of concept only, not ready for submitting. BUG=670166 ==========
amp@chromium.org changed reviewers: + bajones@chromium.org, billorr@chromium.org, mthiesse@chromium.org
I don't think this is the best way to intercept calls, but I was able to get it working this way. SetDeviceProvider is called eventually when a site uses the WebVR GetDevices API. A better solution might be to try and create some kind of observer on the WebVR API and have it call back into VrShellDelegate directly. We may need to change VrShellDelegate native life-cycle slightly, though, as it currently (behavior before my change here) only gets created if the everything is up to date, but we will need it to always be registered if we are going to use it for prompting users to update their libraries. WDYT?
https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (left): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:163: if (mVrSupportLevel == VR_NOT_AVAILABLE) return; What if we added a new VrSupportLevel for VR_DAYDREAM_NEEDS_UPDATE, so that don't create the native VrShellDelegate when it's not needed. https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:538: @CalledByNative I think you would want to check for the support level being the new VR_DAYDREAM_NEEDS_UPDATE instead of checking isVrCoreCompatible. If you use isVrCoreCompatible(), then aren't you breaking cardboard support?
https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (left): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:163: if (mVrSupportLevel == VR_NOT_AVAILABLE) return; On 2017/02/21 15:56:18, mthiesse wrote: > What if we added a new VrSupportLevel for VR_DAYDREAM_NEEDS_UPDATE, so that > don't create the native VrShellDelegate when it's not needed. Aside from limiting the creation of thenative VrShellDelegate, that's kind of what I did in https://codereview.chromium.org/2699163003/ I could update that to update the overall vr support level, but I don't think we want the general vr support level to be sdk specific. https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:538: @CalledByNative On 2017/02/21 15:56:18, mthiesse wrote: > I think you would want to check for the support level being the new > VR_DAYDREAM_NEEDS_UPDATE instead of checking isVrCoreCompatible. If you use > isVrCoreCompatible(), then aren't you breaking cardboard support? I could wire it up to be a bit more specific. I'm not sure this method of calling back on 'SetDeviceProvider' is a good idea and perhaps we should just setup a WebVR API observer (which could then check the VR support level and show the infobar as needed).
https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (left): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:163: if (mVrSupportLevel == VR_NOT_AVAILABLE) return; On 2017/02/21 19:50:03, amp wrote: > On 2017/02/21 15:56:18, mthiesse wrote: > > What if we added a new VrSupportLevel for VR_DAYDREAM_NEEDS_UPDATE, so that > > don't create the native VrShellDelegate when it's not needed. > > Aside from limiting the creation of thenative VrShellDelegate, that's kind of > what I did in https://codereview.chromium.org/2699163003/ > > I could update that to update the overall vr support level, but I don't think we > want the general vr support level to be sdk specific. Well one concern with this change is that now we're creating the native VrShellDelegate on platforms where daydream support isn't currently possible (or even planned to be possible). Do we have a way to check that VR support is even possible before we initialize more VR code than is necessary? Maybe I'm crazy to worry about this at all, but it is unnecessary memory usage on lower-end devices. https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:538: @CalledByNative On 2017/02/21 19:50:03, amp wrote: > On 2017/02/21 15:56:18, mthiesse wrote: > > I think you would want to check for the support level being the new > > VR_DAYDREAM_NEEDS_UPDATE instead of checking isVrCoreCompatible. If you use > > isVrCoreCompatible(), then aren't you breaking cardboard support? > > I could wire it up to be a bit more specific. > > I'm not sure this method of calling back on 'SetDeviceProvider' is a good idea > and perhaps we should just setup a WebVR API observer (which could then check > the VR support level and show the infobar as needed). I don't follow what a WebVR API Observer would look like, or what function it would serve. Checking on SetDeviceProvider seems fine. If support changes we could then remove the device and destroy the delegate, rather than having an observer API.
https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (left): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:163: if (mVrSupportLevel == VR_NOT_AVAILABLE) return; On 2017/02/21 20:23:00, mthiesse wrote: > On 2017/02/21 19:50:03, amp wrote: > > On 2017/02/21 15:56:18, mthiesse wrote: > > > What if we added a new VrSupportLevel for VR_DAYDREAM_NEEDS_UPDATE, so that > > > don't create the native VrShellDelegate when it's not needed. > > > > Aside from limiting the creation of thenative VrShellDelegate, that's kind of > > what I did in https://codereview.chromium.org/2699163003/ > > > > I could update that to update the overall vr support level, but I don't think > we > > want the general vr support level to be sdk specific. > > Well one concern with this change is that now we're creating the native > VrShellDelegate on platforms where daydream support isn't currently possible (or > even planned to be possible). Do we have a way to check that VR support is even > possible before we initialize more VR code than is necessary? I don't think we do, at least not that I'm aware of. > > Maybe I'm crazy to worry about this at all, but it is unnecessary memory usage > on lower-end devices. I agree that is one of the main concerns of this change. The current infobar implementation is triggered through the Java side, so if we want to use the current implementation we would have to somehow get the native WebVR API calls to call back up to the Java side. The problem is that if VrCore is not up to date (or not installed), the native vr delegate isn't created and then there is no link back to the java side to create the infobar asking the user to install it or update it. We could try to switch to a native infobar implementation (which I'm not sure exactly how it works, but it would have it's own java integrations for actually displaying the visuals correctly) which wouldn't require a native side integration in our vr shell code. It would require wiring the WebVR API up to 'something', though, and we could have that be a simpler (in theory smaller memory footprint) listener of the WebVR API that could figure out if VR support is possible and whether to notify the user about it or not. https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:538: @CalledByNative On 2017/02/21 20:23:00, mthiesse wrote: > On 2017/02/21 19:50:03, amp wrote: > > On 2017/02/21 15:56:18, mthiesse wrote: > > > I think you would want to check for the support level being the new > > > VR_DAYDREAM_NEEDS_UPDATE instead of checking isVrCoreCompatible. If you use > > > isVrCoreCompatible(), then aren't you breaking cardboard support? > > > > I could wire it up to be a bit more specific. > > > > I'm not sure this method of calling back on 'SetDeviceProvider' is a good idea > > and perhaps we should just setup a WebVR API observer (which could then check > > the VR support level and show the infobar as needed). > > I don't follow what a WebVR API Observer would look like, or what function it > would serve. Checking on SetDeviceProvider seems fine. If support changes we > could then remove the device and destroy the delegate, rather than having an > observer API. See comment above on not initializing native vr delegate if possible. If it is reasonable to assume that SetDeviceProdiver is always called before any WebVR API (this doesn't intuitively make sense to me just from the names on the methods) then the 'Observer' of the API could be a static vr support checker called from the WebVR API side implementation, rather than adding full support of generic observers which could be registered etc. I'm not sure how the dependencies would work out, though, so we may still need a generic observer pattern so the API impl code can call back into the browser code (via registered observers).
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (left): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:163: if (mVrSupportLevel == VR_NOT_AVAILABLE) return; On 2017/02/21 22:15:09, amp wrote: > On 2017/02/21 20:23:00, mthiesse wrote: > > On 2017/02/21 19:50:03, amp wrote: > > > On 2017/02/21 15:56:18, mthiesse wrote: > > > > What if we added a new VrSupportLevel for VR_DAYDREAM_NEEDS_UPDATE, so > that > > > > don't create the native VrShellDelegate when it's not needed. > > > > > > Aside from limiting the creation of thenative VrShellDelegate, that's kind > of > > > what I did in https://codereview.chromium.org/2699163003/ > > > > > > I could update that to update the overall vr support level, but I don't > think > > we > > > want the general vr support level to be sdk specific. > > > > Well one concern with this change is that now we're creating the native > > VrShellDelegate on platforms where daydream support isn't currently possible > (or > > even planned to be possible). Do we have a way to check that VR support is > even > > possible before we initialize more VR code than is necessary? > > I don't think we do, at least not that I'm aware of. > > > > Maybe I'm crazy to worry about this at all, but it is unnecessary memory usage > > on lower-end devices. > > I agree that is one of the main concerns of this change. The current infobar > implementation is triggered through the Java side, so if we want to use the > current implementation we would have to somehow get the native WebVR API calls > to call back up to the Java side. > > The problem is that if VrCore is not up to date (or not installed), the native > vr delegate isn't created and then there is no link back to the java side to > create the infobar asking the user to install it or update it. > > We could try to switch to a native infobar implementation (which I'm not sure > exactly how it works, but it would have it's own java integrations for actually > displaying the visuals correctly) which wouldn't require a native side > integration in our vr shell code. > > It would require wiring the WebVR API up to 'something', though, and we could > have that be a simpler (in theory smaller memory footprint) listener of the > WebVR API that could figure out if VR support is possible and whether to notify > the user about it or not. I think something like `VR_[SDK_]NEEDS_UPDATE` would address the need without being platform-specific. This will help us separate no support (J) from needs an install/update (K+). Does that give us enough information to disambiguate the install/update, UI, though? Regarding the memory usage issue: * We are already doing this for a subset of (usually high-end) devices regardless of whether VR will be used. We should fix this. * We should avoid doing this on the majority of devices. The issue is described in https://bugs.chromium.org/p/chromium/issues/detail?id=655297#c17. mthiesse is going to work on that. I think we should try to drive this CL to a state where it's ready to land on top of his fix. Hopefully, mthiesse can provide guidance on what that will look like. https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:538: @CalledByNative On 2017/02/21 22:15:09, amp wrote: > On 2017/02/21 20:23:00, mthiesse wrote: > > On 2017/02/21 19:50:03, amp wrote: > > > On 2017/02/21 15:56:18, mthiesse wrote: > > > > I think you would want to check for the support level being the new > > > > VR_DAYDREAM_NEEDS_UPDATE instead of checking isVrCoreCompatible. If you > use > > > > isVrCoreCompatible(), then aren't you breaking cardboard support? > > > > > > I could wire it up to be a bit more specific. > > > > > > I'm not sure this method of calling back on 'SetDeviceProvider' is a good > idea > > > and perhaps we should just setup a WebVR API observer (which could then > check > > > the VR support level and show the infobar as needed). > > > > I don't follow what a WebVR API Observer would look like, or what function it > > would serve. Checking on SetDeviceProvider seems fine. If support changes we > > could then remove the device and destroy the delegate, rather than having an > > observer API. > > See comment above on not initializing native vr delegate if possible. > > If it is reasonable to assume that SetDeviceProdiver is always called before any > WebVR API (this doesn't intuitively make sense to me just from the names on the > methods) then the 'Observer' of the API could be a static vr support checker > called from the WebVR API side implementation, rather than adding full support > of generic observers which could be registered etc. > > I'm not sure how the dependencies would work out, though, so we may still need a > generic observer pattern so the API impl code can call back into the browser > code (via registered observers). My understanding of https://bugs.chromium.org/p/chromium/issues/detail?id=655297#c17 is that we would lazily invoke this code when it is needed, such as getVRDisplays(). An advantage of doing it here is that all VR-related code will benefit from the prompts rather than having to add checks or observers to each VR-related API or feature.
amp@chromium.org changed reviewers: + bshe@chromium.org
Thanks for the feedback. I updated the change a bit to try adding in a VR_UPDATE_NEEDED level and refactoring the finch experiment check so we didn't do it twice. https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (left): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:163: if (mVrSupportLevel == VR_NOT_AVAILABLE) return; On 2017/02/23 23:23:32, ddorwin wrote: > On 2017/02/21 22:15:09, amp wrote: > > On 2017/02/21 20:23:00, mthiesse wrote: > > > On 2017/02/21 19:50:03, amp wrote: > > > > On 2017/02/21 15:56:18, mthiesse wrote: > > > > > What if we added a new VrSupportLevel for VR_DAYDREAM_NEEDS_UPDATE, so > > that > > > > > don't create the native VrShellDelegate when it's not needed. > > > > > > > > Aside from limiting the creation of thenative VrShellDelegate, that's kind > > of > > > > what I did in https://codereview.chromium.org/2699163003/ > > > > > > > > I could update that to update the overall vr support level, but I don't > > think > > > we > > > > want the general vr support level to be sdk specific. > > > > > > Well one concern with this change is that now we're creating the native > > > VrShellDelegate on platforms where daydream support isn't currently possible > > (or > > > even planned to be possible). Do we have a way to check that VR support is > > even > > > possible before we initialize more VR code than is necessary? > > > > I don't think we do, at least not that I'm aware of. > > > > > > Maybe I'm crazy to worry about this at all, but it is unnecessary memory > usage > > > on lower-end devices. > > > > I agree that is one of the main concerns of this change. The current infobar > > implementation is triggered through the Java side, so if we want to use the > > current implementation we would have to somehow get the native WebVR API calls > > to call back up to the Java side. > > > > The problem is that if VrCore is not up to date (or not installed), the native > > vr delegate isn't created and then there is no link back to the java side to > > create the infobar asking the user to install it or update it. > > > > We could try to switch to a native infobar implementation (which I'm not sure > > exactly how it works, but it would have it's own java integrations for > actually > > displaying the visuals correctly) which wouldn't require a native side > > integration in our vr shell code. > > > > It would require wiring the WebVR API up to 'something', though, and we could > > have that be a simpler (in theory smaller memory footprint) listener of the > > WebVR API that could figure out if VR support is possible and whether to > notify > > the user about it or not. > > I think something like `VR_[SDK_]NEEDS_UPDATE` would address the need without > being platform-specific. This will help us separate no support (J) from needs an > install/update (K+). > > Does that give us enough information to disambiguate the install/update, UI, > though? I think we can update the VR_SUPPORT_LEVEL to include a NEEDS_UPDATE level that can separate out devices which will never have support. Then when we actually show the prompt the VrCoreCompatibility checker will differentiate between update vs install. > > Regarding the memory usage issue: > * We are already doing this for a subset of (usually high-end) devices > regardless of whether VR will be used. We should fix this. > * We should avoid doing this on the majority of devices. > > The issue is described in > https://bugs.chromium.org/p/chromium/issues/detail?id=655297#c17. mthiesse is > going to work on that. I think we should try to drive this CL to a state where > it's ready to land on top of his fix. Hopefully, mthiesse can provide guidance > on what that will look like. That sounds great. I was trying to look at how to lazy initialize things, but it was a little much for me to wrap my head around. I'll sync up with Michael about what that will look like and build this change on top of it. https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:538: @CalledByNative On 2017/02/23 23:23:31, ddorwin wrote: > On 2017/02/21 22:15:09, amp wrote: > > On 2017/02/21 20:23:00, mthiesse wrote: > > > On 2017/02/21 19:50:03, amp wrote: > > > > On 2017/02/21 15:56:18, mthiesse wrote: > > > > > I think you would want to check for the support level being the new > > > > > VR_DAYDREAM_NEEDS_UPDATE instead of checking isVrCoreCompatible. If you > > use > > > > > isVrCoreCompatible(), then aren't you breaking cardboard support? > > > > > > > > I could wire it up to be a bit more specific. > > > > > > > > I'm not sure this method of calling back on 'SetDeviceProvider' is a good > > idea > > > > and perhaps we should just setup a WebVR API observer (which could then > > check > > > > the VR support level and show the infobar as needed). > > > > > > I don't follow what a WebVR API Observer would look like, or what function > it > > > would serve. Checking on SetDeviceProvider seems fine. If support changes we > > > could then remove the device and destroy the delegate, rather than having an > > > observer API. > > > > See comment above on not initializing native vr delegate if possible. > > > > If it is reasonable to assume that SetDeviceProdiver is always called before > any > > WebVR API (this doesn't intuitively make sense to me just from the names on > the > > methods) then the 'Observer' of the API could be a static vr support checker > > called from the WebVR API side implementation, rather than adding full support > > of generic observers which could be registered etc. > > > > I'm not sure how the dependencies would work out, though, so we may still need > a > > generic observer pattern so the API impl code can call back into the browser > > code (via registered observers). > > My understanding of > https://bugs.chromium.org/p/chromium/issues/detail?id=655297#c17 is that we > would lazily invoke this code when it is needed, such as getVRDisplays(). > > An advantage of doing it here is that all VR-related code will benefit from the > prompts rather than having to add checks or observers to each VR-related API or > feature. Ok. I'll wait for the underlying lazy init parts from Michael. I agree that doing it here once rather than for each API is cleaner in that regard and if the dependencies work out then I'm fine with a more hard wired path like I have now. I did change it from the 'isVrCoreCompatible' check to something that returns the VrSupportLevel as mentioned in the other comment.
On 2017/02/24 01:20:01, amp wrote: > Thanks for the feedback. I updated the change a bit to try adding in a > VR_UPDATE_NEEDED level and refactoring the finch experiment check so we didn't > do it twice. > > https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java > (left): > > https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:163: > if (mVrSupportLevel == VR_NOT_AVAILABLE) return; > On 2017/02/23 23:23:32, ddorwin wrote: > > On 2017/02/21 22:15:09, amp wrote: > > > On 2017/02/21 20:23:00, mthiesse wrote: > > > > On 2017/02/21 19:50:03, amp wrote: > > > > > On 2017/02/21 15:56:18, mthiesse wrote: > > > > > > What if we added a new VrSupportLevel for VR_DAYDREAM_NEEDS_UPDATE, so > > > that > > > > > > don't create the native VrShellDelegate when it's not needed. > > > > > > > > > > Aside from limiting the creation of thenative VrShellDelegate, that's > kind > > > of > > > > > what I did in https://codereview.chromium.org/2699163003/ > > > > > > > > > > I could update that to update the overall vr support level, but I don't > > > think > > > > we > > > > > want the general vr support level to be sdk specific. > > > > > > > > Well one concern with this change is that now we're creating the native > > > > VrShellDelegate on platforms where daydream support isn't currently > possible > > > (or > > > > even planned to be possible). Do we have a way to check that VR support is > > > even > > > > possible before we initialize more VR code than is necessary? > > > > > > I don't think we do, at least not that I'm aware of. > > > > > > > > Maybe I'm crazy to worry about this at all, but it is unnecessary memory > > usage > > > > on lower-end devices. > > > > > > I agree that is one of the main concerns of this change. The current > infobar > > > implementation is triggered through the Java side, so if we want to use the > > > current implementation we would have to somehow get the native WebVR API > calls > > > to call back up to the Java side. > > > > > > The problem is that if VrCore is not up to date (or not installed), the > native > > > vr delegate isn't created and then there is no link back to the java side to > > > create the infobar asking the user to install it or update it. > > > > > > We could try to switch to a native infobar implementation (which I'm not > sure > > > exactly how it works, but it would have it's own java integrations for > > actually > > > displaying the visuals correctly) which wouldn't require a native side > > > integration in our vr shell code. > > > > > > It would require wiring the WebVR API up to 'something', though, and we > could > > > have that be a simpler (in theory smaller memory footprint) listener of the > > > WebVR API that could figure out if VR support is possible and whether to > > notify > > > the user about it or not. > > > > I think something like `VR_[SDK_]NEEDS_UPDATE` would address the need without > > being platform-specific. This will help us separate no support (J) from needs > an > > install/update (K+). > > > > Does that give us enough information to disambiguate the install/update, UI, > > though? > I think we can update the VR_SUPPORT_LEVEL to include a NEEDS_UPDATE level that > can separate out devices which will never have support. Then when we actually > show the prompt the VrCoreCompatibility checker will differentiate between > update vs install. > > > > Regarding the memory usage issue: > > * We are already doing this for a subset of (usually high-end) devices > > regardless of whether VR will be used. We should fix this. > > * We should avoid doing this on the majority of devices. > > > > The issue is described in > > https://bugs.chromium.org/p/chromium/issues/detail?id=655297#c17. mthiesse is > > going to work on that. I think we should try to drive this CL to a state where > > it's ready to land on top of his fix. Hopefully, mthiesse can provide guidance > > on what that will look like. > > That sounds great. I was trying to look at how to lazy initialize things, but > it was a little much for me to wrap my head around. I'll sync up with Michael > about what that will look like and build this change on top of it. > > https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java > (right): > > https://codereview.chromium.org/2701523008/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:538: > @CalledByNative > On 2017/02/23 23:23:31, ddorwin wrote: > > On 2017/02/21 22:15:09, amp wrote: > > > On 2017/02/21 20:23:00, mthiesse wrote: > > > > On 2017/02/21 19:50:03, amp wrote: > > > > > On 2017/02/21 15:56:18, mthiesse wrote: > > > > > > I think you would want to check for the support level being the new > > > > > > VR_DAYDREAM_NEEDS_UPDATE instead of checking isVrCoreCompatible. If > you > > > use > > > > > > isVrCoreCompatible(), then aren't you breaking cardboard support? > > > > > > > > > > I could wire it up to be a bit more specific. > > > > > > > > > > I'm not sure this method of calling back on 'SetDeviceProvider' is a > good > > > idea > > > > > and perhaps we should just setup a WebVR API observer (which could then > > > check > > > > > the VR support level and show the infobar as needed). > > > > > > > > I don't follow what a WebVR API Observer would look like, or what function > > it > > > > would serve. Checking on SetDeviceProvider seems fine. If support changes > we > > > > could then remove the device and destroy the delegate, rather than having > an > > > > observer API. > > > > > > See comment above on not initializing native vr delegate if possible. > > > > > > If it is reasonable to assume that SetDeviceProdiver is always called before > > any > > > WebVR API (this doesn't intuitively make sense to me just from the names on > > the > > > methods) then the 'Observer' of the API could be a static vr support checker > > > called from the WebVR API side implementation, rather than adding full > support > > > of generic observers which could be registered etc. > > > > > > I'm not sure how the dependencies would work out, though, so we may still > need > > a > > > generic observer pattern so the API impl code can call back into the browser > > > code (via registered observers). > > > > My understanding of > > https://bugs.chromium.org/p/chromium/issues/detail?id=655297#c17 is that we > > would lazily invoke this code when it is needed, such as getVRDisplays(). > > > > An advantage of doing it here is that all VR-related code will benefit from > the > > prompts rather than having to add checks or observers to each VR-related API > or > > feature. > > Ok. I'll wait for the underlying lazy init parts from Michael. I agree that > doing it here once rather than for each API is cleaner in that regard and if the > dependencies work out then I'm fine with a more hard wired path like I have now. > > I did change it from the 'isVrCoreCompatible' check to something that returns > the VrSupportLevel as mentioned in the other comment. High level question, is there a reason not to do the version check before create a non presenting delegate? Creating such delegate would create a GvrLayout, which needs the native library in VrCore. Essentially, if you create a GvrLayout, you need the right version native library from VrCore. For presenting delegate, I believe you have already covered it in your previous CL.
On 2017/02/24 20:58:55, bshe wrote: > High level question, is there a reason not to do the version check before create > a non > presenting delegate? Creating such delegate would create a GvrLayout, which > needs > the native library in VrCore. Essentially, if you create a GvrLayout, you need > the right > version native library from VrCore. > For presenting delegate, I believe you have already covered it in your previous > CL. My latest patch does check before creating the non presenting delegate, see SetDeviceProvider in native VrShellDelegate. That's why I had to update the clean up of native vr shell delegate to check if a non presenting delegate exsits before trying to clean it up.
amp@chromium.org changed reviewers: - bajones@chromium.org, billorr@chromium.org
PTAL (removing a few reviewers) Latest patch is on top of Michael's lifecycle changes and allowed me to simplify quite a bit. This no longer requires native side changes and instead we check before the non-presenting delegate is created to intercept WebVR api calls. I've verified it on a Pixel, I'll try out some other devices to see how it holds up in more scenario's.
lgtm
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by amp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2701523008/#ps120001 (title: "rebase on latest patch")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2727873002 Patch 120001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488508864758870,
"parent_rev": "1a3b9e6a70c04989ec277114f50417164fc9f32f", "commit_rev":
"24b50378a13f0099e231bcbfefaea67eec005ab3"}
Message was sent while issue was closed.
Description was changed from ========== Intercept WebVR api calls and check for VrCore compatibility. Proof of concept only, not ready for submitting. BUG=670166 ========== to ========== Intercept WebVR api calls and check for VrCore compatibility. Proof of concept only, not ready for submitting. BUG=670166 Review-Url: https://codereview.chromium.org/2701523008 Cr-Commit-Position: refs/heads/master@{#454482} Committed: https://chromium.googlesource.com/chromium/src/+/24b50378a13f0099e231bcbfefae... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/24b50378a13f0099e231bcbfefae... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
