Merge mozilla-mobile#7323 mozilla-mobile#7343 mozilla-mobile#7344

7323: For mozilla-mobile#7301: Fix drawable ripple issues on BrowserToolbar r=pocmo a=sblatz Co-authored-by: pocmo <[email protected]> ![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 mozilla-mobile#6996: Slightly increase delay to fix intermittent loginDialog issues r=Amejia481 a=sblatz 7344: Closes mozilla-mobile#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 mozilla-mobile#6680. I've also added a missing test for the `onFirstContentfulPaint` observer which was missing from mozilla-mobile#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 <[email protected]> Co-authored-by: Christian Sadilek <[email protected]>
jonalmeida · Jun 11, 2020 · 4df1ee8 · 4df1ee8
4 parents ff0389f + 663d4a8 + 452194b + 03bb8b7
commit 4df1ee8
Show file tree

Hide file tree

Showing 15 changed files with 154 additions and 138 deletions.
diff --git a/...ne-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/...ne-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() }
         }
 

diff --git a/...ngine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt b/...ngine-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)
         }
     }

diff --git a/...ecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/...ecko-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

diff --git a/...e-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt b/...e-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)
     }
 

diff --git a/...gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/...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() }
         }
 

diff --git a/...ne-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt b/...ne-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)
         }
     }

diff --git a/...o-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/...o-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

diff --git a/...ecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt b/...ecko-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)
     }
 

diff --git a/.../engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/.../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(

diff --git a/...ser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt b/...ser/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)
         }
     }