Merge #7323 #7343 #7344

7323: For #7301: Fix drawable ripple issues on BrowserToolbar r=pocmo a=sblatz
Co-authored-by: pocmo <skaspari@mozilla.com>


![improved 2020-06-10 14_33_30](https://user-images.githubusercontent.com/4400286/84321015-5f598d00-ab27-11ea-80e4-70ef7485f646.gif)


Thanks for the much simpler solution @pocmo! 😄 



7343: For #6996: Slightly increase delay to fix intermittent loginDialog issues r=Amejia481 a=sblatz





7344: Closes #6680: Handle exceptions thrown by capturePixels r=jonalmeida a=csadilek

We're still seeing `IllegalStateException`s (Compositor not ready) when capturing thumbnails.

We fixed this a while ago by checking if `firstContentfulPaint` has happened, but that's not working/reliable. There's currently no other way to handle this but to catch-all and return an empty/null bitmap. See https://github.com/mozilla-mobile/android-components/issues/6680.

I've also added a missing test for the `onFirstContentfulPaint` observer which was missing from https://github.com/mozilla-mobile/android-components/pull/6844/

.

The internal var we can remove now.

r? @jonalmeida ticket is labelled "skittle", but this is really independent of the new tabs tray work.
Co-authored-by: Sawyer Blatz <sdblatz@gmail.com>
Co-authored-by: Christian Sadilek <christian.sadilek@gmail.com>

components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt

@@ -72,10 +72,6 @@ class GeckoEngineSession(
     internal var currentUrl: String? = null
     internal var scrollY: Int = 0
 
-    // This is set once the first content paint has occurred and can be used to
-    // decide if it's safe to call capturePixels on the view.
-    internal var firstContentfulPaint = false
-
     internal var job: Job = Job()
     private var lastSessionState: GeckoSession.SessionState? = null
     private var stateBeforeCrash: GeckoSession.SessionState? = null
@@ -328,7 +324,6 @@ class GeckoEngineSession(
         super.close()
         job.cancel()
         geckoSession.close()
-        firstContentfulPaint = false
     }
 
     /**
@@ -616,7 +611,6 @@ class GeckoEngineSession(
         override fun onFirstComposite(session: GeckoSession) = Unit
 
         override fun onFirstContentfulPaint(session: GeckoSession) {
-            firstContentfulPaint = true
             notifyObservers { onFirstContentfulPaint() }
         }
 

components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt

@@ -189,10 +189,9 @@ class GeckoEngineView @JvmOverloads constructor(
         currentGeckoView.setDynamicToolbarMaxHeight(height)
     }
 
+    @Suppress("TooGenericExceptionCaught")
     override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) {
-        // Do not capture pixels if no content has been rendered for this session yet. GeckoView
-        // might otherwise throw an IllegalStateException if the compositor isn't ready.
-        if (currentSession?.firstContentfulPaint == true) {
+        try {
             val geckoResult = currentGeckoView.capturePixels()
             geckoResult.then({ bitmap ->
                 onFinish(bitmap)
@@ -201,7 +200,13 @@ class GeckoEngineView @JvmOverloads constructor(
                 onFinish(null)
                 GeckoResult<Void>()
             })
-        } else {
+        } catch (e: Exception) {
+            // There's currently no reliable way for consumers of GeckoView to
+            // know whether or not the compositor is ready. So we have to add
+            // a catch-all here. In the future, GeckoView will invoke our error
+            // callback instead and this block can be removed:
+            // https://bugzilla.mozilla.org/show_bug.cgi?id=1645114
+            // https://github.com/mozilla-mobile/android-components/issues/6680
             onFinish(null)
         }
     }

components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt

@@ -1914,6 +1914,25 @@ class GeckoEngineSessionTest {
         assertEquals(state, actualState)
     }
 
+    @Test
+    fun `onFirstContentfulPaint notifies observers`() {
+        val engineSession = GeckoEngineSession(mock(),
+            geckoSessionProvider = geckoSessionProvider)
+
+        captureDelegates()
+
+        var observed = false
+
+        engineSession.register(object : EngineSession.Observer {
+            override fun onFirstContentfulPaint() {
+                observed = true
+            }
+        })
+
+        contentDelegate.value.onFirstContentfulPaint(mock())
+        assertTrue(observed)
+    }
+
     @Test
     fun `onCrash notifies observers about crash`() {
         val engineSession = GeckoEngineSession(mock(),
@@ -2390,21 +2409,6 @@ class GeckoEngineSessionTest {
         assertEquals(WindowRequest.Type.CLOSE, receivedWindowRequest!!.type)
     }
 
-    @Test
-    fun managesStateOfFirstContentfulPaint() {
-        val engineSession = GeckoEngineSession(mock(),
-                geckoSessionProvider = geckoSessionProvider)
-
-        captureDelegates()
-
-        assertFalse(engineSession.firstContentfulPaint)
-        contentDelegate.value.onFirstContentfulPaint(geckoSession)
-        assertTrue(engineSession.firstContentfulPaint)
-
-        engineSession.close()
-        assertFalse(engineSession.firstContentfulPaint)
-    }
-
     class MockSecurityInformation(
         origin: String? = null,
         certificate: X509Certificate? = null

components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt

@@ -28,6 +28,7 @@ import org.mockito.Mockito.verify
 import org.mozilla.geckoview.GeckoResult
 import org.mozilla.geckoview.GeckoSession
 import org.robolectric.Robolectric.buildActivity
+import java.lang.IllegalStateException
 
 @RunWith(AndroidJUnit4::class)
 class GeckoEngineViewTest {
@@ -55,7 +56,6 @@ class GeckoEngineViewTest {
 
     @Test
     fun captureThumbnail() {
-        val geckoSession: GeckoEngineSession = mock()
         val engineView = GeckoEngineView(context)
         val mockGeckoView = mock<NestedGeckoView>()
         var thumbnail: Bitmap? = null
@@ -64,18 +64,7 @@ class GeckoEngineViewTest {
         whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
         engineView.currentGeckoView = mockGeckoView
 
-        engineView.captureThumbnail {
-            thumbnail = it
-        }
-        verify(mockGeckoView, never()).capturePixels()
-
-        engineView.currentSession = geckoSession
-        engineView.captureThumbnail {
-            thumbnail = it
-        }
-        verify(mockGeckoView, never()).capturePixels()
-
-        whenever(geckoSession.firstContentfulPaint).thenReturn(true)
+        // Test GeckoResult resolves successfuly
         engineView.captureThumbnail {
             thumbnail = it
         }
@@ -86,24 +75,20 @@ class GeckoEngineViewTest {
         geckoResult = GeckoResult()
         whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
 
+        // Test GeckoResult resolves in error
         engineView.captureThumbnail {
             thumbnail = it
         }
-
         geckoResult.completeExceptionally(mock())
         assertNull(thumbnail)
 
-        // Verify that with `firstContentfulPaint` set to false, capturePixels returns a null bitmap
-        geckoResult = GeckoResult()
+        // Test GeckoView throwing an exception
+        whenever(mockGeckoView.capturePixels()).thenThrow(IllegalStateException("Compositor not ready"))
 
         thumbnail = mock()
-        whenever(geckoSession.firstContentfulPaint).thenReturn(false)
         engineView.captureThumbnail {
             thumbnail = it
         }
-        // capturePixels should not have been called again because `firstContentfulPaint` is false
-        verify(mockGeckoView, times(2)).capturePixels()
-        geckoResult.complete(mock())
         assertNull(thumbnail)
     }
 

components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt

@@ -72,10 +72,6 @@ class GeckoEngineSession(
     internal var currentUrl: String? = null
     internal var scrollY: Int = 0
 
-    // This is set once the first content paint has occurred and can be used to
-    // decide if it's safe to call capturePixels on the view.
-    internal var firstContentfulPaint = false
-
     internal var job: Job = Job()
     private var lastSessionState: GeckoSession.SessionState? = null
     private var stateBeforeCrash: GeckoSession.SessionState? = null
@@ -328,7 +324,6 @@ class GeckoEngineSession(
         super.close()
         job.cancel()
         geckoSession.close()
-        firstContentfulPaint = false
     }
 
     /**
@@ -616,7 +611,6 @@ class GeckoEngineSession(
         override fun onFirstComposite(session: GeckoSession) = Unit
 
         override fun onFirstContentfulPaint(session: GeckoSession) {
-            firstContentfulPaint = true
             notifyObservers { onFirstContentfulPaint() }
         }
 

components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt

@@ -189,10 +189,9 @@ class GeckoEngineView @JvmOverloads constructor(
         currentGeckoView.setDynamicToolbarMaxHeight(height)
     }
 
+    @Suppress("TooGenericExceptionCaught")
     override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) {
-        // Do not capture pixels if no content has been rendered for this session yet. GeckoView
-        // might otherwise throw an IllegalStateException if the compositor isn't ready.
-        if (currentSession?.firstContentfulPaint == true) {
+        try {
             val geckoResult = currentGeckoView.capturePixels()
             geckoResult.then({ bitmap ->
                 onFinish(bitmap)
@@ -201,7 +200,13 @@ class GeckoEngineView @JvmOverloads constructor(
                 onFinish(null)
                 GeckoResult<Void>()
             })
-        } else {
+        } catch (e: Exception) {
+            // There's currently no reliable way for consumers of GeckoView to
+            // know whether or not the compositor is ready. So we have to add
+            // a catch-all here. In the future, GeckoView will invoke our error
+            // callback instead and this block can be removed:
+            // https://bugzilla.mozilla.org/show_bug.cgi?id=1645114
+            // https://github.com/mozilla-mobile/android-components/issues/6680
             onFinish(null)
         }
     }

components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt

@@ -1914,6 +1914,25 @@ class GeckoEngineSessionTest {
         assertEquals(state, actualState)
     }
 
+    @Test
+    fun `onFirstContentfulPaint notifies observers`() {
+        val engineSession = GeckoEngineSession(mock(),
+                geckoSessionProvider = geckoSessionProvider)
+
+        captureDelegates()
+
+        var observed = false
+
+        engineSession.register(object : EngineSession.Observer {
+            override fun onFirstContentfulPaint() {
+                observed = true
+            }
+        })
+
+        contentDelegate.value.onFirstContentfulPaint(mock())
+        assertTrue(observed)
+    }
+
     @Test
     fun `onCrash notifies observers about crash`() {
         val engineSession = GeckoEngineSession(mock(),
@@ -2390,21 +2409,6 @@ class GeckoEngineSessionTest {
         assertEquals(WindowRequest.Type.CLOSE, receivedWindowRequest!!.type)
     }
 
-    @Test
-    fun managesStateOfFirstContentfulPaint() {
-        val engineSession = GeckoEngineSession(mock(),
-                geckoSessionProvider = geckoSessionProvider)
-
-        captureDelegates()
-
-        assertFalse(engineSession.firstContentfulPaint)
-        contentDelegate.value.onFirstContentfulPaint(geckoSession)
-        assertTrue(engineSession.firstContentfulPaint)
-
-        engineSession.close()
-        assertFalse(engineSession.firstContentfulPaint)
-    }
-
     class MockSecurityInformation(
         origin: String? = null,
         certificate: X509Certificate? = null

components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt

@@ -28,6 +28,7 @@ import org.mockito.Mockito.verify
 import org.mozilla.geckoview.GeckoResult
 import org.mozilla.geckoview.GeckoSession
 import org.robolectric.Robolectric.buildActivity
+import java.lang.IllegalStateException
 
 @RunWith(AndroidJUnit4::class)
 class GeckoEngineViewTest {
@@ -55,7 +56,6 @@ class GeckoEngineViewTest {
 
     @Test
     fun captureThumbnail() {
-        val geckoSession: GeckoEngineSession = mock()
         val engineView = GeckoEngineView(context)
         val mockGeckoView = mock<NestedGeckoView>()
         var thumbnail: Bitmap? = null
@@ -64,18 +64,7 @@ class GeckoEngineViewTest {
         whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
         engineView.currentGeckoView = mockGeckoView
 
-        engineView.captureThumbnail {
-            thumbnail = it
-        }
-        verify(mockGeckoView, never()).capturePixels()
-
-        engineView.currentSession = geckoSession
-        engineView.captureThumbnail {
-            thumbnail = it
-        }
-        verify(mockGeckoView, never()).capturePixels()
-
-        whenever(geckoSession.firstContentfulPaint).thenReturn(true)
+        // Test GeckoResult resolves successfuly
         engineView.captureThumbnail {
             thumbnail = it
         }
@@ -86,24 +75,20 @@ class GeckoEngineViewTest {
         geckoResult = GeckoResult()
         whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
 
+        // Test GeckoResult resolves in error
         engineView.captureThumbnail {
             thumbnail = it
         }
-
         geckoResult.completeExceptionally(mock())
         assertNull(thumbnail)
 
-        // Verify that with `firstContentfulPaint` set to false, capturePixels returns a null bitmap
-        geckoResult = GeckoResult()
+        // Test GeckoView throwing an exception
+        whenever(mockGeckoView.capturePixels()).thenThrow(IllegalStateException("Compositor not ready"))
 
         thumbnail = mock()
-        whenever(geckoSession.firstContentfulPaint).thenReturn(false)
         engineView.captureThumbnail {
             thumbnail = it
         }
-        // capturePixels should not have been called again because `firstContentfulPaint` is false
-        verify(mockGeckoView, times(2)).capturePixels()
-        geckoResult.complete(mock())
         assertNull(thumbnail)
     }
 

components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt

@@ -71,10 +71,6 @@ class GeckoEngineSession(
     internal var currentUrl: String? = null
     internal var scrollY: Int = 0
 
-    // This is set once the first content paint has occurred and can be used to
-    // decide if it's safe to call capturePixels on the view.
-    internal var firstContentfulPaint = false
-
     internal var job: Job = Job()
     private var lastSessionState: GeckoSession.SessionState? = null
     private var stateBeforeCrash: GeckoSession.SessionState? = null
@@ -327,7 +323,6 @@ class GeckoEngineSession(
         super.close()
         job.cancel()
         geckoSession.close()
-        firstContentfulPaint = false
     }
 
     /**
@@ -583,7 +578,7 @@ class GeckoEngineSession(
         override fun onFirstComposite(session: GeckoSession) = Unit
 
         override fun onFirstContentfulPaint(session: GeckoSession) {
-            firstContentfulPaint = true
+            notifyObservers { onFirstContentfulPaint() }
         }
 
         override fun onContextMenu(

components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt

@@ -189,10 +189,9 @@ class GeckoEngineView @JvmOverloads constructor(
         currentGeckoView.setDynamicToolbarMaxHeight(height)
     }
 
+    @Suppress("TooGenericExceptionCaught")
     override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) {
-        // Do not capture pixels if no content has been rendered for this session yet. GeckoView
-        // might otherwise throw an IllegalStateException if the compositor isn't ready.
-        if (currentSession?.firstContentfulPaint == true) {
+        try {
             val geckoResult = currentGeckoView.capturePixels()
             geckoResult.then({ bitmap ->
                 onFinish(bitmap)
@@ -201,7 +200,13 @@ class GeckoEngineView @JvmOverloads constructor(
                 onFinish(null)
                 GeckoResult<Void>()
             })
-        } else {
+        } catch (e: Exception) {
+            // There's currently no reliable way for consumers of GeckoView to
+            // know whether or not the compositor is ready. So we have to add
+            // a catch-all here. In the future, GeckoView will invoke our error
+            // callback instead and this block can be removed:
+            // https://bugzilla.mozilla.org/show_bug.cgi?id=1645114
+            // https://github.com/mozilla-mobile/android-components/issues/6680
             onFinish(null)
         }
     }

components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt

@@ -1883,6 +1883,25 @@ class GeckoEngineSessionTest {
         assertEquals(state, actualState)
     }
 
+    @Test
+    fun `onFirstContentfulPaint notifies observers`() {
+        val engineSession = GeckoEngineSession(mock(),
+            geckoSessionProvider = geckoSessionProvider)
+
+        captureDelegates()
+
+        var observed = false
+
+        engineSession.register(object : EngineSession.Observer {
+            override fun onFirstContentfulPaint() {
+                observed = true
+            }
+        })
+
+        contentDelegate.value.onFirstContentfulPaint(mock())
+        assertTrue(observed)
+    }
+
     @Test
     fun `onCrash notifies observers about crash`() {
         val engineSession = GeckoEngineSession(mock(),
@@ -2310,21 +2329,6 @@ class GeckoEngineSessionTest {
         assertEquals(WindowRequest.Type.CLOSE, receivedWindowRequest!!.type)
     }
 
-    @Test
-    fun managesStateOfFirstContentfulPaint() {
-        val engineSession = GeckoEngineSession(mock(),
-                geckoSessionProvider = geckoSessionProvider)
-
-        captureDelegates()
-
-        assertFalse(engineSession.firstContentfulPaint)
-        contentDelegate.value.onFirstContentfulPaint(geckoSession)
-        assertTrue(engineSession.firstContentfulPaint)
-
-        engineSession.close()
-        assertFalse(engineSession.firstContentfulPaint)
-    }
-
     class MockSecurityInformation(
         origin: String? = null,
         certificate: X509Certificate? = null

components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt

@@ -28,6 +28,7 @@ import org.mockito.Mockito.verify
 import org.mozilla.geckoview.GeckoResult
 import org.mozilla.geckoview.GeckoSession
 import org.robolectric.Robolectric.buildActivity
+import java.lang.IllegalStateException
 
 @RunWith(AndroidJUnit4::class)
 class GeckoEngineViewTest {
@@ -55,7 +56,6 @@ class GeckoEngineViewTest {
 
     @Test
     fun captureThumbnail() {
-        val geckoSession: GeckoEngineSession = mock()
         val engineView = GeckoEngineView(context)
         val mockGeckoView = mock<NestedGeckoView>()
         var thumbnail: Bitmap? = null
@@ -64,18 +64,7 @@ class GeckoEngineViewTest {
         whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
         engineView.currentGeckoView = mockGeckoView
 
-        engineView.captureThumbnail {
-            thumbnail = it
-        }
-        verify(mockGeckoView, never()).capturePixels()
-
-        engineView.currentSession = geckoSession
-        engineView.captureThumbnail {
-            thumbnail = it
-        }
-        verify(mockGeckoView, never()).capturePixels()
-
-        whenever(geckoSession.firstContentfulPaint).thenReturn(true)
+        // Test GeckoResult resolves successfuly
         engineView.captureThumbnail {
             thumbnail = it
         }
@@ -86,24 +75,20 @@ class GeckoEngineViewTest {
         geckoResult = GeckoResult()
         whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
 
+        // Test GeckoResult resolves in error
         engineView.captureThumbnail {
             thumbnail = it
         }
-
         geckoResult.completeExceptionally(mock())
         assertNull(thumbnail)
 
-        // Verify that with `firstContentfulPaint` set to false, capturePixels returns a null bitmap
-        geckoResult = GeckoResult()
+        // Test GeckoView throwing an exception
+        whenever(mockGeckoView.capturePixels()).thenThrow(IllegalStateException("Compositor not ready"))
 
         thumbnail = mock()
-        whenever(geckoSession.firstContentfulPaint).thenReturn(false)
         engineView.captureThumbnail {
             thumbnail = it
         }
-        // capturePixels should not have been called again because `firstContentfulPaint` is false
-        verify(mockGeckoView, times(2)).capturePixels()
-        geckoResult.complete(mock())
         assertNull(thumbnail)
     }
 

components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/display/DisplayToolbarView.kt

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.browser.toolbar.display
+
+import android.content.Context
+import android.graphics.Canvas
+import android.util.AttributeSet
+import android.view.View
+import android.widget.ImageView
+import androidx.constraintlayout.widget.ConstraintLayout
+import mozilla.components.browser.toolbar.R
+
+/**
+ * Custom ConstraintLayout for DisplayToolbar that allows us to draw ripple backgrounds on the toolbar
+ * by setting a background to transparent.
+ */
+class DisplayToolbarView @JvmOverloads constructor(
+    context: Context,
+    attrs: AttributeSet? = null,
+    defStyleAttr: Int = 0
+) : ConstraintLayout(context, attrs, defStyleAttr) {
+    init {
+        // Forcing transparent background so that draw methods will get called and ripple effect
+        // for children will be drawn on this layout.
+        setBackgroundColor(0x00000000)
+    }
+
+    lateinit var backgroundView: ImageView
+
+    override fun onFinishInflate() {
+        backgroundView = findViewById(R.id.mozac_browser_toolbar_background)
+        backgroundView.visibility = View.INVISIBLE
+
+        super.onFinishInflate()
+    }
+
+    // Overriding draw instead of onDraw since we want to draw the background before the actual
+    // (transparent) background (with a ripple effect) is drawn.
+    override fun draw(canvas: Canvas) {
+        canvas.save()
+        canvas.translate(backgroundView.x, backgroundView.y)
+
+        backgroundView.drawable?.draw(canvas)
+
+        canvas.restore()
+
+        super.draw(canvas)
+    }
+}