IID: version dependent on FirebaseInstallations #4533
Conversation
…ase-ios-sdk into mm/fis-integration-master
…ase-ios-sdk into mm/fis-integration-master
* Fix IID FCM token check. * Unused constant deleted. * FCM tests: fix bad merge.
…ase-ios-sdk into mm/fis-integration-master
#4482) * IID: use a cached value of FID for -[FIRInstanceID appInstanceID:] * TODO comment * TODO updated
| @@ -16,14 +16,10 @@ | |||
|
|
|||
| #import "FIRInstanceIDKeychain.h" | |||
There was a problem hiding this comment.
This might be some thing I can help in another PR but I wonder if FIRInstanceIDKeychain is needed at all after FIS is in.
There was a problem hiding this comment.
As far as I can see it is still used by FIRInstanceIDAuthKeyChain which is used in the FIRInstanceIDTokenManager, FIRInstanceIDCheckinStore and couple more classes.
There was a problem hiding this comment.
I see, so it's used to store token and checkin, not just IID. We can deal with that later then. Thanks!
| @@ -2059,6 +2050,13 @@ | |||
| name = "Podspec Metadata"; | |||
| sourceTree = "<group>"; | |||
| }; | |||
| 85A00DB227791D3B865D0CE2 /* Pods */ = { | |||
| @@ -194,4 +194,5 @@ - (void)testTokenInconsistentWithIID { | |||
| firebaseAppID:FIRInstanceIDFirebaseAppID()]; | |||
| XCTAssertFalse([self.validTokenInfo isFreshWithIID:kIID]); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Delete unless this is a style requirement.
| @@ -56,23 +59,23 @@ - (BOOL)checkTokenRefreshPolicyForIID:(NSString *)IID; | |||
| - (void)updateToAPNSDeviceToken:(NSData *)deviceToken isSandbox:(BOOL)isSandbox; | |||
| /** | |||
| * Create a fetch operation. This method can be stubbed to return a particular operation instance, | |||
| * which makes it easier to unit test different behaviors. | |||
| * which makes it easier to unit test different behaviours. | |||
There was a problem hiding this comment.
Hmm, an American to Canadian change. @ryanwilson Do we have any standard guidance about English dialects in comments?
There was a problem hiding this comment.
I can revert it. I just followed Xcode suggestions
| @@ -46,7 +45,7 @@ typedef NS_OPTIONS(NSUInteger, FIRInstanceIDInvalidTokenReason) { | |||
| * | |||
| * @param authorizedEntity The authorized entity for the token, should not be nil. | |||
| * @param scope The scope for the token, should not be nil. | |||
| * @param keyPair The keyPair that represents the app identity. | |||
| * @param instanceID The unique string identifying the app instance. | |||
| @@ -77,7 +76,7 @@ typedef NS_OPTIONS(NSUInteger, FIRInstanceIDInvalidTokenReason) { | |||
| * | |||
| * @param authorizedEntity The authorized entity for the token, should not be nil. | |||
| * @param scope The scope for the token, should not be nil. | |||
| * @param keyPair The keyPair that represents the app identity. | |||
| * @param instanceID The unique string identifying the app instance. | |||
|
@paulb777 Thank you for the review! I pushed the fixes. |
|
Note that if you want to keep the individual commits in the git history, do a command line merge to master. |
|
@paulb777 Initially I though not to keep the separate commits, though it is probably a good idea if we are fine with several extra commit pushed to the branch without PRs (the recent ones and conflict resolutions from the past). WDYT? |
|
I'm ok either way. Even if you squash, you can still include the PR's from the commit message that GitHub generates automatically. |
|
I'll squash and merge including the PR's in the commit message. |
The contains:
The changes in the branch have been already reviewed in the PRs into the branch, but a brief review is appreciated anyway.