WorryFree Computers   »   [go: up one dir, main page]

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

charlotteliang
Copy link
Contributor

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

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

Choose a reason for hiding this comment

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

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

@paulb777 paulb777 added this to the M30 milestone Jul 7, 2018
Copy link
Contributor
@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a nil-check here before swizzling?

check the UIApplication is nil before swizzling
Copy link
Contributor
@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

LGTM

@charlotteliang charlotteliang merged commit 954e4d5 into master Jul 9, 2018
@charlotteliang charlotteliang deleted the fcm-app-extension branch July 9, 2018 21:45
@firebase firebase locked and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants