diff --git a/mozilla-lint-rules/src/main/java/org/mozilla/fenix/lintrules/ConstraintLayoutPerfDetector.kt b/mozilla-lint-rules/src/main/java/org/mozilla/fenix/lintrules/ConstraintLayoutPerfDetector.kt new file mode 100644 index 000000000..7ae61bce5 --- /dev/null +++ b/mozilla-lint-rules/src/main/java/org/mozilla/fenix/lintrules/ConstraintLayoutPerfDetector.kt @@ -0,0 +1,227 @@ +/* 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.lintrules + +import com.android.resources.ResourceFolderType +import com.android.tools.lint.detector.api.Category +import com.android.tools.lint.detector.api.Context +import com.android.tools.lint.detector.api.Implementation +import com.android.tools.lint.detector.api.Issue +import com.android.tools.lint.detector.api.ResourceXmlDetector +import com.android.tools.lint.detector.api.Scope +import com.android.tools.lint.detector.api.Severity +import com.android.tools.lint.detector.api.XmlContext +import org.w3c.dom.Element + +private val FRAMEWORK_ELEMENTS = setOf( + "androidx.constraintlayout.widget.ConstraintLayout", + + // Android framework views that extend ConstraintLayout + "androidx.constraintlayout.motion.widget.MotionLayout" +) +private val FRAMEWORK_ELEMENTS_BULLETED_LIST = FRAMEWORK_ELEMENTS.map { "- `$it`" }.joinToString("\n") + +private const val FENIX_PREFIX = "org.mozilla.fenix" +private const val AC_PREFIX = "mozilla.components" + +/** + * Custom views that we wrote that extend ConstraintLayout. + * + * This data was manually added from the Type Hierarchy IDE action. Last update: 8/5/2020. + * In the future, we could generate this list statically to prevent transcription errors and + * errors from refactors. + */ +private val CUSTOM_VIEW_ELEMENTS = setOf( + "$FENIX_PREFIX.library.LibrarySiteItemView", + "$FENIX_PREFIX.settings.deletebrowsingdata.DeleteBrowsingDataItem", + "$FENIX_PREFIX.trackingprotection.SwitchWithDescription", + "$FENIX_PREFIX.trackingprotection.TrackingProtectionCategoryItem", + + "$AC_PREFIX.feature.readerview.view.ReaderViewControlsBar", + "$AC_PREFIX.feature.findinpage.view.FindInPageBar", + "$AC_PREFIX.feature.prompts.login.LoginSelectBar", + "$AC_PREFIX.ui.widgets.WidgetSiteItemView", + "$AC_PREFIX.browser.toolbar.display.DisplayToolbarView" +) +private val CUSTOM_VIEW_ELEMENTS_BULLETED_LIST = CUSTOM_VIEW_ELEMENTS.map { "- `$it`" }.joinToString("\n") + +private val ALL_APPLICABLE_ELEMENTS = FRAMEWORK_ELEMENTS + CUSTOM_VIEW_ELEMENTS + +private const val FRAMEWORK_ISSUE_DESCRIPTION = "2+ ConstraintLayouts in one hierarchy can be inefficient" +private const val FRAMEWORK_ISSUE_MESSAGE = "Flatten the view hierarchy by using one `ConstraintLayout`, " + + "if possible. If the alternative is several nested `ViewGroup`, it may not help performance and " + + "this may be worth suppressing." +private val FRAMEWORK_ISSUE_EXPLANATION = """`ConstraintLayout`'s main performance benefit is + that it enables devs to define layouts with very little view nesting. However, they are slow to + inflate so we only want to use as many as are strictly necessary. In most cases, this is one: a + layout can have a ConstraintLayout defined at the root and the entire hierarchy can be a direct + descendant of it. + + This check will report if there is more than one ConstraintLayout, or a view that extends it, + defined in a single layout file to remind the implementor of these performance concerns. + + **WARNING: replacing a ConstraintLayout with several nested ViewGroups is unlikely to improve + performance. Remember, a goal is to reduce nesting. If you're unable to remove the extra + ConstraintLayout(s) without doing this, it's probably better to suppress this violation.** + + Here are **known cases where ConstraintLayouts are unnecessarily nested and the alternatives:** + - To set a background color. Instead, use a `` of the appropriate size directly in the ConstraintLayout + - To change visibility on a group of sub-views. Instead, use `androidx.constraintlayout.widget.Group` + + There are a few cases the perf team is not 100% sure of the best solution - let us know if you + have better solutions: + - ScrollView can only have one child so perhaps making it a ConstraintLayout is fine, despite nesting + - a11y may rely on nesting views. Maybe `Group` solves this problem? + + The following views that are, or that extend, ConstraintLayout are inspected in this check: +""".trimIndent() + .lines() + .joinToStringRemovingManualWrapping() + + "\n$FRAMEWORK_ELEMENTS_BULLETED_LIST\n\n" + +private val CUSTOM_VIEW_ISSUE_DESCRIPTION = "Caution regarding custom views extending `ConstraintLayout`" +private val CUSTOM_VIEW_ISSUE_MESSAGE = "Custom views extending `ConstraintLayout` are less " + + "efficient because they cannot share other `ConstraintLayout` defined in file." +private val CUSTOM_VIEW_ISSUE_EXPLANATION = """`ConstraintLayout` is slow to inflate so we + should aim to have as few as possible. However, the performance team doesn't have a good generic + solution for when custom views extend ConstraintLayout because the encapsulation may be worth + it and the replacement may not be more performant. Therefore, we warn the implementor that this + may be undesireable but don't fail the build on it. + + More generic information about the performance concerns of ConstraintLayout is found below, + copied from our more strict ConstraintLayout check. + + The following additional views are inspected for this less strict check: +""".trimIndent() + .lines() + .joinToStringRemovingManualWrapping() + + "\n$CUSTOM_VIEW_ELEMENTS_BULLETED_LIST\n\n" + + "---\n\n" + // divider + FRAMEWORK_ISSUE_EXPLANATION + +/** + * An issue where multiple ConstraintLayouts that are defined by the framework are detected in the + * same file. This is distinct from [CUSTOM_VIEW_ISSUE] because that one handles custom views. + * We don't want to combine them because solving a custom view ConstraintLayout problem rarely has + * a clear cut solution and we don't want to fail the build on checks that aren't clear cut. + */ +private val FRAMEWORK_ISSUE = Issue.create( + id = "MozMultipleConstraintLayouts", + briefDescription = FRAMEWORK_ISSUE_DESCRIPTION, + explanation = FRAMEWORK_ISSUE_EXPLANATION, + category = Category.PERFORMANCE, + priority = 8, // UseCompoundDrawables is a 6 and I think this is much more impactful. + severity = Severity.ERROR, + implementation = Implementation( + ConstraintLayoutPerfDetector::class.java, + Scope.RESOURCE_FILE_SCOPE + ) +) + +/** + * An issue where multiple ConstraintLayouts, that may also be defined by us as custom views, are + * detected in the same file. See [FRAMEWORK_ISSUE] for why these are distinct. + */ +private val CUSTOM_VIEW_ISSUE = Issue.create( + id = "MozMultipleConstraintLayoutsAndCustomViews", + briefDescription = CUSTOM_VIEW_ISSUE_DESCRIPTION, + explanation = CUSTOM_VIEW_ISSUE_EXPLANATION, + category = Category.PERFORMANCE, + priority = 6, // UseCompoundDrawables is a 6 and this seems more impactful but less actionable. + severity = Severity.WARNING, + implementation = Implementation( + ConstraintLayoutPerfDetector::class.java, + Scope.RESOURCE_FILE_SCOPE + ) +) + +/** + * A custom lint detector that helps us ensure we're using ConstraintLayout performantly. + * + * This detector has a few shortcomings: + * - it will not notice if a ConstraintLayout is in the same view hierarchy at runtime but added + * from a different file (e.g. via ) + * - it will not notice if a ConstraintLayout is added at runtime + * - if we add additional views that extend ConstraintLayout, this check won't know about them + * (though we could generate this information and include it in this check via static analysis) + */ +class ConstraintLayoutPerfDetector : ResourceXmlDetector() { + + companion object { + val ISSUES = listOf( + FRAMEWORK_ISSUE, + CUSTOM_VIEW_ISSUE + ) + } + + private var frameworkConstraintLayoutCountInFile = 0 + private var customViewConstraintLayoutCountInFile = 0 + + override fun appliesTo(folderType: ResourceFolderType): Boolean = folderType == ResourceFolderType.LAYOUT + override fun getApplicableElements(): Collection? = ALL_APPLICABLE_ELEMENTS + + override fun beforeCheckFile(context: Context) { + frameworkConstraintLayoutCountInFile = 0 + customViewConstraintLayoutCountInFile = 0 + } + + override fun visitElement(context: XmlContext, element: Element) { + super.visitElement(context, element) + + // This scope is unideal: if the root element is a ConstraintLayout and is suppressed, all + // ConstraintLayout children will also be suppressed. If more ConstraintLayout children are + // added, the root ConstraintLayout will suppress them too. Ideally, we'd want a suppression + // to only affect a node and not its children. + val scope = element + val location = context.getElementLocation(element) + + // For these inner visit methods, we only warn on 2+ violations because: + // - we avoid having 2 warnings for the first intuitive violation (i.e. 2 ConstraintLayouts + // in a file is one violation on the second ConstraintLayout) + // - of the suppression & scope issues mentioned above. The first found element is most + // likely to be a parent of other ConstraintLayouts because of the traversal order so if + // there's one node we don't report on, that's the best one. + // + // The fact that we use two different issues can create a weird situation where if a custom + // view is a sibling above a ConstraintLayout, it won't report but if it's below the + // ConstraintLayout, it will report. I don't think it's worth the complexity to address this + // and in-IDE inspection seems to break when we use afterCheckFile, which would be useful for this. + fun visitFrameworkElement() { + frameworkConstraintLayoutCountInFile += 1 + if (frameworkConstraintLayoutCountInFile > 1) { + context.report(FRAMEWORK_ISSUE, scope, location, FRAMEWORK_ISSUE_MESSAGE) + } + } + + fun visitCustomViewElement() { + customViewConstraintLayoutCountInFile += 1 + + // We want to report if there are too many ConstraintLayouts in the file. This issue + // intentionally includes both framework CL and our custom view CL so we add both counts. + if (frameworkConstraintLayoutCountInFile + customViewConstraintLayoutCountInFile > 1) { + context.report(CUSTOM_VIEW_ISSUE, scope, location, CUSTOM_VIEW_ISSUE_MESSAGE) + } + } + + when (element.tagName) { + in FRAMEWORK_ELEMENTS -> visitFrameworkElement() + in CUSTOM_VIEW_ELEMENTS -> visitCustomViewElement() + else -> throw IllegalStateException("Unexpected tag seen: ${element.tagName}") + } + } +} + +/** + * Takes paragraphs wrapped by hand and combines them into a single line, preserving white space + * between paragraphs. + */ +private fun List.joinToStringRemovingManualWrapping(): String = fold("") { acc, str -> + val toAppend = when { + str.isBlank() -> "\n\n" // new paragraph + str.trim().startsWith("-") -> "\n$str" // bulleted list + else -> str + } + acc + toAppend +} diff --git a/mozilla-lint-rules/src/main/java/org/mozilla/fenix/lintrules/LintIssueRegistry.kt b/mozilla-lint-rules/src/main/java/org/mozilla/fenix/lintrules/LintIssueRegistry.kt index 920c0aeda..7f3945eb9 100644 --- a/mozilla-lint-rules/src/main/java/org/mozilla/fenix/lintrules/LintIssueRegistry.kt +++ b/mozilla-lint-rules/src/main/java/org/mozilla/fenix/lintrules/LintIssueRegistry.kt @@ -18,5 +18,5 @@ class LintIssueRegistry : IssueRegistry() { AndroidSrcXmlDetector.ISSUE_XML_SRC_USAGE, TextViewAndroidSrcXmlDetector.ISSUE_XML_SRC_USAGE, ImageViewAndroidTintXmlDetector.ISSUE_XML_SRC_USAGE - ) + ) + ConstraintLayoutPerfDetector.ISSUES }