Skip to content
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

Merge IID removal code to master #7814

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

@charlotteliang charlotteliang 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.

charlotteliang and others added 26 commits July 23, 2020 14:13
@google-cla google-cla bot added the cla: yes label Mar 30, 2021
@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.
    Filename Base (a7ada99) Head (f8ca45f) Diff
    FIRMessaging.m 68.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.m 90.65% 89.93% -0.72%
    FIRMessagingPubSub.m 53.01% 53.85% +0.83%
    FIRMessagingRemoteNotificationsProxy.m 62.97% 73.64% +10.67%
    FIRMessagingRmqManager.m 58.95% 58.57% -0.38%
    FIRMessagingSyncMessageManager.m 76.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.m 44.23% 59.89% +15.65%

Test Logs

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.

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
Contributor Author

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
Contributor Author

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
Contributor Author

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.

@morganchen12 morganchen12 added this to the 7.11.0 - M94 milestone Mar 31, 2021
Copy link
Contributor Author

@charlotteliang charlotteliang 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
Contributor Author

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
Contributor Author

Choose a reason for hiding this comment

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

Done.

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.

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

@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.

7.10 release branch has been made.

@charlotteliang charlotteliang merged commit ec13222 into master Mar 31, 2021
@charlotteliang charlotteliang deleted the chen/fm-master branch March 31, 2021 16:52
@paulb777 paulb777 mentioned this pull request Apr 1, 2021
@firebase firebase locked and limited conversation to collaborators May 1, 2021
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.

5 participants