Implement detaching the TabModelSelector from the CompositorViewHolder

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}
Committed: https://chromium.googlesource.com/chromium/src/+/19d9d40d78985410d311f7937186943928580fb1

[mthiesse, 2017-01-18 22:43:16]

Patchset #1 (id:1) has been deleted

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

https://codereview.chromium.org/2641043002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerHost.java
(right):

https://codereview.chromium.org/2641043002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerHost.java:97:
void onOverlayPanelContentViewCoreAdded(ContentViewCore content);
Note that this rename isn't necessary, I just found it overly broad and
confusing as it's only used for OverlayPanels and not any other
ContentViewCores. Happy to revert or rename differently...

[mthiesse, 2017-01-18 22:45:11]

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

[mthiesse, 2017-01-18 22:45:11]

PTAL

[mthiesse, 2017-01-18 22:45:55]

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

[commit-bot: I haz the power, 2017-01-18 22:46:34]

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-01-18 22:48:46]

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

[commit-bot: I haz the power, 2017-01-18 22:48:47]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mthiesse, 2017-01-18 23:15:11]

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

[commit-bot: I haz the power, 2017-01-18 23:15:45]

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-01-18 23:55:52]

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

[commit-bot: I haz the power, 2017-01-18 23:55:53]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-01-19 01:02:01]

tedchoc@chromium.org changed reviewers:
+ mdjones@chromium.org

[Ted C, 2017-01-19 01:02:03]

Overall this looks way better to me!

