Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2980453002:
[vr] Show DOFF when starting Chrome with a VR intent without FRE completion (Closed)
Description[vr] Show DOFF when starting Chrome with a VR intent without FRE completion
This CL adds VrFirstRunActivity which has the android:enableVrMode attribute which keeps
us in VR mode when starting Chrome. We need this because VrCore doesn't allow non-active VR
apps to call DOFF. Starting Chrome with a pure VR activity sets Chrome to be the active VR
app in time for us to call DOFF.
The new activity can be removed when VrCore allows transitioning activities to call DOFF
without being the active VR app.
BUG=721885
Review-Url: https://codereview.chromium.org/2980453002
Cr-Commit-Position: refs/heads/master@{#486812}
Committed: https://chromium.googlesource.com/chromium/src/+/e6e30e12aea101642e142906e6c00ff355ca8e99
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 2
Patch Set 3 : Address review comments #
Total comments: 16
Patch Set 4 : address review comments #Patch Set 5 : rebase #
Total comments: 14
Patch Set 6 : more review comments #
Total comments: 2
Patch Set 7 : make VrFirstRunActivity conditional #
Depends on Patchset: Messages
Total messages: 27 (11 generated)
Description was changed from ========== [vr] Show DOFF when starting Chrome with a VR intent without FRE completion BUG= ========== to ========== [vr] Show DOFF when starting Chrome with a VR intent without FRE completion This CL adds VrFirstRunActivity which has the android:enableVrMode attribute which keeps us in VR mode when starting Chrome. We need this because VrCore doesn't allow non-active VR apps to call DOFF. Starting Chrome with a pure VR activity sets Chrome to be the active VR app in time for us to call DOFF. The new activity can be removed when VrCore allows transitioning activities to call DOFF without being the active VR app. BUG=721885 ==========
ymalik@chromium.org changed reviewers: + mthiesse@chromium.org
@mthiesse, PTAL before I send to clank reviewers :)
https://codereview.chromium.org/2980453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2980453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:183: private static VrDoffHelper sDoffHelperInstance; Let's not have another static unless absolutely necessary. It looks like you could move this into a new .java file, make the static functions you need like getVrClassesWrapper() package visible, and just have this helper as a non-static member in VrFirstRunActivity?
mthiesse, PTAL :) https://codereview.chromium.org/2980453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2980453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:183: private static VrDoffHelper sDoffHelperInstance; On 2017/07/10 14:31:37, mthiesse wrote: > Let's not have another static unless absolutely necessary. It looks like you > could move this into a new .java file, make the static functions you need like > getVrClassesWrapper() package visible, and just have this helper as a non-static > member in VrFirstRunActivity? You're right. Done.
https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java (right): https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java:13: public final class VrDoffHelper { Sorry for giving a shallow review the first time. I don't see any reason for this class to exist, this would probably be much simpler if you just moved the necessary state directly into VrFirstRunActivity https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java:23: mWrapper = VrShellDelegate.createVrClassesWrapper(); Should probably do something sane if this happens to be null, even though it shouldn't be. https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java:27: mWrapper.setVrModeEnabled(mActivity, true); Why is this gated behind isDaydreamCurrentViewer? Also, you probably don't need this call at all since you set the manifest flag for this activity, right? https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java:33: if (!mIsDaydreamCurrentViewer) return; return a boolean value, and handle failures at the callsite, or just call onExitVrResult directly? Though you probably don't need to do anything in the RESULT_OK case, because exiting the VR activity will automatically turn VR mode off. https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java:48: mWrapper.setVrModeEnabled(mActivity, false); This call isn't necessary, right? VR mode won't be enabled for the FRE activity, and the Vr activity uses the manifest flag to set VR mode, so it would automatically be turned off. https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java (right): https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:28: mDoffHelper.showDoff(true); What if you do nothing because you're in cardboard mode? You'll never start the FRE, right? https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:283: public static void onExitVrResult(Activity activity, int resultCode) { unnecessary change? https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:988: private boolean showDoffInternal(boolean optional) { unnecessary change?
mthiesse, PTAL :) https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java (right): https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java:13: public final class VrDoffHelper { On 2017/07/10 22:49:46, mthiesse wrote: > Sorry for giving a shallow review the first time. I don't see any reason for > this class to exist, this would probably be much simpler if you just moved the > necessary state directly into VrFirstRunActivity Acknowledged. https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java:23: mWrapper = VrShellDelegate.createVrClassesWrapper(); On 2017/07/10 22:49:46, mthiesse wrote: > Should probably do something sane if this happens to be null, even though it > shouldn't be. Just calling standard FRE in VrFirstRunActivity. https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java:27: mWrapper.setVrModeEnabled(mActivity, true); On 2017/07/10 22:49:46, mthiesse wrote: > Why is this gated behind isDaydreamCurrentViewer? Also, you probably don't need > this call at all since you set the manifest flag for this activity, right? So VrCore has a fix which allows the transitioning activity to call DOFF without it being the active VR app. That still requires you to setVrModeEnabled programmatically. Up until then, our hack here works, but there's still a race every now and then that causes Chrome to crash (that's why we have wait 500ms before calling DOFF and I couldn't repro it locally after that). Calling setVrModeEnabled here ensures that this never happens for users running the latest version of VrCore that should ship with M61. We should remove this activity as a whole once we're confident that most users will have VrCore Api 21. https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java:33: if (!mIsDaydreamCurrentViewer) return; On 2017/07/10 22:49:46, mthiesse wrote: > return a boolean value, and handle failures at the callsite, or just call > onExitVrResult directly? Though you probably don't need to do anything in the > RESULT_OK case, because exiting the VR activity will automatically turn VR mode > off. N/A with latest patch. https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDoffHelper.java:48: mWrapper.setVrModeEnabled(mActivity, false); On 2017/07/10 22:49:46, mthiesse wrote: > This call isn't necessary, right? VR mode won't be enabled for the FRE activity, > and the Vr activity uses the manifest flag to set VR mode, so it would > automatically be turned off. Yes. https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java (right): https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:28: mDoffHelper.showDoff(true); On 2017/07/10 22:49:47, mthiesse wrote: > What if you do nothing because you're in cardboard mode? You'll never start the > FRE, right? Yes. Fixed. https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:283: public static void onExitVrResult(Activity activity, int resultCode) { On 2017/07/10 22:49:47, mthiesse wrote: > unnecessary change? Oops, removed. https://codereview.chromium.org/2980453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:988: private boolean showDoffInternal(boolean optional) { On 2017/07/10 22:49:47, mthiesse wrote: > unnecessary change? Oops, carried over from stale change.
mthiesse, PTAL :)
ymalik@chromium.org changed reviewers: + dtrainor@chromium.org
dtrainor PTAL for non vr_shell related changes :)
https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:427: android:theme="@android:style/Theme.Translucent.NoTitleBar" Use "@style/VrActivityTheme" Also you should probably include android:configChanges="orientation|keyboardHidden|keyboard|screenSiz e|mcc|mnc|screenLayout|smallestScreenSize|uiMode" https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:422: freIntent = VrShellDelegate.setupVrFreIntent(caller, freIntent); nit: Move this onto previous line and get rid of braces. https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java (right): https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:31: if (wrapper != null) { nit: if (wrapper == null) { showFre(); return; } https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:32: wrapper.setVrModeEnabled(this, true); Please add a comment on why we appear to call setVrMode redundantly here. https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:34: if (mApi.isDaydreamCurrentViewer()) { nit: if (!mApi.isDaydreamCurrentViewer()) { showFre(); return; } https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:52: if (requestCode == VrShellDelegate.EXIT_VR_RESULT) { nit: if (requestCode != VrShellDelegate.EXIT_VR_RESULT) return; https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:66: IntentUtils.safeStartActivity(this, freIntent); nit: call finish() from showFre(), then you can clean up code elsewhere more.
@mthisse / dtrainor PTAL :) https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:427: android:theme="@android:style/Theme.Translucent.NoTitleBar" On 2017/07/12 14:44:46, mthiesse wrote: > Use "@style/VrActivityTheme" > > Also you should probably include > android:configChanges="orientation|keyboardHidden|keyboard|screenSiz > e|mcc|mnc|screenLayout|smallestScreenSize|uiMode" Done. https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:422: freIntent = VrShellDelegate.setupVrFreIntent(caller, freIntent); On 2017/07/12 14:44:46, mthiesse wrote: > nit: Move this onto previous line and get rid of braces. Done. https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java (right): https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:31: if (wrapper != null) { On 2017/07/12 14:44:46, mthiesse wrote: > nit: if (wrapper == null) { > showFre(); > return; > } Done. https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:32: wrapper.setVrModeEnabled(this, true); On 2017/07/12 14:44:46, mthiesse wrote: > Please add a comment on why we appear to call setVrMode redundantly here. Done. https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:34: if (mApi.isDaydreamCurrentViewer()) { On 2017/07/12 14:44:46, mthiesse wrote: > nit: if (!mApi.isDaydreamCurrentViewer()) { > showFre(); > return; > } Done. https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:52: if (requestCode == VrShellDelegate.EXIT_VR_RESULT) { On 2017/07/12 14:44:46, mthiesse wrote: > nit: if (requestCode != VrShellDelegate.EXIT_VR_RESULT) return; Done. https://codereview.chromium.org/2980453002/diff/70001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrFirstRunActivity.java:66: IntentUtils.safeStartActivity(this, freIntent); On 2017/07/12 14:44:46, mthiesse wrote: > nit: call finish() from showFre(), then you can clean up code elsewhere more. Done.
lgtm
lgtm
https://codereview.chromium.org/2980453002/diff/90001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2980453002/diff/90001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:427: android:theme="@style/VrActivityTheme" You'll need to surround this android:theme and android:enableVrMode with {% if enable_vr == "true" %} to prevent compile errors on x86/x64. Actually, you might be able to surround the whole <activity> block here in {% if enable_vr == "true" %}? I think it'll compile, and as long as you don't try to start the VrFirstRunActivity it should be fine? Maybe dtrainor can confirm.
@dtrainor / @mthiesse, PTAL :) https://codereview.chromium.org/2980453002/diff/90001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2980453002/diff/90001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:427: android:theme="@style/VrActivityTheme" On 2017/07/13 17:26:48, mthiesse wrote: > You'll need to surround this android:theme and android:enableVrMode with {% if > enable_vr == "true" %} to prevent compile errors on x86/x64. > > Actually, you might be able to surround the whole <activity> block here in {% if > enable_vr == "true" %}? I think it'll compile, and as long as you don't try to > start the VrFirstRunActivity it should be fine? Maybe dtrainor can confirm. Thanks for the tip! Surrounded the entire activity with {% if enable_vr == "true" %}. and made sure we only call the VrFirstRunActivity when VR is enabled. I think* this should be fine but I'm not sure if this is the recommended way of doing this. @dtrainor, thoughts? Should the conditional be for the vr specific attributes or the entire activity?
The CQ bit was checked by ymalik@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.
On 2017/07/14 06:14:29, ymalik wrote: > @dtrainor / @mthiesse, PTAL :) > > https://codereview.chromium.org/2980453002/diff/90001/chrome/android/java/And... > File chrome/android/java/AndroidManifest.xml (right): > > https://codereview.chromium.org/2980453002/diff/90001/chrome/android/java/And... > chrome/android/java/AndroidManifest.xml:427: > android:theme="@style/VrActivityTheme" > On 2017/07/13 17:26:48, mthiesse wrote: > > You'll need to surround this android:theme and android:enableVrMode with {% if > > enable_vr == "true" %} to prevent compile errors on x86/x64. > > > > Actually, you might be able to surround the whole <activity> block here in {% > if > > enable_vr == "true" %}? I think it'll compile, and as long as you don't try to > > start the VrFirstRunActivity it should be fine? Maybe dtrainor can confirm. > > Thanks for the tip! Surrounded the entire activity with {% if enable_vr == > "true" %}. and made sure we only call the VrFirstRunActivity when VR is enabled. > I think* this should be fine but I'm not sure if this is the recommended way of > doing this. > > @dtrainor, thoughts? Should the conditional be for the vr specific attributes or > the entire activity? @dtrainor, will land with this now and can change as required :)
The CQ bit was checked by ymalik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2980453002/#ps110001 (title: "make VrFirstRunActivity conditional")
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": 110001, "attempt_start_ts": 1500055386316030,
"parent_rev": "007abfb715a7f00fb9d2d301626b5271940ad246", "commit_rev":
"e6e30e12aea101642e142906e6c00ff355ca8e99"}
Message was sent while issue was closed.
Description was changed from ========== [vr] Show DOFF when starting Chrome with a VR intent without FRE completion This CL adds VrFirstRunActivity which has the android:enableVrMode attribute which keeps us in VR mode when starting Chrome. We need this because VrCore doesn't allow non-active VR apps to call DOFF. Starting Chrome with a pure VR activity sets Chrome to be the active VR app in time for us to call DOFF. The new activity can be removed when VrCore allows transitioning activities to call DOFF without being the active VR app. BUG=721885 ========== to ========== [vr] Show DOFF when starting Chrome with a VR intent without FRE completion This CL adds VrFirstRunActivity which has the android:enableVrMode attribute which keeps us in VR mode when starting Chrome. We need this because VrCore doesn't allow non-active VR apps to call DOFF. Starting Chrome with a pure VR activity sets Chrome to be the active VR app in time for us to call DOFF. The new activity can be removed when VrCore allows transitioning activities to call DOFF without being the active VR app. BUG=721885 Review-Url: https://codereview.chromium.org/2980453002 Cr-Commit-Position: refs/heads/master@{#486812} Committed: https://chromium.googlesource.com/chromium/src/+/e6e30e12aea101642e142906e6c0... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001) as https://chromium.googlesource.com/chromium/src/+/e6e30e12aea101642e142906e6c0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
