Skip to content

Commit 5e7acc7

Browse files
authored
FF suspend version of writing favicon to disk (#5990)
Task/Issue URL: https://app.asana.com/1/137249556945/project/1202552961248957/task/1210122551240113?focus=true ### Description ### Steps to test this PR - [x] Smoke test favicon download and store
1 parent a8956c2 commit 5e7acc7

File tree

4 files changed

+61
-3
lines changed

4 files changed

+61
-3
lines changed

app/src/androidTest/java/com/duckduckgo/app/browser/favicon/FileBasedFaviconPersisterTest.kt

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,16 @@
1818

1919
package com.duckduckgo.app.browser.favicon
2020

21+
import android.annotation.SuppressLint
2122
import android.graphics.Bitmap
2223
import android.graphics.BitmapFactory
2324
import androidx.test.platform.app.InstrumentationRegistry
2425
import com.duckduckgo.app.global.file.FileDeleter
26+
import com.duckduckgo.app.pixels.remoteconfig.AndroidBrowserConfigFeature
2527
import com.duckduckgo.common.test.CoroutineTestRule
2628
import com.duckduckgo.common.utils.sha256
29+
import com.duckduckgo.feature.toggles.api.FakeFeatureToggleFactory
30+
import com.duckduckgo.feature.toggles.api.Toggle
2731
import java.io.File
2832
import java.io.FileOutputStream
2933
import kotlinx.coroutines.test.runTest
@@ -40,6 +44,7 @@ import org.mockito.kotlin.argumentCaptor
4044
import org.mockito.kotlin.mock
4145
import org.mockito.kotlin.verify
4246

47+
@SuppressLint("DenyListedApi")
4348
class FileBasedFaviconPersisterTest {
4449

4550
@get:Rule
@@ -52,10 +57,12 @@ class FileBasedFaviconPersisterTest {
5257
private val secondaryTestDirectory = "otherTest"
5358
private val subFolder = "subFolder"
5459
private val domain = "www.example.com"
60+
private val fakeBrowserConfigFeature = FakeFeatureToggleFactory.create(AndroidBrowserConfigFeature::class.java)
5561

5662
@Before
5763
fun setup() {
58-
testee = FileBasedFaviconPersister(context, mockFileDeleter, coroutineRule.testDispatcherProvider)
64+
fakeBrowserConfigFeature.storeFaviconSuspend().setRawStoredState(Toggle.State(true))
65+
testee = FileBasedFaviconPersister(context, mockFileDeleter, fakeBrowserConfigFeature, coroutineRule.testDispatcherProvider)
5966
}
6067

6168
@After

app/src/main/java/com/duckduckgo/app/browser/di/BrowserModule.kt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ import com.duckduckgo.app.global.events.db.UserEventsStore
6868
import com.duckduckgo.app.global.file.FileDeleter
6969
import com.duckduckgo.app.global.install.AppInstallStore
7070
import com.duckduckgo.app.lifecycle.MainProcessLifecycleObserver
71+
import com.duckduckgo.app.pixels.remoteconfig.AndroidBrowserConfigFeature
7172
import com.duckduckgo.app.privacy.db.PrivacyProtectionCountDao
7273
import com.duckduckgo.app.referral.AppReferrerDataStore
7374
import com.duckduckgo.app.settings.db.SettingsDataStore
@@ -256,8 +257,9 @@ class BrowserModule {
256257
context: Context,
257258
fileDeleter: FileDeleter,
258259
dispatcherProvider: DispatcherProvider,
260+
androidBrowserConfigFeature: AndroidBrowserConfigFeature,
259261
): FaviconPersister {
260-
return FileBasedFaviconPersister(context, fileDeleter, dispatcherProvider)
262+
return FileBasedFaviconPersister(context, fileDeleter, androidBrowserConfigFeature, dispatcherProvider)
261263
}
262264

263265
@Provides

app/src/main/java/com/duckduckgo/app/browser/favicon/FaviconPersister.kt

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,14 @@ import android.content.Context
2020
import android.graphics.Bitmap
2121
import android.graphics.BitmapFactory
2222
import com.duckduckgo.app.global.file.FileDeleter
23+
import com.duckduckgo.app.pixels.remoteconfig.AndroidBrowserConfigFeature
2324
import com.duckduckgo.common.utils.DispatcherProvider
2425
import com.duckduckgo.common.utils.sha256
2526
import java.io.File
2627
import java.io.FileOutputStream
2728
import kotlinx.coroutines.NonCancellable
29+
import kotlinx.coroutines.sync.Mutex
30+
import kotlinx.coroutines.sync.withLock
2831
import kotlinx.coroutines.withContext
2932
import timber.log.Timber
3033

@@ -61,9 +64,12 @@ interface FaviconPersister {
6164
class FileBasedFaviconPersister(
6265
val context: Context,
6366
private val fileDeleter: FileDeleter,
67+
private val androidBrowserConfigFeature: AndroidBrowserConfigFeature,
6468
private val dispatcherProvider: DispatcherProvider,
6569
) : FaviconPersister {
6670

71+
val mutex = Mutex()
72+
6773
override suspend fun deleteAll(directory: String) {
6874
fileDeleter.deleteDirectory(faviconDirectory(directory))
6975
}
@@ -100,7 +106,11 @@ class FileBasedFaviconPersister(
100106
domain: String,
101107
): File? {
102108
return withContext(dispatcherProvider.io() + NonCancellable) {
103-
writeToDisk(directory, subFolder, bitmap, domain)
109+
if (androidBrowserConfigFeature.storeFaviconSuspend().isEnabled()) {
110+
writeToDiskAsync(directory, subFolder, bitmap, domain)
111+
} else {
112+
writeToDisk(directory, subFolder, bitmap, domain)
113+
}
104114
}
105115
}
106116

@@ -181,6 +191,42 @@ class FileBasedFaviconPersister(
181191
}
182192
}
183193

194+
private suspend fun writeToDiskAsync(
195+
directory: String,
196+
subFolder: String,
197+
bitmap: Bitmap,
198+
domain: String,
199+
): File? {
200+
mutex.withLock {
201+
val existingFile = fileForFavicon(directory, subFolder, domain)
202+
203+
if (existingFile.exists()) {
204+
Timber.i("Favicon favicon exists for $domain in $subFolder")
205+
val existingFavicon = BitmapFactory.decodeFile(existingFile.absolutePath)
206+
207+
existingFavicon?.let {
208+
if (it.width > bitmap.width) {
209+
return null // Stored file has better quality
210+
}
211+
}
212+
}
213+
214+
val faviconFile = prepareDestinationFile(directory, subFolder, domain)
215+
runCatching {
216+
FileOutputStream(faviconFile).use { outputStream ->
217+
bitmap.compress(Bitmap.CompressFormat.PNG, 100, outputStream)
218+
outputStream.flush()
219+
}
220+
}
221+
222+
return if (faviconFile.exists()) {
223+
faviconFile
224+
} else {
225+
null
226+
}
227+
}
228+
}
229+
184230
@Synchronized
185231
private fun writeBytesToFile(
186232
file: File,

app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,4 +119,7 @@ interface AndroidBrowserConfigFeature {
119119
*/
120120
@Toggle.DefaultValue(DefaultFeatureValue.FALSE)
121121
fun omnibarAnimation(): Toggle
122+
123+
@Toggle.DefaultValue(DefaultFeatureValue.TRUE)
124+
fun storeFaviconSuspend(): Toggle
122125
}

0 commit comments

Comments
 (0)