Fix broken VR transitions due to ChromeTabbedActivity not being exported.

Fix broken VR transitions due to ChromeTabbedActivity not being exported.

Recently, VR transitions broke due to us properly registering as a VR capable app. For some reason this lead VrCore to handle us differently and try launching ChromeTabbedActivity, which it couldn't because it's not exported.

This CL updates our transition code to use the new daydream API calls, giving us enter and exit VR transitions. I also introduce an activity-alias for VR purposes so that VrCore can call back into us after the VR DON flow completes.

BUG=665006
Committed: https://crrev.com/d7d11fc27adf47033a732dc446f87ac55d29f53a
Cr-Commit-Position: refs/heads/master@{#432727}

[mthiesse, 2016-11-15 18:52:34]

mthiesse@chromium.org changed reviewers:
+ bajones@chromium.org, bshe@chromium.org

[mthiesse, 2016-11-15 18:52:35]

PTAL, speedy review appreciated, this is blocking a lot of development.

[bajones, 2016-11-15 19:39:43]

On 2016/11/15 18:52:35, mthiesse wrote:
> PTAL, speedy review appreciated, this is blocking a lot of development.

vr_shell/ LGTM, though as a general observation we're starting to get a lot of
functions like:

shutdownVR(true);

where the booleans are pretty tough to parse from the call sites. Maybe (in a
future CL) we could transition those to something like this for clarity:

shutdownVR(SHUTDOWN_DESTINATION.2D);

[mthiesse, 2016-11-15 19:43:56]

mthiesse@chromium.org changed reviewers:
+ tedchoc@chromium.org

[mthiesse, 2016-11-15 19:43:56]

tedchoc@chromium.org: Please review java and AndroidManifest changes.

[bshe, 2016-11-15 19:45:00]

quick review before I head to vanpool

https://codereview.chromium.org/2500153003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2500153003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:19:
import com.google.vr.ndk.base.AndroidCompat;
huh. VrShellDelegate shouldn't have access to com.google.vr.ndk.*

https://codereview.chromium.org/2500153003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:201:
if (inWebVR) mVrShell.setWebVrModeEnabled(true);
nit: if (mInVr && inWebVR)

[mthiesse, 2016-11-15 19:50:49]

Patchset #4 (id:60001) has been deleted

[mthiesse, 2016-11-15 19:52:40]

https://codereview.chromium.org/2500153003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2500153003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:19:
import com.google.vr.ndk.base.AndroidCompat;
On 2016/11/15 19:45:00, bshe wrote:
> huh. VrShellDelegate shouldn't have access to com.google.vr.ndk.*

Done.

https://codereview.chromium.org/2500153003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:201:
if (inWebVR) mVrShell.setWebVrModeEnabled(true);
On 2016/11/15 19:45:00, bshe wrote:
> nit: if (mInVr && inWebVR)

No, we want to return early regardless, but if transitioning to webVR mode
inside VR Shell, we still need to call setWebVrModeEnabled.

[bshe, 2016-11-16 01:04:56]

On 2016/11/15 19:52:40, mthiesse wrote:
>
https://codereview.chromium.org/2500153003/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
> (right):
> 
>
https://codereview.chromium.org/2500153003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:19:
> import com.google.vr.ndk.base.AndroidCompat;
> On 2016/11/15 19:45:00, bshe wrote:
> > huh. VrShellDelegate shouldn't have access to com.google.vr.ndk.*
> 
> Done.
> 
>
https://codereview.chromium.org/2500153003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:201:
> if (inWebVR) mVrShell.setWebVrModeEnabled(true);
> On 2016/11/15 19:45:00, bshe wrote:
> > nit: if (mInVr && inWebVR)
> 
> No, we want to return early regardless, but if transitioning to webVR mode
> inside VR Shell, we still need to call setWebVrModeEnabled.

lgtm

[Ted C, 2016-11-16 18:07:00]

not lgtm

Again sorry, but we can't do this.

Why is DayDream calling back into our activity?  Can they accept a pending
intent?  In general, we need to push back on their API to not require exporting
of activities.

https://codereview.chromium.org/2500153003/diff/140001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2500153003/diff/140001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:376: android:exported="true"
Sorry, but I don't think we can/should do this.

By doing this, external applications will be able to send intents directly to
ChromeTabbedActivity (if they hardcode the classname as that bypasses the intent
filters).

This means any sanitizing of intents we do in ChromeLauncher would be skipped,
which is why we don't export our activities as it ensures a single place to do
this.

Also, it would allow third parties to change Chrome's behavior by bypassing our
access point.

[mthiesse, 2016-11-16 18:45:12]

https://codereview.chromium.org/2500153003/diff/140001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2500153003/diff/140001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:376: android:exported="true"
On 2016/11/16 18:07:00, Ted C wrote:
> Sorry, but I don't think we can/should do this.
> 
> By doing this, external applications will be able to send intents directly to
> ChromeTabbedActivity (if they hardcode the classname as that bypasses the
intent
> filters).
> 
> This means any sanitizing of intents we do in ChromeLauncher would be skipped,
> which is why we don't export our activities as it ensures a single place to do
> this.
> 
> Also, it would allow third parties to change Chrome's behavior by bypassing
our
> access point.

Ah, yes thanks for pointing that out. I've switched to using pendingIntents and
their API seems to be fine with that.

The alias is no longer exported, though we should keep it around for the
enableVrMode flag so that the transition is less jarring.

[Ted C, 2016-11-16 21:17:32]

much better...thank you!

https://codereview.chromium.org/2500153003/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2500153003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:39:
public static final int EXIT_VR_RESULT = 102;
I would use a slightly higher number to avoid conflicts with CTA (it uses 101
already and this could start causing unexpected conflicts if some one naively
adds a new one there).

https://codereview.chromium.org/2500153003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:130:
public boolean enterVRFromIntent(Intent intent) {
javadoc non-private methods

https://codereview.chromium.org/2500153003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:133:
mActivity.setIntent(null);
why the need for this?

would it be better to strip out the extras that we don't want to linger behind?

https://codereview.chromium.org/2500153003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:134:
if (!isVrShellEnabled()) assert transitionType != DAYDREAM_DON_TYPE_VR_SHELL;
while I "think" this is fine, asserts are removed in release builds and I hope
the compiler doesn't do something silly where it would then try to move the
assignment of inWebVR up because there are no braces.

https://codereview.chromium.org/2500153003/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right):

https://codereview.chromium.org/2500153003/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:70: bool
VrShellDelegate::RequestWebVRPresent(
why have a return value at this point?

[mthiesse, 2016-11-16 21:45:43]

Thanks Ted for the reviews - we'll look into adding an owner for the vr_shell
directory.

https://codereview.chromium.org/2500153003/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2500153003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:39:
public static final int EXIT_VR_RESULT = 102;
On 2016/11/16 21:17:32, Ted C wrote:
> I would use a slightly higher number to avoid conflicts with CTA (it uses 101
> already and this could start causing unexpected conflicts if some one naively
> adds a new one there).

Done.

https://codereview.chromium.org/2500153003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:130:
public boolean enterVRFromIntent(Intent intent) {
On 2016/11/16 21:17:32, Ted C wrote:
> javadoc non-private methods

Done.

https://codereview.chromium.org/2500153003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:133:
mActivity.setIntent(null);
On 2016/11/16 21:17:32, Ted C wrote:
> why the need for this?
> 
> would it be better to strip out the extras that we don't want to linger
behind?

Meant to delete this.

https://codereview.chromium.org/2500153003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:134:
if (!isVrShellEnabled()) assert transitionType != DAYDREAM_DON_TYPE_VR_SHELL;
On 2016/11/16 21:17:32, Ted C wrote:
> while I "think" this is fine, asserts are removed in release builds and I hope
> the compiler doesn't do something silly where it would then try to move the
> assignment of inWebVR up because there are no braces.

Done.

https://codereview.chromium.org/2500153003/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right):

https://codereview.chromium.org/2500153003/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:70: bool
VrShellDelegate::RequestWebVRPresent(
On 2016/11/16 21:17:32, Ted C wrote:
> why have a return value at this point?

No reason, I have another CL out for review that refactors and fixes this, so
I'd prefer to leave this for now. (https://codereview.chromium.org/2510873003)

[Ted C, 2016-11-16 22:05:27]

lgtm

[mthiesse, 2016-11-16 22:09:51]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-11-16 22:09:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2500153003/#ps180001
(title: "Address comments")

[commit-bot: I haz the power, 2016-11-16 22:10:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 00:11:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 00:11:32]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mthiesse, 2016-11-17 00:18:37]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-11-17 00:18:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, bajones@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2500153003/#ps200001
(title: "Handle null intent.getCategories()")

[commit-bot: I haz the power, 2016-11-17 00:19:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 00:25:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 00:25:57]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mthiesse, 2016-11-17 00:35:40]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-11-17 00:35:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, bajones@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2500153003/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-17 00:37:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 02:40:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 02:40:40]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)

[mthiesse, 2016-11-17 02:49:50]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2016-11-17 02:50:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 03:39:40]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-11-17 03:42:02]

