16900 make navgraph inflation asynchronous (#18889)
* For #16900: implement async navgraph inflation For #16900: removed nav graph from xml For #16900: inflate navGraph programatically For #16900: Made NavGraph inflation asynchronous For #16900: Changed to block with runBlocking For #16900: Refactored blocking call into a function For 16900: NavGraph inflation is now async We now attach the nav graph (or check if its attached) on every nav call ( an extension function for NavController). This is done by checking the value of the job stored in PerfNavController.map which keeps track of the job with the NavController as a Key. If the job hasn't been completed, it will block the main thread until the job is done. The job itself is responsible for attaching the navgraph to the navcontroller (and the inflation of the latter too) For 16900: rebased upstream master For 16900: Rebase on master For #16900: Fixed Async Navgraph navigation per review comments. 1)The Asynchronous method is now found in NavGraphProvider.kt. It creates a job on the IO dispatcher 2)The Job is tracked through a WeakHashMap from Controller --> NavGraph 3)The Coroutine scope doesn't use MainScope() anymore 4)The Coroutine is cancelled if the Activity is destroyed 5)The tests mockk the blockForNavGraphInflation method through the FenixReoboelectricTestApplication instead of calling the mock every setup() For #16900: inflateNavGraphAsync now takes navController For #16900: Pass lifecycleScope to NavGraphProvider For #16900: removed unused mock For #16900: Added linter rules for navigate calls We need linting rules to make sure no one calls the NavController.navigate() methods For #16900: Added TestRule to help abstract the mocks in the code For 16900: Fix linting problems For #16900: Cleaned duplicated code in tests For #16900: cleaned up NavGraphTestRule for finished test For #16900: had to revert an accidentally edited file For #16900: rebased master * For #16900: Review nits for async navgraph This is composed of squash commits, the original messages can be found below: -> DisableNavGraphProviderAssertionRule + kdoc. Use test rule in RobolectricApplication. Fix failing CrashReporterControllerTest Fix blame by -> navigate in tests. This commit was generated by the following commands only: ``` find app/src/test -type f -exec sed -i '' "/import org.mozilla.fenix.ext.navigateBlockingForAsyncNavGraph/d" {} \; find app/src/test -type f -exec sed -i "" "s/navigateBlockingForAsyncNavGraph/navigate/g" {} \; git checkout app/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker ``` Fix various blame This is expected to be squashed into the first commit so, if so, it'd fix the blame. Move test rule to helpers pkg. add missing license header Add import change I missed fix unused imports Replace robolectricTestrunner with test rule. Improve navGraphProvider docs Remove unnecessary rule as defined by robolectric. add clarifying comment to robolectric remove unnecessary space * For #16900: nit fixes for MozillaNavigateCheck and lint fixes 3 squash commits: *Changed violation message and fixed the lint rule for MozillaNavigateCheck *Added suppression to NavController.kt *Fixed detekt violations * For 16900: Fixed failing tests Co-authored-by: Michael Comella <michael.l.comella@gmail.com>upstream-sync
MarcLeclair
3 years ago
committed by
GitHub
parent
973c891c5e
commit
990bfa7e6d
@ -0,0 +1,59 @@
|
||||
/* 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 org.mozilla.fenix.perf
|
||||
|
||||
import androidx.lifecycle.LifecycleCoroutineScope
|
||||
import androidx.navigation.NavController
|
||||
import java.util.WeakHashMap
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.Job
|
||||
import kotlinx.coroutines.launch
|
||||
import org.mozilla.fenix.R
|
||||
|
||||
/**
|
||||
* This class asynchronously loads the navigation graph XML. This is a performance optimization:
|
||||
* large nav graphs take ~29ms to inflate on the Moto G5 (#16900). This also seemingly prevents the
|
||||
* HomeFragment layout XML from being unnecessarily inflated when it isn't used, improving perf by
|
||||
* ~148ms on the Moto G5 for VIEW start up (#18245) though it was unintentional and we may wish to
|
||||
* implement more intentional for that.
|
||||
*
|
||||
* This class is defined as an Object, rather than as a class instance in our Components, because
|
||||
* it needs to be called by the [NavController] extension function which can't easily access Components.
|
||||
*
|
||||
* To use this class properly, [inflateNavGraphAsync] must be called first before
|
||||
* [blockForNavGraphInflation] using the same [NavController] instance.
|
||||
*/
|
||||
object NavGraphProvider {
|
||||
|
||||
// We want to store member state on the NavController. However, there is no way to do this.
|
||||
// Instead, we store state as part of a map: NavController instance -> State. In order to
|
||||
// garbage collect our data when the NavController is no longer relevant, we use a WeakHashMap.
|
||||
private val map = WeakHashMap<NavController, Job>()
|
||||
|
||||
fun inflateNavGraphAsync(navController: NavController, lifecycleScope: LifecycleCoroutineScope) {
|
||||
val inflationJob = lifecycleScope.launch(Dispatchers.IO) {
|
||||
val inflater = navController.navInflater
|
||||
navController.graph = inflater.inflate(R.navigation.nav_graph)
|
||||
}
|
||||
|
||||
map[navController] = inflationJob
|
||||
}
|
||||
|
||||
/**
|
||||
* The job should block the main thread if it isn't completed so that the NavGraph can be loaded
|
||||
* before any navigation is done.
|
||||
*
|
||||
* [inflateNavGraphAsync] must be called before this method.
|
||||
*
|
||||
* @throws IllegalStateException if [inflateNavGraphAsync] wasn't called first for this [NavController]
|
||||
*/
|
||||
fun blockForNavGraphInflation(navController: NavController) {
|
||||
val inflationJob = map[navController] ?: throw IllegalStateException("Expected " +
|
||||
"`NavGraphProvider.inflateNavGraphAsync` to be called before this method with the same " +
|
||||
"`NavController` instance. If this occurred in a test, you probably need to add the " +
|
||||
"DisableNavGraphProviderAssertionRule.")
|
||||
runBlockingIncrement { inflationJob.join() }
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,43 @@
|
||||
/* 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 org.mozilla.fenix.helpers
|
||||
|
||||
import io.mockk.every
|
||||
import io.mockk.mockkObject
|
||||
import io.mockk.unmockkObject
|
||||
import org.junit.rules.TestWatcher
|
||||
import org.junit.runner.Description
|
||||
import org.mozilla.fenix.perf.NavGraphProvider
|
||||
|
||||
/**
|
||||
* Disables the call order assertions defined by the [NavGraphProvider] for use in testing.
|
||||
* This is necessary because unit tests generally don't follow the application lifecycle and thus
|
||||
* call the methods out of order, causing an assertion to be thrown unexpectedly. You may need to
|
||||
* apply this rule if you see the following exception in your test:
|
||||
*
|
||||
* Unfortunately, JUnit 4 discourages setting test state globally so we apply this to each test that
|
||||
* has the failure rather than disabling it globally.
|
||||
*/
|
||||
class DisableNavGraphProviderAssertionRule : TestWatcher() {
|
||||
|
||||
// public for code reuse.
|
||||
fun setUp() {
|
||||
mockkObject(NavGraphProvider)
|
||||
every { NavGraphProvider.blockForNavGraphInflation(any()) } returns Unit
|
||||
}
|
||||
|
||||
// public for code reuse.
|
||||
fun tearDown() { //
|
||||
unmockkObject(NavGraphProvider)
|
||||
}
|
||||
|
||||
override fun starting(description: Description?) {
|
||||
setUp()
|
||||
}
|
||||
|
||||
override fun finished(description: Description?) {
|
||||
tearDown()
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,50 @@
|
||||
/* 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 https://mozilla.org/MPL/2.0/. */
|
||||
|
||||
package org.mozilla.fenix.detektrules.perf
|
||||
|
||||
import io.gitlab.arturbosch.detekt.api.CodeSmell
|
||||
import io.gitlab.arturbosch.detekt.api.Config
|
||||
import io.gitlab.arturbosch.detekt.api.Debt
|
||||
import io.gitlab.arturbosch.detekt.api.Entity
|
||||
import io.gitlab.arturbosch.detekt.api.Issue
|
||||
import io.gitlab.arturbosch.detekt.api.Rule
|
||||
import io.gitlab.arturbosch.detekt.api.Severity
|
||||
import org.jetbrains.kotlin.psi.KtCallExpression
|
||||
|
||||
private const val VIOLATION_MSG = "If you named a method `navigate`, please rename it to something a bit" +
|
||||
"more specific such as `navigateTo...`. However, if you are trying to invoke the Android NavController.navigate" +
|
||||
"please use `org.mozilla.fenix.ext.NavController.navigateBlockingForAsyncNavGraph`" +
|
||||
"instead. Because using `navigate` directly in the code can lead to the NavGraph not being loaded" +
|
||||
"since it relies on a blocking call done in `navigateBlockingForAsyncNavGraph`."
|
||||
|
||||
/**
|
||||
* A check to prevent the use of `navController.navigate`. Since the NavGraph is loaded dynamically
|
||||
* and asynchronously, there is a check in place to make sure the graph is loaded by wrapping the
|
||||
* `navigate` function. However, using it directly might lead to code that needs the NavGraph being
|
||||
* called before it being inflated.
|
||||
*/
|
||||
class MozillaNavigateCheck(config: Config = Config.empty) : Rule(config) {
|
||||
|
||||
override val issue = Issue(
|
||||
"MozillaNavigateCheck",
|
||||
Severity.Performance,
|
||||
"Prevents us from calling `navController.navigate` instead of the functions that" +
|
||||
"wrap it",
|
||||
Debt.TEN_MINS
|
||||
)
|
||||
|
||||
|
||||
override fun visitCallExpression(expression: KtCallExpression) {
|
||||
super.visitCallExpression(expression)
|
||||
|
||||
val calledMethod = expression.calleeExpression?.firstChild?.node?.chars
|
||||
|
||||
//We check for the navigate method and we have to ignore our extension function file, since
|
||||
//we call navigate there
|
||||
if (calledMethod == "navigate" ) {
|
||||
report(CodeSmell(issue, Entity.from(expression), VIOLATION_MSG))
|
||||
}
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue