+ianwen
After bisecting the change that causes this is:
7a609035f <ianwen@chromium.org> - Refactor the view hierarchy of snackbars and infobars https://codereview.chromium.org/2623493003

+mthiesse
It looks like changing the CompositorViewHolder hierarchy impacts the vr shell dimensions, but it's not clear why/how.

The plan for such refactoring was stated in this doc. https://docs.google.com/a/google.com/document/d/1aWv_LHmrn2undbuwhQ_FSG2iVK0ib1TDHU0744VEHDU/edit?usp=sharing

Thanks for the docs Ian.  I'll try to look through them and see how that might have impacted vr shell.

Not as important for M57 as WebVR issues.

This actually break Cardboard rendering on TOT. The incorrect size results wrong stereo rendering. I locally verified that it is due to the same CL.

So change it back P1.

I've been trying to figure out how the changes to the CompositorViewHolder and Tab could possible affect VrShell, but nothing is jumping out.

The correct size is getting passed into VrShellImpl setContentCssSize, but somewhere during layout it is getting converted into 1600x900 (very suspicious that the ratio is just 16:9 times 100) when a resize comes through to third_party/WebKit/Source/web/WebViewImpl.cpp WebViewImpl::performResize

It also later comes through as 878x494 later on, but the javascript/html visible dimensions come back as:
 Screen {availWidth: 1025, availHeight: 577, width: 1025, height: 577, colorDepth: 0…}
 Window (visual viewport?) {innerWidth: 1600, innerHeight: 900}
 Window {outerWidth: 1600, outerHeight: 900}
 Document element (html tag, should be layout viewport) {clientWidth: 1600, clientHeight: 900}
 Html tag size {offsetWidth: 1600, offsetHeight: 1499}
 Document body {clientWidth: 1600, clientHeight: 900}

None of this should be impacted by changing the Java side views as VrShell creates it's own Display and VrWindow.  So I am still very confused as to why the change in comment #1 broke anything.

The issue is our hack stopped working with the view hierarchy changes. Until  issue 622847  is fixed, we'll keep having to resort to fragile hacks to set the css size.


We call mContentCVC#onSizeChanged to set the css size, because the CVC size propagates all the way through to blink. We used to have hacks to reparent the CVC to a parent that was sized the way we wanted it to be, but that was causing crashes and other weird things to happen outside of VR Shell, so for now we just directly call onSizeChanged.

However, with the view hierarchy changes, some asynchronous resize event is updating the CVC size again after we set it.

We have a few options if we want to fix this in the short term (that will be removed once  issue 622847  is fixed):
1. We could add a new ignoreSizeChanges() function to CVC so that we can tell it to ignore size changes that aren't ours while in VR Shell.

2. We could poll CVC's size to make sure it stays what we want it to be, and call onSizeChanged again when it changes (this does actually work, there isn't a cycle here where us calling sizeChanged causes sizeChanged to be called).

+cc tedchoc
Ted, Ian, do either of these short term fixes sound reasonable? Any other suggestions?

This issue may also be responsible for trashing the HTML UI.  When the UI starts, we pull HTML DOM element coordinates out of the rendered page, and use them to render parts of the UI natively.

Now, about 50% of the time, the native rendering portion gets incorrect coordinates, and elements are drawn incorrectly.  If page layout changed, that would do it.

We could listen for relayout, and re-query element coordinates, as a workaround.

Chris, try patching https://codereview.chromium.org/2635123002/ and see if it fixes the UI issues for you.

+boliu

Per #8, I still don't know what the problem is you are trying to address.

CVC#onSizeChanged, calls WasResized and sends an IPC to the renderer with the new size.  CVC#onPhysicalBackingSizeChanged does the same.  Setting the top/bottom controls height will do the same.

In your case, what is going wrong?  Is the size reported from CVC#onSizedChanged not the one you want?  If not, why?  What parent are you attached to?

---- Following is assumptions based on possible answers to the above ----

I don't know what the state of VR is, but I'm going to assume it is attached to CompositorViewHolder and that is giving you grief.

If that is the case, then you need to fix that.  Waiting for 622847 to be fixed likely won't be your silver bullet either as CompositorViewHolder will still assume ownership.

Adding things to the content layer to work around this is definitely not the right thing to do.  My continual ask is that anything VR needs to be as isolated to VR as possible (i.e. no changes to content)...sometimes Chrome will need to know, but again as minimal as possible.

