Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[local_auth] Fix activity leak in LocalAuthPlugin #4449

Merged
merged 3 commits into from
Nov 19, 2021
Merged

[local_auth] Fix activity leak in LocalAuthPlugin #4449

merged 3 commits into from
Nov 19, 2021

Conversation

@0xZOne
Copy link
Member

@0xZOne 0xZOne commented Oct 25, 2021

Fixes flutter/flutter#92412

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.
@google-cla google-cla bot added the cla: yes label Oct 25, 2021
@0xZOne 0xZOne force-pushed the task/local_auth_leak branch from fe8aa2c to 37282bb Compare October 29, 2021 21:45
@stuartmorgan-g
Copy link
Contributor

@blasten Ping on this review
@0xZOne 0xZOne force-pushed the task/local_auth_leak branch from 37282bb to 2f80c02 Compare November 18, 2021 12:24
stuartmorgan-g
stuartmorgan-g approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

One small nit; once that's fixed I can add the second approval and land this.
packages/local_auth/android/src/main/java/io/flutter/plugins/localauth/LocalAuthPlugin.java Outdated
}

@VisibleForTesting
/*package*/ final Activity getActivity() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't check in commented-out code.
Copy link
Member Author

Choose a reason for hiding this comment

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

Please don't check in commented-out code.

Thank you for replying.

This note was added intentionally. Package level protection is the default, but adding the note there will be more clear.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't check in commented-out code.

This note was added intentionally.

I understand that you did it intentionally, but we don't check in things like this.

Package level protection is the default, but adding the note there will be more clear.

We don't comment standard language behavior.
Copy link
Member Author

Choose a reason for hiding this comment

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

Please don't check in commented-out code.

This note was added intentionally.

I understand that you did it intentionally, but we don't check in things like this.

Package level protection is the default, but adding the note there will be more clear.

We don't comment standard language behavior.

Thanks, I got it now.
stuartmorgan-g
stuartmorgan-g suggested changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

(Sorry, wrong option in the review UI; that wasn't meant to be an approval yet)
stuartmorgan-g
stuartmorgan-g approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM
@stuartmorgan-g stuartmorgan-g added last mile waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. labels Nov 19, 2021
@fluttergithubbot fluttergithubbot merged commit cbe6449 into flutter:master Nov 19, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 19, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 19, 2021
@engine-flutter-autoroll
cbe6449 [local_auth] Fix activity leak in LocalAuthPlugin (flutter/pl…
a9e538b 
…ugins#4449)
amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021
@0xZOne @amantoux
[local_auth] Fix activity leak in LocalAuthPlugin (flutter#4449)
947efac
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
@0xZOne @KyleFin
[local_auth] Fix activity leak in LocalAuthPlugin (flutter#4449)
195f3ad
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes p: local_auth platform-android waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.

4 participants

@0xZOne @stuartmorgan-g @blasten @fluttergithubbot