Launch Chrome when phone inserted into Daydream headset IFF Chrome was the active app

Launch Chrome when phone inserted into Daydream headset IFF Chrome was the active app

This is implemented by using Daydream Api. After a headset is detected by phone,
instead of launch Daydream Home app, ChromeVR is launched iff Chrome was the foreground
app. Follow up CLs will trigger auto presentation if VR aware content was showing.

BUG=641401
Committed: https://crrev.com/1754d506140c20ef7f263d75eff24a58a60398ed
Cr-Commit-Position: refs/heads/master@{#427070}

[bshe, 2016-10-17 18:46:37]

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

[bshe, 2016-10-17 18:46:38]

Hi Micheal and David.

Could you please take a look at this CL?

Thanks!

[mthiesse, 2016-10-17 19:02:04]

https://codereview.chromium.org/2428563003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2428563003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:422:
Intent intent = getIntent();
Why are you using getIntent(), which may be null here? You probably want to
create a new intent.

Also, you should probably move all of this logic into
mVrShellDelegate.registerDaydreamIntent().

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

https://codereview.chromium.org/2428563003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:51:
"org.chromium.chrome.browser.vr_shell.LAUNCH_FROM_HEADSET";
nit: LAUNCH_FROM_HEADSET is maybe too specific, in that it isn't necessarily the
headset triggering this (though it may be at the moment).

How about DAYDREAM_DON_TRANSITION?

[bshe, 2016-10-18 15:39:58]

https://codereview.chromium.org/2428563003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2428563003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:422:
Intent intent = getIntent();
On 2016/10/17 19:02:04, mthiesse wrote:
> Why are you using getIntent(), which may be null here? You probably want to
> create a new intent.
> 
> Also, you should probably move all of this logic into
> mVrShellDelegate.registerDaydreamIntent().

Good point. getIntent is not the right thing to use here.

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

https://codereview.chromium.org/2428563003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:51:
"org.chromium.chrome.browser.vr_shell.LAUNCH_FROM_HEADSET";
On 2016/10/17 19:02:04, mthiesse wrote:
> nit: LAUNCH_FROM_HEADSET is maybe too specific, in that it isn't necessarily
the
> headset triggering this (though it may be at the moment).
> 
> How about DAYDREAM_DON_TRANSITION?

What's the other things that could trigger this? I can think of "headset",
"webvr present", "video present" and perhaps a "manual entry". In that sense, I
feel LAUNCH_FROM_HEADSET makes more sense. I want to distinguish them as much as
possible. So we could have better UMA tracking if we want. Ideally, we want to
have a enum/string field to specify the reason why VrShell is triggered. But
before we actually use UMA, I want to use a boolean to keep it as simple as
possible.

[mthiesse, 2016-10-18 15:46:00]

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

https://codereview.chromium.org/2428563003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:51:
"org.chromium.chrome.browser.vr_shell.LAUNCH_FROM_HEADSET";
On 2016/10/18 15:39:57, bshe wrote:
> On 2016/10/17 19:02:04, mthiesse wrote:
> > nit: LAUNCH_FROM_HEADSET is maybe too specific, in that it isn't necessarily
> the
> > headset triggering this (though it may be at the moment).
> > 
> > How about DAYDREAM_DON_TRANSITION?
> 
> What's the other things that could trigger this? I can think of "headset",
> "webvr present", "video present" and perhaps a "manual entry". In that sense,
I
> feel LAUNCH_FROM_HEADSET makes more sense. I want to distinguish them as much
as
> possible. So we could have better UMA tracking if we want. Ideally, we want to
> have a enum/string field to specify the reason why VrShell is triggered. But
> before we actually use UMA, I want to use a boolean to keep it as simple as
> possible.

LAUNCH_FROM_HEADSET is a daydream implementation detail, equivalent at the
moment to the more generic DAYDREAM_DON_TRANSITION. Perhaps daydream
auto-transition won't always be triggered from the headset - we shouldn't care
how daydream decides it's time to auto transition. Your point about UMA is
irrelevant, as what we call this flag has no impact on whether we distinguish
this from other types of transition.

If you're worried about DAYDREAM_DON_TRANSITION being slightly ambiguous, then
use DAYDREAM_DON_AUTO_TRANSITION.

[bshe, 2016-10-19 15:42:19]

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

https://codereview.chromium.org/2428563003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:51:
"org.chromium.chrome.browser.vr_shell.LAUNCH_FROM_HEADSET";
On 2016/10/18 15:46:00, mthiesse wrote:
> On 2016/10/18 15:39:57, bshe wrote:
> > On 2016/10/17 19:02:04, mthiesse wrote:
> > > nit: LAUNCH_FROM_HEADSET is maybe too specific, in that it isn't
necessarily
> > the
> > > headset triggering this (though it may be at the moment).
> > > 
> > > How about DAYDREAM_DON_TRANSITION?
> > 
> > What's the other things that could trigger this? I can think of "headset",
> > "webvr present", "video present" and perhaps a "manual entry". In that
sense,
> I
> > feel LAUNCH_FROM_HEADSET makes more sense. I want to distinguish them as
much
> as
> > possible. So we could have better UMA tracking if we want. Ideally, we want
to
> > have a enum/string field to specify the reason why VrShell is triggered. But
> > before we actually use UMA, I want to use a boolean to keep it as simple as
> > possible.
> 
> LAUNCH_FROM_HEADSET is a daydream implementation detail, equivalent at the
> moment to the more generic DAYDREAM_DON_TRANSITION. Perhaps daydream
> auto-transition won't always be triggered from the headset - we shouldn't care
> how daydream decides it's time to auto transition. Your point about UMA is
> irrelevant, as what we call this flag has no impact on whether we distinguish
> this from other types of transition.
> 
> If you're worried about DAYDREAM_DON_TRANSITION being slightly ambiguous, then
> use DAYDREAM_DON_AUTO_TRANSITION.

discussed offline. DON_TRANSITION is a little bit too broad. Renamed to
DAYDREAM_DON_AUTO_TRANSITION.

[bshe, 2016-10-19 15:43:00]

The CQ bit was checked by bshe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 15:43:18]

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

