Skip to content

Ensure token refresh notification is triggered any time a new token is refreshed. b/77662386 #2562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 22, 2019

Conversation

charlotteliang
Copy link
Contributor

@charlotteliang charlotteliang commented Mar 15, 2019

Before there's this strange handler created to finish some logic of defaultTokenWithHandler: and it is only defined and passing in defaultTokenWithHandler: at two places, where the rest of the defaultTokenWithHandler: calls are not. This could cause issue because it contains critical token refresh code. Move the logic inside defaultTokenWithHandler: so it's always triggered and more readable.

So merge repetitive code and move inside defaultTokenWithHandler: and after that I notice fetchDefaultToken call is simply just calling defaultTokenWithHandler:, so remove it too as there are too many layers of calling default token.

Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

LGTM if we consider just minimal required changes.
Though it seems that the code of the class can be improved for better readability. @chliangGoogle I have some ideas on it which we can discuss separately.

Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

LGTM

@charlotteliang charlotteliang merged commit b34a278 into master Mar 22, 2019
@paulb777 paulb777 deleted the lc-more-iid-cleanup branch April 13, 2019 16:00
Corrob pushed a commit that referenced this pull request Apr 25, 2019
@firebase firebase locked and limited conversation to collaborators Oct 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants