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

avoid calling [UIApplication sharedApplication] in app extensions #1503

Merged
merged 4 commits into from Jul 9, 2018

Conversation

Projects
None yet
4 participants
@chliangGoogle
Copy link
Collaborator

commented Jul 6, 2018

This should make FCM compilable inside app extensions as requested by #1490

@googlebot googlebot added the cla: yes label Jul 6, 2018

@chliangGoogle chliangGoogle requested review from morganchen12, paulb777 and zlydh Jul 6, 2018

#ifdef COCOAPODS
#import <GoogleUtilities/GULAppEnvironmentUtil.h>
#else
#import "third_party/firebase/ios/Source/GoogleUtilities/Environment/third_party/GULAppEnvironmentUtil.h"

This comment has been minimized.

Copy link
@paulb777

paulb777 Jul 6, 2018

Member

Don't publish internal paths - let copybara make the translation.

@paulb777 paulb777 added this to the M30 milestone Jul 7, 2018

@morganchen12
Copy link
Collaborator

left a comment

Had some questions about behavior, but mostly LGTM.

@@ -611,7 +611,8 @@ - (BOOL)shouldBeConnectedAutomatically {
// We require a token from Instance ID
NSString *token = self.defaultFcmToken;
// Only on foreground connections
UIApplicationState applicationState = [UIApplication sharedApplication].applicationState;
UIApplication *application = FIRMessagingUIApplication();
UIApplicationState applicationState = application.applicationState;

This comment has been minimized.

Copy link
@morganchen12

morganchen12 Jul 9, 2018

Collaborator

This may return UIApplicationStateActive for extensions regardless of the host application state. Is that the correct behavior?

This comment has been minimized.

Copy link
@chliangGoogle

chliangGoogle Jul 9, 2018

Author Collaborator

We haven't finalized the official support for extensions so any function is called in extensions is not guarantee to be working. This CL just to ensure compilation works.

This comment has been minimized.

Copy link
@chliangGoogle

chliangGoogle Jul 9, 2018

Author Collaborator

If developer wants to establish a MCS connection in app extensions, it should probably work so the application state is for extension app.

This comment has been minimized.

Copy link
@chliangGoogle

chliangGoogle Jul 9, 2018

Author Collaborator

Actually no, the application object would be nil, so there's no way to check the application state in extensions. Basically we should return no for this case.

I'm also adding nil check for all the cases here.

@@ -98,7 +99,8 @@ - (void)swizzleMethodsIfPossible {
return;
}

NSObject<UIApplicationDelegate> *appDelegate = [[UIApplication sharedApplication] delegate];
UIApplication *application = FIRMessagingUIApplication();
NSObject<UIApplicationDelegate> *appDelegate = [application delegate];

This comment has been minimized.

Copy link
@morganchen12

morganchen12 Jul 9, 2018

Collaborator

Should there be a nil-check here before swizzling?

Update FIRMessagingRemoteNotificationsProxy.m
check the UIApplication is nil before swizzling
@morganchen12
Copy link
Collaborator

left a comment

LGTM

@chliangGoogle chliangGoogle merged commit 954e4d5 into master Jul 9, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chliangGoogle chliangGoogle deleted the fcm-app-extension branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.