Fix VrShell resizing incorrectly by setting the CompositorViewHolder to a fixed size when in VR.

Fix VrShell resizing incorrectly by setting the CompositorViewHolder to a fixed size when in VR.

BUG=680240

[mthiesse, 2017-01-16 23:09:03]

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

[mthiesse, 2017-01-16 23:09:03]

PTAL. Open to other suggestions, this is a pretty hacky approach but should at
least be really easy to reason about and not impact non-VR clank.

[boliu, 2017-01-17 16:05:49]

On 2017/01/16 23:09:03, mthiesse wrote:
> PTAL. Open to other suggestions, this is a pretty hacky approach but should at
> least be really easy to reason about and not impact non-VR clank.

Yeah that's a hack..

Can VR play with the layout of ContentView such that it's never resized? so like
some sort of fixed layout?

[mthiesse1, 2017-01-17 17:15:17]

On 2017/01/17 16:05:49, boliu wrote:
> On 2017/01/16 23:09:03, mthiesse wrote:
> > PTAL. Open to other suggestions, this is a pretty hacky approach but should
at
> > least be really easy to reason about and not impact non-VR clank.
> 
> Yeah that's a hack..
> 
> Can VR play with the layout of ContentView such that it's never resized? so
like
> some sort of fixed layout?

So I'm worried that that's a worse hack. We've been down that path before of
changing the layout of the ContentView, and ran into really complex cases of
other parts of the codebase also changing the layout at other times. We were
also crashing for still unknown reasons relating to layout params being the
wrong type. The current approach on ToT of calling onSizeChanged on CVC has been
the least intrusive and most reliable so far, so I would prefer to stick with it
if possible.

[boliu, 2017-01-17 17:30:52]

On 2017/01/17 17:15:17, mthiesse1 wrote:
> On 2017/01/17 16:05:49, boliu wrote:
> > On 2017/01/16 23:09:03, mthiesse wrote:
> > > PTAL. Open to other suggestions, this is a pretty hacky approach but
should
> at
> > > least be really easy to reason about and not impact non-VR clank.
> > 
> > Yeah that's a hack..
> > 
> > Can VR play with the layout of ContentView such that it's never resized? so
> like
> > some sort of fixed layout?
> 
> So I'm worried that that's a worse hack. We've been down that path before of
> changing the layout of the ContentView, and ran into really complex cases of
> other parts of the codebase also changing the layout at other times. We were
> also crashing for still unknown reasons relating to layout params being the
> wrong type.

But changing layout and interacting correctly with the rest of the views is
exactly what VR eventually needs to do, right? Have you talked to the UI owners
about how VR will be integrated into existing layout, and about the problems
you've run into?

This hack has no basis in correctness, so probably has many issues as well. It
just happens you didn't run into any of them. Eg the physical backing size won't
match viewport size anymore and we already know that can lead to weird rendering
results: crbug.com/671967

> The current approach on ToT of calling onSizeChanged on CVC has been
> the least intrusive and most reliable so far, so I would prefer to stick with
it
> if possible.

[mthiesse, 2017-01-17 18:08:58]

On 2017/01/17 17:30:52, boliu wrote:
> On 2017/01/17 17:15:17, mthiesse1 wrote:
> > On 2017/01/17 16:05:49, boliu wrote:
> > > On 2017/01/16 23:09:03, mthiesse wrote:
> > > > PTAL. Open to other suggestions, this is a pretty hacky approach but
> should
> > at
> > > > least be really easy to reason about and not impact non-VR clank.
> > > 
> > > Yeah that's a hack..
> > > 
> > > Can VR play with the layout of ContentView such that it's never resized?
so
> > like
> > > some sort of fixed layout?
> > 
> > So I'm worried that that's a worse hack. We've been down that path before of
> > changing the layout of the ContentView, and ran into really complex cases of
> > other parts of the codebase also changing the layout at other times. We were
> > also crashing for still unknown reasons relating to layout params being the
> > wrong type.
> 
> But changing layout and interacting correctly with the rest of the views is
> exactly what VR eventually needs to do, right? Have you talked to the UI
owners
> about how VR will be integrated into existing layout, and about the problems
> you've run into?
> 
> This hack has no basis in correctness, so probably has many issues as well. It
> just happens you didn't run into any of them. Eg the physical backing size
won't
> match viewport size anymore and we already know that can lead to weird
rendering
> results: crbug.com/671967