Here, I'd recommend adding new methods to CompositorViewHolder.  Something like, detachFromTabModelSelector and attachToTabModelSelector.  In detach, it removes the view and stops listening for changes in setTab (essentially it would call setTab with null and no longer listen for calls to onContentChanged).  Attach would call onContentChanged() and initialize everything as needed.

At this point, you could add (or not) the ContentView associated with the tab in a container that you can force a particular size to (i.e. overwrite onMeasure to ignore anything or set explicit height and width layout params).

But again, I would like to know why the sizes you are seeing are incorrect and why.

Alright, thanks Ted, I'll try that and see if that works for us.

Ted/Bo, would there be opposition to changing the LayoutParams of the CompositorViewHolder itself? We would update them to fixed width/height, and restore them when exiting VR. Seems to work, and we don't have to worry about things like views expecting to have certain parents/children not having them, or something else overwriting our size changes.

Even with that approach, you have no guarantee that someone else won't come in and try to set the layout params of the compositor view holder out from underneath you.

I think something like what I suggest in #12 is going to be the best option.  It is an explicit API that makes it easy to reason about and you know the implications.  Also, I thought VR didn't want anything to do with CompositorViewHolder/CompositorView so this seems like another win as well.

Did you attempt that?  Did it not work?

I haven't finished implementing what you suggest in #12 yet - I actually don't think it's easy to reason about the implications of that, like what happens when I click a link and content opens in a new tab? Right now we at least get notified because our current tab gets hidden, but if we've detached our tab and from the CompositorViewHolder, we'll need to find some other way of detecting a tab switch, presumably. 

How do I know something else isn't going to reparent or move the current tab around? You're not suggesting removing the tab from the tabModelSelector are you? If not, the Tab and ContentView are still reachable from a ton of places that could do anything with them...

I mean, maybe these questions have obvious answers, and I just need a better understanding of this part of the codebase.

To go back a bit and address your question in #12 about what is going wrong - the current code doesn't care what container the CVC is in; we manually call CVC#onSizeChanged and CVC#onPhysicalBackingSizeChanged with the size we want it to be after the view hierarchy has been set for VR and hope nothing comes by and changes it, which we knew was a hack, and we were waiting for  issue 622847  so we could properly fix it. After ian's change, something is calling onSizeChanged after we do.

When you say "Waiting for 622847 to be fixed likely won't be your silver bullet either as CompositorViewHolder will still assume ownership.", what do you mean by CompositorViewHolder will still assume ownership?

Right now the VR intrudes into the rest of clank in 3 ways: 
1. It hides the omnibox and CompositorViewHolder.
2. It swaps out the WindowAndroid for the current tab.
3. It tricks CVC into thinking it's a different size to set the css size and physical backing size.

I don't see that first and second way going away until we eventually uncouple from CTA. The third way disappears with 622847 doesn't it? We just swap out the ViewRoot for a VrViewRoot that ignores view sizes in addition to what we currently do by swapping out the WindowAndroid. I'm not sure how CompositorViewHolder assuming ownership matters here; it can own the tab and do what it wants to it as long as VR controls still owns compositing whatever the current tab is (as is currently the case).


I don't think I understand what the appeal to the solution in #12 is, as to me it looks like it introduces a lot of complexity that only VR cares about, but still doesn't really guarantee that something else isn't going to to come by a reparent tabs into containers we don't control, does it?

Fixing the size of the CompositorViewHolder, while pretty hacky, is definitely really easy to reason about and unintrusive. If something else comes by in the future that also wants to set the CompositorViewHolder size, it seems like coordinating with whatever that future thing might be would be simpler than detaching from the tabModelSelector. But this change will probably be gone before that happens anyways, as the relevant parts of 622847 are almost done.

I don't think either of these solutions help for the eventual goal of moving VR into its own activity, so I'm not sure if in the long run it matters how we solve this problem now, other than the complexity it currently adds to clank.

> When you say "Waiting for 622847 to be fixed likely won't be your silver bullet either as CompositorViewHolder will still assume ownership.", what do you mean by CompositorViewHolder will still assume ownership?

CompositorViewHolder will still be the source of size (and probably touch) events.

You are assuming ViewRoot of a WebContents can be swapped, which does seem like the best solution here if that refactor were all done. From what I can see, ability to swap ViewRoot should remain. But that refactor is still going to take awhile, and who knows how ViewRoot will be integrated into everything by then. You are welcome to help with that effort.


