Skip to content

Commit c307f39

Browse files
authored
Fix: Prevent the first tab from always loading (#6186)
Task/Issue URL: https://app.asana.com/1/137249556945/project/1207418217763355/task/1210452628649549?focus=true ### Description This PR prevents the 1st tab to load by default if it's not the selected tab. I also added code that can determine whether the load request is for a tab in the foreground (currently only used for logging). ### Steps to test this PR _Non-first tab_ - [x] Make sure you have a couple of tabs already - [x] Make sure the `swipingTabs` flag is enabled - [x] Select a tab other than the first tab - [x] Kill the app - [x] In logcat, add a filter for `$$$: Page load time` - [x] Start the app - [x] Verify the tab is loaded - [x] Verify the "Page load time" is logged only once _First tab_ - [x] Make sure you have a couple of tabs already - [x] Select the first tab - [x] Kill the app and restart it - [x] Verify the tab is loaded - [x] In logcat, add a filter for `$$$: Page load time` - [x] Verify the "Page load time" is logged only once
1 parent 9087456 commit c307f39

File tree

11 files changed

+64
-33
lines changed

11 files changed

+64
-33
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ import com.duckduckgo.app.browser.refreshpixels.RefreshPixelSender
127127
import com.duckduckgo.app.browser.remotemessage.RemoteMessagingModel
128128
import com.duckduckgo.app.browser.senseofprotection.SenseOfProtectionExperiment
129129
import com.duckduckgo.app.browser.session.WebViewSessionStorage
130+
import com.duckduckgo.app.browser.tabs.TabManager
130131
import com.duckduckgo.app.browser.trafficquality.AndroidFeaturesHeaderPlugin.Companion.X_DUCKDUCKGO_ANDROID_HEADER
131132
import com.duckduckgo.app.browser.viewstate.BrowserViewState
132133
import com.duckduckgo.app.browser.viewstate.CtaViewState
@@ -556,6 +557,7 @@ class BrowserTabViewModelTest {
556557
private val mockSiteHttpErrorHandler: HttpCodeSiteErrorHandler = mock()
557558
private val mockSubscriptionsJSHelper: SubscriptionsJSHelper = mock()
558559
private val mockReactivateUsersExperiment: ReactivateUsersExperiment = mock()
560+
private val tabManager: TabManager = mock()
559561

560562
private val fakeOnboardingDesignExperimentToggles: OnboardingDesignExperimentToggles =
561563
FakeFeatureToggleFactory.create(OnboardingDesignExperimentToggles::class.java)
@@ -763,6 +765,7 @@ class BrowserTabViewModelTest {
763765
senseOfProtectionExperiment = mockSenseOfProtectionExperiment,
764766
subscriptionsJSHelper = mockSubscriptionsJSHelper,
765767
onboardingDesignExperimentToggles = fakeOnboardingDesignExperimentToggles,
768+
tabManager = tabManager,
766769
)
767770

768771
testee.loadData("abc", null, false, false)

app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -977,7 +977,7 @@ class BrowserWebViewClientTest {
977977
fun whenPageFinishesBeforeStartingThenPixelIsNotFired() {
978978
val mockWebView = getImmediatelyInvokedMockWebView()
979979
testee.onPageFinished(mockWebView, EXAMPLE_URL)
980-
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any())
980+
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any(), any())
981981
}
982982

983983
@Test
@@ -991,7 +991,7 @@ class BrowserWebViewClientTest {
991991
testee.onPageFinished(mockWebView, EXAMPLE_URL)
992992
val startArgumentCaptor = argumentCaptor<Long>()
993993
val endArgumentCaptor = argumentCaptor<Long>()
994-
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture())
994+
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture(), any())
995995
assertEquals(0L, startArgumentCaptor.firstValue)
996996
assertEquals(10L, endArgumentCaptor.firstValue)
997997
}
@@ -1004,7 +1004,7 @@ class BrowserWebViewClientTest {
10041004
whenever(mockWebView.settings).thenReturn(mock())
10051005
testee.onPageStarted(mockWebView, "about:blank", null)
10061006
testee.onPageFinished(mockWebView, "about:blank")
1007-
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any())
1007+
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any(), any())
10081008
}
10091009

10101010
@Test
@@ -1013,7 +1013,7 @@ class BrowserWebViewClientTest {
10131013
whenever(mockWebView.settings).thenReturn(mock())
10141014
testee.onPageStarted(mockWebView, EXAMPLE_URL, null)
10151015
testee.onPageFinished(mockWebView, EXAMPLE_URL)
1016-
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any())
1016+
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any(), any())
10171017
}
10181018

10191019
@Test
@@ -1046,7 +1046,7 @@ class BrowserWebViewClientTest {
10461046

10471047
val startArgumentCaptor = argumentCaptor<Long>()
10481048
val endArgumentCaptor = argumentCaptor<Long>()
1049-
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture())
1049+
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture(), any())
10501050
assertEquals(0L, startArgumentCaptor.firstValue)
10511051
assertEquals(10L, endArgumentCaptor.firstValue)
10521052
}
@@ -1068,7 +1068,7 @@ class BrowserWebViewClientTest {
10681068
testee.onPageFinished(mockWebView, EXAMPLE_URL)
10691069
val startArgumentCaptor = argumentCaptor<Long>()
10701070
val endArgumentCaptor = argumentCaptor<Long>()
1071-
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture())
1071+
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture(), any())
10721072
assertEquals(5L, startArgumentCaptor.firstValue)
10731073
assertEquals(10L, endArgumentCaptor.firstValue)
10741074
}

app/src/androidTest/java/com/duckduckgo/app/browser/pageloadpixel/PageLoadedHandlerTest.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,15 +51,15 @@ class PageLoadedHandlerTest {
5151

5252
@Test
5353
fun whenInvokingWithValidUrlThenPixelIsAdded() {
54-
testee.onPageLoaded(VALID_URL, "title", 0L, 10L)
54+
testee.onPageLoaded(VALID_URL, "title", 0L, 10L, true)
5555
val argumentCaptor = argumentCaptor<PageLoadedPixelEntity>()
5656
verify(pageLoadedPixelDao).add(argumentCaptor.capture())
5757
Assert.assertEquals(10L, argumentCaptor.firstValue.elapsedTime)
5858
}
5959

6060
@Test
6161
fun whenInvokingWithInvalidUrlThenPixelIsAdded() {
62-
testee.onPageLoaded(INVALID_URL, "title", 0L, 10L)
62+
testee.onPageLoaded(INVALID_URL, "title", 0L, 10L, true)
6363
verify(pageLoadedPixelDao, never()).add(any())
6464
}
6565
}

app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -223,10 +223,7 @@ open class BrowserActivity : DuckDuckGoActivity() {
223223
}
224224

225225
private val tabPagerAdapter by lazy {
226-
TabPagerAdapter(
227-
activity = this,
228-
swipingTabsFeature = swipingTabsFeature,
229-
)
226+
TabPagerAdapter(activity = this)
230227
}
231228

