Skip to content

Merge IID removal code to master#7814

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 32 commits into from
Mar 31, 2021
Merged

Merge IID removal code to master #7814

merged 32 commits into from
Mar 31, 2021

Conversation

charlotteliang
Copy link
Contributor

@charlotteliangcharlotteliang commented Mar 30, 2021

The chen/fm-master branch contains all the changes required to remove IID from Messaging.
The code change is targeting in M94 that will not include the part we remove the IID dependency in podspec. We will remove the dependency at the I/O breaking change.

@google-oss-bot
Copy link

google-oss-bot commented Mar 30, 2021

Coverage Report

Affected SDKs

  • FirebaseMessaging-iOS-FirebaseMessaging.framework

    SDK overall coverage changed from 62.08% (a7ada99) to 66.00% (f8ca45f) by +3.91%.

    Click to show coverage changes in 21 files.
    FilenameBase (a7ada99)Head (f8ca45f)Diff
    FIRMessaging.m68.63%65.92%-2.70%
    FIRMessagingAPNSInfo.m?87.23%?
    FIRMessagingAuthKeychain.m?88.64%?
    FIRMessagingAuthService.m?86.67%?
    FIRMessagingBackupExcludedPlist.m?91.78%?
    FIRMessagingCheckinPreferences.m?98.18%?
    FIRMessagingCheckinService.m?91.76%?
    FIRMessagingCheckinStore.m?79.87%?
    FIRMessagingKeychain.m?93.89%?
    FIRMessagingPendingTopicsList.m90.65%89.93%-0.72%
    FIRMessagingPubSub.m53.01%53.85%+0.83%
    FIRMessagingRemoteNotificationsProxy.m62.97%73.64%+10.67%
    FIRMessagingRmqManager.m58.95%58.57%-0.38%
    FIRMessagingSyncMessageManager.m76.92%73.08%-3.85%
    FIRMessagingTokenDeleteOperation.m?9.64%?
    FIRMessagingTokenFetchOperation.m?75.32%?
    FIRMessagingTokenInfo.m?82.78%?
    FIRMessagingTokenManager.m?42.15%?
    FIRMessagingTokenOperation.m?96.84%?
    FIRMessagingTokenStore.m?68.42%?
    FIRMessagingUtilities.m44.23%59.89%+15.65%

Test Logs

Copy link
Member

@paulb777paulb777 left a comment

Choose a reason for hiding this comment

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

Just a few nits from a cursory review since this has already been reviewed in the branch

@@ -42,7 +42,7 @@ device, and it is completely free.
'Firebase/InstanceID/Public/*.h',
'FirebaseInstallations/Source/Library/Private/*.h',
]
s.requires_arc = base_dir + 'Sources/*.m'
s.requires_arc = base_dir + 'Sources/**/*.m'
Copy link
Member

Choose a reason for hiding this comment

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

Now that protobuf's non-arc files are gone, this line could be deleted since requires_arc is the default

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Done

s.dependency 'FirebaseInstanceID', '~> 7.0'
s.dependency 'FirebaseInstallations', '~> 7.0'
s.dependency 'FirebaseCore', '~> 7.0'
Copy link
Member

Choose a reason for hiding this comment

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

nit: alphabetize dependencies

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Done.

@@ -1,3 +1,6 @@
#unreleased
Copy link
Member

Choose a reason for hiding this comment

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

Will be 7.11.0

FOUNDATION_EXPORT const int kFIRMessagingSendTtlDefault; // 24 hours

/**
* Value included in a structured response or GCM message from IID, indicating
Copy link
Member

Choose a reason for hiding this comment

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

Is it still called IID when it's integrated in FCM?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The description doesn't make sense. I updated. But yeah FM code-wise should not depend on IID. But might still handle some signals sending from IID in case users still use IID.

@morganchen12morganchen12 added this to the 7.11.0 - M94 milestone Mar 31, 2021
Copy link
ContributorAuthor

@charlotteliangcharlotteliang left a comment

Choose a reason for hiding this comment

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

Fixed the podspec as well.

FOUNDATION_EXPORT const int kFIRMessagingSendTtlDefault; // 24 hours

/**
* Value included in a structured response or GCM message from IID, indicating
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The description doesn't make sense. I updated. But yeah FM code-wise should not depend on IID. But might still handle some signals sending from IID in case users still use IID.

s.dependency 'FirebaseInstanceID', '~> 7.0'
s.dependency 'FirebaseInstallations', '~> 7.0'
s.dependency 'FirebaseCore', '~> 7.0'
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@maksymmalyhinmaksymmalyhin left a comment

Choose a reason for hiding this comment

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

I've looked through the changes, but didn't dive too deep assuming that the changes were previously reviewed. LGTM. Please make sure not to merge it before M93 branch cut off.

Copy link
Member

@paulb777paulb777 left a comment

Choose a reason for hiding this comment

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

7.10 release branch has been made.

@charlotteliangcharlotteliang merged commit ec13222 into masterMar 31, 2021
@charlotteliangcharlotteliang deleted the chen/fm-master branch March 31, 2021 16:52
@paulb777paulb777 mentioned this pull request Apr 1, 2021
@firebasefirebase locked and limited conversation to collaborators May 1, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.
5 participants
@charlotteliang@google-oss-bot@paulb777@maksymmalyhin@morganchen12
close