Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2969923002:
[vr] Hide 2D UI when entering VR for WebVR auto-presentation (Closed)
Description[vr] Hide 2D UI when entering VR for WebVR auto-presentation
This CL starts CTA for VR with a custom animation to keep it hidden while
we enter VR mode. This is needed to hide the 2D Chrome UI while the phone
is already in the VR headset (e.g when we're deep-linking from Daydream).
The animation alone is not sufficient, so we add a black overlay until
we're ready to show the WebVR page.
BUG=721857
Review-Url: https://codereview.chromium.org/2969923002
Cr-Commit-Position: refs/heads/master@{#486763}
Committed: https://chromium.googlesource.com/chromium/src/+/5f35110e22745dd99f24ca64ba5ed958613d8f29
Patch Set 1 #Patch Set 2 : rebase master #
Total comments: 2
Patch Set 3 : Remove android manifest flag #Patch Set 4 : Add cold-start case #
Total comments: 4
Patch Set 5 : Address review comments #
Total comments: 8
Patch Set 6 : Address review comments #
Total comments: 11
Patch Set 7 : address mthiesse's review comments #
Total comments: 2
Patch Set 8 : address tedchoc's comment #Patch Set 9 : fix find bugs bot failure #
Messages
Total messages: 52 (19 generated)
ymalik@chromium.org changed reviewers: + mthiesse@chromium.org
@mthiesse, PTAL before I add clank reviewers :)
lgtm
ymalik@chromium.org changed reviewers: + dtrainor@chromium.org
@dtrainor PTAL :) This CL leverages the stuff added in: https://chromium-review.googlesource.com/c/519483
dtrainor, gentle ping :)
ymalik@chromium.org changed reviewers: + mariakhomenko@google.com
+mariakhomenko PTAL since dtrainor is ooo :)
Description was changed from ========== [vr] Hide 2D UI when entering VR for WebVR auto-presentation This CL starts CTA for VR with a custom animation to keep it hidden while we enter VR mode. This is needed to hide the 2D Chrome UI while the phone is already in the VR headset (e.g when we're deep-linking from Daydream). The animation alone is not sufficient, so we add a black overlay until we're ready to show the WebVR page. This CL also makes ChromeLaucherActivity a VR supported activity which is needed to launch Chrome from Daydream. BUG=721857 ========== to ========== [vr] Hide 2D UI when entering VR for WebVR auto-presentation This CL starts CTA for VR with a custom animation to keep it hidden while we enter VR mode. This is needed to hide the 2D Chrome UI while the phone is already in the VR headset (e.g when we're deep-linking from Daydream). The animation alone is not sufficient, so we add a black overlay until we're ready to show the WebVR page. This CL also makes ChromeLaucherActivity a VR supported activity which is needed to launch Chrome from Daydream. BUG=721857 ==========
ymalik@chromium.org changed reviewers: - dtrainor@chromium.org, mariakhomenko@google.com
ymalik@chromium.org changed reviewers: + yusufo@chromium.org
ymalik@chromium.org changed reviewers: - yusufo@chromium.org
ymalik@chromium.org changed reviewers: + yfriedman@chromium.org
+Yaron, can you PTAL, all owners in MTV are on vacation :)
i'm really not the right reviewer for thsi. would prefer to wait until monday if it can
i'm really not the right reviewer for thsi. would prefer to wait until monday if it can
ymalik@chromium.org changed reviewers: + dtrainor@chromium.org - yfriedman@chromium.org
+dtrainor, PTAL :)
dtrainor@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc@ who might have some ideas as to why we need both. https://codereview.chromium.org/2969923002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2969923002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:752: // These options are used to start the Activity with a custom animation to keep it hidden It feels odd to me that we need both of these. Is one to hide the screenshot and one to let us have time to properly draw the correct UI (e.g. are we drawing the wrong UI for one or two frames before fix it)?
https://codereview.chromium.org/2969923002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2969923002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:752: // These options are used to start the Activity with a custom animation to keep it hidden On 2017/07/10 17:30:21, David Trainor-ping after 12h wrote: > It feels odd to me that we need both of these. Is one to hide the screenshot > and one to let us have time to properly draw the correct UI (e.g. are we drawing > the wrong UI for one or two frames before fix it)? We need the animation to hide the screenshot until we put the black overlay. We need the black overlay because the animation gets cancelled unexpectedly and we see 2D UI before we're ready to enter VR. I couldn't figure out why this happens, but it has something to do with the VR entry screens. I have a bug to investigate this in more details: crbug.com/740606.
On 2017/07/10 17:48:31, ymalik wrote: > https://codereview.chromium.org/2969923002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java > (right): > > https://codereview.chromium.org/2969923002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:752: > // These options are used to start the Activity with a custom animation to keep > it hidden > On 2017/07/10 17:30:21, David Trainor-ping after 12h wrote: > > It feels odd to me that we need both of these. Is one to hide the screenshot > > and one to let us have time to properly draw the correct UI (e.g. are we > drawing > > the wrong UI for one or two frames before fix it)? > > We need the animation to hide the screenshot until we put the black overlay. We > need the black overlay because the animation gets cancelled unexpectedly and we > see 2D UI before we're ready to enter VR. I couldn't figure out why this > happens, but it has something to do with the VR entry screens. I have a bug to > investigate this in more details: crbug.com/740606. Actually, please hold off on reviewing this, I'm trying something.
Description was changed from ========== [vr] Hide 2D UI when entering VR for WebVR auto-presentation This CL starts CTA for VR with a custom animation to keep it hidden while we enter VR mode. This is needed to hide the 2D Chrome UI while the phone is already in the VR headset (e.g when we're deep-linking from Daydream). The animation alone is not sufficient, so we add a black overlay until we're ready to show the WebVR page. This CL also makes ChromeLaucherActivity a VR supported activity which is needed to launch Chrome from Daydream. BUG=721857 ========== to ========== [vr] Hide 2D UI when entering VR for WebVR auto-presentation This CL starts CTA for VR with a custom animation to keep it hidden while we enter VR mode. This is needed to hide the 2D Chrome UI while the phone is already in the VR headset (e.g when we're deep-linking from Daydream). The animation alone is not sufficient, so we add a black overlay until we're ready to show the WebVR page. BUG=721857 ==========
On 2017/07/10 18:00:02, ymalik wrote: > On 2017/07/10 17:48:31, ymalik wrote: > > > https://codereview.chromium.org/2969923002/diff/20001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java > > (right): > > > > > https://codereview.chromium.org/2969923002/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:752: > > // These options are used to start the Activity with a custom animation to > keep > > it hidden > > On 2017/07/10 17:30:21, David Trainor-ping after 12h wrote: > > > It feels odd to me that we need both of these. Is one to hide the > screenshot > > > and one to let us have time to properly draw the correct UI (e.g. are we > > drawing > > > the wrong UI for one or two frames before fix it)? > > > > We need the animation to hide the screenshot until we put the black overlay. > We > > need the black overlay because the animation gets cancelled unexpectedly and > we > > see 2D UI before we're ready to enter VR. I couldn't figure out why this > > happens, but it has something to do with the VR entry screens. I have a bug to > > investigate this in more details: crbug.com/740606. > > Actually, please hold off on reviewing this, I'm trying something. @tedchoc, @dtrainor, It didn't work. Please go ahead and review.
Sorry for the delay, but I am a bit nervous that we don't have a clear enough handle on what we are trying to work around in this CL yet. 1.) Can you upload a video of this in action to the bug? 2.) What are you seeing in the various stages? * We show a fake window background that looks like the omnibox during activity inflation. This was recently changed to just a white background (https://chromium-review.googlesource.com/c/548218/). * We then inflate the Android views before initializing native. At this point, you'd see the omnibox and "Search or type" url. * In later versions of Android, it will cache a copy of the SurfaceView and show it on focusing Chrome, but this should only be for MAIN intents, which these should not be. And this would be for the page content and not necessarily the android UI. 3.) What of the above, are you seeing during this transition? What isn't done by the time that the 500ms has elapsed? 4.) VrShellDelegate.onNewIntent is called from processUrlViewIntent, which isn't actually triggered until after native has been fully loaded. Any of your logic to hide the java UI will be hugely delayed by this? In theory, you should only need to "fight" the window background as that can not be conditionally controlled. If you are seeing android views, then you should be able to find a point in the code that allows you to disable those earlier. I would look at postInflationStartup, but you'll need to figure out whether the intent you see is actually valid (i.e. getIntent() is not valid at that point if getSavedInstanceState() != null, so you might need this and a combination of onNewIntent()).
> Sorry for the delay, but I am a bit nervous that we don't have a clear enough > handle on what we are trying to work around in this CL yet. > > 1.) Can you upload a video of this in action to the bug? Done. > 2.) What are you seeing in the various stages? > * We show a fake window background that looks like the omnibox during activity > inflation. This was recently changed to just a white background > (https://chromium-review.googlesource.com/c/548218/). > * We then inflate the Android views before initializing native. At this > point, you'd see the omnibox and "Search or type" url. > * In later versions of Android, it will cache a copy of the SurfaceView and > show it on focusing Chrome, but this should only be for MAIN intents, which > these should not be. And this would be for the page content and not necessarily > the android UI. > 3.) What of the above, are you seeing during this transition? What isn't done > by the time that the 500ms has elapsed? So before this patch, we see fake window background and the the android views. > 4.) VrShellDelegate.onNewIntent is called from processUrlViewIntent, which isn't > actually triggered until after native has been fully loaded. Any of your logic > to hide the java UI will be hugely delayed by this? Yes, this is true. There's two cases here: 1) cold starting Chrome and 2) Chrome is running in the background. This patch was only handling the "Chrome in background" case above, and your proposed solution of handling this in postInflationStartup was for the "cold starting Chrome" that bshe had a patch for: https://chromium-review.googlesource.com/c/563880 (now merged here since it probably makes more sense). > In theory, you should only need to "fight" the window background as that can not > be conditionally controlled. If you are seeing android views, then you should > be able to find a point in the code that allows you to disable those earlier. I > would look at postInflationStartup, but you'll need to figure out whether the > intent you see is actually valid (i.e. getIntent() is not valid at that point if > getSavedInstanceState() != null, so you might need this and a combination of > onNewIntent()). To summarize, Yes. We "fight" the window background with the animation and the android views with the black screen. The black screen needs to be added in two places - in postInflationStartup when Chrome is cold started, and VrShellDelegate.onNewIntent when Chrome is running in the background. Does this make sense?
ymalik@chromium.org changed reviewers: + bshe@chromium.org
@tedchoc / @bshe, PTAL :)
https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:789: private static void addBlackOverlayViewForActivity(ChromeActivity activity) { Sorry. I should probably specify that my code is not a finished code. We probably also need to track which activity added this overlay.
On 2017/07/11 19:22:15, bshe wrote: > https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java > (right): > > https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:789: > private static void addBlackOverlayViewForActivity(ChromeActivity activity) { > Sorry. I should probably specify that my code is not a finished code. We > probably also need to track which activity added this overlay. Agreed. tedchoc@, feel free to review the non vr_shell changes since they'll remain the same.
+cc wychen to think about better intent handling during our start up refactoring https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1416: VrShellDelegate.mayStartWithVrIntent(this); While I think this code looks correct, I think we are likely to have issues with the vr intent handling done in drastically different lifecycles off the app. For handling the overlay, I think we should have a different method entirely maybeStartWithVrIntentPreNative or "something" of that sort. Then I think it should be called from here and from onNewIntent (explicitly not onNewIntentWithNative). That will make the timeline the same for the processing and should make it consistent. Both wouldn't be called for the same intent. Ideally, this is an area we should improve as well. This should be easier for developers to reason about instead of having to add this duplicate logic.
@tedchoc PTAL :) https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1416: VrShellDelegate.mayStartWithVrIntent(this); On 2017/07/11 21:16:23, Ted C wrote: > While I think this code looks correct, I think we are likely to have issues with > the vr intent handling done in drastically different lifecycles off the app. > > For handling the overlay, I think we should have a different method entirely > > maybeStartWithVrIntentPreNative or "something" of that sort. > > Then I think it should be called from here and from onNewIntent (explicitly not > onNewIntentWithNative). That will make the timeline the same for the processing > and should make it consistent. Both wouldn't be called for the same intent. > > Ideally, this is an area we should improve as well. This should be easier for > developers to reason about instead of having to add this duplicate logic. This makes sense. Added startWithVrIntentPreNative. https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:789: private static void addBlackOverlayViewForActivity(ChromeActivity activity) { On 2017/07/11 19:22:15, bshe wrote: > Sorry. I should probably specify that my code is not a finished code. We > probably also need to track which activity added this overlay. Actually thinking about this again, I don't know if this is necessary because we're only adding this when we are expecting a Daydream VR intent for the activity that we are going to auto-present for. I think adding activity tracking logic will add needless complexity.
@tedchoc PTAL :) https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1416: VrShellDelegate.mayStartWithVrIntent(this); On 2017/07/11 21:16:23, Ted C wrote: > While I think this code looks correct, I think we are likely to have issues with > the vr intent handling done in drastically different lifecycles off the app. > > For handling the overlay, I think we should have a different method entirely > > maybeStartWithVrIntentPreNative or "something" of that sort. > > Then I think it should be called from here and from onNewIntent (explicitly not > onNewIntentWithNative). That will make the timeline the same for the processing > and should make it consistent. Both wouldn't be called for the same intent. > > Ideally, this is an area we should improve as well. This should be easier for > developers to reason about instead of having to add this duplicate logic. This makes sense. Added startWithVrIntentPreNative. https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:789: private static void addBlackOverlayViewForActivity(ChromeActivity activity) { On 2017/07/11 19:22:15, bshe wrote: > Sorry. I should probably specify that my code is not a finished code. We > probably also need to track which activity added this overlay. Actually thinking about this again, I don't know if this is necessary because we're only adding this when we are expecting a Daydream VR intent for the activity that we are going to auto-present for. I think adding activity tracking logic will add needless complexity.
On 2017/07/12 14:48:05, ymalik wrote: > @tedchoc PTAL :) > > https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java > (right): > > https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1416: > VrShellDelegate.mayStartWithVrIntent(this); > On 2017/07/11 21:16:23, Ted C wrote: > > While I think this code looks correct, I think we are likely to have issues > with > > the vr intent handling done in drastically different lifecycles off the app. > > > > For handling the overlay, I think we should have a different method entirely > > > > maybeStartWithVrIntentPreNative or "something" of that sort. > > > > Then I think it should be called from here and from onNewIntent (explicitly > not > > onNewIntentWithNative). That will make the timeline the same for the > processing > > and should make it consistent. Both wouldn't be called for the same intent. > > > > Ideally, this is an area we should improve as well. This should be easier for > > developers to reason about instead of having to add this duplicate logic. > > This makes sense. Added startWithVrIntentPreNative. > > https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java > (right): > > https://codereview.chromium.org/2969923002/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:789: > private static void addBlackOverlayViewForActivity(ChromeActivity activity) { > On 2017/07/11 19:22:15, bshe wrote: > > Sorry. I should probably specify that my code is not a finished code. We > > probably also need to track which activity added this overlay. > > Actually thinking about this again, I don't know if this is necessary because > we're only adding this when we are expecting a Daydream VR intent for the > activity that we are going to auto-present for. > > I think adding activity tracking logic will add needless complexity. lgtm for VrShell
https://codereview.chromium.org/2969923002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2969923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:793: FrameLayout decor = (FrameLayout) activity.getWindow().getDecorView(); Instead of adding something to the decor view, have you looked at doing Window#addContentView here instead? I think that is the generally more accepted way of doing this. https://codereview.chromium.org/2969923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:799: private static void removeBlackOverlayViewForActivity(ChromeActivity activity) { Do you need this to be activity aware? Could this be: if (sBlackOverlayView == null || sBlackOverlayView.getParent() == null) return; ((ViewGroup) sBlackOverlayView.getParent()).removeView(sBlackOverlayView); https://codereview.chromium.org/2969923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:822: public static void onNewIntent(ChromeTabbedActivity activity, Intent intent) { Why ChromeTabbedActivity? Seems like you could keep this as the more generic ChromeActivity. https://codereview.chromium.org/2969923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:834: public static void startWithVrIntentPreNative(ChromeTabbedActivity activity, Intent intent) { I think it might be clearer in the other call site if this included something that it "might" be taking action. maybeHandleVrIntentPreNative or something. Although including maybe is pretty gross as well :-/. Nice bikeshed you have over there.
@tedchoc/@mthiesse, PTAL I re-worked the intent handling a bit. We were previously calling VrShellDelegate.onNewIntent from processUrlViewIntent. VrShellDelegate.onNewIntent had code that depended on native initialization, but it was weird to have it be called before onNativeLibraryAvailable. I changed it so that we only called VrShellDelegate.onNewIntent from ChromeActivity.onNewIntentWithNative. https://codereview.chromium.org/2969923002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2969923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:793: FrameLayout decor = (FrameLayout) activity.getWindow().getDecorView(); On 2017/07/12 16:12:06, Ted C wrote: > Instead of adding something to the decor view, have you looked at doing > Window#addContentView here instead? I think that is the generally more accepted > way of doing this. Thank you! Done. https://codereview.chromium.org/2969923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:799: private static void removeBlackOverlayViewForActivity(ChromeActivity activity) { On 2017/07/12 16:12:06, Ted C wrote: > Do you need this to be activity aware? > > Could this be: > > if (sBlackOverlayView == null || sBlackOverlayView.getParent() == null) return; > ((ViewGroup) sBlackOverlayView.getParent()).removeView(sBlackOverlayView); Yes absolutely. https://codereview.chromium.org/2969923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:822: public static void onNewIntent(ChromeTabbedActivity activity, Intent intent) { On 2017/07/12 16:12:06, Ted C wrote: > Why ChromeTabbedActivity? Seems like you could keep this as the more generic > ChromeActivity. I changed it because this was only called from ChromeTabbedActivity. I think this bit could be cleaned up, but I'd like to do that in a follow up CL. VrShellDelegate.onNewIntent gets called from processUrlViewIntent in ChromeTabbedActivity. This is where we create an instance of VrShellDelegate. We could just get rid of this function if we can create an instance of VrShellDelegate from the new "maybeHandleVrIntentPreNative" as that would be a lot cleaner, and we'd be handling the intent in one place. BUT, that change is a bit involved because creating VrShellDelegate depends on native libraries (but can be decoupled). I have a bug for this clean up: https://bugs.chromium.org/p/chromium/issues/detail?id=741916 https://codereview.chromium.org/2969923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:834: public static void startWithVrIntentPreNative(ChromeTabbedActivity activity, Intent intent) { On 2017/07/12 16:12:06, Ted C wrote: > I think it might be clearer in the other call site if this included something > that it "might" be taking action. maybeHandleVrIntentPreNative or something. > Although including maybe is pretty gross as well :-/. > > Nice bikeshed you have over there. Went with maybeHandleVrIntentPreNative due the the lack of a better name. https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1232: Intent intent = getSavedInstanceState() == null ? getIntent() : null; @tedchoc, I verified that this is safe to do by setting "Don't keep activities" option and resuming Chrome from recent apps to verify that the intent we see here is the old intent and not Main. It's also not obvious to me why it wouldn't work looking at the code.
https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1232: Intent intent = getSavedInstanceState() == null ? getIntent() : null; nit: move this logic inside of VrShellDelegate, VrShellDelegate.onNativeLibraryAvailable(this) should be sufficient. https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:813: if (sBlackOverlayView == null || sBlackOverlayView.getParent() == null) return; Use: if (sBlackOverlayView != null) UiUtils.removeViewFromParent(sBlackOverlayView); https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:839: public static void onNewIntent(ChromeActivity activity, Intent intent) { nit: onNewIntentWithNative? https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:839: public static void onNewIntent(ChromeActivity activity, Intent intent) { This function currently assumes we're paused, which is probably a good assumption, but you should double-check. I think we'll always be paused because any intents we get have to come through the ChromeLauncherActivity, which should pause us when it starts I believe. In any case, assert that we're paused here to make this clear. https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:999: if (cancelStartupAnimationIfNeeded()) { nit: move return; onto this line, remove braces.
@tedchoc / @mthiesse PTAL :) https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1232: Intent intent = getSavedInstanceState() == null ? getIntent() : null; On 2017/07/13 21:20:23, mthiesse wrote: > nit: move this logic inside of VrShellDelegate, > VrShellDelegate.onNativeLibraryAvailable(this) should be sufficient. We can't access getSavedInstanceState since its protected. We need this to ensure that we're not handling this intent again on resume when the activity gets killed and restarted (e.g when you have the "Don't keep activities" developer setting). https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:813: if (sBlackOverlayView == null || sBlackOverlayView.getParent() == null) return; On 2017/07/13 21:20:23, mthiesse wrote: > Use: > if (sBlackOverlayView != null) UiUtils.removeViewFromParent(sBlackOverlayView); That's perfect. ty! https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:839: public static void onNewIntent(ChromeActivity activity, Intent intent) { On 2017/07/13 21:20:23, mthiesse wrote: > nit: onNewIntentWithNative? Absolutely. Done. https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:839: public static void onNewIntent(ChromeActivity activity, Intent intent) { On 2017/07/13 21:20:23, mthiesse wrote: > This function currently assumes we're paused, which is probably a good > assumption, but you should double-check. I think we'll always be paused because > any intents we get have to come through the ChromeLauncherActivity, which should > pause us when it starts I believe. > > In any case, assert that we're paused here to make this clear. Outside of the call in VrShellDelegate.onNativeLibraryAvailable, this only gets called in Activity.onNewIntentWithNative, which gets called from Activity.onNewIntent which by definition gets called when the activity is paused. From docs: "An activity will always be paused before receiving a new intent, so you can count on onResume() being called after this method." I think that's what other onNewIntentWithNative overrides also expect. I don't see much value in refactoring this out just for an assert (its also used by onNativeLibraryAvailable) https://codereview.chromium.org/2969923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:999: if (cancelStartupAnimationIfNeeded()) { On 2017/07/13 21:20:24, mthiesse wrote: > nit: move return; onto this line, remove braces. Done.
lgtm
lgtm https://codereview.chromium.org/2969923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2969923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1233: VrShellDelegate.onNativeLibraryAvailable(this, intent); I'd recommend not passing the intent in this call, but to keep it more predictable on the vr side to do: VrShellDelegate.onNativeLibraryAvailable(this); if (getSavedInstanceState() == null && getIntent() != null) { VrShellDelegate.onNewIntentWithNative(this, getIntent()); } I think having an api do the same as another just leads to more headache IMO.
https://codereview.chromium.org/2969923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2969923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1233: VrShellDelegate.onNativeLibraryAvailable(this, intent); On 2017/07/14 00:12:05, Ted C wrote: > I'd recommend not passing the intent in this call, but to keep it more > predictable on the vr side to do: > > VrShellDelegate.onNativeLibraryAvailable(this); > if (getSavedInstanceState() == null && getIntent() != null) { > VrShellDelegate.onNewIntentWithNative(this, getIntent()); > } > > I think having an api do the same as another just leads to more headache IMO. Thanks, Done.
The CQ bit was checked by ymalik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org, tedchoc@chromium.org, mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2969923002/#ps140001 (title: "address tedchoc's comment")
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by ymalik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, mthiesse@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2969923002/#ps160001 (title: "fix find bugs bot failure")
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": 160001, "attempt_start_ts": 1500042561223550,
"parent_rev": "a13b5a0fc318d7f7b4ff9e5fb01f31f1bf701913", "commit_rev":
"5f35110e22745dd99f24ca64ba5ed958613d8f29"}
Message was sent while issue was closed.
Description was changed from ========== [vr] Hide 2D UI when entering VR for WebVR auto-presentation This CL starts CTA for VR with a custom animation to keep it hidden while we enter VR mode. This is needed to hide the 2D Chrome UI while the phone is already in the VR headset (e.g when we're deep-linking from Daydream). The animation alone is not sufficient, so we add a black overlay until we're ready to show the WebVR page. BUG=721857 ========== to ========== [vr] Hide 2D UI when entering VR for WebVR auto-presentation This CL starts CTA for VR with a custom animation to keep it hidden while we enter VR mode. This is needed to hide the 2D Chrome UI while the phone is already in the VR headset (e.g when we're deep-linking from Daydream). The animation alone is not sufficient, so we add a black overlay until we're ready to show the WebVR page. BUG=721857 Review-Url: https://codereview.chromium.org/2969923002 Cr-Commit-Position: refs/heads/master@{#486763} Committed: https://chromium.googlesource.com/chromium/src/+/5f35110e22745dd99f24ca64ba5e... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/5f35110e22745dd99f24ca64ba5e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
