Fix WebVR presentation is distorted if device is in portrait mode

Fix WebVR presentation is distorted if device is in portrait mode

BUG=681059

[shaobo.yan, 2017-01-17 01:59:30]

Description was changed from

==========
Fix WebVR presentation is distorted if device is in portrait mode

BUG=681059
==========

to

==========
Fix WebVR presentation is distorted if device is in portrait mode

BUG=681059
==========

[shaobo.yan, 2017-01-17 01:59:30]

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

[shaobo.yan, 2017-01-17 02:36:56]

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

[girard, 2017-01-17 03:00:42]

lgtm with one nit

https://codereview.chromium.org/2633243002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2633243002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:142:
if (primaryDisplay.getPhysicalDisplayWidth()
Format nit: move > to end of first line

[shaobo.yan, 2017-01-17 03:07:40]

On 2017/01/17 03:00:42, girard wrote:
> lgtm with one nit
> 
>
https://codereview.chromium.org/2633243002/diff/1/chrome/android/java/src/org...
> File
> chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
> (right):
> 
>
https://codereview.chromium.org/2633243002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:142:
> if (primaryDisplay.getPhysicalDisplayWidth()
> Format nit: move > to end of first line

Thx for reviewing. But the presubmit check tool suggest me to place '>' in a new
line.

[mthiesse, 2017-01-17 03:08:14]

This doesn't make sense, the device is forced into landscape mode before
VrShellImpl is created.

[mthiesse, 2017-01-17 03:11:25]

On 2017/01/17 03:08:14, mthiesse wrote:
> This doesn't make sense, the device is forced into landscape mode before
> VrShellImpl is created.

I mean this probably does 'fix' the problem in that we use landscape dimensions
even in portrait mode, but we shouldn't be in portrait in the first place.

[girard, 2017-01-17 03:15:37]

On 2017/01/17 03:11:25, mthiesse wrote:
> On 2017/01/17 03:08:14, mthiesse wrote:
> > This doesn't make sense, the device is forced into landscape mode before
> > VrShellImpl is created.
> 
> I mean this probably does 'fix' the problem in that we use landscape
dimensions
> even in portrait mode, but we shouldn't be in portrait in the first place.

mthiesse: Check out the video in crbug.com/681059 - it appears that the user has
locked the device in portrait mode, then goes into chrome, then puts chrome into
vr mode.

[shaobo.yan, 2017-01-17 03:16:32]

On 2017/01/17 03:11:25, mthiesse wrote:
> On 2017/01/17 03:08:14, mthiesse wrote:
> > This doesn't make sense, the device is forced into landscape mode before
> > VrShellImpl is created.
> 
> I mean this probably does 'fix' the problem in that we use landscape
dimensions
> even in portrait mode, but we shouldn't be in portrait in the first place.

That's right. But I'm a user whose like keep screen in portrait when I use
phone. And I guess, in this situation, when there is a button named "Enter VR",
and I click it, the blank will appear.

[mthiesse, 2017-01-17 03:18:13]

On 2017/01/17 03:15:37, girard wrote:
> On 2017/01/17 03:11:25, mthiesse wrote:
> > On 2017/01/17 03:08:14, mthiesse wrote:
> > > This doesn't make sense, the device is forced into landscape mode before
> > > VrShellImpl is created.
> > 
> > I mean this probably does 'fix' the problem in that we use landscape
> dimensions
> > even in portrait mode, but we shouldn't be in portrait in the first place.
> 
> mthiesse: Check out the video in crbug.com/681059 - it appears that the user
has
> locked the device in portrait mode, then goes into chrome, then puts chrome
into
> vr mode.

That setting only disables auto-rotate. Apps can still force themselves into
landscape. Maybe we're not doing that correctly.

[shaobo.yan, 2017-01-17 03:24:24]

On 2017/01/17 03:18:13, mthiesse wrote:
> On 2017/01/17 03:15:37, girard wrote:
> > On 2017/01/17 03:11:25, mthiesse wrote:
> > > On 2017/01/17 03:08:14, mthiesse wrote:
> > > > This doesn't make sense, the device is forced into landscape mode before
> > > > VrShellImpl is created.
> > > 
> > > I mean this probably does 'fix' the problem in that we use landscape
> > dimensions
> > > even in portrait mode, but we shouldn't be in portrait in the first place.
> > 
> > mthiesse: Check out the video in crbug.com/681059 - it appears that the user
> has
> > locked the device in portrait mode, then goes into chrome, then puts chrome
> into
> > vr mode.
> 
> That setting only disables auto-rotate. Apps can still force themselves into
> landscape. Maybe we're not doing that correctly.

I know what you mean now.

[shaobo.yan, 2017-01-17 03:27:20]

On 2017/01/17 03:18:13, mthiesse wrote:
> On 2017/01/17 03:15:37, girard wrote:
> > On 2017/01/17 03:11:25, mthiesse wrote:
> > > On 2017/01/17 03:08:14, mthiesse wrote:
> > > > This doesn't make sense, the device is forced into landscape mode before
> > > > VrShellImpl is created.
> > > 
> > > I mean this probably does 'fix' the problem in that we use landscape
> > dimensions
> > > even in portrait mode, but we shouldn't be in portrait in the first place.
> > 
> > mthiesse: Check out the video in crbug.com/681059 - it appears that the user
> has
> > locked the device in portrait mode, then goes into chrome, then puts chrome
> into
> > vr mode.
> 
> That setting only disables auto-rotate. Apps can still force themselves into
> landscape. Maybe we're not doing that correctly.

I know what you mean now.
It seems that the code
here(https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java?l=215)
not work proper.And we should figure out the reason and fix it.

[mthiesse, 2017-01-17 15:35:24]

On 2017/01/17 03:27:20, shaobo.yan wrote:
> On 2017/01/17 03:18:13, mthiesse wrote:
> > On 2017/01/17 03:15:37, girard wrote:
> > > On 2017/01/17 03:11:25, mthiesse wrote:
> > > > On 2017/01/17 03:08:14, mthiesse wrote:
> > > > > This doesn't make sense, the device is forced into landscape mode
before
> > > > > VrShellImpl is created.
> > > > 
> > > > I mean this probably does 'fix' the problem in that we use landscape
> > > dimensions
> > > > even in portrait mode, but we shouldn't be in portrait in the first
place.
> > > 
> > > mthiesse: Check out the video in crbug.com/681059 - it appears that the
user
> > has
> > > locked the device in portrait mode, then goes into chrome, then puts
chrome
> > into
> > > vr mode.
> > 
> > That setting only disables auto-rotate. Apps can still force themselves into
> > landscape. Maybe we're not doing that correctly.
> 
> I know what you mean now.
> It seems that the code
>
here(https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java?l=215)
> not work proper.And we should figure out the reason and fix it.

I think this is fixed by my patch here:
https://codereview.chromium.org/2635123002/
Can you please verify?

[shaobo.yan, 2017-01-18 01:27:37]

On 2017/01/17 15:35:24, mthiesse wrote:
> On 2017/01/17 03:27:20, shaobo.yan wrote:
> > On 2017/01/17 03:18:13, mthiesse wrote:
> > > On 2017/01/17 03:15:37, girard wrote:
> > > > On 2017/01/17 03:11:25, mthiesse wrote:
> > > > > On 2017/01/17 03:08:14, mthiesse wrote:
> > > > > > This doesn't make sense, the device is forced into landscape mode
> before
> > > > > > VrShellImpl is created.
> > > > > 
> > > > > I mean this probably does 'fix' the problem in that we use landscape
> > > > dimensions
> > > > > even in portrait mode, but we shouldn't be in portrait in the first
> place.
> > > > 
> > > > mthiesse: Check out the video in crbug.com/681059 - it appears that the
> user
> > > has
> > > > locked the device in portrait mode, then goes into chrome, then puts
> chrome
> > > into
> > > > vr mode.
> > > 
> > > That setting only disables auto-rotate. Apps can still force themselves
into
> > > landscape. Maybe we're not doing that correctly.
> > 
> > I know what you mean now.
> > It seems that the code
> >
>
here(https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java?l=215)
> > not work proper.And we should figure out the reason and fix it.
> 
> I think this is fixed by my patch here:
> https://codereview.chromium.org/2635123002/
> Can you please verify?

It indeed fix the bug :)
I'll close this issue.

[shaobo.yan, 2017-01-18 01:29:42]

With verified, the root cause of this bug is resolved by mthiesse@ patch here:
https://codereview.chromium.org/2635123002/.
Close this issue.