Skip to content

Enables Firebase Messaging push notification function on watch only and independent watch app#4016

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

Merged
merged 34 commits into from
Dec 24, 2019

Conversation

charlotteliang
Copy link
Contributor

@charlotteliangcharlotteliang commented Oct 7, 2019

This PR enables FCM push notification function on watch only app, and you can also send images/videos on notifications that deliver to watch only app or independent watch app. This is different than the watch app with an iOS companion app, where the watch app notification is delivered by apple system. For the independent watch app and watch only app, developers can request push notification independently inside the watch app, so it has its own apns token.

This is also part of the effort #269.

We can conditionally disable some of the FireLog and Reachability functionality if detected watchOS. Any feedback is welcome.

@charlotteliangcharlotteliang self-assigned this Nov 7, 2019
@charlotteliangcharlotteliang changed the title [Draft] Adding watchOS on podspec[Draft] Enables FCM push notification function on watch only appNov 7, 2019
@paulb777
Copy link
Member

It looks like there are code format issues.

What is the unit testing strategy before @morganchen12's OCMock PR lands?

@charlotteliang
Copy link
ContributorAuthor

This probably will break travis as I comment out systemConfiguration from podspec file, I've left the discussion in the description box.

@paulb777
Copy link
Member

Here is an example of platform specific Apple framework dependencies - https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseStorage.podspec#L29

@charlotteliang
Copy link
ContributorAuthor

Nice Thanks Paul!

@paulb777
Copy link
Member

To go forward, we should update relevant test_specs with all platforms except watchos like CocoaPods/CocoaPods#8283 (comment)

@charlotteliang
Copy link
ContributorAuthor

We have to specify iOS 7 because pod lib lint failed at UIBackgroundFetchResult which is API_AVAILABLE(ios(7.0)). For some reason, this failed and not recognize UIBackgroundFetchResult if not specify iOS 7 above.

@charlotteliang
Copy link
ContributorAuthor

Pod lib lint GoogleUtilities.podspec succeed locally but failed on travis :
- ERROR | [watchOS] unknown: Encountered an unknown error (Simulator for watchOS 6.0 is not available.) during validation.
Not sure why this only happens at GoogleUtilities pod.

@paulb777
Copy link
Member

Might be because the other libraries run platform specific pod lib lint runs.

That would probably be a better fix to update .travis.yml similarly for GoogleUtilities.

Copy link
Member

@paulb777paulb777 left a comment

Choose a reason for hiding this comment

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

Looking close - two more comments.

@@ -370,7 +370,9 @@ jobs:
env:
- PROJECT=GoogleUtilities METHOD=pod-lib-lint
script:
Copy link
Member

Choose a reason for hiding this comment

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

Looks good. Please make the same change for the GoogleUtilitiesCron job below.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Done. There are two cmds under GoogleUtilitiesCron so I split each of them to 3 platforms.

@@ -940,8 +943,7 @@ - (void)application:(GULApplication *)application
}
}

#if __IPHONE_OS_VERSION_MAX_ALLOWED >= 70000
// This is needed to for the library to be warning free on iOS versions < 7.
#if __IPHONE_OS_VERSION_MAX_ALLOWED >= 70000 && !TARGET_OS_WATCH
Copy link
Member

Choose a reason for hiding this comment

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

No longer needed now?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This is needed because of UIBackgroundFetchResult which is API_AVAILABLE(ios(7.0)). Somehow pod lib lint will fail if we ignore this check. I think something to do with the syntax API_AVAILABLE that checks below iOS 7 and it thinks UIBackgroundFetchResult is not recognized.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Considering this is already there before my change, I can also add a todo for the owner to double check on this.

@paulb777paulb777 mentioned this pull request Dec 20, 2019
Copy link
Member

@paulb777paulb777 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@mikehaney24mikehaney24 left a comment

Choose a reason for hiding this comment

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

LGTM for GDT changes. More work will need to be done to better support watchOS, but hopefully that can be addressed later rather than sooner.

@charlotteliang
Copy link
ContributorAuthor

I also tested again on iOS and OSX to make sure the certificates are routed correctly. Will test tvOS once the lab is setup in new office again.

@charlotteliangcharlotteliang merged commit e4456c0 into masterDec 24, 2019
@charlotteliangcharlotteliang deleted the fcm-watch branch December 24, 2019 00:45
@paulb777paulb777 added this to the M62 milestone Dec 24, 2019
@firebasefirebase locked and limited conversation to collaborators Jan 23, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.
close