In the current world, the correct change is what ted said in #12. It is complex because chrome views layout is complex, and you are trying to carve out conditions specifically for VR. To avoid chrome breaking VR in the future, I'd recommend documenting VR's expectations, add an automated, and be ready to answer when other developers inevitably break that test again.

I strongly prefer the approach I put forth in #12.  Before anything else is landed, I want to see why that doesn't work.  It is the cleanest, clearest, and most maintainable solution.

As long as CompositorViewHolder reacts to changes in the TabModel, it will attempt to take action on them.

CompositorViewHolder and the VR equivalent are driving the rendering and input of Chrome.  We should make explicit APIs to signal when one should be operating and one should not.

Okay, fair points. I'm sure that solution will work, I just hoped to find a less complicated solution. I'll close that other CL and get started on #12's solution.

The main thing I'm asking for is a feasibility investigation.  I "hope" the code won't be much more than what I suggested in the comment above.  If you find it spiraling out of control, then let's revisit this discussion (if you find that it is trying to enter the tab switcher or other crazy layout operations...it would be good to suppress those as well and they might be minimal code changes, but I don't want to derail the VR work for 57 either).

Michael, can I suggest you call a meeting with yourself, Adam, David and the other folks on this thread to resolve?  From what I can tell there is lots of discussion here but no definite resolution on how to proceed here.  And since this is one (of several) things blocking WebVR support on Cardboard, we need to converge on a solution ASAP.

We've settled on a solution, and it's out for review here: https://codereview.chromium.org/2641043002/

Note that I after trying ToT today it didn't exhibit the layout issues.  Has anything else changed recently that would resolve this in a different way than has been discussed?

I can try to bisect again to see when it resolved itself if that would be useful...

I verified that https://codereview.chromium.org/2641813003/ fixes this issue.  This fits into the theory that something else was sending a resize after transitioning to vr and makes it clear that the orientation updates are what was the cause (it's still not clear to me why the original breaking change in comment #1 caused those resizes to start coming through though).

I'm not sure if the work in mthiesse@ change in comment #25 is still needed or not (it looks generally useful, but there isn't an immediate issue that needs resolving anymore).

The work in #25 is still needed to ensure this doesn't happen again by solving the underlying problem rather than fixing the symptoms :P

Bugdroid appears to be acting up, https://codereview.chromium.org/2641043002/ landed, and fixes this issue.

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9341b1c39c879f65c46421c7876c310d590a8de0

commit 9341b1c39c879f65c46421c7876c310d590a8de0
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Thu Jan 26 20:56:00 2017

Implement detaching the TabModelSelector from the CompositorViewHolder

Essentially returns the CompositorViewHolder to a state where the TabModelSelector hasn't been set, and allows the TabModelSelector to be re-attached.
This allows VR Shell to control the size of CVCs, as the CompositorViewHolder no longer owns the CVC while in VR.

BUG= 680240 

Review-Url: https://codereview.chromium.org/2641043002
Cr-Commit-Position: refs/heads/master@{#445419}
(cherry picked from commit 19d9d40d78985410d311f7937186943928580fb1)

Review-Url: https://codereview.chromium.org/2658903002 .
Cr-Commit-Position: refs/branch-heads/2987@{#118}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/9341b1c39c879f65c46421c7876c310d590a8de0/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/9341b1c39c879f65c46421c7876c310d590a8de0/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
[modify] https://crrev.com/9341b1c39c879f65c46421c7876c310d590a8de0/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerHost.java
[modify] https://crrev.com/9341b1c39c879f65c46421c7876c310d590a8de0/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrClassesWrapper.java
[modify] https://crrev.com/9341b1c39c879f65c46421c7876c310d590a8de0/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrClassesWrapperImpl.java
[modify] https://crrev.com/9341b1c39c879f65c46421c7876c310d590a8de0/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
[modify] https://crrev.com/9341b1c39c879f65c46421c7876c310d590a8de0/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/9341b1c39c879f65c46421c7876c310d590a8de0/chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java
[modify] https://crrev.com/9341b1c39c879f65c46421c7876c310d590a8de0/chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/MockLayoutHost.java
[modify] https://crrev.com/9341b1c39c879f65c46421c7876c310d590a8de0/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellTest.java