No, the physical backing size does match the viewport, because we control the
compositor, the viewport size, and the physical backing size. Literally the only
relevant thing we're getting from the CVC is setting the css page size. 

As soon as we have a way to control the css size without going through the CVC
we'll do so.

As for our place in the existing layout, we would like to live effectively at
the root, as we're always fullscreen and draw over everything - we ideally don't
care at all about anything else in the view hierarchy and leave it unchanged.

[boliu, 2017-01-17 18:18:15]

On 2017/01/17 18:08:58, mthiesse wrote:
> On 2017/01/17 17:30:52, boliu wrote:
> > On 2017/01/17 17:15:17, mthiesse1 wrote:
> > > On 2017/01/17 16:05:49, boliu wrote:
> > > > On 2017/01/16 23:09:03, mthiesse wrote:
> > > > > PTAL. Open to other suggestions, this is a pretty hacky approach but
> > should
> > > at
> > > > > least be really easy to reason about and not impact non-VR clank.
> > > > 
> > > > Yeah that's a hack..
> > > > 
> > > > Can VR play with the layout of ContentView such that it's never resized?
> so
> > > like
> > > > some sort of fixed layout?
> > > 
> > > So I'm worried that that's a worse hack. We've been down that path before
of
> > > changing the layout of the ContentView, and ran into really complex cases
of
> > > other parts of the codebase also changing the layout at other times. We
were
> > > also crashing for still unknown reasons relating to layout params being
the
> > > wrong type.
> > 
> > But changing layout and interacting correctly with the rest of the views is
> > exactly what VR eventually needs to do, right? Have you talked to the UI
> owners
> > about how VR will be integrated into existing layout, and about the problems
> > you've run into?
> > 
> > This hack has no basis in correctness, so probably has many issues as well.
It
> > just happens you didn't run into any of them. Eg the physical backing size
> won't
> > match viewport size anymore and we already know that can lead to weird
> rendering
> > results: crbug.com/671967
> 
> No, the physical backing size does match the viewport, because we control the
> compositor, the viewport size, and the physical backing size. Literally the
only
> relevant thing we're getting from the CVC is setting the css page size.

But that's not expressed in this CL. Someone else comes along and wants to use
this doesn't know that they will also need to do A and B and C and D to make
things work.

> 
> As soon as we have a way to control the css size without going through the CVC
> we'll do so.
> 
> As for our place in the existing layout, we would like to live effectively at
> the root, as we're always fullscreen and draw over everything - we ideally
don't
> care at all about anything else in the view hierarchy and leave it unchanged.

Have you looked into fullscreen video and what happens in that case? Because the
web contents also needs to be resized to fullscreen. Can that path be reused?

[amp, 2017-01-17 18:31:08]

On 2017/01/17 18:18:15, boliu wrote:
> On 2017/01/17 18:08:58, mthiesse wrote:
> > On 2017/01/17 17:30:52, boliu wrote:
> > > On 2017/01/17 17:15:17, mthiesse1 wrote:
> > > > On 2017/01/17 16:05:49, boliu wrote:
> > > > > On 2017/01/16 23:09:03, mthiesse wrote:
> > > > > > PTAL. Open to other suggestions, this is a pretty hacky approach but
> > > should
> > > > at
> > > > > > least be really easy to reason about and not impact non-VR clank.
> > > > > 
> > > > > Yeah that's a hack..
> > > > > 
> > > > > Can VR play with the layout of ContentView such that it's never
resized?
> > so
> > > > like
> > > > > some sort of fixed layout?
> > > > 
> > > > So I'm worried that that's a worse hack. We've been down that path
before
> of
> > > > changing the layout of the ContentView, and ran into really complex
cases
> of
> > > > other parts of the codebase also changing the layout at other times. We
> were
> > > > also crashing for still unknown reasons relating to layout params being
> the
> > > > wrong type.
> > > 
> > > But changing layout and interacting correctly with the rest of the views
is
> > > exactly what VR eventually needs to do, right? Have you talked to the UI
> > owners
> > > about how VR will be integrated into existing layout, and about the
problems
> > > you've run into?
> > > 
> > > This hack has no basis in correctness, so probably has many issues as
well.
> It
> > > just happens you didn't run into any of them. Eg the physical backing size
> > won't
> > > match viewport size anymore and we already know that can lead to weird
> > rendering
> > > results: crbug.com/671967
> > 
> > No, the physical backing size does match the viewport, because we control
the
> > compositor, the viewport size, and the physical backing size. Literally the
> only
> > relevant thing we're getting from the CVC is setting the css page size.
> 
> But that's not expressed in this CL. Someone else comes along and wants to use
> this doesn't know that they will also need to do A and B and C and D to make
> things work.
> 
> > 
> > As soon as we have a way to control the css size without going through the
CVC
> > we'll do so.
> > 
> > As for our place in the existing layout, we would like to live effectively
at
> > the root, as we're always fullscreen and draw over everything - we ideally
> don't
> > care at all about anything else in the view hierarchy and leave it
unchanged.
> 
> Have you looked into fullscreen video and what happens in that case? Because
the
> web contents also needs to be resized to fullscreen. Can that path be reused?

Does this work for media going full screen in vr shell?  (I'll try to patch it
and verify).

There still needs to be a differentiation between web contents in vr at default
size and webcontents in vr at full screen.

For instance a user browsing youtube in vr (not using the native app) should be
able to navigate the site and then click on the full screen button of a video
(which has a special treatment in vr shell).

If we ignore size change messages I think we might lose that.

[mthiesse, 2017-01-17 18:43:38]

On 2017/01/17 18:31:08, amp wrote:
> On 2017/01/17 18:18:15, boliu wrote:
> > On 2017/01/17 18:08:58, mthiesse wrote:
> > > On 2017/01/17 17:30:52, boliu wrote:
> > > > On 2017/01/17 17:15:17, mthiesse1 wrote:
> > > > > On 2017/01/17 16:05:49, boliu wrote:
> > > > > > On 2017/01/16 23:09:03, mthiesse wrote:
> > > > > > > PTAL. Open to other suggestions, this is a pretty hacky approach
but
> > > > should
> > > > > at
> > > > > > > least be really easy to reason about and not impact non-VR clank.
> > > > > > 
> > > > > > Yeah that's a hack..
> > > > > > 
> > > > > > Can VR play with the layout of ContentView such that it's never
> resized?
> > > so
> > > > > like
> > > > > > some sort of fixed layout?
> > > > > 
> > > > > So I'm worried that that's a worse hack. We've been down that path
> before
> > of
> > > > > changing the layout of the ContentView, and ran into really complex
> cases
> > of
> > > > > other parts of the codebase also changing the layout at other times.
We
> > were
> > > > > also crashing for still unknown reasons relating to layout params
being
> > the
> > > > > wrong type.
> > > > 
> > > > But changing layout and interacting correctly with the rest of the views
> is
> > > > exactly what VR eventually needs to do, right? Have you talked to the UI
> > > owners
> > > > about how VR will be integrated into existing layout, and about the
> problems
> > > > you've run into?
> > > > 
> > > > This hack has no basis in correctness, so probably has many issues as
> well.
> > It
> > > > just happens you didn't run into any of them. Eg the physical backing
size
> > > won't
> > > > match viewport size anymore and we already know that can lead to weird
> > > rendering
> > > > results: crbug.com/671967
> > > 
> > > No, the physical backing size does match the viewport, because we control
> the
> > > compositor, the viewport size, and the physical backing size. Literally
the
> > only
> > > relevant thing we're getting from the CVC is setting the css page size.
> > 
> > But that's not expressed in this CL. Someone else comes along and wants to
use
> > this doesn't know that they will also need to do A and B and C and D to make
> > things work.
> > 
> > > 
> > > As soon as we have a way to control the css size without going through the
> CVC
> > > we'll do so.
> > > 
> > > As for our place in the existing layout, we would like to live effectively
> at
> > > the root, as we're always fullscreen and draw over everything - we ideally
> > don't
> > > care at all about anything else in the view hierarchy and leave it
> unchanged.
> > 
> > Have you looked into fullscreen video and what happens in that case? Because
> the
> > web contents also needs to be resized to fullscreen. Can that path be
reused?

