8000 Make call to `onSiteChanged` safe for any thread to call (#619) · androiddev2019/Android@01a4676 · GitHub
[go: up one dir, main page]
Skip to content

Commit 01a4676

Browse files
CDRussellCDRussell
authored
Make call to onSiteChanged safe for any thread to call (duckduckgo#619)
* Define DispatchersProvider as a first step to injectable dispatchers * Add test dispatcher provider to CoroutineTestRule * Ensure onSiteChanged isn't doing heavy lifting on the main thread
1 parent 369aeb3 commit 01a4676

File tree

4 files changed

+122
-53
lines changed

4 files changed

+122
-53
lines changed

‎app/src/androidTest/java/com/duckduckgo/app/CoroutineTestRule.kt

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package com.duckduckgo.app
1818

19+
import com.duckduckgo.app.global.DispatcherProvider
20+
import kotlinx.coroutines.CoroutineDispatcher
1921
import kotlinx.coroutines.Dispatchers
2022
import kotlinx.coroutines.ExperimentalCoroutinesApi
2123
import kotlinx.coroutines.test.TestCoroutineDispatcher
@@ -25,9 +27,15 @@ import org.junit.rules.TestWatcher
2527
import org.junit.runner.Description
2628

2729
@ExperimentalCoroutinesApi
28-
class CoroutineTestRule(
29-
private val testDispatcher: TestCoroutineDispatcher = TestCoroutineDispatcher()
30-
) : TestWatcher() {
30+
class CoroutineTestRule(val testDispatcher: TestCoroutineDispatcher = TestCoroutineDispatcher()) : TestWatcher() {
31+
32+
val testDispatcherProvider = object : DispatcherProvider {
33+
override fun default(): CoroutineDispatcher = testDispatcher
34+
override fun io(): CoroutineDispatcher = testDispatcher
35+
override fun main(): CoroutineDispatcher = testDispatcher
36+
override fun unconfined(): CoroutineDispatcher = testDispatcher
37+
38+
}
3139

3240
override fun starting(description: Description?) {
3341
super.starting(description)

‎app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt

Lines changed: 55 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,11 @@ import com.duckduckgo.app.widget.ui.WidgetCapabilities
7171
import com.nhaarman.mockitokotlin2.*
7272
import io.reactivex.Observable
7373
import io.reactivex.Single
74+
import kotlinx.coroutines.Dispatchers
7475
import kotlinx.coroutines.ExperimentalCoroutinesApi
7576
import kotlinx.coroutines.runBlocking
77+
import kotlinx.coroutines.test.runBlockingTest
78+
import kotlinx.coroutines.withContext
7679
import org.junit.After
7780
import org.junit.Assert.*
7881
import org.junit.Before
@@ -84,6 +87,7 @@ import org.mockito.Mockito.never
8487
import org.mockito.Mockito.verify
8588
import java.util.concurrent.TimeUnit
8689

90+
@ExperimentalCoroutinesApi
8791
class BrowserTabViewModelTest {
8892

8993
@get:Rule
@@ -94,7 +98,7 @@ class BrowserTabViewModelTest {
9498

9599
@ExperimentalCoroutinesApi
96100
@get:Rule
97-
var coroutinesTestRule = CoroutineTestRule()
101+
var coroutineRule = CoroutineTestRule()
98102

99103
@Mock
100104
private lateinit var mockPrevalenceStore: PrevalenceStore
@@ -196,7 +200,7 @@ class BrowserTabViewModelTest {
196200
)
197201

198202
val siteFactory = SiteFactory(mockPrivacyPractices, mockTrackerNetworks, prevalenceStore = mockPrevalenceStore)
199-
runBlocking<Unit> {
203+
runBlockingTest {
200204
whenever(mockTabsRepository.retrieveSiteData(any())).thenReturn(MutableLiveData())
201205
whenever(mockPrivacyPractices.privacyPracticesFor(any())).thenReturn(PrivacyPractices.UNKNOWN)
202206
whenever(mockAppInstallStore.installTimestamp).thenReturn(System.currentTimeMillis() - TimeUnit.DAYS.toMillis(1))
@@ -221,7 +225,8 @@ class BrowserTabViewModelTest {
221225
ctaViewModel = ctaViewModel,
222226
searchCountDao = mockSearchCountDao,
223227
pixel = mockPixel,
224-
variantManager = mockVariantManager
228+
variantManager = mockVariantManager,
229+
dispatchers = coroutineRule.testDispatcherProvider
225230
)
226231

227232
testee.loadData("abc", null, false)
@@ -263,7 +268,7 @@ class BrowserTabViewModelTest {
263268
}
264269

265270
@Test
266-
fun whenOpenInNewBackgroundRequestedThenTabRepositoryUpdatedAndCommandIssued() = runBlocking<Unit> {
271+
fun whenOpenInNewBackgroundRequestedThenTabRepositoryUpdatedAndCommandIssued() = runBlockingTest {
267272
val url = "http://www.example.com"
268273
testee.openInNewBackgroundTab(url)
269274

@@ -314,13 +319,13 @@ class BrowserTabViewModelTest {
314319
}
315320

316321
@Test
317-
fun whenBookmarkEditedThenDaoIsUpdated() = runBlocking<Unit> {
322+
fun whenBookmarkEditedThenDaoIsUpdated() = runBlockingTest {
318323
testee.editBookmark(0, "A title", "www.example.com")
319324
verify(mockBookmarksDao).update(BookmarkEntity(title = "A title", url = "www.example.com"))
320325
}
321326

322327
@Test
323-
fun whenBookmarkAddedThenDaoIsUpdatedAndUserNotified() = runBlocking<Unit> {
328+
fun whenBookmarkAddedThenDaoIsUpdatedAndUserNotified() = runBlockingTest {
324329
loadUrl("www.example.com", "A title")
325330

326331
testee.onBookmarkAddRequested()
@@ -356,7 +361,7 @@ class BrowserTabViewModelTest {
356361
}
357362

358363
@Test
359-
fun whenBrowsingAndUrlLoadedThenSiteVisitedEntryAddedToLeaderboardDao() {
364+
fun whenBrowsingAndUrlLoadedThenSiteVisitedEntryAddedToLeaderboardDao() = runBlockingTest {
360365
loadUrl("http://example.com/abc", isBrowserShowing = true)
361366
verify(mockNetworkLeaderboardDao).incrementSitesVisited()
362367
}
@@ -425,9 +430,11 @@ class BrowserTabViewModelTest {
425430
}
426431

427432
@Test
428-
fun whenViewModelNotifiedThatUrlGotFocusThenViewStateIsUpdated() {
429-
testee.onOmnibarInputStateChanged("", true, hasQueryChanged = false)
430-
assertTrue(omnibarViewState().isEditing)
433+
fun whenViewModelNotifiedThatUrlGotFocusThenViewStateIsUpdated() = runBlockingTest {
434+
withContext(Dispatchers.Main) {
435+
testee.onOmnibarInputStateChanged("", true, hasQueryChanged = false)
436+
assertTrue(omnibarViewState().isEditing)
437+
}
431438
}
432439

433440
@Test
@@ -491,17 +498,21 @@ class BrowserTabViewModelTest {
491498
}
492499

493500
@Test
494-
fun whenUrlClearedThenPrivacyGradeIsCleared() = runBlocking<Unit> {
495-
loadUrl("https://duckduckgo.com")
496-
assertNotNull(testee.privacyGrade.value)
497-
loadUrl(null)
498-
assertNull(testee.privacyGrade.value)
501+
fun whenUrlClearedThenPrivacyGradeIsCleared() = runBlockingTest {
502+
withContext(Dispatchers.Main) {
503+
loadUrl("https://duckduckgo.com")
504+
assertNotNull(testee.privacyGrade.value)
505+
loadUrl(null)
506+
assertNull(testee.privacyGrade.value)
507+
}
499508
}
500509

501510
@Test
502-
fun whenUrlLoadedThenPrivacyGradeIsReset() = runBlocking<Unit> {
503-
loadUrl("https://duckduckgo.com")
504-
assertNotNull(testee.privacyGrade.value)
511+
fun whenUrlLoadedThenPrivacyGradeIsReset() = runBlockingTest {
512+
withContext(Dispatchers.Main) {
513+
loadUrl("https://duckduckgo.com")
514+
assertNotNull(testee.privacyGrade.value)
515+
}
505516
}
506517

507518
@Test
@@ -924,7 +935,7 @@ class BrowserTabViewModelTest {
924935
}
925936

926937
@Test
927-
fun whenSiteLoadedAndUserSelectsToAddBookmarkThenAddBookmarkCommandSentWithUrlAndTitle() = runBlocking<Unit> {
938+
fun whenSiteLoadedAndUserSelectsToAddBookmarkThenAddBookmarkCommandSentWithUrlAndTitle() = runBlockingTest {
928939
loadUrl("foo.com")
929940
testee.titleReceived("Foo Title")
930941
testee.onBookmarkAddRequested()
@@ -934,7 +945,7 @@ class BrowserTabViewModelTest {
934945
}
935946

936947
@Test
937-
fun whenNoSiteAndUserSelectsToAddBookmarkThenBookmarkAddedWithBlankTitleAndUrl() = runBlocking<Unit> {
948+
fun whenNoSiteAndUserSelectsToAddBookmarkThenBookmarkAddedWithBlankTitleAndUrl() = runBlockingTest {
938949
whenever(mockBookmarksDao.insert(any())).thenReturn(1)
939950
testee.onBookmarkAddRequested()
940951
verify(mockBookmarksDao).insert(BookmarkEntity(title = "", url = ""))
@@ -953,7 +964,7 @@ class BrowserTabViewModelTest {
953964
}
954965

955966
@Test
956-
fun whenOnSiteAndBrokenSiteSelectedThenBrokenSiteFeedbackCommandSentWithUrl() = runBlocking<Unit> {
967+
fun whenOnSiteAndBrokenSiteSelectedThenBrokenSiteFeedbackCommandSentWithUrl() = runBlockingTest {
957968
loadUrl("foo.com", isBrowserShowing = true)
958969
testee.onBrokenSiteSelected()
959970
val command = captureCommands().value as Command.BrokenSiteFeedback
@@ -1012,24 +1023,30 @@ class BrowserTabViewModelTest {
10121023
}
10131024

10141025
@Test
1015-
fun whenUrlNullThenSetBrowserNotShowing() {
1016-
testee.loadData("id", null, false)
1017-
testee.determineShowBrowser()
1018-
assertEquals(false, testee.browserViewState.value?.browserShowing)
1026+
fun whenUrlNullThenSetBrowserNotShowing() = runBlockingTest {
1027+
withContext(Dispatchers.Main) {
1028+
testee.loadData("id", null, false)
1029+
testee.determineShowBrowser()
1030+
assertEquals(false, testee.browserViewState.value?.browserShowing)
1031+
}
10191032
}
10201033

10211034
@Test
1022-
fun whenUrlBlankThenSetBrowserNotShowing() {
1023-
testee.loadData("id", " ", false)
1024-
testee.determineShowBrowser()
1025-
assertEquals(false, testee.browserViewState.value?.browserShowing)
1035+
fun whenUrlBlankThenSetBrowserNotShowing() = runBlockingTest {
1036+
withContext(Dispatchers.Main) {
1037+
testee.loadData("id", " ", false)
1038+
testee.determineShowBrowser()
1039+
assertEquals(false, testee.browserViewState.value?.browserShowing)
1040+
}
10261041
}
10271042

10281043
@Test
1029-
fun whenUrlPresentThenSetBrowserShowing() {
1030-
testee.loadData("id", "https://example.com", false)
1031-
testee.determineShowBrowser()
1032-
assertEquals(true, testee.browserViewState.value?.browserShowing)
1044+
fun whenUrlPresentThenSetBrowserShowing() = runBlockingTest {
1045+
withContext(Dispatchers.Main) {
1046+
testee.loadData("id", "https://example.com", false)
1047+
testee.determineShowBrowser()
1048+
assertEquals(true, testee.browserViewState.value?.browserShowing)
1049+
}
10331050
}
10341051

10351052
@Test
@@ -1087,9 +1104,11 @@ class BrowserTabViewModelTest {
10871104
}
10881105

10891106
@Test
1090-
fun whenUserSelectToEditQueryThenMoveCaretToTheEnd() {
1091-
testee.onUserSelectedToEditQuery("foo")
1092-
assertTrue(omnibarViewState().shouldMoveCaretToEnd)
1107+
fun whenUserSelectToEditQueryThenMoveCaretToTheEnd() = runBlockingTest {
1108+
withContext(Dispatchers.Main) {
1109+
testee.onUserSelectedToEditQuery("foo")
1110+
assertTrue(omnibarViewState().shouldMoveCaretToEnd)
1111+
}
10931112
}
10941113

10951114
@Test

‎app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,6 @@ import com.duckduckgo.app.usage.search.SearchCountDao
7373
import com.jakewharton.rxrelay2.PublishRelay
7474
import io.reactivex.android.schedulers.AndroidSchedulers
7575
import io.reactivex.schedulers.Schedulers
76-
import kotlinx.coroutines.Dispatchers
7776
import kotlinx.coroutines.Job
7877
import kotlinx.coroutines.launch
7978
import kotlinx.coroutines.withContext
@@ -99,7 +98,8 @@ class BrowserTabViewModel(
9998
private val searchCountDao: SearchCountDao,
10099
private val pixel: Pixel,
101100
private val variantManager: VariantManager,
102-
appConfigurationDao: AppConfigurationDao
101+
appConfigurationDao: AppConfigurationDao,
102+
private val dispatchers: DispatcherProvider = DefaultDispatcherProvider()
103103
) : WebViewClientListener, EditBookmarkListener, HttpAuthenticationListener, ViewModel() {
104104

105105
private var buildingSiteFactoryJob: Job? = null
@@ -253,10 +253,10 @@ class BrowserTabViewModel(
253253
onSiteChanged()
254254
buildingSiteFactoryJob = viewModelScope.launch {
255255
site?.let {
256-
withContext(Dispatchers.IO) {
256+
withContext(dispatchers.io()) {
257257
siteFactory.loadFullSiteDetails(it)
258+
onSiteChanged()
258259
}
259-
onSiteChanged()
260260
}
261261
}
262262
}
@@ -302,7 +302,7 @@ class BrowserTabViewModel(
302302

303303
suspend fun fireAutocompletePixel(suggestion: AutoCompleteSuggestion) {
304304
val currentViewState = currentAutoCompleteViewState()
305-
val hasBookmarks = withContext(Dispatchers.IO) {
305+
val hasBookmarks = withContext(dispatchers.io()) {
306306
bookmarksDao.hasBookmarks()
307307
}
308308
val params = mapOf(
@@ -325,7 +325,7 @@ class BrowserTabViewModel(
325325
command.value = HideKeyboard
326326
val trimmedInput = input.trim()
327327

328-
viewModelScope.launch(Dispatchers.IO) {
328+
viewModelScope.launch(dispatchers.io()) {
329329
searchCountDao.incrementSearchCount()
330330
}
331331

@@ -555,10 +555,20 @@ class BrowserTabViewModel(
555555
}
556556

557557
private fun onSiteChanged() {
558-
siteLiveData.postValue(site)
559-
privacyGrade.postValue(site?.calculateGrades()?.improvedGrade)
560-
viewModelScope.launch(Dispatchers.IO) {
561-
tabRepository.update(tabId, site)
558+
viewModelScope.launch {
559+
560+
val improvedGrade = withContext(dispatchers.io()) {
561+
site?.calculateGrades()?.improvedGrade
562+
}
563+
564+
withContext(dispatchers.main()) {
565+
siteLiveData.value = site
566+
privacyGrade.value = improvedGrade
567+
}
568+
569+
withContext(dispatchers.io()) {
570+
tabRepository.update(tabId, site)
571+
}
562572
}
563573
}
564574

@@ -608,22 +618,22 @@ class BrowserTabViewModel(
608618
suspend fun onBookmarkAddRequested() {
609619
val url = url ?: ""
610620
val title = title ?: ""
611-
val id = withContext(Dispatchers.IO) {
621+
val id = withContext(dispatchers.io()) {
612622
bookmarksDao.insert(BookmarkEntity(title = title, url = url))
613623
}
614-
withContext(Dispatchers.Main) {
624+
withContext(dispatchers.main()) {
615625
command.value = ShowBookmarkAddedConfirmation(id, title, url)
616626
}
617627
}
618628

619629
override fun onBookmarkEdited(id: Long, title: String, url: String) {
620-
viewModelScope.launch(Dispatchers.IO) {
630+
viewModelScope.launch(dispatchers.io()) {
621631
editBookmark(id, title, url)
622632
}
623633
}
624634

625635
suspend fun editBookmark(id: Long, title: String, url: String) {
626-
withContext(Dispatchers.IO) {
636+
withContext(dispatchers.io()) {
627637
bookmarksDao.update(BookmarkEntity(id, title, url))
628638
}
629639
}

‎app/src/main/java/com/duckduckgo/app/global/DispatcherProvider.kt

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* Copyright (c) 2019 DuckDuckGo
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.duckduckgo.app.global
18+
19+
import kotlinx.coroutines.CoroutineDispatcher
20+
import kotlinx.coroutines.Dispatchers
21+
22+
23+
interface DispatcherProvider {
24+
25+
fun main(): CoroutineDispatcher = Dispatchers.Main
26+
fun default(): CoroutineDispatcher = Dispatchers.Default
27+
fun io(): CoroutineDispatcher = Dispatchers.IO
28+
fun unconfined(): CoroutineDispatcher = Dispatchers.Unconfined
29+
30+
}
31+
32+
class DefaultDispatcherProvider : DispatcherProvider

0 commit comments

Comments
 (0)
0