Bug 1799230 - Use Non-Transitive R Classes

[fice-t]

According to Android documentation:

You should use non-transitive R classes to have faster builds for applications with multiple modules. Doing so helps prevent resource duplication by ensuring that each module's R class only contains references to its own resources, without pulling references from its dependencies. This leads to faster builds and the corresponding benefits of compilation avoidance.

Full clean build before (./gradlew --profile --rerun-tasks assembleDebug): ~14m 50s
Full clean build after (./gradlew --profile --rerun-tasks assembleDebug): ~12m 30s

I didn't test incremental compilation, but it should show stronger results on changes to modules that have resources that aren't used by (many) other modules.

This also appears to decrease the apk size of Fenix by ~1MB.

Pull Request checklist

• Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry or does not need one
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

• Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
• Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

GitHub Automation

Used by GitHub Actions.

[jonalmeida]

This is really cool. 🙂

Did you use the "Migrate to Non-Transitive R Classes" to generate these? I suspect not or partially.

EDIT: to answer my own question, the "Migrate to Non-Transitive R Classes" tool inlines the namespace for each resource reference as seen in the example patch below. Any reason for doing it manually over using the tool? 🙂

example patch

diff --git a/android-components/components/browser/errorpages/src/main/java/mozilla/components/browser/errorpages/ErrorPages.kt b/android-components/components/browser/errorpages/src/main/java/mozilla/components/browser/errorpages/ErrorPages.kt
index 703644dac0..8bbcfa8879 100644
--- a/android-components/components/browser/errorpages/src/main/java/mozilla/components/browser/errorpages/ErrorPages.kt
+++ b/android-components/components/browser/errorpages/src/main/java/mozilla/components/browser/errorpages/ErrorPages.kt
@@ -111,113 +111,113 @@ enum class ErrorType(
     ERROR_SECURITY_SSL(
         R.string.mozac_browser_errorpages_security_ssl_title,
         R.string.mozac_browser_errorpages_security_ssl_message,
-        imageNameRes = R.string.mozac_error_lock,
+        imageNameRes = mozilla.components.ui.icons.R.string.mozac_error_lock,
     ),
     ERROR_SECURITY_BAD_CERT(
         R.string.mozac_browser_errorpages_security_bad_cert_title,
         R.string.mozac_browser_errorpages_security_bad_cert_message,
-        imageNameRes = R.string.mozac_error_lock,
+        imageNameRes = mozilla.components.ui.icons.R.string.mozac_error_lock,
     ),

[fice-t]

I used the tool initially and then did some manual conversions afterwards where the inlined namespaces resulted in overly long lines or where there was duplication. The tool also missed a couple dependencies which I added.

Another possible benefit of non-transitive R classes is that if the main users of A-C use them as well, then the long resource prefixes could be removed (e.g. mozac_feature_addons_rating_content_description -> rating_content_description).

[gabrielluong]

I think we would prefer to import the path and use an appropriate alias instead of inlining the full path, but maybe this requires some discussion.

[fice-t]

Dealer's choice, but I think the utility of an alias makes more sense the more times it's used in a file. Using an alias just for one or two instances with no line length issue could be argued in the name of consistency, though.

[gabrielluong]

I think I feel strongly enough about this that I would suggest adding the imports. I would like to avoid inadvertently encouraging others to think it's okay to use the full namespace.

[gabrielluong]

Before proceeding with my suggested change, I will bring this change up with the team just for general awareness. I don't foresee any concerns coming out of it, but I just want to avoid encouraging any additional changes before knowing for sure if we'll accept the proposed changes.

[onemahon]

I'd agree, @gabrielluong - inlining the path seems unnecessarily messy. I'd personally prefer the style of having paths imported (and, optionally, include an alias for the import - particularly if there are name collisions that require that).

[onemahon]

Actually, I think I'd phrase my preference as even more strict: always import the path (up to but not including .R, so every resource can start with the standard R.- prefix), and allow optional aliases but only when doing so adds some useful clarity to which resource you're referencing.

Am I correct in believing that Android Studio adds the import statements for you automatically with the default text completion? If so, that seems like it would be the option that requires the least amount of thought while developing.

[fice-t]

Actually, I think I'd phrase my preference as even more strict: always import the path (up to but not including .R, so every resource can start with the standard R.- prefix)

I don't think that Kotlin allows this. When attempting to import e.g. mozilla.components.ui.colors instead of mozilla.components.ui.colors.R I get: Packages cannot be imported. According to the docs it seems like you can't import packages like you can in e.g. Python. One could use a wildcard, but that unnecessarily brings along all other references as well (which may result in more conflicts).

Am I correct in believing that Android Studio adds the import statements for you automatically with the default text completion?

I had to enable the option, but the option does exist. I don't think text completion for R classes works without first getting the import correct. though.

[kycn]

Sorry for the flow of ideas in the next couple of sentences. Just pointing out some possibilities (i imagine) on both hands 😄

I agree with using alias for the consistency. Though we have files with only one or two changes now, and seems reasonable to skip using alias for them; I believe it is possible in the future that people will refer to the resources more and may populate the files with more full.path.resource.files.

But this raises another concern: Ensuring newly added resources are using alias, not the full path, (cause the auto complete will use the full path) 😄

Apart from consistency advantage of use of alias, on the other hand, going on with what IDE is suggesting atm may not be a bad option, too. Just in case they bring new refactoring tools for those in the future and risk of alias not being recognized may be on the table.

[gabrielluong]

@fice-t could you file a bug for this in https://bugzilla.mozilla.org/enter_bug.cgi?product=Fenix? Ideally, we should have a way to trace back the PR to a bug/issue with some description of the task.

[fice-t]

OK, I created https://bugzilla.mozilla.org/show_bug.cgi?id=1799230.

[onemahon]

Was this part of the automatic conversion tool? There's a whole bunch of similar instances throughout the PR. Any idea why these got changed?

(Correct me if I'm wrong, but I thought both of these represent non-transitive resource path references, so I wouldn't expect there to be a performance difference. If that's the case, I tend to get a little nervous about automated changes whose purpose we don't have a clear justification for)

So, essentially:

1. Does this risk changing any runtime behavior? (e.g. referencing the wrong resource)
2. Does this actually add any value, or was it just automatically updated? (And, if so, do we know why the tool picked these changes?)

[fice-t]

The parts where the import name changes are manual changes that the automatic tool actually missed; these changes point the R reference to the proper class whereas before they were pointing to a different class that just so happened to pull in the correct R resource(s) upon dependency resolution (which no longer occurs for non-transitive R classes).

Correct me if I'm wrong, but I thought both of these represent non-transitive resource path references

The removed import is actually a transitive reference as the addons module's R doesn't directly contain the resource (in this case the color photonBlue40), and instead depends on the ui-colors module that contains the R class that contains the color resource.

Before: AddonsPermissionsAdapterTest -> addons R -> ui-colors R -> photonBlue40
After: AddonsPermissionsAdapterTest -> ui-colors R -> photonBlue40

Does this risk changing any runtime behavior? (e.g. referencing the wrong resource)

No, the changes were required to compile successfully with android.nonTransitiveRClass=true. I also checked that there were no other possible resources that the statements could have been referencing instead.

[onemahon]

Ahh, that all makes sense, I was indeed misinterpreting the context of non-transitive R classes. Appreciate the thorough rundown.

[onemahon]

Just left a couple comments, but I'm planning on leaving full review up to the experts - this is a really interesting idea, and a nice catch on the potential performance boost! Thanks for bringing this up!

[gabrielluong]

@fice-t I have already asked the team to weigh in and their feedback is already present here so I think we're okay with proceeding. In terms of full name vs alias, I am still in favour of going with alias for everything. Even if there is currently only 1 or 2 usages where it might make sense to just allow the full namespace path in a file, I think it would be better to future-proof ourselves by using the alias from the get-go since this will mitigate future refactoring if we suddenly had more usages of the R class in the file.

We won't be able to land this work immediately since this Thursday will be our code freeze for the upcoming release and the earliest time will be Monday. So, take your time as necessary and thank you again for your contribution!

[gabrielluong]

From my own testing:

Full clean build before (./gradlew --profile --rerun-tasks assembleDebug): ~4m 31s
Full clean build after (./gradlew --profile --rerun-tasks assembleDebug): ~3m 18s

[gabrielluong]

@fice-t Thank you again for the patch! I landed this #170