Update to Gradle 6.1.1, Android Gradle plugin 4.0.1, Kotlin 1.4 and Coroutines 1.3.9. #8717
Conversation
…oroutines 1.3.9. This patch reintroduces the changes from PR mozilla-mobile#8360 that we reverted in PR mozilla-mobile#8623. Now the issues in Fenix are resolved and we can land this again.
pocmo
requested review from
Amejia481,
csadilek,
ekager,
grigoryk,
jonalmeida,
rocketsroger and
a team
as code owners
|
That's a lot of CODEOWNER spam right here. But it does touch a lot of areas... 😅 |
| @@ -143,7 +143,7 @@ class WebAppSiteControlsFeature( | |||
| } else { | |||
| @Suppress("Deprecation") | |||
| Notification.Builder(applicationContext).apply { | |||
| setPriority(NotificationCompat.PRIORITY_MIN) | |||
There was a problem hiding this comment.
nit: Is not always better to use the NotificationCompat version?
There was a problem hiding this comment.
If your builder is also a compat builder yes. But here in this part of the code it's a normal builder and then lint doesn't seem to like if you use the compat class.
| @@ -84,6 +85,7 @@ class AndroidAssetFinder { | |||
| } | |||
| } | |||
|
|
|||
| @SuppressLint("PackageManagerGetSignatures") | |||
| private fun PackageManager.getPackageSignatureInfo(packageName: String): PackageInfo? { | |||
There was a problem hiding this comment.
Should we open an issue for this or the suppressing is just fine? I'm not completely aware of the context of the suppression but the lint description looks a bit spooky
There was a problem hiding this comment.
The issue mentioned was already fixed in Android 4+. I am not sure why the lint check didn't complain previously. In our cases we always look at all certificates, so that seems fine.
| @@ -172,6 +173,7 @@ object AccountSharing { | |||
| * but not signature rotation. | |||
| * @return A certificate SHA256 fingerprint, if one could be reliably obtained. | |||
| */ | |||
| @SuppressLint("PackageManagerGetSignatures") | |||
| fun getSignaturePreAPI28(packageManager: PackageManager, packageName: String): String? { | |||
There was a problem hiding this comment.
This code looks similar to what we have in AndroidAssetFinder maybe we should file a issue to reuse and I have one.
There was a problem hiding this comment.
Yeah, although, afaik in a discussion with @grigoryk we said that we could get rid of this. I'll file something for that.
| import java.lang.IllegalStateException | ||
| import org.mozilla.geckoview.WebNotification as GeckoViewWebNotification | ||
|
|
||
| @RunWith(AndroidJUnit4::class) |
There was a problem hiding this comment.
We don't need these tests anymore?
There was a problem hiding this comment.
We still have GeckoWebNotificationDelegate.kt
There was a problem hiding this comment.
@jonalmeida asked this in an earlier version of this PR, see here: #8359
pocmo
added
🛬 needs landing
This patch reintroduces the changes from PR #8360 that we reverted in PR #8623. Now the issues in
Fenix are resolved and we can land this again.