-
Notifications
You must be signed in to change notification settings - Fork 328
Bug 1870050 - Upgrade AGP and kotlin dsl version. #4797
Conversation
🧪 |
c9dfc79
to
4994045
Compare
c433151
to
7bb1c4a
Compare
f73c063
to
84e35cd
Compare
a9286ae
to
f7b9821
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look fine to me on the surface. I haven't run it locally, but nothing here looks suspicious enough to warrant that.
@@ -3,7 +3,7 @@ | |||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
|
|||
plugins { | |||
id "org.gradle.kotlin.kotlin-dsl" version "4.1.2" | |||
id "org.gradle.kotlin.kotlin-dsl" version "4.2.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo! Goodbye warnings!
Is there a way to move this version to the DependenciesPlugin or catalogs in the future? I wouldn't change your PR today, maybe in a future revision so we can land this sooner.
@@ -121,7 +121,7 @@ class FenixSnackbar private constructor( | |||
* Suppressing ComplexCondition. Yes it's unfortunately complex but that's the nature | |||
* of the snackbar handling by Android. It will be simpler once dynamic toolbar is always on. | |||
*/ | |||
@Suppress("ComplexCondition") | |||
@Suppress("ComplexCondition", "RestrictedApi") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we file an issue to remove access to what is causing this restricted API?
Same question for the one in fenix/app/src/main/java/org/mozilla/fenix/ext/Toolbar.kt
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -180,11 +180,13 @@ | |||
<service | |||
android:name=".downloads.DownloadService" | |||
android:exported="false" | |||
android:foregroundServiceType="dataSync" /> | |||
android:foregroundServiceType="dataSync" | |||
tools:ignore="ForegroundServicePermission" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we ignore this here? Is it because we request this permission at runtime? If so, can we leave that as a note here: "Permission requested at runtime."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a false positive because the required permission is in the merged manifest and not in the Focus manifest. An alternative would be adding the permissions explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would be better for future references to have the permissions declared explicitly instead of having suppressions ( that would likely be fixed in future lint releases)
Also sync lint version.
Skipping this parameter would result in a UnspecifiedRegisterReceiverFlag error.
The permissions already included the merged manifest, but linting shows error if not added directly.
lgtm! |
Merging this on Monday. |
Pull Request checklist
After merge
To download an APK when reviewing a PR (after all CI tasks finished running):
Checks
at the top of the PR page.firefoxci-taskcluster
group on the left to expand all tasks.build-apk-{fenix,focus,klar}-debug
task you're interested in.View task in Taskcluster
in the newDETAILS
section.GitHub Automation
https://bugzilla.mozilla.org/show_bug.cgi?id=1870050