+mdjones to confirm that this isn't too crazy but I think this makes a clear
case for VR when CompositorViewHolder is essentially inactive.

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

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:978:
public TabModelSelector detachFromTabModelSelector() {
javadoc

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:988:
* @param tabModelSelector
needs to be flushed out here too.

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

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:87:
TabModelSelector mTabModelSelector;
private?

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java
(right):

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1767:
final ReturnObject object = new ReturnObject();
AtomicReference<TabModelSelector> would be what you'd want to use.

Otherwise, you can use Callable<TabModelSelector> for ThreadUtils and return it
from one call, marking it final and then that would allow you to use it below.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1773:
UiUtils.settleDownUI(getInstrumentation());
why do you need this settleDownUI?  In general, it is a bad indication for a
test (it is essentially a sleep).

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1783:
compositorViewHolder.setLayoutParams(
Instead of creating a new layout params, just reuse the existing ones (then
you're not messing with margins or if the parent view type changes then this
won't break).

ViewGroup.LayoutParams layoutParams = compositorViewHolder.getLayoutParams();
layoutParams.width = 123;
layoutParams.height = 456;
compositorViewHolder.requestLayout();

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1787:
UiUtils.settleDownUI(getInstrumentation());
Instead of doing this, I would do something like:

CriteriaHelper.pollUiThread(Criteria.equals(123, new Callable<Integer>() {
    @Override
    public Integer call() {
        return
getActivity().findViewById(R.id.compositor_view_holder).getMeasuredWidth();
    }
}));

At that point, you are verifying that the view was resized by android and the
underlying CVC should have received an OnResize if applicable.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1804:
UiUtils.settleDownUI(getInstrumentation());
Hmm...this one is harder as you want to verify that the renderer received the
value and act only after that so some delay is needed.

I wonder if instead of resizeHappened here, we could instead look at
ContentViewCore#getRenderCoordinates()#getContentWidthPix() matches the above
value.  If that is the case, then you know you got a renderer frame back with a
newly updated value.

[mdjones, 2017-01-19 02:38:52]

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

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:978:
public TabModelSelector detachFromTabModelSelector() {
The TabMoselSelector is never used outside of this class other than for giving
it back to this class. Will there be a future need to replace the selector with
something other than the original? If not, I think a concept like
enter/exitVRMode() would make it more clear what this is supposed to be for and
would prevent exposure of the TabModelSelector. What do you think?

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:983:
setTab(null);
Does tab need to be reset in attachToTabModelSelector or does that happen as a
result of some other function call?

[Ted C, 2017-01-19 21:39:04]

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

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:978:
public TabModelSelector detachFromTabModelSelector() {
On 2017/01/19 02:38:52, mdjones wrote:
> The TabMoselSelector is never used outside of this class other than for giving
> it back to this class. Will there be a future need to replace the selector
with
> something other than the original? If not, I think a concept like
> enter/exitVRMode() would make it more clear what this is supposed to be for
and
> would prevent exposure of the TabModelSelector. What do you think?

I'm the one that suggested detachFromTabModel, but I guess being very
explicit about VR makes it even clearer.

If we ever need to support multiple cases that do similar logic, then
we can make this more generic.  I guess no reason to make it generic
for a problem that may never arise.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:983:
setTab(null);
On 2017/01/19 02:38:52, mdjones wrote:
> Does tab need to be reset in attachToTabModelSelector or does that happen as a
> result of some other function call?

onContentChanged called below will trigger the tab to be reattached if I recall

[mdjones, 2017-01-19 22:06:05]

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

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:978:
public TabModelSelector detachFromTabModelSelector() {
On 2017/01/19 21:39:03, Ted C wrote:
> On 2017/01/19 02:38:52, mdjones wrote:
> > The TabMoselSelector is never used outside of this class other than for
giving
> > it back to this class. Will there be a future need to replace the selector
> with
> > something other than the original? If not, I think a concept like
> > enter/exitVRMode() would make it more clear what this is supposed to be for
> and
> > would prevent exposure of the TabModelSelector. What do you think?
> 
> I'm the one that suggested detachFromTabModel, but I guess being very
> explicit about VR makes it even clearer.
> 
> If we ever need to support multiple cases that do similar logic, then
> we can make this more generic.  I guess no reason to make it generic
> for a problem that may never arise.

I think that makes sense as long as there are multiple TabModelSelectors. My
understanding was that TabModelSelector was 1:1 with the activity and wouldn't
change. I would expect there to be a different TabModel for VR rather than a
whole different selector (but I'm not really clear on how VR is built into
chrome).

My concern was primarily with the unnecessary exposure of resources.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:983:
setTab(null);
On 2017/01/19 21:39:03, Ted C wrote:
> On 2017/01/19 02:38:52, mdjones wrote:
> > Does tab need to be reset in attachToTabModelSelector or does that happen as
a
> > result of some other function call?
> 
> onContentChanged called below will trigger the tab to be reattached if I
recall

Acknowledged.

[mthiesse, 2017-01-20 01:49:53]

Not sure the test should live in TabsTest (pretty sure it shouldn't?) but I'm
not sure where the test should live... please advise.

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

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:978:
public TabModelSelector detachFromTabModelSelector() {
On 2017/01/19 22:06:05, mdjones wrote:
> On 2017/01/19 21:39:03, Ted C wrote:
> > On 2017/01/19 02:38:52, mdjones wrote:
> > > The TabMoselSelector is never used outside of this class other than for
> giving
> > > it back to this class. Will there be a future need to replace the selector
> > with
> > > something other than the original? If not, I think a concept like
> > > enter/exitVRMode() would make it more clear what this is supposed to be
for
> > and
> > > would prevent exposure of the TabModelSelector. What do you think?
> > 
> > I'm the one that suggested detachFromTabModel, but I guess being very
> > explicit about VR makes it even clearer.
> > 
> > If we ever need to support multiple cases that do similar logic, then
> > we can make this more generic.  I guess no reason to make it generic
> > for a problem that may never arise.
> 
> I think that makes sense as long as there are multiple TabModelSelectors. My
> understanding was that TabModelSelector was 1:1 with the activity and wouldn't
> change. I would expect there to be a different TabModel for VR rather than a
> whole different selector (but I'm not really clear on how VR is built into
> chrome).
> 
> My concern was primarily with the unnecessary exposure of resources.

Not to get too far out of the scope of this CL, but for now VR shares the
regular and incognito TabModels with CTA. This is intentional, because VR is
aiming to be just a different way to view the content you already have open (and
any changes to tab state you make while in VR are reflected when you leave), not
a new context that only exists when in VR.

Anyways, feel free to bikeshed on the names some more, I chose names that made
sense to me.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:983:
setTab(null);
On 2017/01/19 22:06:05, mdjones wrote:
> On 2017/01/19 21:39:03, Ted C wrote:
> > On 2017/01/19 02:38:52, mdjones wrote:
> > > Does tab need to be reset in attachToTabModelSelector or does that happen
as
> a
> > > result of some other function call?
> > 
> > onContentChanged called below will trigger the tab to be reattached if I
> recall
> 
> Acknowledged.

Yep, as Ted suggests, onContentChanged() takes care of setting the tab, and
everything that goes with that, like restoring the PhysicalBackingSize after VR
changes it.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:988:
* @param tabModelSelector
On 2017/01/19 01:02:02, Ted C wrote:
> needs to be flushed out here too.

Done.

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

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:87:
TabModelSelector mTabModelSelector;
On 2017/01/19 01:02:02, Ted C wrote:
> private?

Done.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java
(right):

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1767:
final ReturnObject object = new ReturnObject();
On 2017/01/19 01:02:02, Ted C wrote:
> AtomicReference<TabModelSelector> would be what you'd want to use.
> 
> Otherwise, you can use Callable<TabModelSelector> for ThreadUtils and return
it
> from one call, marking it final and then that would allow you to use it below.

Done.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1773:
UiUtils.settleDownUI(getInstrumentation());
On 2017/01/19 01:02:02, Ted C wrote:
> why do you need this settleDownUI?  In general, it is a bad indication for a
> test (it is essentially a sleep).

Removed.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1783:
compositorViewHolder.setLayoutParams(
On 2017/01/19 01:02:02, Ted C wrote:
> Instead of creating a new layout params, just reuse the existing ones (then
> you're not messing with margins or if the parent view type changes then this
> won't break).
> 
> ViewGroup.LayoutParams layoutParams = compositorViewHolder.getLayoutParams();
> layoutParams.width = 123;
> layoutParams.height = 456;
> compositorViewHolder.requestLayout();

Done.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1787:
UiUtils.settleDownUI(getInstrumentation());
On 2017/01/19 01:02:02, Ted C wrote:
> Instead of doing this, I would do something like:
> 
> CriteriaHelper.pollUiThread(Criteria.equals(123, new Callable<Integer>() {
>     @Override
>     public Integer call() {
>         return
> getActivity().findViewById(R.id.compositor_view_holder).getMeasuredWidth();
>     }
> }));
> 
> At that point, you are verifying that the view was resized by android and the
> underlying CVC should have received an OnResize if applicable.

Done.

https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1804:
UiUtils.settleDownUI(getInstrumentation());
On 2017/01/19 01:02:02, Ted C wrote:
> Hmm...this one is harder as you want to verify that the renderer received the
> value and act only after that so some delay is needed.
> 
> I wonder if instead of resizeHappened here, we could instead look at
> ContentViewCore#getRenderCoordinates()#getContentWidthPix() matches the above
> value.  If that is the case, then you know you got a renderer frame back with
a
> newly updated value.

Found a good alternative.

[Ted C, 2017-01-20 23:54:26]

Overall, this lgtm.  I'll defer to Matt about the apis in CompositorViewHolder
but on his approval this is good.

On 2017/01/20 01:49:53, mthiesse wrote:
> Not sure the test should live in TabsTest (pretty sure it shouldn't?) but I'm
> not sure where the test should live... please advise.

If there isn't an existing VR test class that makes sense.  I would just
add something like VrIntegrationTest.java and put it there.

> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
> (right):
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:978:
> public TabModelSelector detachFromTabModelSelector() {
> On 2017/01/19 22:06:05, mdjones wrote:
> > On 2017/01/19 21:39:03, Ted C wrote:
> > > On 2017/01/19 02:38:52, mdjones wrote:
> > > > The TabMoselSelector is never used outside of this class other than for
> > giving
> > > > it back to this class. Will there be a future need to replace the
selector
> > > with
> > > > something other than the original? If not, I think a concept like
> > > > enter/exitVRMode() would make it more clear what this is supposed to be
> for
> > > and
> > > > would prevent exposure of the TabModelSelector. What do you think?
> > > 
> > > I'm the one that suggested detachFromTabModel, but I guess being very
> > > explicit about VR makes it even clearer.
> > > 
> > > If we ever need to support multiple cases that do similar logic, then
> > > we can make this more generic.  I guess no reason to make it generic
> > > for a problem that may never arise.
> > 
> > I think that makes sense as long as there are multiple TabModelSelectors. My
> > understanding was that TabModelSelector was 1:1 with the activity and
wouldn't
> > change. I would expect there to be a different TabModel for VR rather than a
> > whole different selector (but I'm not really clear on how VR is built into
> > chrome).
> > 
> > My concern was primarily with the unnecessary exposure of resources.
> 
> Not to get too far out of the scope of this CL, but for now VR shares the
> regular and incognito TabModels with CTA. This is intentional, because VR is
> aiming to be just a different way to view the content you already have open
(and
> any changes to tab state you make while in VR are reflected when you leave),
not
> a new context that only exists when in VR.
> 
> Anyways, feel free to bikeshed on the names some more, I chose names that made
> sense to me.
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:983:
> setTab(null);
> On 2017/01/19 22:06:05, mdjones wrote:
> > On 2017/01/19 21:39:03, Ted C wrote:
> > > On 2017/01/19 02:38:52, mdjones wrote:
> > > > Does tab need to be reset in attachToTabModelSelector or does that
happen
> as
> > a
> > > > result of some other function call?
> > > 
> > > onContentChanged called below will trigger the tab to be reattached if I
> > recall
> > 
> > Acknowledged.
> 
> Yep, as Ted suggests, onContentChanged() takes care of setting the tab, and
> everything that goes with that, like restoring the PhysicalBackingSize after
VR
> changes it.
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:988:
> * @param tabModelSelector
> On 2017/01/19 01:02:02, Ted C wrote:
> > needs to be flushed out here too.
> 
> Done.
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
> File
> chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
> (right):
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:87:
> TabModelSelector mTabModelSelector;
> On 2017/01/19 01:02:02, Ted C wrote:
> > private?
> 
> Done.
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
> File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java
> (right):
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
> chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1767:
> final ReturnObject object = new ReturnObject();
> On 2017/01/19 01:02:02, Ted C wrote:
> > AtomicReference<TabModelSelector> would be what you'd want to use.
> > 
> > Otherwise, you can use Callable<TabModelSelector> for ThreadUtils and return
> it
> > from one call, marking it final and then that would allow you to use it
below.
> 
> Done.
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
> chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1773:
> UiUtils.settleDownUI(getInstrumentation());
> On 2017/01/19 01:02:02, Ted C wrote:
> > why do you need this settleDownUI?  In general, it is a bad indication for a
> > test (it is essentially a sleep).
> 
> Removed.
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
> chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1783:
> compositorViewHolder.setLayoutParams(
> On 2017/01/19 01:02:02, Ted C wrote:
> > Instead of creating a new layout params, just reuse the existing ones (then
> > you're not messing with margins or if the parent view type changes then this
> > won't break).
> > 
> > ViewGroup.LayoutParams layoutParams =
compositorViewHolder.getLayoutParams();
> > layoutParams.width = 123;
> > layoutParams.height = 456;
> > compositorViewHolder.requestLayout();
> 
> Done.
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
> chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1787:
> UiUtils.settleDownUI(getInstrumentation());
> On 2017/01/19 01:02:02, Ted C wrote:
> > Instead of doing this, I would do something like:
> > 
> > CriteriaHelper.pollUiThread(Criteria.equals(123, new Callable<Integer>() {
> >     @Override
> >     public Integer call() {
> >         return
> > getActivity().findViewById(R.id.compositor_view_holder).getMeasuredWidth();
> >     }
> > }));
> > 
> > At that point, you are verifying that the view was resized by android and
the
> > underlying CVC should have received an OnResize if applicable.
> 
> Done.
> 
>
https://codereview.chromium.org/2641043002/diff/40001/chrome/android/javatest...
> chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1804:
> UiUtils.settleDownUI(getInstrumentation());
> On 2017/01/19 01:02:02, Ted C wrote:
> > Hmm...this one is harder as you want to verify that the renderer received
the
> > value and act only after that so some delay is needed.
> > 
> > I wonder if instead of resizeHappened here, we could instead look at
> > ContentViewCore#getRenderCoordinates()#getContentWidthPix() matches the
above
> > value.  If that is the case, then you know you got a renderer frame back
with
> a
> > newly updated value.
> 
> Found a good alternative.

[mthiesse, 2017-01-23 16:02:28]

On 2017/01/20 23:54:26, Ted C wrote:
> Overall, this lgtm.  I'll defer to Matt about the apis in CompositorViewHolder
> but on his approval this is good.
> 
> On 2017/01/20 01:49:53, mthiesse wrote:
> > Not sure the test should live in TabsTest (pretty sure it shouldn't?) but
I'm
> > not sure where the test should live... please advise.
> 
> If there isn't an existing VR test class that makes sense.  I would just
> add something like VrIntegrationTest.java and put it there.

Moved to vr_shell/VrShellTest.java

[mdjones, 2017-01-23 17:04:57]

The apis look better, thanks! lgtm

[mthiesse, 2017-01-23 17:17:15]

The CQ bit was checked by mthiesse@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2641043002/#ps80001
(title: "Move test to VrShellTest.java")

[commit-bot: I haz the power, 2017-01-23 17:17:45]

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-01-23 17:20:09]

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

[commit-bot: I haz the power, 2017-01-23 17:20:10]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mthiesse, 2017-01-23 17:27:33]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-23 17:27:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2641043002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-23 17:27:51]

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-01-23 17:54:21]

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

[commit-bot: I haz the power, 2017-01-23 17:54:22]

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

[mthiesse, 2017-01-23 18:05:37]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-23 18:05:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2641043002/#ps120001
(title: "Fix compile")

[commit-bot: I haz the power, 2017-01-23 18:05:59]

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-01-23 18:44:34]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485194737054040,
"parent_rev": "2f1771ac371b74ddfff193a764dfea08b4a86314", "commit_rev":
"19d9d40d78985410d311f7937186943928580fb1"}

[commit-bot: I haz the power, 2017-01-23 18:45:08]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/19d9d40d78985410d311f7937186...
==========

[commit-bot: I haz the power, 2017-01-23 18:45:10]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/19d9d40d78985410d311f7937186...