Two comments: First, the fullscreen path is tangential to this discussion
because we don't want fullscreen resolution, we want arbitrary resolution that
we control. Second, we've been down the fullscreen path for hiding the rest of
Clank UI, and it's a can of worms that was far worse than the alternative of
just hiding the CompositorView and drawing over top of the omnibox, partially
because everything always wants to kick you out of fullscreen at every
opportunity.

> Does this work for media going full screen in vr shell?  (I'll try to patch it
> and verify).
> 
> There still needs to be a differentiation between web contents in vr at
default
> size and webcontents in vr at full screen.
> 
> For instance a user browsing youtube in vr (not using the native app) should
be
> able to navigate the site and then click on the full screen button of a video
> (which has a special treatment in vr shell).
> 
> If we ignore size change messages I think we might lose that.

No, it doesn't change this at all. Don't we listen in the WebContentsObserver
for the tab going fullscreen? We then decide ourselves what size we want to make
the content window - the android views involved are irrelevant.

[mthiesse, 2017-01-17 18:44:54]

> But that's not expressed in this CL. Someone else comes along and wants to use
> this doesn't know that they will also need to do A and B and C and D to make
> things work.

If this is the concern, we can make this function as scary as need be, so nobody
else uses it.

[amp, 2017-01-17 19:01:33]

On 2017/01/17 18:31:08, amp wrote:
> On 2017/01/17 18:18:15, boliu wrote:
> > On 2017/01/17 18:08:58, mthiesse wrote:
> > > On 2017/01/17 17:30:52, boliu wrote:
> > > > On 2017/01/17 17:15:17, mthiesse1 wrote:
> > > > > On 2017/01/17 16:05:49, boliu wrote:
> > > > > > On 2017/01/16 23:09:03, mthiesse wrote:
> > > > > > > PTAL. Open to other suggestions, this is a pretty hacky approach
but
> > > > should
> > > > > at
> > > > > > > least be really easy to reason about and not impact non-VR clank.
> > > > > > 
> > > > > > Yeah that's a hack..
> > > > > > 
> > > > > > Can VR play with the layout of ContentView such that it's never
> resized?
> > > so
> > > > > like
> > > > > > some sort of fixed layout?
> > > > > 
> > > > > So I'm worried that that's a worse hack. We've been down that path
> before
> > of
> > > > > changing the layout of the ContentView, and ran into really complex
> cases
> > of
> > > > > other parts of the codebase also changing the layout at other times.
We
> > were
> > > > > also crashing for still unknown reasons relating to layout params
being
> > the
> > > > > wrong type.
> > > > 
> > > > But changing layout and interacting correctly with the rest of the views
> is
> > > > exactly what VR eventually needs to do, right? Have you talked to the UI
> > > owners
> > > > about how VR will be integrated into existing layout, and about the
> problems
> > > > you've run into?
> > > > 
> > > > This hack has no basis in correctness, so probably has many issues as
> well.
> > It
> > > > just happens you didn't run into any of them. Eg the physical backing
size
> > > won't
> > > > match viewport size anymore and we already know that can lead to weird
> > > rendering
> > > > results: crbug.com/671967
> > > 
> > > No, the physical backing size does match the viewport, because we control
> the
> > > compositor, the viewport size, and the physical backing size. Literally
the
> > only
> > > relevant thing we're getting from the CVC is setting the css page size.
> > 
> > But that's not expressed in this CL. Someone else comes along and wants to
use
> > this doesn't know that they will also need to do A and B and C and D to make
> > things work.
> > 
> > > 
> > > As soon as we have a way to control the css size without going through the
> CVC
> > > we'll do so.
> > > 
> > > As for our place in the existing layout, we would like to live effectively
> at
> > > the root, as we're always fullscreen and draw over everything - we ideally
> > don't
> > > care at all about anything else in the view hierarchy and leave it
> unchanged.
> > 
> > Have you looked into fullscreen video and what happens in that case? Because
> the
> > web contents also needs to be resized to fullscreen. Can that path be
reused?
> 
> Does this work for media going full screen in vr shell?  (I'll try to patch it
> and verify).
> 
> There still needs to be a differentiation between web contents in vr at
default
> size and webcontents in vr at full screen.
> 
> For instance a user browsing youtube in vr (not using the native app) should
be
> able to navigate the site and then click on the full screen button of a video
> (which has a special treatment in vr shell).
> 
> If we ignore size change messages I think we might lose that.