Description was changed from

==========
Fix broken VR transitions due to ChromeTabbedActivity not being exported.

Recently, VR transitions broke due to us properly registering as a VR capable
app. For some reason this lead VrCore to handle us differently and try launching
ChromeTabbedActivity, which it couldn't because it's not exported.

This CL updates our transition code to use the new daydream API calls, giving us
enter and exit VR transitions. I also introduce an activity-alias for VR
purposes so that VrCore can call back into us after the VR DON flow completes.

BUG=665006
==========

to

==========
Fix broken VR transitions due to ChromeTabbedActivity not being exported.

Recently, VR transitions broke due to us properly registering as a VR capable
app. For some reason this lead VrCore to handle us differently and try launching
ChromeTabbedActivity, which it couldn't because it's not exported.

This CL updates our transition code to use the new daydream API calls, giving us
enter and exit VR transitions. I also introduce an activity-alias for VR
purposes so that VrCore can call back into us after the VR DON flow completes.

BUG=665006

Committed: https://crrev.com/d7d11fc27adf47033a732dc446f87ac55d29f53a
Cr-Commit-Position: refs/heads/master@{#432727}
==========

[commit-bot: I haz the power, 2016-11-17 03:42:03]

Patchset 11 (id:??) landed as
https://crrev.com/d7d11fc27adf47033a732dc446f87ac55d29f53a
Cr-Commit-Position: refs/heads/master@{#432727}