Merge #7356

7356: Issue #7313: Use ThumbnailLoader for the TabViewHolder r=gabrielluong,boek a=jonalmeida

Instead of trying to inline the thumbnail images from disk into the TabsTrayPresenter, we can load them from the `ThumbnailStorage` via the `ThumbnailLoader` and rely on the `TabsTrayPresenter` to consume new thumbnail updates only from the store.

This fixes some perf issues, inconsistencies, and duplicate updates to the tabs tray.



Co-authored-by: Jonathan Almeida <[email protected]>

components/browser/icons/src/main/java/mozilla/components/browser/icons/BrowserIcons.kt

@@ -40,7 +40,7 @@ import mozilla.components.browser.icons.preparer.TippyTopIconPreparer
 import mozilla.components.browser.icons.processor.DiskIconProcessor
 import mozilla.components.browser.icons.processor.IconProcessor
 import mozilla.components.browser.icons.processor.MemoryIconProcessor
-import mozilla.components.browser.icons.utils.CancelOnDetach
+import mozilla.components.support.images.CancelOnDetach
 import mozilla.components.browser.icons.utils.IconDiskCache
 import mozilla.components.browser.icons.utils.IconMemoryCache
 import mozilla.components.browser.state.state.BrowserState

components/browser/tabstray/build.gradle