I verified full screen transitions in vr work even with this patch.  The native
messages are apparently unaffected by this change (which makes sense as it only
touches the java side).

[boliu, 2017-01-17 19:06:23]

On 2017/01/17 18:44:54, mthiesse wrote:
> > But that's not expressed in this CL. Someone else comes along and wants to
use
> > this doesn't know that they will also need to do A and B and C and D to make
> > things work.
> 
> If this is the concern, we can make this function as scary as need be, so
nobody
> else uses it.

No, the concern is this is a hack. That's just part of the problem of being a
hack.

The fullscreen point was that it also involved putting the ContentView front and
center. I thought you could look at the layout changes involved as an example.
But if it's not the same thing, then oh well.

[mthiesse, 2017-01-17 19:15:34]

On 2017/01/17 19:06:23, boliu wrote:
> On 2017/01/17 18:44:54, mthiesse wrote:
> > > But that's not expressed in this CL. Someone else comes along and wants to
> use
> > > this doesn't know that they will also need to do A and B and C and D to
make
> > > things work.
> > 
> > If this is the concern, we can make this function as scary as need be, so
> nobody
> > else uses it.
> 
> No, the concern is this is a hack. That's just part of the problem of being a
> hack.
> 
> The fullscreen point was that it also involved putting the ContentView front
and
> center. I thought you could look at the layout changes involved as an example.
> But if it's not the same thing, then oh well.

Changing the layoutparams of the ContentView is also a hack, is it not? Until
crbug.com/622847 is resolved and css sizing information doesn't come from the
CVC, I'm not sure what else we can do.

[mthiesse, 2017-01-17 21:21:16]

Description was changed from

==========
Fix VrShell resizing incorrectly by adding a toggle for onSizeChanged in
ContentViewCore.

Adds a toggle to CVC that tells it to ignore any incoming sizeChanged events.
When in VR we don't care about the CVCs size in the view hierarchy.

BUG=680240
==========

to

==========
Fix VrShell resizing incorrectly by setting the CompositorViewHolder to a fixed
size when in VR.

BUG=680240
==========

[mthiesse, 2017-01-17 21:23:03]

PTAL, this is more analogous to your original suggestion of changing the
ContentView LayoutParams, but works at a higher level so we don't have to worry
about ContentViews changing, size, reparenting, swapping out, etc.

[boliu, 2017-01-18 00:50:36]

On 2017/01/17 21:23:03, mthiesse wrote:
> PTAL, this is more analogous to your original suggestion of changing the
> ContentView LayoutParams, but works at a higher level so we don't have to
worry
> about ContentViews changing, size, reparenting, swapping out, etc.

I don't own chrome code.

Chrome code currently doesn't have the flexibility you want, and behind its back
like this is still very brittle. A chrome developer can still inadvertently
stomp your layout.

At this point, this needs to modify chrome code a bit to add exactly what VR
needs. I imagine it's not going to be too different from
mOverlayContentWidth/HeightMeasureSpec? Maybe can refactor that to be generic?
And then add a test.

[boliu, 2017-01-18 01:22:35]

On 2017/01/18 00:50:36, boliu wrote:
> At this point, this needs to modify chrome code a bit to add exactly what VR
> needs. I imagine it's not going to be too different from
> mOverlayContentWidth/HeightMeasureSpec? Maybe can refactor that to be generic?
> And then add a test.

Or yeah, do what ted said on the bug. I don't know chrome code all that well, so
shouldn't take my advice on it.