Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2829193003:
Autopresent WebVR content in CCT if it was opened by a Daydream Intent (Closed)
DescriptionAutopresent WebVR content in CCT if it was opened by a Daydream Intent
[Abandoned]
BUG=713369
Patch Set 1 #
Total comments: 13
Patch Set 2 : reviews #
Messages
Total messages: 11 (2 generated)
bshe@chromium.org changed reviewers: + dtrainor@chromium.org, mthiesse@chromium.org, yusufo@chromium.org
PTAL. Feel free to ping me if you have any question/concern. +yusufo for CustomTabActivity.java +dtrainor for ChromeActivity.java +mthiesse for VrShellDelegate.java
https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:638: public boolean shouldAutoPresent() { Maybe call this shouldAutoPresentOnVrIntent()? https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:537: public final boolean shouldAutoPresent() { If you call this shouldAutoPresentOnVrIntent(), you can simply return true for CCT, and deal with the logic of when we get a VR intent inside VrShellDelegate. https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:538: return VrShellDelegate.isDaydreamVrIntent(getIntent()); Instead of using getIntent(), you should have VrShellDelegate listen to onNewIntentWithNative, so that we only present when we first get the intent, and not later on if the intent hasn't changed.
https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:638: public boolean shouldAutoPresent() { On 2017/04/21 14:24:27, mthiesse wrote: > Maybe call this shouldAutoPresentOnVrIntent()? supportsAutoPresent()? If we're decoupling it from needing/checking the intent maybe we just don't include that at all?
https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:638: public boolean shouldAutoPresent() { supportsAutoPresent is probably fine, but we should be careful to make it clear that this is a policy decision, and not a technical one. All ChromeActivitys technically support autopresent, but we only want to enable it for CCT direct linking.
https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:638: public boolean shouldAutoPresent() { On 2017/04/21 17:03:23, mthiesse wrote: > supportsAutoPresent is probably fine, but we should be careful to make it clear > that this is a policy decision, and not a technical one. All ChromeActivitys > technically support autopresent, but we only want to enable it for CCT direct > linking. > I actually intentionally want to avoid associate this function to a specific thing such as(intent) as I imagine that we will need auto present check in other cases in the future. And we probably don't want to have multiple shouldAutoPresent(OnVrIntent|OnNavigation*) functions. supportsAutoPresent is an option. But since all activity technically supports auto present, I think use shouldAutoPresent probably makes more sense? https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:537: public final boolean shouldAutoPresent() { On 2017/04/21 14:24:27, mthiesse wrote: > If you call this shouldAutoPresentOnVrIntent(), you can simply return true for > CCT, and deal with the logic of when we get a VR intent inside VrShellDelegate. See below. It looks like VrShellDelegate wont get intent from this activity by onNewIntentWithNative, which I assume is what you mean. https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:538: return VrShellDelegate.isDaydreamVrIntent(getIntent()); On 2017/04/21 14:24:27, mthiesse wrote: > Instead of using getIntent(), you should have VrShellDelegate listen to > onNewIntentWithNative, so that we only present when we first get the intent, and > not later on if the intent hasn't changed. If I understand correctly, onNewIntentWithNative wont be called since this is not launched in singleTop mode. I add a log in the overrided function and it doesnt seem to be called.
https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:538: return VrShellDelegate.isDaydreamVrIntent(getIntent()); Why look at the intent at all? Why not tie into CCT startup, and see if the creator of the CCT was a daydream activity?
https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:638: public boolean shouldAutoPresent() { On 2017/04/21 17:54:01, bshe wrote: > On 2017/04/21 17:03:23, mthiesse wrote: > > supportsAutoPresent is probably fine, but we should be careful to make it > clear > > that this is a policy decision, and not a technical one. All ChromeActivitys > > technically support autopresent, but we only want to enable it for CCT direct > > linking. > > > > I actually intentionally want to avoid associate this function to a specific > thing such as(intent) as I imagine that we will need auto present check in other > cases > in the future. And we probably don't want to have multiple > shouldAutoPresent(OnVrIntent|OnNavigation*) functions. > > supportsAutoPresent is an option. But since all activity technically supports > auto present, > I think use shouldAutoPresent probably makes more sense? It is just that what Present means is not clear from a general perspective. What are we presenting here? Without the VR context, I dont really understand what that means. https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:538: return VrShellDelegate.isDaydreamVrIntent(getIntent()); On 2017/04/21 18:35:39, mthiesse wrote: > Why look at the intent at all? Why not tie into CCT startup, and see if the > creator of the CCT was a daydream activity? I am assuming they dont connect through our service which means it is not possible to verify what type of Activity they are? Or is there a VR specific way of checking this?
https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:638: public boolean shouldAutoPresent() { On 2017/04/21 23:10:38, Yusuf wrote: > On 2017/04/21 17:54:01, bshe wrote: > > On 2017/04/21 17:03:23, mthiesse wrote: > > > supportsAutoPresent is probably fine, but we should be careful to make it > > clear > > > that this is a policy decision, and not a technical one. All ChromeActivitys > > > technically support autopresent, but we only want to enable it for CCT > direct > > > linking. > > > > > > > I actually intentionally want to avoid associate this function to a specific > > thing such as(intent) as I imagine that we will need auto present check in > other > > cases > > in the future. And we probably don't want to have multiple > > shouldAutoPresent(OnVrIntent|OnNavigation*) functions. > > > > supportsAutoPresent is an option. But since all activity technically supports > > auto present, > > I think use shouldAutoPresent probably makes more sense? > > It is just that what Present means is not clear from a general perspective. What > are we presenting here? Without the VR context, I dont really understand what > that means. Present in VR means the activity entering into a mode which is similar to fullscreen in the sense that 2D browser UI is invisible. And it also starts to show left and right images on the screen. From this point, it only make sense to view this activity in a VR headset like Cardboard. The presented content is a WebVR content, which is essentially a WebGl canvas. The canvas draws left and right images and presenting it on screen. Hopefully, this answers your question. Please feel free to let me know if you have any further questions. https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2829193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:538: return VrShellDelegate.isDaydreamVrIntent(getIntent()); On 2017/04/21 23:10:38, Yusuf wrote: > On 2017/04/21 18:35:39, mthiesse wrote: > > Why look at the intent at all? Why not tie into CCT startup, and see if the > > creator of the CCT was a daydream activity? > > I am assuming they dont connect through our service which means it is not > possible to verify what type of Activity they are? > > Or is there a VR specific way of checking this? There is no VR specific way of checking it. For issue 713369, we will make sure it connect through CCT service so it is possible to verify it. In general, we don't have a way to verify that in VR.
lgtm % yusuf's review. I'm going to defer to him on how he wants the API structured since it deals with CCT. Sorry for the delayed replies I was OOO last Thurs/Fri.
Description was changed from ========== Autopresent WebVR content in CCT if it was opened by a Daydream Intent BUG=713369 ========== to ========== Autopresent WebVR content in CCT if it was opened by a Daydream Intent [Abandoned] BUG=713369 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