232229
private lateinit var toolbarMockupBinding: IncludeOmnibarToolbarMockupBinding
@@ -1012,6 +1009,7 @@ open class BrowserActivity : DuckDuckGoActivity() {
10121009

10131010
private fun onMoveToTabRequested(index: Int) {
10141011
tabPager.post {
1012+
tabPagerAdapter.currentTabIndex = index
10151013
tabPager.setCurrentItem(index, false)
10161014
}
10171015
}

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,7 @@ import com.duckduckgo.app.browser.omnibar.model.OmnibarPosition.TOP
205205
import com.duckduckgo.app.browser.refreshpixels.RefreshPixelSender
206206
import com.duckduckgo.app.browser.senseofprotection.SenseOfProtectionExperiment
207207
import com.duckduckgo.app.browser.session.WebViewSessionStorage
208+
import com.duckduckgo.app.browser.tabs.TabManager
208209
import com.duckduckgo.app.browser.urlextraction.UrlExtractionListener
209210
import com.duckduckgo.app.browser.viewstate.AccessibilityViewState
210211
import com.duckduckgo.app.browser.viewstate.AutoCompleteViewState
@@ -465,6 +466,7 @@ class BrowserTabViewModel @Inject constructor(
465466
private val senseOfProtectionExperiment: SenseOfProtectionExperiment,
466467
private val subscriptionsJSHelper: SubscriptionsJSHelper,
467468
private val onboardingDesignExperimentToggles: OnboardingDesignExperimentToggles,
469+
private val tabManager: TabManager,
468470
) : WebViewClientListener,
469471
EditSavedSiteListener,
470472
DeleteBookmarkListener,
@@ -1242,6 +1244,14 @@ class BrowserTabViewModel @Inject constructor(
12421244

12431245
override fun isDesktopSiteEnabled(): Boolean = currentBrowserViewState().isDesktopBrowsingMode
12441246

1247+
override fun isTabInForeground(): Boolean {
1248+
return if (swipingTabsFeature.isEnabled) {
1249+
tabId == tabManager.getSelectedTabId()
1250+
} else {
1251+
true
1252+
}
1253+
}
1254+
12451255
override fun closeCurrentTab() {
12461256
viewModelScope.launch {
12471257
removeCurrentTabFromRepository()

app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,13 @@ class BrowserWebViewClient @Inject constructor(
479479
if (url != ABOUT_BLANK) {
480480
start?.let { safeStart ->
481481
// TODO (cbarreiro - 22/05/2024): Extract to plugins
482-
pageLoadedHandler.onPageLoaded(it, navigationList.currentItem?.title, safeStart, currentTimeProvider.elapsedRealtime())
482+
pageLoadedHandler.onPageLoaded(
483+
url = it,
484+
title = navigationList.currentItem?.title,
485+
start = safeStart,
486+
end = currentTimeProvider.elapsedRealtime(),
487+
isTabInForeground = webViewClientListener?.isTabInForeground() ?: true,
488+
)
483489
shouldSendPagePaintedPixel(webView = webView, url = it)
484490
appCoroutineScope.launch(dispatcherProvider.io()) {
485491
if (duckPlayer.getDuckPlayerState() == ENABLED && duckPlayer.isSimulatedYoutubeNoCookie(uri)) {

app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,8 @@ interface WebViewClientListener {
8282
fun surrogateDetected(surrogate: SurrogateResponse)
8383
fun isDesktopSiteEnabled(): Boolean
8484

85+
fun isTabInForeground(): Boolean
86+
8587
fun loginDetected()
8688
fun dosAttackDetected()
8789
fun iconReceived(

app/src/main/java/com/duckduckgo/app/browser/pageloadpixel/PageLoadedHandler.kt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,10 @@ import com.squareup.anvil.annotations.ContributesBinding
2929
import javax.inject.Inject
3030
import kotlinx.coroutines.CoroutineScope
3131
import kotlinx.coroutines.launch
32+
import logcat.logcat
3233

3334
interface PageLoadedHandler {
34-
fun onPageLoaded(url: String, title: String?, start: Long, end: Long)
35+
fun onPageLoaded(url: String, title: String?, start: Long, end: Long, isTabInForeground: Boolean)
3536
}
3637

3738
@ContributesBinding(AppScope::class)
@@ -45,7 +46,7 @@ class RealPageLoadedHandler @Inject constructor(
4546
private val optimizeTrackerEvaluationRCWrapper: OptimizeTrackerEvaluationRCWrapper,
4647
) : PageLoadedHandler {
4748

48-
override fun onPageLoaded(url: String, title: String?, start: Long, end: Long) {
49+
override fun onPageLoaded(url: String, title: String?, start: Long, end: Long, isTabInForeground: Boolean) {
4950
appCoroutineScope.launch(dispatcherProvider.io()) {
5051
if (sites.any { UriString.sameOrSubdomain(url, it) }) {
5152
pageLoadedPixelDao.add(
@@ -58,6 +59,7 @@ class RealPageLoadedHandler @Inject constructor(
5859
),
5960
)
6061
}
62+
logcat { "$$$: Page load time: ${end - start}, foreground: $isTabInForeground" }
6163
}
6264
}
6365
}

app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import com.duckduckgo.app.tabs.model.TabRepository
2424
import com.duckduckgo.common.utils.DispatcherProvider
2525
import com.duckduckgo.di.scopes.ActivityScope
2626
import com.squareup.anvil.annotations.ContributesBinding
27+
import dagger.SingleInstanceIn
2728
import javax.inject.Inject
2829
import kotlinx.coroutines.withContext
2930
import logcat.LogPriority.INFO
@@ -50,6 +51,7 @@ interface TabManager {
5051
)
5152
}
5253

54+
@SingleInstanceIn(ActivityScope::class)
5355
@ContributesBinding(ActivityScope::class)
5456
class DefaultTabManager @Inject constructor(
5557
private val tabRepository: TabRepository,

app/src/main/java/com/duckduckgo/app/browser/tabs/adapter/FragmentStateAdapter.java

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@
4949
import androidx.viewpager2.widget.ViewPager2;
5050

5151
import com.duckduckgo.app.browser.tabs.TabManager;
52-
import com.duckduckgo.common.ui.tabs.SwipingTabsFeatureProvider;
5352

5453
import java.util.ArrayDeque;
5554
import java.util.ArrayList;
@@ -102,9 +101,6 @@ public abstract class FragmentStateAdapter extends RecyclerView.Adapter<Fragment
102101
@SuppressWarnings("WeakerAccess") // to avoid creation of a synthetic accessor
103102
final FragmentManager mFragmentManager;
104103

105-
@SuppressWarnings("WeakerAccess") // to avoid creation of a synthetic accessor
106-
final SwipingTabsFeatureProvider mSwipingTabsFeature;
107-
108104
// Fragment bookkeeping
109105
@SuppressWarnings("WeakerAccess") // to avoid creation of a synthetic accessor
110106
final LongSparseArray<Fragment> mFragments = new LongSparseArray<>();
@@ -131,28 +127,22 @@ public abstract class FragmentStateAdapter extends RecyclerView.Adapter<Fragment
131127
/**
132128
* @param fragmentActivity if the {@link ViewPager2} lives directly in a {@link
133129
* FragmentActivity} subclass.
134-
* @param swipingTabsFeature Feature flag to enable swiping tabs fixes
135130
*/
136131
public FragmentStateAdapter(
137-
@NonNull FragmentActivity fragmentActivity,
138-
SwipingTabsFeatureProvider swipingTabsFeature) {
132+
@NonNull FragmentActivity fragmentActivity) {
139133
this(fragmentActivity.getSupportFragmentManager(),
140-
fragmentActivity.getLifecycle(),
141-
swipingTabsFeature);
134+
fragmentActivity.getLifecycle());
142135
}
143136

144137
/**
145138
* @param fragmentManager of {@link ViewPager2}'s host
146139
* @param lifecycle of {@link ViewPager2}'s host
147-
* @param swipingTabsFeature Feature flag to enable swiping tabs fixes
148140
*/
149141
public FragmentStateAdapter(
150142
@NonNull FragmentManager fragmentManager,
151-
@NonNull Lifecycle lifecycle,
152-
SwipingTabsFeatureProvider swipingTabsFeature) {
143+
@NonNull Lifecycle lifecycle) {
153144
mFragmentManager = fragmentManager;
154145
mLifecycle = lifecycle;
155-
mSwipingTabsFeature = swipingTabsFeature;
156146
super.setHasStableIds(true);
157147
}
158148

@@ -186,6 +176,8 @@ public void onDetachedFromRecyclerView(@NonNull RecyclerView recyclerView) {
186176
*/
187177
public abstract @NonNull Fragment createFragment(int position);
188178

179+
public abstract @NonNull Boolean shouldPlaceFragmentInViewHolder(int position);
180+
189181
@NonNull
190182
@Override
191183
public final FragmentViewHolder onCreateViewHolder(@NonNull ViewGroup parent, int viewType) {
@@ -292,8 +284,10 @@ private void ensureFragment(int position) {
292284

293285
@Override
294286
public final void onViewAttachedToWindow(@NonNull final FragmentViewHolder holder) {
295-
placeFragmentInViewHolder(holder);
296-
gcFragments();
287+
if (shouldPlaceFragmentInViewHolder(holder.getBindingAdapterPosition())) {
288+
placeFragmentInViewHolder(holder);
289+
gcFragments();
290+
}
297291
}
298292

299293
/**

0 commit comments

Comments
 (0)