Skip to content

Adding macOS support for Messaging and InstanceID#2880

Merged
charlotteliang merged 46 commits into
masterfrom
fcm-iid-macos
May 30, 2019
Merged

Adding macOS support for Messaging and InstanceID#2880
charlotteliang merged 46 commits into
masterfrom
fcm-iid-macos

Conversation

@charlotteliang
Copy link
Copy Markdown
Member

@charlotteliang charlotteliang commented Apr 23, 2019 •
edited
Loading

With this change, developers can use FCM to send push notifications on macOS app.

@wilhuff
Copy link
Copy Markdown
Contributor

wilhuff commented Apr 23, 2019

Please resolve style issues.

paulb777
paulb777 reviewed Apr 26, 2019
View reviewed changes
Comment thread FirebaseInstanceID.podspec Outdated
@paulb777 paulb777 mentioned this pull request Apr 26, 2019
Closed
@maksymmalyhin
Copy link
Copy Markdown
Contributor

maksymmalyhin commented May 2, 2019

@chliangGoogle FYI #2911. The PR is waiting for a review from @tejasd

maksymmalyhin
maksymmalyhin reviewed May 2, 2019
View reviewed changes
Comment thread Firebase/Messaging/FIRMessagingRemoteNotificationsProxy.m Outdated
@paulb777
Copy link
Copy Markdown
Member

paulb777 commented May 6, 2019

Seeing tvOS build errors - firebase-ios-sdk/Firebase/InstanceID/FIRInstanceID.m:1183:8: error: use of undeclared identifier 'apsEnvironment'

@charlotteliang charlotteliang changed the title Make Messaging and InstanceID compatible with macOS Adding macOS support for Messaging and InstanceID May 6, 2019
@charlotteliang charlotteliang requested a review from paulb777 May 29, 2019 18:08
paulb777
paulb777 reviewed May 29, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Update .travis.yml to run InstanceID for macos (two places)

Also, if you like, the architecture flags may no longer be necessary after Maksym's stability fixes.

@charlotteliang
Copy link
Copy Markdown
Member Author

charlotteliang commented May 29, 2019 •
edited
Loading

The architecture flags are the updated version, I believe, but I can let Maksym test again to see if some of them can be removed.

Regarding the error: https://travis-ci.org/firebase/firebase-ios-sdk/jobs/538889246#L1757

I have to manually set "Weak References in Manual Retain Release" to true under the FirebaseMessaging-MacOS-Unit-Unit pod, I wonder what I need to do in podspec to reflect such flag automatically so travis can pass green?

@paulb777
Copy link
Copy Markdown
Member

paulb777 commented May 29, 2019

You can set options for the test target's build config here - https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseMessaging.podspec#L56.

Out of curiosity, what's special about macos that it needs that flag?

@charlotteliang
Copy link
Copy Markdown
Member Author

charlotteliang commented May 29, 2019

So that's the weird part: for iOS and tvOS, xcode is default "true", for macOS it's default "false".

@charlotteliang
Copy link
Copy Markdown
Member Author

charlotteliang commented May 29, 2019

There are some breakage but none of them seem related to fcm/iid.

@paulb777
Copy link
Copy Markdown
Member

paulb777 commented May 30, 2019

I see two issues - one might be an iOS flake, but the second seems like real tvOS issues.

In https://travis-ci.org/firebase/firebase-ios-sdk/jobs/538966556:

Failing tests:
-[FIRInstanceIDTokenManagerTest testDeleteTokenSuccess]
** TEST FAILED **

In https://travis-ci.org/firebase/firebase-ios-sdk/jobs/538966560

 - WARN  | xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Firebase/Messaging/FIRMessagingRemoteNotificationsProxy.m:395:1: warning: implementing deprecated method [-Wdeprecated-implementations]
- NOTE  | xcodebuild:  /Applications/Xcode-10.1.app/Contents/Developer/Platforms/AppleTVSimulator.platform/Developer/SDKs/AppleTVSimulator12.1.sdk/System/Library/Frameworks/UIKit.framework/Headers/UIApplication.h:346:1: note: method 'application:didReceiveRemoteNotification:' declared here
- WARN  | [tvOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingRemoteNotificationsProxyTest.m:68:1: warning: implementing deprecated method [-Wdeprecated-implementations]
- WARN  | [tvOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingRemoteNotificationsProxyTest.m:196:26: warning: 'application:didReceiveRemoteNotification:' is deprecated: first deprecated in tvOS 10.0 - Use UserNotifications Framework's -[UNUserNotificationCenterDelegate willPresentNotification:withCompletionHandler:] or -[UNUserNotificationCenterDelegate didReceiveNotificationResponse:withCompletionHandler:] for user visible notifications and -[UIApplicationDelegate application:didReceiveRemoteNotification:fetchCompletionHandler:] for silent remote notifications [-Wdeprecated-declarations]
- NOTE  | xcodebuild:  /Applications/Xcode-10.1.app/Contents/Developer/Platforms/AppleTVSimulator.platform/Developer/SDKs/AppleTVSimulator12.1.sdk/System/Library/Frameworks/UIKit.framework/Headers/UIApplication.h:346:1: note: 'application:didReceiveRemoteNotification:' has been explicitly marked deprecated here
- WARN  | [tvOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingRemoteNotificationsProxyTest.m:230:16: warning: 'application:didReceiveRemoteNotification:' is deprecated: first deprecated in tvOS 10.0 - Use UserNotifications Framework's -[UNUserNotificationCenterDelegate willPresentNotification:withCompletionHandler:] or -[UNUserNotificationCenterDelegate didReceiveNotificationResponse:withCompletionHandler:] for user visible notifications and -[UIApplicationDelegate application:didReceiveRemoteNotification:fetchCompletionHandler:] for silent remote notifications [-Wdeprecated-declarations]

paulb777
paulb777 approved these changes May 30, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Nice!

maksymmalyhin
maksymmalyhin approved these changes May 30, 2019
View reviewed changes
Copy link
Copy Markdown
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.

👍

@charlotteliang charlotteliang merged commit 7eb1747 into master May 30, 2019
@charlotteliang charlotteliang deleted the fcm-iid-macos branch May 30, 2019 22:31
@morganchen12 morganchen12 mentioned this pull request May 30, 2019
Closed
@firebase firebase locked and limited conversation to collaborators Oct 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@paulb777 paulb777 paulb777 approved these changes

@ryanwilson ryanwilson Awaiting requested review from ryanwilson

@wilhuff wilhuff Awaiting requested review from wilhuff

+1 more reviewer

@maksymmalyhin maksymmalyhin maksymmalyhin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@charlotteliang @wilhuff @maksymmalyhin @paulb777 @googlebot