Skip to content

Conversation

@charlotteliang
Copy link
Contributor

@charlotteliang charlotteliang commented Jul 17, 2020

The token branch had a strange 'git rebase' conflicts problem today after I merged from master. Will try to push to a new branch (#6103) so all the changes are not lost and I will start from a new dev branch with smaller number of commits.

Please ignore this one.

…ceIDTokenInfo and FIRInstanceIDAPNSInfo class name
charlotteliang and others added 5 commits July 17, 2020 15:10
* Fix unguarded multithreaded access

* Update changelog

* fix typo in doc comments

* fix nullability build error

* Serialize via FIRAuthGlobalWorkQueue

* remove unused code
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Some initial comments. Will continue review later ...

// Notify InstanceID that APNS Token has been set.
NSDictionary *userInfo = @{kFIRMessagingAPNSTokenType : @(type)};
// TODO(chliang) This is sent to InstanceID in case users are still using the deprecated SDK.
// Should be safe to remove once InstanceID is removed.
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't users be using InstanceID long after we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No if we remove it completely from firebase-ios-sdk. if they use previous version of IID it's probably fine as messaging still kept those communications.

completion:^(NSString *_Nullable FCMToken, NSError *_Nullable error) {
if (error) {
FIRMessagingLoggerError(kFIRMessagingMessageCodeMessaging012,
@"The unsubscription operation failed due to "
Copy link
Member

Choose a reason for hiding this comment

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

s/unsubscription/unsubscribe/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will have to adjust integration test with it as well. The same as the subscription message. I can do it in a separate PR so we can merge in g3 to fix integration test as this PR might not be checked in to master for a while.

@charlotteliang charlotteliang deleted the token branch July 22, 2020 00:59
@firebase firebase locked and limited conversation to collaborators Aug 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.