[mthiesse, 2016-10-19 15:47:11]

lgtm

[commit-bot: I haz the power, 2016-10-19 17:37:40]

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

[commit-bot: I haz the power, 2016-10-19 17:37:41]

Dry run: This issue passed the CQ dry run.

[bshe, 2016-10-21 14:44:11]

On 2016/10/19 15:47:11, mthiesse wrote:
> lgtm

friendly ping? David, do you mind to take a look at java files? Thanks!

[David Trainor- moved to gerrit, 2016-10-21 16:17:20]

lgtm % nits

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:420:
if (mVrShellDelegate.isVrShellEnabled()) {
Should these just be rolled into the resume/pause APIs?  Do we need a separate
call into the delegate from here?

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrPrivateApi.java
(right):

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrPrivateApi.java:10:
* Abstract away DaydreamImpl class, which may or may not be present at runtime
depending on compile
Should this just be VrDaydream then?  VrPrivateApi is pretty ambiguous.

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrPrivateApiImpl.java
(right):

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrPrivateApiImpl.java:41:
mDaydreamApi.close();
Check if mDaydreamApi != null?

[bshe, 2016-10-24 13:45:25]

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:420:
if (mVrShellDelegate.isVrShellEnabled()) {
On 2016/10/21 16:17:20, David Trainor wrote:
> Should these just be rolled into the resume/pause APIs?  Do we need a separate
> call into the delegate from here?

Done.

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrPrivateApi.java
(right):

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrPrivateApi.java:10:
* Abstract away DaydreamImpl class, which may or may not be present at runtime
depending on compile
On 2016/10/21 16:17:20, David Trainor wrote:
> Should this just be VrDaydream then?  VrPrivateApi is pretty ambiguous.

Done.

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrPrivateApiImpl.java
(right):

https://codereview.chromium.org/2428563003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrPrivateApiImpl.java:41:
mDaydreamApi.close();
On 2016/10/21 16:17:20, David Trainor wrote:
> Check if mDaydreamApi != null?

Done.

[bshe, 2016-10-24 13:45:32]

The CQ bit was checked by bshe@chromium.org

[bshe, 2016-10-24 13:45:32]

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/2428563003/#ps80001
(title: "review")

[commit-bot: I haz the power, 2016-10-24 13:45:51]

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-10-24 14:47:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-24 14:51:37]

Description was changed from

==========
Launch Chrome when phone inserted into Daydream headset IFF Chrome was the
active app

This is implemented by using Daydream Api. After a headset is detected by phone,
instead of launch Daydream Home app, ChromeVR is launched iff Chrome was the
foreground
app. Follow up CLs will trigger auto presentation if VR aware content was
showing.


BUG=641401
==========

to

==========
Launch Chrome when phone inserted into Daydream headset IFF Chrome was the
active app

This is implemented by using Daydream Api. After a headset is detected by phone,
instead of launch Daydream Home app, ChromeVR is launched iff Chrome was the
foreground
app. Follow up CLs will trigger auto presentation if VR aware content was
showing.

BUG=641401

Committed: https://crrev.com/1754d506140c20ef7f263d75eff24a58a60398ed
Cr-Commit-Position: refs/heads/master@{#427070}
==========

[commit-bot: I haz the power, 2016-10-24 14:51:39]

Patchset 5 (id:??) landed as
https://crrev.com/1754d506140c20ef7f263d75eff24a58a60398ed
Cr-Commit-Position: refs/heads/master@{#427070}