Don't dispatch vrdisplayactivate event if the page that listens for it is invisible

Don't dispatch vrdisplayactivate event if the page that listens for it is invisible

BUG=669190
Committed: https://crrev.com/af675fa58d6a3557a6704a9faeac8012dc360209
Cr-Commit-Position: refs/heads/master@{#435500}

[bshe, 2016-11-30 18:40:12]

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

[commit-bot: I haz the power, 2016-11-30 18:40:36]

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

[bshe, 2016-11-30 18:41:07]

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

[bshe, 2016-11-30 18:41:07]

Hi guys. PTAL. Thanks.

[commit-bot: I haz the power, 2016-11-30 18:59:20]

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

[commit-bot: I haz the power, 2016-11-30 18:59:21]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[mthiesse, 2016-11-30 19:02:03]

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

https://codereview.chromium.org/2539763004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:153:
if (mListeningForWebVrActivateBeforePause && !mRequestedWebVR) {
Should add a block for if (!mListeningForWebVrActivateBeforePause &&
!mRequestedWebVR && !isVrShellEnabled()) {
  mDaydreamApi.launchVrHomescreen()
}

[bshe, 2016-11-30 19:16:28]

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

https://codereview.chromium.org/2539763004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:153:
if (mListeningForWebVrActivateBeforePause && !mRequestedWebVR) {
On 2016/11/30 19:02:03, mthiesse wrote:
> Should add a block for if (!mListeningForWebVrActivateBeforePause &&
> !mRequestedWebVR && !isVrShellEnabled()) {
>   mDaydreamApi.launchVrHomescreen()
> }

Agree it should be fixed. But this is a different issue in my mind. So
a separate CL would make more sense?

[bshe, 2016-11-30 19:26:58]

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

[commit-bot: I haz the power, 2016-11-30 19:27:36]

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

[mthiesse, 2016-11-30 19:31:42]

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

https://codereview.chromium.org/2539763004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:153:
if (mListeningForWebVrActivateBeforePause && !mRequestedWebVR) {
On 2016/11/30 19:16:28, bshe wrote:
> On 2016/11/30 19:02:03, mthiesse wrote:
> > Should add a block for if (!mListeningForWebVrActivateBeforePause &&
> > !mRequestedWebVR && !isVrShellEnabled()) {
> >   mDaydreamApi.launchVrHomescreen()
> > }
> 
> Agree it should be fixed. But this is a different issue in my mind. So
> a separate CL would make more sense?

I think it's an issue that's caused by this CL, so we should fix it here. Before
this CL we had the assumption that when we we're called back from Daydream, we
had something to show in VR. That's no longer the case; there's now a good
chance that we have nothing to show when we're called back.

[commit-bot: I haz the power, 2016-11-30 20:41:11]

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

[commit-bot: I haz the power, 2016-11-30 20:41:12]

Dry run: This issue passed the CQ dry run.

[bshe, 2016-11-30 22:01:09]

PTAL

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

https://codereview.chromium.org/2539763004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:153:
if (mListeningForWebVrActivateBeforePause && !mRequestedWebVR) {
On 2016/11/30 19:31:42, mthiesse wrote:
> On 2016/11/30 19:16:28, bshe wrote:
> > On 2016/11/30 19:02:03, mthiesse wrote:
> > > Should add a block for if (!mListeningForWebVrActivateBeforePause &&
> > > !mRequestedWebVR && !isVrShellEnabled()) {
> > >   mDaydreamApi.launchVrHomescreen()
> > > }
> > 
> > Agree it should be fixed. But this is a different issue in my mind. So
> > a separate CL would make more sense?
> 
> I think it's an issue that's caused by this CL, so we should fix it here.
Before
> this CL we had the assumption that when we we're called back from Daydream, we
> had something to show in VR. That's no longer the case; there's now a good
> chance that we have nothing to show when we're called back.

Discussed offline. There is another issue which results DaydreamIntent not
properly
unregistered or failed to unregister, which makes the behavior confusing. So
include the
proposed fix here.

[bshe, 2016-11-30 22:01:13]

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

[commit-bot: I haz the power, 2016-11-30 22:02:12]

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

[mthiesse, 2016-11-30 22:03:32]

lgtm

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

https://codereview.chromium.org/2539763004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:157:
if (!mListeningForWebVrActivateBeforePause && !mRequestedWebVR &&
!isVrShellEnabled()) {
nit: add a comment about why we have to do this.

[bajones, 2016-11-30 22:07:47]

LGTM too

[bshe, 2016-11-30 23:10:12]

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

https://codereview.chromium.org/2539763004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:157:
if (!mListeningForWebVrActivateBeforePause && !mRequestedWebVR &&
!isVrShellEnabled()) {
On 2016/11/30 22:03:32, mthiesse wrote:
> nit: add a comment about why we have to do this.

Done.

[bshe, 2016-11-30 23:10:19]

The CQ bit was checked by bshe@chromium.org

[bshe, 2016-11-30 23:10:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2539763004/#ps80001
(title: "nit")

[commit-bot: I haz the power, 2016-11-30 23:10:46]

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

[haraken, 2016-11-30 23:46:27]

Drive-by: I'm not sure if there's an easy way to test a scenario that changes
page visibility (maybe we need to add window.internals APIs to change page
visibility), but it would be nicer to have tests for this kind of change.

[commit-bot: I haz the power, 2016-12-01 00:43:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480547419376860,
"parent_rev": "86223ffc9f291332282388dc129a99f8354d1f60", "commit_rev":
"66f46d076f8011652bb9c9105124d7587a6d5b23"}

[commit-bot: I haz the power, 2016-12-01 00:43:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-01 00:46:11]

Description was changed from

==========
Don't dispatch vrdisplayactivate event if the page that listens for it is
invisible


BUG=669190
==========

to

==========
Don't dispatch vrdisplayactivate event if the page that listens for it is
invisible

BUG=669190

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

[commit-bot: I haz the power, 2016-12-01 00:46:13]

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