Tab swiping: Feature flag cache #5323
Conversation
| class IsSwipingTabsFeatureEnabled @Inject constructor( | ||
| swipingTabsFeature: SwipingTabsFeature, | ||
| ) { | ||
| private val isEnabled: Boolean = swipingTabsFeature.self().isEnabled() |
There was a problem hiding this comment.
are you sure this doens't call IO right when the class is instantiated? If it does (I think it does) we should change it.
Also, I would question why do you need this class anyway, you can more easily do the same (without the overhead of a whole new class) by using lazy in the caller (RealTabManager)
nit: also about the invoke() pattern, we don't use it very often and I think we should not use it. it's in general more difficult to get around when working with the IDE
There was a problem hiding this comment.
are you sure this doens't call IO right when the class is instantiated? If it does (I think it does) we should change it.
You mean when the SwipingTabsFeature is being initialized for the first time? I don’t think so, at least from what I saw when stepping through the code. But I couldn’t peek inside the generated store so I may be wrong. We can initialize the variable lazily, though, to be safe.
Also, I would question why do you need this class anyway, you can more easily do the same (without the overhead of a whole new class) by using lazy in the caller (RealTabManager)
I’m checking for the flag in multiple places (not here but in a subsequent PR), so I need to store the value somewhere for consistency.
Task/Issue URL: https://app.asana.com/0/1207418217763355/1208852760335259/f
Description
This PR just caches the feature flag value.
I’ve set the scope of the
IsSwipingTabsFeatureEnabledtoAppScopebecause it’s used inside theTabManager, which is used across 2 different activities (BrowserActivityandTabSwitcherActivity).Steps to test this PR
Testing optional.