Improve transition between opaque and translucent compositor views.

chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java

Index: chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java
diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java
new file mode 100644
index 0000000000000000000000000000000000000000..705841603ddf004387c497e84bd8a5e5cbf344fc
--- /dev/null
+++ b/chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java
@@ -0,0 +1,436 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.compositor;
+
+import android.app.Activity;
+import android.graphics.PixelFormat;
+import android.view.Surface;
+import android.view.SurfaceHolder;
+import android.view.SurfaceView;
+import android.widget.FrameLayout;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.robolectric.Robolectric;
+import org.robolectric.Shadows;
+import org.robolectric.annotation.Config;
+import org.robolectric.annotation.Implementation;
+import org.robolectric.annotation.Implements;
+import org.robolectric.annotation.RealObject;
+import org.robolectric.shadows.ShadowLooper;
+import org.robolectric.shadows.ShadowSurfaceView;
+
+import org.chromium.base.test.util.Feature;
+import org.chromium.testing.local.LocalRobolectricTestRunner;
+
+import java.util.Set;
+
+/**
+ * Unit tests for the CompositorSurfaceManager.
+ */
+@RunWith(LocalRobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class CompositorSurfaceManagerTest {
+    @Mock
+    private SurfaceHolder.Callback mCallback;
+
+    private CompositorSurfaceManager mManager;
+
+    private FrameLayout mLayout;
+
+    /**
+     * Implementation of a SurfaceView shadow that provides additional functionality for controlling
+     * the state of the underlying (fake) Surface.
+     */
+    @Implements(SurfaceView.class)
+    public static class MyShadowSurfaceView extends ShadowSurfaceView {
+        @RealObject
+        private SurfaceView mRealSurfaceView;

[Ted C, 2017/01/31 23:17:51]

Not familiar with RealObject, but this doesn't seem to be used anywhere

[liberato (no reviews please), 2017/02/01 19:19:48]

docs made me think that it was needed, but seems not.  Done.

+
+        private final MyFakeSurfaceHolder mHolder = new MyFakeSurfaceHolder();
+
+        /**
+         * Robolectric's FakeSurfaceHolder doesn't keep track of the format, etc.
+         */
+        public static class MyFakeSurfaceHolder extends ShadowSurfaceView.FakeSurfaceHolder {
+            /**
+             * Fake surface that lets us control whether it's valid or not.
+             */
+            public static class MyFakeSurface extends Surface {
+                public boolean valid = false;
+
+                @Override
+                public boolean isValid() {
+                    return valid;
+                }
+            }
+
+            private int mFormat = PixelFormat.UNKNOWN;
+            private final MyFakeSurface mSurface = new MyFakeSurface();
+
+            @Implementation
+            public void setFormat(int format) {
+                mFormat = format;
+            }
+
+            public int getFormat() {
+                return mFormat;
+            }
+
+            // Return a surface that we can control if it's valid or not.
+            @Override
+            public Surface getSurface() {
+                return getFakeSurface();
+            }
+
+            public MyFakeSurface getFakeSurface() {
+                return mSurface;
+            }
+        }
+
+        public MyShadowSurfaceView() {}
+
+        @Implementation
+        public SurfaceHolder getHolder() {
+            return getMyFakeSurfaceHolder();
+        }
+
+        @Override
+        public FakeSurfaceHolder getFakeSurfaceHolder() {
+            return getMyFakeSurfaceHolder();
+        }
+
+        public MyFakeSurfaceHolder getMyFakeSurfaceHolder() {
+            return mHolder;
+        }
+    }
+
+    @Before
+    public void beforeTest() {
+        MockitoAnnotations.initMocks(this);
+        Activity activity = Robolectric.buildActivity(Activity.class).setup().get();
+        mLayout = new FrameLayout(activity);
+        mManager = new CompositorSurfaceManager(mLayout, mCallback);
+    }
+
+    private void runDelayedTasks() {
+        ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
+    }
+
+    /**
+     * Return the callback for |view|, or null.  Will get mad if there's more than one.
+     */
+    private SurfaceHolder.Callback callbackFor(SurfaceView view) {
+        MyShadowSurfaceView viewShadow = (MyShadowSurfaceView) Shadows.shadowOf(view);
+        ShadowSurfaceView.FakeSurfaceHolder viewHolder = viewShadow.getFakeSurfaceHolder();
+        Set<SurfaceHolder.Callback> callbacks = viewHolder.getCallbacks();
+        // Zero or one is okay.
+        assertTrue(callbacks.size() < 2);

[Ted C, 2017/01/31 23:17:51]

I don't know if we have access to it, but it would be nice to use:
http://hamcrest.org/JavaHamcrest/javadoc/1.3/org/hamcrest/number/OrderingComp...

assertThat(callbacks.size(), lessThan(2));

[liberato (no reviews please), 2017/02/01 19:19:48]

Done.

+
+        if (callbacks.size() == 1) return callbacks.iterator().next();
+
+        return null;
+    }
+
+    private MyShadowSurfaceView.MyFakeSurfaceHolder fakeHolderFor(SurfaceView view) {
+        MyShadowSurfaceView viewShadow = (MyShadowSurfaceView) Shadows.shadowOf(view);
+        return viewShadow.getMyFakeSurfaceHolder();
+    }
+
+    private void setSurfaceValid(SurfaceView view, boolean valid) {
+        fakeHolderFor(view).getFakeSurface().valid = valid;
+    }
+
+    /**
+     * Find and return the SurfaceView with format |format|.
+     */
+    private SurfaceView findSurface(int format) {
+        final int childCount = mLayout.getChildCount();
+        for (int i = 0; i < childCount; i++) {
+            final SurfaceView child = (SurfaceView) mLayout.getChildAt(i);
+            if (fakeHolderFor(child).getFormat() == format) return child;
+        }
+
+        return null;
+    }
+
+    /**
+     * Request the pixel format |format|, and return the SurfaceView for it if it's attached.
+     */
+    private SurfaceView requestSurface(int format) {
+        mManager.requestSurface(format);
+        runDelayedTasks();
+
+        return findSurface(format);
+    }
+
+    /**
+     * Request format |format|, and send create / changed for it.
+     */
+    private SurfaceView requestAndGet(int format) {

[Ted C, 2017/01/31 23:17:51]

Hmm...it would be hard to understand the difference between requestSurface and
requestAndGet.

Maybe requestSurfaceAndNotifyCallback or "something"

[liberato (no reviews please), 2017/02/01 19:19:48]

i switched this to requestThenCreateSurface, and updated the comments for both
this and requestSurface to make the difference clear.

+        SurfaceView view = requestSurface(format);
+        setSurfaceValid(view, true);
+        callbackFor(view).surfaceCreated(view.getHolder());
+        final int actualFormat =
+                (format == PixelFormat.OPAQUE) ? PixelFormat.RGB_565 : PixelFormat.RGBA_8888;
+        final int width = 320;
+        final int height = 240;
+        callbackFor(view).surfaceChanged(view.getHolder(), actualFormat, width, height);
+
+        return view;
+    }
+
+    @Test
+    @Feature("Compositor")
+    @Config(shadows = {MyShadowSurfaceView.class})
+    public void testRequestOpaqueSurface() {
+        // Request a SurfaceView, and test in detail that it worked.
+        SurfaceView opaque = requestSurface(PixelFormat.OPAQUE);
+        verify(mCallback, times(0)).surfaceCreated(ArgumentMatchers.<SurfaceHolder>any());
+        verify(mCallback, times(0))
+                .surfaceChanged(
+                        ArgumentMatchers.<SurfaceHolder>any(), anyInt(), anyInt(), anyInt());
+        verify(mCallback, times(0)).surfaceDestroyed(ArgumentMatchers.<SurfaceHolder>any());
+
+        // Check that there's an opaque SurfaceView .
+        assertEquals(1, mLayout.getChildCount());
+        assertTrue(fakeHolderFor(opaque).getFormat() == PixelFormat.OPAQUE);
+
+        // Verify that we are notified when the surface is created.
+        callbackFor(opaque).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(0)).surfaceDestroyed(ArgumentMatchers.<SurfaceHolder>any());
+
+        // Verify that we are notified when the surface is changed.
+        final int format = PixelFormat.RGB_565;
+        final int width = 320;
+        final int height = 240;
+        callbackFor(opaque).surfaceChanged(opaque.getHolder(), format, width, height);
+        verify(mCallback, times(1)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceChanged(opaque.getHolder(), format, width, height);
+        verify(mCallback, times(0)).surfaceDestroyed(ArgumentMatchers.<SurfaceHolder>any());
+
+        // Verify that we are notified when the surface is destroyed.
+        callbackFor(opaque).surfaceDestroyed(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceChanged(opaque.getHolder(), format, width, height);
+        verify(mCallback, times(1)).surfaceDestroyed(opaque.getHolder());
+    }
+
+    @Test
+    @Feature("Compositor")
+    @Config(shadows = {MyShadowSurfaceView.class})
+    public void testRequestOpaqueThenTranslucentSurface() {
+        // Request opaque then translucent.
+        SurfaceView opaque = requestAndGet(PixelFormat.OPAQUE);
+        SurfaceView translucent = requestAndGet(PixelFormat.TRANSLUCENT);
+
+        // Verify that we received a destroy for |opaque| and created / changed for |translucent|.
+        verify(mCallback, times(1)).surfaceDestroyed(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceCreated(translucent.getHolder());
+        verify(mCallback, times(1))
+                .surfaceChanged(eq(translucent.getHolder()), anyInt(), anyInt(), anyInt());
+
+        // Both views should be present.
+        assertEquals(2, mLayout.getChildCount());
+
+        // Only the translucent surface should be left.  Note that the old view is still valid.
+        mManager.doneWithUnownedSurface();
+        runDelayedTasks();
+        assertEquals(1, mLayout.getChildCount());
+        assertTrue(findSurface(PixelFormat.TRANSLUCENT) != null);

[Ted C, 2017/01/31 23:17:51]

would assertNotNull work here?

[liberato (no reviews please), 2017/02/01 19:19:48]

done, and elsewhere.

+    }
+
+    @Test
+    @Feature("Compositor")
+    @Config(shadows = {MyShadowSurfaceView.class})
+    public void testRequestSameSurface() {
+        // Request an opaque surface, get it, then request it again.  Verify that we get synthetic
+        // create / destroy callbacks.
+        SurfaceView opaque = requestAndGet(PixelFormat.OPAQUE);
+        verify(mCallback, times(1)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1))
+                .surfaceChanged(eq(opaque.getHolder()), anyInt(), anyInt(), anyInt());
+        verify(mCallback, times(0)).surfaceDestroyed(opaque.getHolder());
+
+        // Surface is curerntly valid.  Request again.  We should get back a destroy and create.
+        assertEquals(opaque, requestSurface(PixelFormat.OPAQUE));
+        verify(mCallback, times(2)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(2))
+                .surfaceChanged(eq(opaque.getHolder()), anyInt(), anyInt(), anyInt());
+        verify(mCallback, times(1)).surfaceDestroyed(opaque.getHolder());
+        assertEquals(1, mLayout.getChildCount());
+    }
+
+    @Test
+    @Feature("Compositor")
+    @Config(shadows = {MyShadowSurfaceView.class})
+    public void testRequestSameSurfaceBeforeReady() {
+        // Request an opaque surface, then request it again before the first one shows up.
+        SurfaceView opaque = requestSurface(PixelFormat.OPAQUE);
+        verify(mCallback, times(0)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(0))
+                .surfaceChanged(eq(opaque.getHolder()), anyInt(), anyInt(), anyInt());
+        verify(mCallback, times(0)).surfaceDestroyed(opaque.getHolder());
+
+        // Request again.  We shouldn't get any callbacks, since the surface is still pending.
+        assertEquals(opaque, requestSurface(PixelFormat.OPAQUE));
+        verify(mCallback, times(0)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(0))
+                .surfaceChanged(eq(opaque.getHolder()), anyInt(), anyInt(), anyInt());
+        verify(mCallback, times(0)).surfaceDestroyed(opaque.getHolder());
+
+        // Only the opaque view should be attached.
+        assertEquals(1, mLayout.getChildCount());
+
+        // When the surface is created, we should get notified created / changed, but not destroyed.
+        callbackFor(opaque).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceCreated(opaque.getHolder());
+
+        callbackFor(opaque).surfaceChanged(opaque.getHolder(), PixelFormat.RGB_565, 320, 240);
+        verify(mCallback, times(1))
+                .surfaceChanged(eq(opaque.getHolder()), anyInt(), anyInt(), anyInt());
+        verify(mCallback, times(0)).surfaceDestroyed(opaque.getHolder());
+    }
+
+    @Test
+    @Feature("Compositor")
+    @Config(shadows = {MyShadowSurfaceView.class})
+    public void testRequestDifferentSurfacesBeforeReady() {
+        // Request an opaque surface, then request the translucent one before the it one shows up.
+        SurfaceView opaque = requestSurface(PixelFormat.OPAQUE);
+        verify(mCallback, times(0)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(0))
+                .surfaceChanged(eq(opaque.getHolder()), anyInt(), anyInt(), anyInt());
+        verify(mCallback, times(0)).surfaceDestroyed(opaque.getHolder());
+
+        // Request translucent.  We should get no callbacks, but both views should be attached.
+        SurfaceView translucent = requestSurface(PixelFormat.TRANSLUCENT);
+        verify(mCallback, times(0)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(0)).surfaceCreated(translucent.getHolder());
+        assertEquals(2, mLayout.getChildCount());
+
+        // If the opaque surface arrives, we shouldn't hear about it.  It should be detached, since
+        // we've requested the other one.
+        callbackFor(opaque).surfaceCreated(opaque.getHolder());
+        runDelayedTasks();
+        assertEquals(1, mLayout.getChildCount());
+        verify(mCallback, times(0)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(0)).surfaceCreated(translucent.getHolder());
+        verify(mCallback, times(0)).surfaceDestroyed(opaque.getHolder());
+
+        // When we create the translucent surface, we should be notified.
+        callbackFor(translucent).surfaceCreated(translucent.getHolder());
+        verify(mCallback, times(0)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceCreated(translucent.getHolder());
+    }
+
+    @Test
+    @Feature("Compositor")
+    @Config(shadows = {MyShadowSurfaceView.class})
+    public void testPendingSurfaceChangedCallback() {
+        // Request an opaque surface, and request it again between 'created' and 'changed'.  We
+        // should get a synthetic 'created', but a real 'changed' callback.
+        SurfaceView opaque = requestSurface(PixelFormat.OPAQUE);
+        callbackFor(opaque).surfaceCreated(opaque.getHolder());
+        runDelayedTasks();
+
+        // Sanity check.
+        verify(mCallback, times(1)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(0))
+                .surfaceChanged(eq(opaque.getHolder()), anyInt(), anyInt(), anyInt());
+
+        // Re-request while 'changed' is still pending.  We should get a synthetic 'created'.
+        assertEquals(opaque, requestSurface(PixelFormat.OPAQUE));
+        verify(mCallback, times(2)).surfaceCreated(opaque.getHolder());

[Ted C, 2017/01/31 23:17:51]

For the normal SurfaceView lifetime, should you always expect a single create
followed by a single destroy (potentially followed by the same over and over)? 
Wondering if having it receive 2 creates is not a standard behavior.  Or would
we have synthesized a destroy as well?

[liberato (no reviews please), 2017/02/01 19:19:48]

we should get a synthetic destroyed too.  i've added a check for it.

+        verify(mCallback, times(0))
+                .surfaceChanged(eq(opaque.getHolder()), anyInt(), anyInt(), anyInt());
+
+        // Send 'changed', and expect that we'll receive it.
+        callbackFor(opaque).surfaceChanged(opaque.getHolder(), PixelFormat.RGB_565, 320, 240);
+        verify(mCallback, times(1))
+                .surfaceChanged(eq(opaque.getHolder()), anyInt(), anyInt(), anyInt());
+    }
+
+    @Test
+    @Feature("Compositor")
+    @Config(shadows = {MyShadowSurfaceView.class})
+    public void testJellyBeanWorkaround() {
+        // See if recreateSurfaceForJellyBean destroys / re-creates the surface.
+        // should get a synthetic 'created', but a real 'changed' callback.
+        SurfaceView opaque = requestAndGet(PixelFormat.OPAQUE);
+        verify(mCallback, times(1)).surfaceCreated(opaque.getHolder());
+        assertEquals(1, mLayout.getChildCount());
+
+        // We should be notified that the surface was destroyed via synthetic callback, and the
+        // surface should be detached.
+        mManager.recreateSurfaceForJellyBean();
+        verify(mCallback, times(1)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceDestroyed(opaque.getHolder());
+        assertEquals(0, mLayout.getChildCount());
+
+        // When the surface really is destroyed, it should be re-attached.  We should not be
+        // notified again, though.
+        callbackFor(opaque).surfaceDestroyed(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceDestroyed(opaque.getHolder());
+        assertEquals(1, mLayout.getChildCount());
+
+        // When the surface is re-created, we should be notified.
+        callbackFor(opaque).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(2)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceDestroyed(opaque.getHolder());
+        assertEquals(1, mLayout.getChildCount());
+    }
+
+    @Test
+    @Feature("Compositor")
+    @Config(shadows = {MyShadowSurfaceView.class})
+    public void testRequestSurfaceDuringDestruction() {
+        // If we re-request a surface while we're tearing it down, it should be re-attached and
+        // given back to us once the destruction completes.
+        SurfaceView opaque = requestAndGet(PixelFormat.OPAQUE);
+        SurfaceView translucent = requestAndGet(PixelFormat.TRANSLUCENT);
+        mManager.doneWithUnownedSurface();
+
+        // The transparent surface should be attached, and the opaque one detached.
+        assertEquals(1, mLayout.getChildCount());
+        assertTrue(findSurface(PixelFormat.TRANSLUCENT) != null);
+
+        // Re-request the opaque surface.  Nothing should happen until it's destroyed.  It should
+        // not be re-attached, since that is also deferred until destruction.
+        assertEquals(null, requestSurface(PixelFormat.OPAQUE));
+        assertEquals(1, mLayout.getChildCount());
+        assertTrue(findSurface(PixelFormat.TRANSLUCENT) != null);
+
+        // When the opaque surface is destroyed, then it should be re-attached.  No callbacks shoud
+        // have arrived yet, except for initial creation and (synthetic) destroyed when we got the
+        // translucent surface.
+        callbackFor(opaque).surfaceDestroyed(opaque.getHolder());
+        assertEquals(2, mLayout.getChildCount());
+        verify(mCallback, times(1)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceDestroyed(opaque.getHolder());
+        verify(mCallback, times(0)).surfaceDestroyed(translucent.getHolder());
+
+        // When the opaque surface becomes available, we'll get the synthetic destroy for the
+        // translucent one that we lost ownership of, and the real create for the opaque one.
+        callbackFor(opaque).surfaceCreated(opaque.getHolder());
+        assertEquals(2, mLayout.getChildCount());
+        verify(mCallback, times(2)).surfaceCreated(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceDestroyed(opaque.getHolder());
+        verify(mCallback, times(1)).surfaceDestroyed(translucent.getHolder());
+    }
+}