Fix Initializing VrShellImpl can throw a NullPointerException.

Fix Initializing VrShellImpl can throw a NullPointerException.

When device in portrait mode and daydream setting enable "skip enter
VR", VrShell will crash in initializing process.

This patch fix this issue.

BUG=690625

[shaobo.yan, 2017-02-20 08:13:54]

shaobo.yan@intel.com changed reviewers:
+ mthiesse@chromium.org

[shaobo.yan, 2017-02-20 08:13:56]

mthiesse@ : PTAL.

https://codereview.chromium.org/2707803003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
(right):

https://codereview.chromium.org/2707803003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:986:
public boolean isTabModelNull() {
I tried some method like onOrientationChanged or onConfigurationChanged for
informing VrShellDelegate whether the View finished initializing after
orientation changed. But they are all not what I need.
So I created this function. If you have better idea, pls let me know.

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

https://codereview.chromium.org/2707803003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:239:
if (mEnteringVr) {
This line is for this situation. Since device received several intent to enter
VR, and after enable skipping enter VR, maybeResume wil execute several times.
Since enterVR will try several times and then modify mEnteringVr to false, it
will cause problem like close VrShell but still got vr mode entered and the
orientation is portrait.

[shaobo.yan, 2017-02-21 08:51:08]

shaobo.yan@intel.com changed reviewers:
+ ddorwin@chromium.org

[mthiesse, 2017-02-21 15:34:41]

I think a better solution would be, in ChromeTabbedActivity, to move
"mVrShellDelegate.onNativeLibraryReady();" to ChromeTabbedActivity#initializeUI,
after initializeCompositorContent. This will ensure that TabModelSelector is
always available.

Then, in VrShellDelegate, instead of checking for a tabModelSelector, you can
check mNativeVrShellDelegate != 0.

[shaobo.yan, 2017-02-22 01:21:31]

On 2017/02/21 15:34:41, mthiesse wrote:
> I think a better solution would be, in ChromeTabbedActivity, to move
> "mVrShellDelegate.onNativeLibraryReady();" to
ChromeTabbedActivity#initializeUI,
> after initializeCompositorContent. This will ensure that TabModelSelector is
> always available.
> 
> Then, in VrShellDelegate, instead of checking for a tabModelSelector, you can
> check mNativeVrShellDelegate != 0.

Thx for the comments! Done.

PTAL.

[mthiesse, 2017-02-22 04:53:30]

lgtm

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

https://codereview.chromium.org/2707803003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:286:
if (mEnteringVr) {
No need to check if mEnteringVr is true, just set it to false.

https://codereview.chromium.org/2707803003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:301:
if (mNativeVrShellDelegate == 0) {
nit: merge this with the if block above, calling prepareToEnterVR() again is
basically a noop.

[mthiesse, 2017-02-22 04:54:31]

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

[mthiesse, 2017-02-22 04:54:31]

dtrainor@, please review CTA change.

[David Trainor- moved to gerrit, 2017-02-22 06:05:04]

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

https://codereview.chromium.org/2707803003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:635:
mVrShellDelegate.onNativeLibraryReady();
Should we rename this to initialize if we're moving it here?

[shaobo.yan, 2017-02-22 08:09:28]

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

https://codereview.chromium.org/2707803003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:635:
mVrShellDelegate.onNativeLibraryReady();
On 2017/02/22 06:05:04, David Trainor wrote:
> Should we rename this to initialize if we're moving it here?

Done.

[shaobo.yan, 2017-02-26 06:51:07]

ping dtrainor@ : PTAL

[shaobo.yan, 2017-02-26 08:15:14]

Sorry for the noise, forget it's weekend...

[elevated1514, 2017-02-26 10:47:50]

On 2017/02/26 08:15:14, shaobo.yan wrote:
> Sorry for the noise, forget it's weekend...

[elevated1514, 2017-02-26 12:21:36]

How do u spell ur name again



On Feb 26, 2017 5:47 AM, <elevated1514@gmail.com> wrote:

> On 2017/02/26 08:15:14, shaobo.yan wrote:
> > Sorry for the noise, forget it's weekend...
>
>
>
> https://codereview.chromium.org/2707803003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mthiesse, 2017-02-27 16:12:06]

Description was changed from

==========
Fix Initializing VrShellImpl can throw a NullPointerException.

When device in portrait mode and daydream setting enable "skip enter
VR", VrShell will crash in initializing process.

This patch fix this issue.

BUG=690625
==========

to

==========
Fix Initializing VrShellImpl can throw a NullPointerException.

When device in portrait mode and daydream setting enable "skip enter
VR", VrShell will crash in initializing process.

This patch fix this issue.

BUG=690625
==========

[mthiesse, 2017-02-27 16:12:06]

mthiesse@chromium.org changed reviewers:
- elevated1514@gmail.com

[David Trainor- moved to gerrit, 2017-02-28 01:14:58]

lgtm sorry for the delay!  am sheriff atm :(.

[shaobo.yan, 2017-02-28 01:17:55]

The CQ bit was checked by shaobo.yan@intel.com

[shaobo.yan, 2017-02-28 01:17:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2707803003/#ps80001
(title: "Address dtrainor@ address")

[commit-bot: I haz the power, 2017-02-28 01:18:23]

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, 2017-02-28 03:15:24]

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

[commit-bot: I haz the power, 2017-02-28 03:15:26]

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

[shaobo.yan, 2017-02-28 03:17:04]

The CQ bit was checked by shaobo.yan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 03:17:35]

Dry run: 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, 2017-02-28 05:02:58]

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

[commit-bot: I haz the power, 2017-02-28 05:02:59]

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

[shaobo.yan, 2017-03-01 00:41:30]

The CQ bit was checked by shaobo.yan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 00:42:20]

Dry run: 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, 2017-03-01 02:30:01]

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

[commit-bot: I haz the power, 2017-03-01 02:30:03]

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

[shaobo.yan, 2017-03-03 02:02:21]

The CQ bit was checked by shaobo.yan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 02:03:17]

Dry run: 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, 2017-03-03 03:00:33]

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

[commit-bot: I haz the power, 2017-03-03 03:00:34]

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

[shaobo.yan, 2017-03-03 05:41:46]

mthiesse@ : I believe that the latest patch you
commit(https://codereview.chromium.org/2727873002/) could fix this issue. If you
agree, I'll close this.

[mthiesse, 2017-03-03 14:07:10]

On 2017/03/03 05:41:46, shaobo.yan wrote:
> mthiesse@ : I believe that the latest patch you
> commit(https://codereview.chromium.org/2727873002/) could fix this issue. If
you
> agree, I'll close this.

Yes, I think it does fix the issue.