Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Set FLAG_IMMUTABLE for all PendingIntents starting from Android 23 #11669

Merged
merged 2 commits into from
Feb 7, 2022
Merged

Set FLAG_IMMUTABLE for all PendingIntents starting from Android 23 #11669

merged 2 commits into from
Feb 7, 2022

Conversation

Mugurell
Copy link
Contributor

@Mugurell Mugurell commented Feb 4, 2022

Showing that this fixed the 6346 Focus issue

DownloadOnFocus.mp4

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.

Amejia481
Amejia481 approved these changes Feb 4, 2022
View reviewed changes
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ !

docs/changelog.md
@@ -11,6 +11,9 @@ permalink: /changelog/
* [Gecko](https://github.com/mozilla-mobile/android-components/blob/main/buildSrc/src/main/java/Gecko.kt)
* [Configuration](https://github.com/mozilla-mobile/android-components/blob/main/.config.yml)

* **support-utils**
* 🌟️️ **Add a `PendingUtils.defaultFlags` property to ease setting PendingIntent mutability as required for Android 31+.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Extra line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
Agree the diff looks a bit interesting but the end result seems like what we'd expect.
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

@Mugurell Mugurell added the 🛬 needs landing PRs that are ready to land label Feb 7, 2022
@Mugurell
Copy link
Contributor Author

Mugurell commented Feb 7, 2022

Merging failed because of

[vcs 2022-02-07T07:15:27.606Z] executing ['git', 'clone', 'https://github.com/mozilla-mobile/android-components', '/builds/worker/checkouts/src']
[vcs 2022-02-07T07:15:27.608Z] Cloning into '/builds/worker/checkouts/src'...
[vcs 2022-02-07T07:15:28.334Z] error: RPC failed; curl 56 GnuTLS recv error (-54): Error in the pull function.
[vcs 2022-02-07T07:15:28.334Z] fatal: The remote end hung up unexpectedly
[vcs 2022-02-07T07:15:28.334Z] fatal: early EOF
[vcs 2022-02-07T07:15:28.335Z] fatal: index-pack failed

Trying again.

@Mugurell Mugurell removed the 🛬 needs landing PRs that are ready to land label Feb 7, 2022
@Mugurell Mugurell closed this Feb 7, 2022
@Mugurell Mugurell reopened this Feb 7, 2022
@Mugurell Mugurell added the 🛬 needs landing PRs that are ready to land label Feb 7, 2022
@Mugurell
Copy link
Contributor Author

Mugurell commented Feb 7, 2022

Although now complete-pr is green Mergify / Queue: Embarked in merge train still sees the old status and avoids merging. - https://github.com/mozilla-mobile/android-components/pull/11669/checks?check_run_id=5089353318
Trying to set the landing flag again.

@Mugurell Mugurell added 🛬 needs landing PRs that are ready to land and removed 🛬 needs landing PRs that are ready to land labels Feb 7, 2022
@Mugurell
Copy link
Contributor Author

Mugurell commented Feb 7, 2022

Rebasing locally to change the commit ids and try again.

@Mugurell Mugurell removed the 🛬 needs landing PRs that are ready to land label Feb 7, 2022
@Mugurell Mugurell force-pushed the pendingIntentMutability branch from 5a820a1 to 170e4b6 Compare February 7, 2022 10:25
@Mugurell Mugurell added the 🛬 needs landing PRs that are ready to land label Feb 7, 2022
@mergify mergify bot merged commit ec02018 into mozilla-mobile:main Feb 7, 2022
@Mugurell Mugurell deleted the pendingIntentMutability branch February 7, 2022 11:07
@Mugurell Mugurell mentioned this pull request Feb 9, 2022
Closed
@sarah541 sarah541 linked an issue Feb 11, 2022 that may be closed by this pull request
Make sure we are indicating the mutability flag for PendingIntent #11314
Closed
@Mugurell Mugurell mentioned this pull request Mar 15, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Amejia481 Amejia481 Amejia481 approved these changes

@csadilek csadilek Awaiting requested review from csadilek csadilek is a code owner

@grigoryk grigoryk Awaiting requested review from grigoryk

@jonalmeida jonalmeida Awaiting requested review from jonalmeida jonalmeida is a code owner

@rocketsroger rocketsroger Awaiting requested review from rocketsroger rocketsroger is a code owner

Assignees
No one assigned
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make sure we are indicating the mutability flag for PendingIntent
2 participants
@Mugurell @Amejia481