Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2776243007:
Enable WebVR presentation from Chrome Custom Tab (Closed)
DescriptionEnable WebVR presentation from Chrome Custom Tab
Since CCT can't be resumed from an Activity PendingIntent, we use a Broadcast PendingIntent to resume the activity that triggered the DON flow.
The REORDER_TASKS permission is required to bring our activity back to the foreground inside the VR Entry result BroadcastReceiver.
BUG=688611
Review-Url: https://codereview.chromium.org/2776243007
Cr-Commit-Position: refs/heads/master@{#464571}
Committed: https://chromium.googlesource.com/chromium/src/+/99465cbe0eed1bd1a5ed3e2267581248a76d3138
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 2
Patch Set 3 : Add bug #
Total comments: 8
Patch Set 4 : Pull out some changes #Patch Set 5 : Remove file IO, fix PendingIntent errors #
Total comments: 1
Patch Set 6 : Remove VrProxyActivity, use Broadcast PendingIntent #
Total comments: 29
Patch Set 7 : Address comments #Patch Set 8 : Address Ted's comments #
Total comments: 10
Patch Set 9 : Address comments #Patch Set 10 : rebase + address nit #Messages
Total messages: 51 (20 generated)
Description was changed from ========== Enable WebVR presentation from Chrome Custom Tab BUG=688611 ========== to ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to b/33074373, whose fix hasn't been rolled into Chrome deps yet. The disk write be replaced with an intent extra as soon as possible. BUG=688611 ==========
mthiesse@chromium.org changed reviewers: + bshe@chromium.org, tedchoc@chromium.org
bshe, please review vr_shell/ changes. tedchoc, please review everything, really. Better approaches would be very welcome. cc jdduke for insight into the difficulties involved in making CCT work for VR, hopefully many of which are fixed already.
Description was changed from ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to b/33074373, whose fix hasn't been rolled into Chrome deps yet. The disk write be replaced with an intent extra as soon as possible. BUG=688611 ========== to ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to b/33074373, whose fix hasn't been rolled into Chrome deps yet. The disk write be replaced with an intent extra as soon as possible. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ==========
On 2017/03/29 22:13:16, mthiesse wrote: > tedchoc, please review everything, really. Better approaches would be very > welcome. I wouldn't trust Ted, he's a very shady fellow. > cc jdduke for insight into the difficulties involved in making CCT work for VR, > hopefully many of which are fixed already. =/ Thanks for the update, and apologies for the complexity involved here. In a perfect world we'd get rid if the need for implicit PendingIntents upon launch completely... hopefully we can find some engineering time to make that happen sooner than later.
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
You can probably replace much of the last (real) paragraph of the description with a crbug reference. nit:I think the current last sentence is missing "will." https://codereview.chromium.org/2776243007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:299: // TODO(mthiesse): This is a workaround for a bug in Daydream (b/33074373) where the Please file a crbug and reference it here and in the CL description.
Description was changed from ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to b/33074373, whose fix hasn't been rolled into Chrome deps yet. The disk write be replaced with an intent extra as soon as possible. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ========== to ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to issue 706845. The disk write be replaced with an intent extra as soon as possible. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ==========
Description was changed from ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to issue 706845. The disk write be replaced with an intent extra as soon as possible. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ========== to ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to crbug.com/706845. The disk write be replaced with an intent extra as soon as possible. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ==========
Description was changed from ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to crbug.com/706845. The disk write be replaced with an intent extra as soon as possible. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ========== to ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to https://crbug.com/706845. The disk write be replaced with an intent extra as soon as possible. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ==========
https://codereview.chromium.org/2776243007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:299: // TODO(mthiesse): This is a workaround for a bug in Daydream (b/33074373) where the On 2017/03/29 22:32:33, ddorwin wrote: > Please file a crbug and reference it here and in the CL description. Done.
Description was changed from ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to https://crbug.com/706845. The disk write be replaced with an intent extra as soon as possible. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ========== to ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to https://crbug.com/706845. The disk write will be replaced with an intent extra as soon as possible. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ==========
tedchoc@chromium.org changed reviewers: + yusufo@chromium.org
Does the pending intent passed to VR need to be an activity pending intent? could we not have a broadcastreceiver to be notified that their setup is done? Does it start another activity? If so, why does it start it with new task? Seems like we should be able to start on top of ours and then call finish and not have to do any of the reordering. This code is getting rather unwieldy and I'm not sure how many more of these bandaids we should be adding before doing some rather needed cleanup. https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:51: <uses-permission android:name="android.permission.REORDER_TASKS"/> @yusufo -- We already have task moveToFront in ActivityDelegateImpl, but I'm wondering if that is different. Since this is manipulating CCTs, the task ID might not be something Chrome owns (since the root task would be something like gmail). In that case, the getAppTasks from ActivityManager wouldn't return the CCT one? https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrProxyActivity.java (right): https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrProxyActivity.java:37: File tidFile = new File(dir, VrShellDelegate.TID_FILE); Are we worried about Chrome dying while this is happening? Any reason we don't just use a static variable in this class and a public static setter? https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrProxyActivity.java:38: File resultFile = new File(dir, VrShellDelegate.RESULT_SUCCESS_FILE); same thing, are we not calling directly into our own activity as a result of this? why can't we use statics if we're in the same process? https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrProxyActivity.java:39: try { you don't need nested try catches. this can be: try { } catch (Exceptions) { } finally { } https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrProxyActivity.java:49: activityManager.moveTaskToFront(tid, 0); does this need to happen after finish? I'm wondering if finish could go into the finally block. https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:320: dayreamApi.registerDaydreamIntent(getEnterVRPendingIntent(dayreamApi, activity)); if it is used multiple times, should it still be ONE_SHOT? Can you not use FLAG_CANCEL_CURRENT?
On 2017/03/31 18:17:19, Ted C wrote: > Does the pending intent passed to VR need to be an activity pending intent? > could we not have a broadcastreceiver to be notified that their setup is done? > > Does it start another activity? If so, why does it start it with new task? > Seems like we should be able to start on top of ours and then call finish and > not have to do any of the reordering. Starting in a new task is due to using pendingIntents - they always start a new task when the activity allows that and has nothing to do with daydream. As to why Daydream needs an intent passed at all - that's a question for jdduke, but I expect it's necessary for technical reasons (and I could probably guess at a few of them). On the topic of BroadcastReceivers, we could probably use them internally to Chrome instead of writing the TID to disk. Would that be preferable? > This code is getting rather unwieldy and I'm not sure how many more of these > bandaids we should be adding before doing some rather needed cleanup. Could you clarify which parts of this code you think need cleaning up, outside of the very hacky code in this CL? We've put a lot of effort into minimizing the API surface between VrShellDelegate and the rest of Clank. The only major point of contention I'm aware of is VR not being in a separate activity, but we've had that conversation (with you) in the past and concluded it wasn't really feasible.
https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrProxyActivity.java (right): https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrProxyActivity.java:37: File tidFile = new File(dir, VrShellDelegate.TID_FILE); On 2017/03/31 18:17:18, Ted C wrote: > Are we worried about Chrome dying while this is happening? Any reason we don't > just use a static variable in this class and a public static setter? Wouldn't we have to somehow start the VrProxyActivity in the process that the CCT runs in so that they can read the same memory? I'm not sure how we would go about doing that. I did try this at first, but I was reading default values from VrProxyActivity instead of the values I set. If you expect this to work I can take another look. https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:320: dayreamApi.registerDaydreamIntent(getEnterVRPendingIntent(dayreamApi, activity)); On 2017/03/31 18:17:18, Ted C wrote: > if it is used multiple times, should it still be ONE_SHOT? Can you not use > FLAG_CANCEL_CURRENT? I'll take a look at this Monday, but I'm actually very confused by how this daydream bug happens at all. It should be impossible for them to call us back with a ONE_SHOT intent multiple times, yet if we change the extras on the intent, we get called back with the same extra we originally set every time, without fail. jdduke, do you know how this is even possible?
PTAL. I removed the need for file IO by using a broadcastReceiver to let VrShellDelegate know the DON flow succeeded. I fixed the issue where pendingIntents were persisting by adding the flag PendingIntent.FLAG_UPDATE_CURRENT. Apparently if you don't include this, Android basically just ignores you when you make a new pendingIntent before consuming the previous one. I did some more cleanup of our entry flow, so hopefully it's easier to follow.
https://codereview.chromium.org/2776243007/diff/80001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2776243007/diff/80001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:84: <uses-permission android:name="{{ manifest_package }}.permission.VR_ENTRY" /> Note that this also requires a change to third_party/catapult/devil/devil/android/device_utils.py or chromium fails to install.
Description was changed from ========== Enable WebVR presentation from Chrome Custom Tab This CL contains a lot of ugly workarounds for the fact that we can't return directly to CCT from VR (among other VR bugs). We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses the taskID of the activity that should be resumed in VR to directly bring that activity to the foreground. Unfortunately when using the taskID to bring an activity to the foreground we can't actually communicate (like via an intent) with that activity to let it know whether or not the DON flow succeded, so we write a file to let the activity know whether or not the DON flow succeeded when it resumes. The fact that we also have to asynchronously write to disk in onResume to record the taskID of the activity that may enter VR is due to https://crbug.com/706845. The disk write will be replaced with an intent extra as soon as possible. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ========== to ========== Enable WebVR presentation from Chrome Custom Tab We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses a broadcast intent to notify the activity that requested VR that the DON flow succeeded and to resume in VR. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ==========
Ping tedchoc
On 2017/04/11 01:00:53, mthiesse wrote: > Ping tedchoc Sorry for the late response. Do you mind expanding the "PendingIntent always starts activities on new task" part? In CCT flows, the client app that launches CCT (on the same task), can give us a pending intent which we use to launch overlay activities on the same task, effectively sandwiching the CCT activity in between two client activities, the last of which is launched by a pendingIntent. Is there any Activity that is a singleTask, singleInstance that is breaking the flow here? A chart with all this activity and receiver components and their various constraints (singleTask, intent vs pendingIntent) would really help. Sorry for the hesitation again, but this is so convoluted that I feel like there must be a simpler solution here.
On 2017/04/11 23:43:22, Yusuf wrote: > On 2017/04/11 01:00:53, mthiesse wrote: > > Ping tedchoc > > Sorry for the late response. > > Do you mind expanding the "PendingIntent always starts activities on new task" > part? > > In CCT flows, the client app that launches CCT (on the same task), can give us a > pending > intent which we use to launch overlay activities on the same task, effectively > sandwiching > the CCT activity in between two client activities, the last of which is launched > by a pendingIntent. > > Is there any Activity that is a singleTask, singleInstance that is breaking the > flow here? > > A chart with all this activity and receiver components and their various > constraints (singleTask, > intent vs pendingIntent) would really help. > > Sorry for the hesitation again, but this is so convoluted that I feel like there > must be a simpler solution here. See the documentation here: https://developer.android.com/reference/android/app/PendingIntent.html#getAct..., int, android.content.Intent, int) Specifically: "Note that the activity will be started outside of the context of an existing activity, so you must use the Intent.FLAG_ACTIVITY_NEW_TASK launch flag in the Intent." Unless there's some other way of creating a PendingIntent I don't know about, in my testing the PendingIntent I create from the CCT that I pass to Daydream was always launching a new CCT, rather than returning to the existing one that created the PendingIntent. Forgetting about the ProxyActivity workaround for a moment, the way launching an app in VR works is we call DaydreamApi.launchInVr(PendingIntent intent). Daydream then runs the DON flow activity in a separate task stack, and starts the pendingIntent when the DON flow is successfully completed. If we could simply return to the existing CCT with this PendingIntent then all of the problems would be resolved. However, as stated above, this isn't returning to the existing CCT, so we use a proxy activity that we *can* create instances of that can communicate with the CCT to let it know the DON flow succeeded.
On 2017/04/11 23:57:49, mthiesse wrote: > On 2017/04/11 23:43:22, Yusuf wrote: > > On 2017/04/11 01:00:53, mthiesse wrote: > > > Ping tedchoc > > > > Sorry for the late response. > > > > Do you mind expanding the "PendingIntent always starts activities on new task" > > part? > > > > In CCT flows, the client app that launches CCT (on the same task), can give us > a > > pending > > intent which we use to launch overlay activities on the same task, effectively > > sandwiching > > the CCT activity in between two client activities, the last of which is > launched > > by a pendingIntent. > > > > Is there any Activity that is a singleTask, singleInstance that is breaking > the > > flow here? > > > > A chart with all this activity and receiver components and their various > > constraints (singleTask, > > intent vs pendingIntent) would really help. > > > > Sorry for the hesitation again, but this is so convoluted that I feel like > there > > must be a simpler solution here. > > See the documentation here: > https://developer.android.com/reference/android/app/PendingIntent.html#getAct..., > int, android.content.Intent, int) > Specifically: "Note that the activity will be started outside of the context of > an existing activity, so you must use the Intent.FLAG_ACTIVITY_NEW_TASK launch > flag in the Intent." > Unless there's some other way of creating a PendingIntent I don't know about, in > my testing the PendingIntent I create from the CCT that I pass to Daydream was > always launching a new CCT, rather than returning to the existing one that > created the PendingIntent. > > Forgetting about the ProxyActivity workaround for a moment, the way launching an > app in VR works is we call DaydreamApi.launchInVr(PendingIntent intent). > Daydream then runs the DON flow activity in a separate task stack, and starts > the pendingIntent when the DON flow is successfully completed. If we could > simply return to the existing CCT with this PendingIntent then all of the > problems would be resolved. However, as stated above, this isn't returning to > the existing CCT, so we use a proxy activity that we *can* create instances of > that can communicate with the CCT to let it know the DON flow succeeded. To be clear, we don't have control over which task stack the Daydream flow runs in, and we don't specify how the Daydream DON flow is even launched, so we can't simply launch the DON flow in CCTs task stack, and have it call finish() to return to the CCT.
On 2017/04/12 00:00:35, mthiesse wrote: > On 2017/04/11 23:57:49, mthiesse wrote: > > On 2017/04/11 23:43:22, Yusuf wrote: > > > On 2017/04/11 01:00:53, mthiesse wrote: > > > > Ping tedchoc > > > > > > Sorry for the late response. > > > > > > Do you mind expanding the "PendingIntent always starts activities on new > task" > > > part? > > > > > > In CCT flows, the client app that launches CCT (on the same task), can give > us > > a > > > pending > > > intent which we use to launch overlay activities on the same task, > effectively > > > sandwiching > > > the CCT activity in between two client activities, the last of which is > > launched > > > by a pendingIntent. > > > > > > Is there any Activity that is a singleTask, singleInstance that is breaking > > the > > > flow here? > > > > > > A chart with all this activity and receiver components and their various > > > constraints (singleTask, > > > intent vs pendingIntent) would really help. > > > > > > Sorry for the hesitation again, but this is so convoluted that I feel like > > there > > > must be a simpler solution here. > > > > See the documentation here: > > > https://developer.android.com/reference/android/app/PendingIntent.html#getAct..., > > int, android.content.Intent, int) > > Specifically: "Note that the activity will be started outside of the context > of > > an existing activity, so you must use the Intent.FLAG_ACTIVITY_NEW_TASK launch > > flag in the Intent." > > Unless there's some other way of creating a PendingIntent I don't know about, > in > > my testing the PendingIntent I create from the CCT that I pass to Daydream was > > always launching a new CCT, rather than returning to the existing one that > > created the PendingIntent. > > > > Forgetting about the ProxyActivity workaround for a moment, the way launching > an > > app in VR works is we call DaydreamApi.launchInVr(PendingIntent intent). > > Daydream then runs the DON flow activity in a separate task stack, and starts > > the pendingIntent when the DON flow is successfully completed. If we could > > simply return to the existing CCT with this PendingIntent then all of the > > problems would be resolved. However, as stated above, this isn't returning to > > the existing CCT, so we use a proxy activity that we *can* create instances of > > that can communicate with the CCT to let it know the DON flow succeeded. > > To be clear, we don't have control over which task stack the Daydream flow runs > in, and we don't specify how the Daydream DON flow is even launched, so we can't > simply launch the DON flow in CCTs task stack, and have it call finish() to > return to the CCT. Thanks a lot for the explanation. I will dig into the documentation and see where my discrepancy stems from. In the mean time, to make sure I am understanding this right, after the DON flow, in recents, what we have is the DayDream owned task as the last task and CCT task as the task before that right? I can see that the API is not written for that right now, but would DayDream calling moveToBack() on its own task not solve the issue and bring back the task for CCT to focus? Are there cases where we are returning back to a different task then the one just before?
> Thanks a lot for the explanation. I will dig into the documentation and see > where my > discrepancy stems from. > > In the mean time, to make sure I am understanding this right, after the DON > flow, in > recents, what we have is the DayDream owned task as the last task and CCT task > as > the task before that right? I can see that the API is not written for that right > now, > but would DayDream calling moveToBack() on its own task not solve the issue and > bring > back the task for CCT to focus? Are there cases where we are returning back to a > different > task then the one just before? So, yes, Daydream calling moveTaskToBack() would probably work in that it would bring the CCT into the foreground, but there are other problems with doing this. First, if our activity is simply resumed without a new intent, we don't know whether the DON flow succeeded, or whether the user actually cancelled entering VR. There doesn't even currently exist an API to check if you were started in VR mode, or if the phone is in VR mode, for complicated reasons I don't have much insight into (though I think they're working on fixing this in the future?). Second, the DON flow doesn't always return to the activity that called it, DaydreamApi#launchInVr(Intent) can launch any activity after the DON flow. Perhaps they could explicitly support a different API that doesn't launch an intent at the end of the DON flow, but Chrome would be unique in needing this API, so I expect they would prefer we work around this problem at least in the near-term. I expect that on the current Daydream roadmap these workarounds will become unnecessary, but I wouldn't expect the fixes soon as they've got their hands pretty full right now.
ping, was really hoping to get this into M59, we have a bunch of app developers that want to use webVR in CCT.
On 2017/04/13 14:44:07, mthiesse wrote: > ping, was really hoping to get this into M59, we have a bunch of app developers > that want to use webVR in CCT. Sorry I still dont get why we are adding a new activity and then have that broadcast to a receiver to move a task. Why does the new activity not move the task?
On 2017/04/13 15:12:37, Yusuf wrote: > On 2017/04/13 14:44:07, mthiesse wrote: > > ping, was really hoping to get this into M59, we have a bunch of app > developers > > that want to use webVR in CCT. > > Sorry I still dont get why we are adding a new activity and then have that > broadcast to a receiver to move a task. Why does > the new activity not move the task? I mean why not add a new static call similar to getEntryHook etc to the delegate to get any VR specific params and avoid adding another intent jump and a new component in between?
On 2017/04/13 15:17:41, Yusuf wrote: > On 2017/04/13 15:12:37, Yusuf wrote: > > On 2017/04/13 14:44:07, mthiesse wrote: > > > ping, was really hoping to get this into M59, we have a bunch of app > > developers > > > that want to use webVR in CCT. > > > > Sorry I still dont get why we are adding a new activity and then have that > > broadcast to a receiver to move a task. Why does > > the new activity not move the task? > > I mean why not add a new static call similar to getEntryHook etc to the delegate > to get any > VR specific params and avoid adding another intent jump and a new component in > between? Actually even better if we can remove the activity and use just BR.one component and no UI.
Thanks so much Yusuf! I had no idea that was possible. This really cleans things up a lot. PTAL
dont forget to update the CL description! mostly styling nits here lgtm after they are addressed, off to tedchoc@ https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:448: VrShellDelegate.onExitVRResult(resultCode); can we have this in ChromeActivity? https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:131: private final WeakReference<Activity> mTargetActivity; space here https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:134: mTargetActivity = new WeakReference<Activity>(activity); I think we should do the activity check here. since this will not work if the activity is not a chromeactivity, then we can just use it as a chromeactivity from here on. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:148: javadoc https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:266: private static VrShellDelegate getInstance(Activity activity) { why not take a ChromeActivity here?
Description was changed from ========== Enable WebVR presentation from Chrome Custom Tab We need to pass VR a pendingIntent to the activity in order for VR to launch the activity after the DON (Device ON) flow completes since the activity is not exported. However, pendingIntents always create new tasks for activities that are not singleTask activities, like CCT, so we both fail to return to the CCT that tried to enter VR, and immediately crash because CCT can't be launched this way. To work around this, this CL introduces the VrProxyActivity, which is always launched after the DON flow, and uses a broadcast intent to notify the activity that requested VR that the DON flow succeeded and to resume in VR. https://codereview.chromium.org/2779033004/ should land before this CL does. BUG=688611 ========== to ========== Enable WebVR presentation from Chrome Custom Tab Since CCT can't be resumed from an Activity PendingIntent, we use a Broadcast PendingIntent to resume the activity that triggered the DON flow. The REORDER_TASKS permission is required to bring our activity back to the foreground inside the VR Entry result BroadcastReceiver. BUG=688611 ==========
https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:448: VrShellDelegate.onExitVRResult(resultCode); On 2017/04/13 17:20:50, Yusuf wrote: > can we have this in ChromeActivity? Done. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:131: private final WeakReference<Activity> mTargetActivity; On 2017/04/13 17:20:50, Yusuf wrote: > space here Done. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:134: mTargetActivity = new WeakReference<Activity>(activity); On 2017/04/13 17:20:50, Yusuf wrote: > I think we should do the activity check here. since this will not work if the > activity is not a chromeactivity, then we can just use it as a chromeactivity > from here on. Done. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:148: On 2017/04/13 17:20:51, Yusuf wrote: > javadoc Done. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:266: private static VrShellDelegate getInstance(Activity activity) { On 2017/04/13 17:20:50, Yusuf wrote: > why not take a ChromeActivity here? Done.
I'll say that neither Yusuf or I will really understand the subtleties in the VR code, so we're relying on one of the other reviewers to look at that much more closely. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (left): https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:584: if (mVrSupportLevel == VR_NOT_AVAILABLE) return; any reason why this check isn't there anymore? https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:87: private static final String VR_ACTIVITY_ALIAS = VR_PACKAGE + ".VrProxyActivity"; remove? https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:117: private boolean mDONSucceeded; in java, acronyms should be treated as works (i.e. Url instead of URL). In that case, should this be mDonSucceeded? Also...I have no idea what DON is. Can we add some clarifying bits about that either here or at least at the commit message level. Is there any document that describes these flows, what the expected paths are? https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:128: // In case there are multiple VrShellDelegates created in different processes, this ensures how is this possible? what processes? All Chrome activities share the same browser process, so can you clarify this more. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:141: int target = intent.getIntExtra(TID_EXTRA, -1); Not really necessary since we're within Chrome, but we should use IntentUtils.safeGetIntExtra to enforce best practices https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:145: ((ActivityManager) context.getSystemService(Context.ACTIVITY_SERVICE)) Any idea if Android will do anything wonky if the task is gone by this point (swiped away by the user)? https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:278: return activity instanceof ChromeTabbedActivity || activity instanceof CustomTabActivity; Any reason all ChromeActivities aren't supported? Webapps should work too IMO. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:318: final VrDaydreamApi dayreamApi, final Activity activity) { s/dayreamApi/daydreamApi And other places https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:600: && (mActivity instanceof ChromeTabbedActivity))) { Why ChromeTabbedActivity here? Why not isSupportedActivity(mActivity)?
https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (left): https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:584: if (mVrSupportLevel == VR_NOT_AVAILABLE) return; On 2017/04/13 19:20:39, Ted C wrote: > any reason why this check isn't there anymore? The check moved down and became "if (mVrSupportLevel != VR_DAYDREAM) return;" Basically, when mNativeVrShellDelegate exists we should always call resume on it. All of the logic below that is DON flow related, which is Daydream-only. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:87: private static final String VR_ACTIVITY_ALIAS = VR_PACKAGE + ".VrProxyActivity"; On 2017/04/13 19:20:39, Ted C wrote: > remove? Done. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:117: private boolean mDONSucceeded; On 2017/04/13 19:20:39, Ted C wrote: > in java, acronyms should be treated as works (i.e. Url instead of URL). In that > case, should this be mDonSucceeded? > > Also...I have no idea what DON is. Can we add some clarifying bits about that > either here or at least at the commit message level. > > Is there any document that describes these flows, what the expected paths are? Added source-level comments. Public-facing DON flow documentation is here: https://developers.google.com/vr/daydream/guides/vr-entry https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:128: // In case there are multiple VrShellDelegates created in different processes, this ensures On 2017/04/13 19:20:39, Ted C wrote: > how is this possible? what processes? > > All Chrome activities share the same browser process, so can you clarify this > more. I was under the mistaken assumption that CCTs can be started in different processes. Removing this. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:141: int target = intent.getIntExtra(TID_EXTRA, -1); On 2017/04/13 19:20:39, Ted C wrote: > Not really necessary since we're within Chrome, but we should use > IntentUtils.safeGetIntExtra to enforce best practices Removed. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:145: ((ActivityManager) context.getSystemService(Context.ACTIVITY_SERVICE)) On 2017/04/13 19:20:39, Ted C wrote: > Any idea if Android will do anything wonky if the task is gone by this point > (swiped away by the user)? As far as I can tell there's no documentation on this, but reading through Android source, they just silently do nothing if they can't find the task matching the ID. Also wouldn't the WeakReference have been nulled if the task was swiped away? https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:278: return activity instanceof ChromeTabbedActivity || activity instanceof CustomTabActivity; On 2017/04/13 19:20:39, Ted C wrote: > Any reason all ChromeActivities aren't supported? Webapps should work too IMO. They should. I just want to flip the bit in a separate CL after doing some testing to make sure it works as expected. I can also re-enable WebVR from multi-window mode after this CL lands. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:318: final VrDaydreamApi dayreamApi, final Activity activity) { On 2017/04/13 19:20:39, Ted C wrote: > s/dayreamApi/daydreamApi > > And other places Done. https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:600: && (mActivity instanceof ChromeTabbedActivity))) { On 2017/04/13 19:20:39, Ted C wrote: > Why ChromeTabbedActivity here? Why not isSupportedActivity(mActivity)? Added a new method activitySupportsVRBrowsing for clarity. At the moment only CTA supports the full VR browser mode, as it's analogous to CTA inside your VR headset (we don't intend to turn CCT into a full browser). Renamed isSupportedActivity to activitySupportsPresentation to make the intent here clearer.
I had a cardboard support question (and a small comment nit). DON flow handling in general look much improved. :) https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:442: // related to gpu surfaces. Set this flag to enter VR on the next resume. This comment (especially last sentence) seems to be slightly off. It looks like we got rid of the mEnteringVr flag it was referencing before. https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:604: if (mVrSupportLevel != VR_DAYDREAM) return; What does this do to cardboard support?
lgtm https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:145: ((ActivityManager) context.getSystemService(Context.ACTIVITY_SERVICE)) On 2017/04/13 19:56:45, mthiesse wrote: > On 2017/04/13 19:20:39, Ted C wrote: > > Any idea if Android will do anything wonky if the task is gone by this point > > (swiped away by the user)? > > As far as I can tell there's no documentation on this, but reading through > Android source, they just silently do nothing if they can't find the task > matching the ID. > > Also wouldn't the WeakReference have been nulled if the task was swiped away? The weakreference "could" be there or not. You're racing with GC at that point. I just didn't know if we should catch any exceptions, but I looked at the ActivityManager code and it looks like it just logs if it can't find the task. https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:53: <uses-permission android:name="android.permission.REORDER_TASKS"/> Since we only need this for newer versions of Android, should we use uses-permission-sdk-m for now? https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:420: private boolean enterVRAfterDON() { this should be enterVrAfterDon per naming. We could have a follow up CL to unify the capitalization of vr in this file though :-P
https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:53: <uses-permission android:name="android.permission.REORDER_TASKS"/> On 2017/04/13 20:17:18, Ted C wrote: > Since we only need this for newer versions of Android, should we use > uses-permission-sdk-m for now? Done. https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:420: private boolean enterVRAfterDON() { On 2017/04/13 20:17:19, Ted C wrote: > this should be enterVrAfterDon per naming. > > We could have a follow up CL to unify the capitalization of vr in this file > though :-P Done. Fixed most others too. https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:442: // related to gpu surfaces. Set this flag to enter VR on the next resume. On 2017/04/13 20:13:56, amp wrote: > This comment (especially last sentence) seems to be slightly off. It looks like > we got rid of the mEnteringVr flag it was referencing before. Done. https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:604: if (mVrSupportLevel != VR_DAYDREAM) return; On 2017/04/13 20:13:56, amp wrote: > What does this do to cardboard support? Nothing. Cardboard will never hit this point because it doesn't go through the DON flow (meaning we won't get paused or resumed when entering VR in cardboard mode).
amp@chromium.org changed reviewers: + amp@chromium.org
lgtm https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:604: if (mVrSupportLevel != VR_DAYDREAM) return; On 2017/04/13 20:27:21, mthiesse wrote: > On 2017/04/13 20:13:56, amp wrote: > > What does this do to cardboard support? > > Nothing. Cardboard will never hit this point because it doesn't go through the > DON flow (meaning we won't get paused or resumed when entering VR in cardboard > mode). Oh yea, totally forgot that part. And because I'm paranoid I pulled down the patch and tried out cardboard anyway and it worked great. :D
The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run
The CQ bit was unchecked by mthiesse@chromium.org
The CQ bit was checked by mthiesse@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...
lgtm https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2776243007/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:252: public static void maybeUnregisterVREntryHook(Activity activity) { nit: ChromeActivity too?
The CQ bit was checked by mthiesse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, tedchoc@chromium.org, bshe@chromium.org, amp@chromium.org Link to the patchset: https://codereview.chromium.org/2776243007/#ps180001 (title: "rebase + address nit")
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": 180001, "attempt_start_ts": 1492118705111620,
"parent_rev": "31af02951adbf93da81916bfd9746b8f55719cca", "commit_rev":
"99465cbe0eed1bd1a5ed3e2267581248a76d3138"}
Message was sent while issue was closed.
Description was changed from ========== Enable WebVR presentation from Chrome Custom Tab Since CCT can't be resumed from an Activity PendingIntent, we use a Broadcast PendingIntent to resume the activity that triggered the DON flow. The REORDER_TASKS permission is required to bring our activity back to the foreground inside the VR Entry result BroadcastReceiver. BUG=688611 ========== to ========== Enable WebVR presentation from Chrome Custom Tab Since CCT can't be resumed from an Activity PendingIntent, we use a Broadcast PendingIntent to resume the activity that triggered the DON flow. The REORDER_TASKS permission is required to bring our activity back to the foreground inside the VR Entry result BroadcastReceiver. BUG=688611 Review-Url: https://codereview.chromium.org/2776243007 Cr-Commit-Position: refs/heads/master@{#464571} Committed: https://chromium.googlesource.com/chromium/src/+/99465cbe0eed1bd1a5ed3e226758... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/99465cbe0eed1bd1a5ed3e226758... |
