Add Firebase info to checkin requests (b/124533860) #2500
Conversation
maksymmalyhin
commented
Mar 8, 2019
maksymmalyhin
commented
- Send Firebase info in the request header
…ckinService to be sent to the server
…ervice tests. Style.
There was a problem hiding this comment.
LGTM on travis green and approvals from @ryanwilson and @chliangGoogle
| FIRApp *app = [[FIRApp alloc] initInstanceWithName:@"FIRInstanceIDTest" options:options]; | ||
|
|
||
| NSString *expectedUserAgent = [[app class] firebaseUserAgent]; | ||
| OCMExpect([self.mockTokenManager setFirebaseUserAgent:[OCMArg isEqual:expectedUserAgent]]); |
There was a problem hiding this comment.
I think you don't need to use OCMArg here and can directly use:
OCMExpect([self.mockTokenManager setFirebaseUserAgent:expectedUserAgent]);
I could be wrong though :)
There was a problem hiding this comment.
You are right! I'll update all [OCMArg isEqual:] I introduced. Thanks.
There was a problem hiding this comment.
Though, I am just thinking, if it would be a bit more readable to keep [OCMArg isEqual:]? Then it will be obvious from the first glance that we are not passing anything to the method but just validating the input parameters.
@ryanwilson What do you think?
There was a problem hiding this comment.
Sounds reasonable to me as long as the validation is being done 👍
There was a problem hiding this comment.
Then I'll keep [OCMArg isEqual:]
|
|
||
| /** | ||
| * A string containing info about SDK components, Xcode version, etc. | ||
| * It will be added to each token request to track stats. |
There was a problem hiding this comment.
Nit: expanding a bit more would be good to clarify the "stats" part. Something like:
It will be added to each token request to count third party libraries and version numbers.
| NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:url]; | ||
|
|
||
| [request setValue:@"application/json" forHTTPHeaderField:@"content-type"]; | ||
| [request setValue:firebaseUserAgent forHTTPHeaderField:kFIRInstanceIDFirebaseUserAgentKey]; |
There was a problem hiding this comment.
Does firebaseUserAgent change a lot or need to query at the beginning of IID initialization? Can you just call [[firApp class] firebaseUserAgent] here instead of passing it in through all the chains??
There was a problem hiding this comment.
@chliangGoogle That was my first thought - just call [FIRApp firebaseUserAgent] from FIRInstanceIDCheckinService. Though, in this case we will introduce an additional dependency for FIRInstanceIDCheckinService as well as an additional responsibility - to retrieve user agent.
It looks to me that none of the classes from the chain should depend on FIRApp or be responsible for retrieving the user-agent string.
Please let me know if it doesn't look right.
There was a problem hiding this comment.
I don't think that's bigger drawback, your code could be much simpler tho by adding a dependency of FIRApp, especially InstanceID depends on core anyway.
|
Closing it in favor of #2509 |
5 participants