@@ -32,6 +32,7 @@ dependencies {
     implementation project(':ui-icons')
     implementation project(':ui-colors')
     implementation project(':support-base')
+    implementation project(':support-images')
     implementation project(':support-ktx')
 
     implementation Dependencies.androidx_appcompat

components/browser/tabstray/src/main/java/mozilla/components/browser/tabstray/TabViewHolder.kt

@@ -14,6 +14,7 @@ import mozilla.components.browser.tabstray.thumbnail.TabThumbnailView
 import mozilla.components.concept.tabstray.Tab
 import mozilla.components.concept.tabstray.TabsTray
 import mozilla.components.support.base.observer.Observable
+import mozilla.components.support.images.loader.ImageLoader
 import mozilla.components.support.ktx.kotlin.tryGetHostFromUrl
 
 /**
@@ -36,7 +37,8 @@ abstract class TabViewHolder(view: View) : RecyclerView.ViewHolder(view) {
  */
 class DefaultTabViewHolder(
     itemView: View,
-    private val tabsTray: BrowserTabsTray
+    private val tabsTray: BrowserTabsTray,
+    private val thumbnailLoader: ImageLoader? = null
 ) : TabViewHolder(itemView) {
     private val iconView: ImageView? = itemView.findViewById(R.id.mozac_browser_tabstray_icon)
     private val titleView: TextView = itemView.findViewById(R.id.mozac_browser_tabstray_title)
@@ -79,7 +81,12 @@ class DefaultTabViewHolder(
             closeView.imageTintList = ColorStateList.valueOf(tabsTray.styling.itemTextColor)
         }
 
-        thumbnailView.setImageBitmap(tab.thumbnail)
+        // In the final else case, we have no cache or fresh screenshot; do nothing instead of clearing the image.
+        if (thumbnailLoader != null && tab.thumbnail == null) {
+            thumbnailLoader.loadIntoView(thumbnailView, tab.id)
+        } else if (tab.thumbnail != null) {
+            thumbnailView.setImageBitmap(tab.thumbnail)
+        }
 
         iconView?.setImageBitmap(tab.icon)
     }

components/browser/tabstray/src/main/java/mozilla/components/browser/tabstray/TabsAdapter.kt

@@ -11,6 +11,7 @@ import mozilla.components.concept.tabstray.Tabs
 import mozilla.components.concept.tabstray.TabsTray
 import mozilla.components.support.base.observer.Observable
 import mozilla.components.support.base.observer.ObserverRegistry
+import mozilla.components.support.images.loader.ImageLoader
 
 /**
  * Function responsible for creating a `TabViewHolder` in the `TabsAdapter`.
@@ -24,16 +25,18 @@ typealias ViewHolderProvider = (ViewGroup, BrowserTabsTray) -> TabViewHolder
  */
 @Suppress("TooManyFunctions")
 open class TabsAdapter(
-    delegate: Observable<TabsTray.Observer> = ObserverRegistry(),
+    thumbnailLoader: ImageLoader? = null,
     private val viewHolderProvider: ViewHolderProvider = { parent, tabsTray ->
         DefaultTabViewHolder(
                 LayoutInflater.from(parent.context).inflate(
                         R.layout.mozac_browser_tabstray_item,
                         parent,
                         false),
-                tabsTray
+                tabsTray,
+                thumbnailLoader
         )
-    }
+    },
+    delegate: Observable<TabsTray.Observer> = ObserverRegistry()
 ) : RecyclerView.Adapter<TabViewHolder>(),
     TabsTray,
     Observable<TabsTray.Observer> by delegate {

components/browser/tabstray/src/main/java/mozilla/components/browser/tabstray/thumbnail/TabThumbnailView.kt

@@ -17,13 +17,17 @@ class TabThumbnailView(context: Context, attrs: AttributeSet) : AppCompatImageVi
 
     @VisibleForTesting
     public override fun setFrame(l: Int, t: Int, r: Int, b: Int): Boolean {
-        val changed = super.setFrame(l, t, r, b)
-        if (changed) {
-            val matrix = imageMatrix
-            val scaleFactor = width / drawable.intrinsicWidth.toFloat()
-            matrix.setScale(scaleFactor, scaleFactor, 0f, 0f)
-            imageMatrix = matrix
+        val result = super.setFrame(l, t, r, b)
+
+        val matrix = imageMatrix
+        val scaleFactor = if (drawable != null) {
+            width / drawable.intrinsicWidth.toFloat()
+        } else {
+            1F
         }
-        return changed
+        matrix.setScale(scaleFactor, scaleFactor, 0f, 0f)
+        imageMatrix = matrix
+
+        return result
     }
 }

components/browser/tabstray/src/test/java/mozilla/components/browser/tabstray/TabViewHolderTest.kt → components/browser/tabstray/src/test/java/mozilla/components/browser/tabstray/DefaultTabViewHolderTest.kt

@@ -13,17 +13,22 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
 import mozilla.components.concept.tabstray.Tab
 import mozilla.components.concept.tabstray.TabsTray
 import mozilla.components.support.base.observer.ObserverRegistry
+import mozilla.components.support.images.loader.ImageLoader
+import mozilla.components.support.test.any
+import mozilla.components.support.test.eq
 import mozilla.components.support.test.mock
+import mozilla.components.support.test.nullable
 import mozilla.components.support.test.robolectric.testContext
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertTrue
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mockito.Mockito.doReturn
+import org.mockito.Mockito.never
 import org.mockito.Mockito.verify
 
 @RunWith(AndroidJUnit4::class)
-class TabViewHolderTest {
+class DefaultTabViewHolderTest {
 
     @Test
     fun `URL from session is assigned to view`() {
@@ -146,6 +151,38 @@ class TabViewHolderTest {
         assertTrue(thumbnailView.drawable != null)
     }
 
+    @Test
+    fun `thumbnail is set from loader`() {
+        val view = LayoutInflater.from(testContext).inflate(R.layout.mozac_browser_tabstray_item, null)
+        val loader: ImageLoader = mock()
+        val viewHolder = DefaultTabViewHolder(view, mockTabsTrayWithStyles(), loader)
+        val tabWithThumbnail = Tab("123", "https://example.com", thumbnail = mock())
+        val tab = Tab("123", "https://example.com")
+
+        viewHolder.bind(tabWithThumbnail, false, mock())
+
+        verify(loader, never()).loadIntoView(any(), eq("123"), nullable(), nullable())
+
+        viewHolder.bind(tab, false, mock())
+
+        verify(loader).loadIntoView(any(), eq("123"), nullable(), nullable())
+    }
+
+    @Test
+    fun `thumbnailView does not change when there is no cache or new thumbnail`() {
+        val view = LayoutInflater.from(testContext).inflate(R.layout.mozac_browser_tabstray_item, null)
+        val viewHolder = DefaultTabViewHolder(view, mockTabsTrayWithStyles())
+        val tab = Tab("123", "https://example.com")
+        val thumbnailView = view.findViewById<ImageView>(R.id.mozac_browser_tabstray_thumbnail)
+
+        thumbnailView.setImageBitmap(mock())
+        val drawable = thumbnailView.drawable
+
+        viewHolder.bind(tab, false, mock())
+
+        assertEquals(drawable, thumbnailView.drawable)
+    }
+
     companion object {
         fun mockTabsTrayWithStyles(): BrowserTabsTray {
             val styles: TabsTrayStyling = mock()

components/browser/tabstray/src/test/java/mozilla/components/browser/tabstray/TabTouchCallbackTest.kt

@@ -7,6 +7,7 @@ package mozilla.components.browser.tabstray
 import android.view.LayoutInflater
 import androidx.recyclerview.widget.ItemTouchHelper
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import mozilla.components.browser.tabstray.DefaultTabViewHolderTest.Companion.mockTabsTrayWithStyles
 import mozilla.components.concept.tabstray.TabsTray
 import mozilla.components.support.base.observer.Observable
 import mozilla.components.support.base.observer.ObserverRegistry
@@ -48,7 +49,7 @@ class TabTouchCallbackTest {
     @Test
     fun `onChildDraw alters alpha of ViewHolder on swipe gesture`() {
         val view = LayoutInflater.from(testContext).inflate(R.layout.mozac_browser_tabstray_item, null)
-        val holder = DefaultTabViewHolder(view, TabViewHolderTest.mockTabsTrayWithStyles())
+        val holder = DefaultTabViewHolder(view, mockTabsTrayWithStyles())
         val callback = TabTouchCallback(mock())
 
         holder.itemView.alpha = 0f

components/browser/tabstray/src/test/java/mozilla/components/browser/tabstray/TabsAdapterTest.kt

@@ -40,7 +40,7 @@ class TabsAdapterTest {
 
     @Test
     fun `onCreateViewHolder will create whatever TabViewHolder is provided`() {
-        val adapter = TabsAdapter { _, _ -> TestTabViewHolder(View(testContext)) }
+        val adapter = TabsAdapter(viewHolderProvider = { _, _ -> TestTabViewHolder(View(testContext)) })
         adapter.tabsTray = mock()
 
         val type = adapter.onCreateViewHolder(FrameLayout(testContext), 0)

components/browser/tabstray/src/test/java/mozilla/components/browser/tabstray/thumbnail/TabThumbnailViewTest.kt

@@ -4,6 +4,7 @@
 
 package mozilla.components.browser.tabstray.thumbnail
 
+import android.graphics.Matrix
 import android.graphics.drawable.Drawable
 import android.widget.ImageView
 import androidx.test.ext.junit.runners.AndroidJUnit4
@@ -14,6 +15,8 @@ import org.junit.Assert.assertNotEquals
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mockito.Mockito.`when`
+import org.mockito.Mockito.spy
+import org.mockito.Mockito.verify
 import org.robolectric.Robolectric.buildAttributeSet
 
 @RunWith(AndroidJUnit4::class)
@@ -61,6 +64,18 @@ class TabThumbnailViewTest {
 
         assertEquals(matrix, matrix2)
     }
+
+    @Test
+    fun `view scaleFactor does not change if there is no drawable`() {
+        val view = spy(TabThumbnailView(testContext, emptyAttributeSet()))
+        val matrix: Matrix = spy(Matrix())
+
+        `when`(view.imageMatrix).thenReturn(matrix)
+
+        view.setFrame(5, 5, 5, 5)
+
+        verify(matrix).setScale(1f, 1f, 0f, 0f)
+    }
 }
 
 private fun emptyAttributeSet() = buildAttributeSet().build()

components/browser/thumbnails/src/main/java/mozilla/components/browser/thumbnails/loader/ThumbnailLoader.kt

@@ -0,0 +1,68 @@
+/* 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.thumbnails.loader
+
+import android.graphics.drawable.Drawable
+import android.widget.ImageView
+import androidx.annotation.MainThread
+import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.Job
+import kotlinx.coroutines.launch
+import mozilla.components.browser.thumbnails.R
+import mozilla.components.browser.thumbnails.storage.ThumbnailStorage
+import mozilla.components.support.images.CancelOnDetach
+import mozilla.components.support.images.loader.ImageLoader
+import java.lang.ref.WeakReference
+
+/**
+ * An implementation of [ImageLoader] for loading thumbnails into a [ImageView].
+ */
+class ThumbnailLoader(private val storage: ThumbnailStorage) : ImageLoader {
+
+    override fun loadIntoView(
+        view: ImageView,
+        id: String,
+        placeholder: Drawable?,
+        error: Drawable?
+    ) {
+        CoroutineScope(Dispatchers.Main).launch {
+            loadIntoViewInternal(WeakReference(view), id, placeholder, error)
+        }
+    }
+
+    @MainThread
+    private suspend fun loadIntoViewInternal(
+        view: WeakReference<ImageView>,
+        id: String,
+        placeholder: Drawable?,
+        error: Drawable?
+    ) {
+        // If we previously started loading into the view, cancel the job.
+        val existingJob = view.get()?.getTag(R.id.mozac_browser_thumbnails_tag_job) as? Job
+        existingJob?.cancel()
+
+        view.get()?.setImageDrawable(placeholder)
+
+        // Create a loading job
+        val deferredThumbnail = storage.loadThumbnail(id)
+
+        view.get()?.setTag(R.id.mozac_browser_thumbnails_tag_job, deferredThumbnail)
+        val onAttachStateChangeListener = CancelOnDetach(deferredThumbnail).also {
+            view.get()?.addOnAttachStateChangeListener(it)
+        }
+
+        try {
+            val thumbnail = deferredThumbnail.await()
+            view.get()?.setImageBitmap(thumbnail)
+        } catch (e: CancellationException) {
+            view.get()?.setImageDrawable(error)
+        } finally {
+            view.get()?.removeOnAttachStateChangeListener(onAttachStateChangeListener)
+            view.get()?.setTag(R.id.mozac_browser_thumbnails_tag_job, null)
+        }
+    }
+}

components/browser/thumbnails/src/main/res/values/tags.xml

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!-- 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/. -->
+
+<resources>
+    <item name="mozac_browser_thumbnails_tag_job" type="id" />
+</resources>