GULAppDelegateSwizzler - proxy APNS methods separately #2835

maksymmalyhin · 2019-04-15T21:52:48Z

Fixes #2807

separate proxying of the APNs related methods (see method + [GULAppDelegateSwizzler proxyOriginalDelegateRemoteNotificationMethods])
don't swizzle APNs related methods until explicitly requested
use NSSelectorFromString and NSInvocation to avoid the Apple review warning
Messaging updated
Auth updated
Documentation comments and README updated

…ead of internal `-proxyAppDelegate:`

…ificationMethods`

GoogleUtilities.podspec

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

GoogleUtilities/Example/GoogleUtilities.xcodeproj/xcshareddata/xcschemes/Example_iOS.xcscheme

…ationMethods`

… test FirebaseMessaging compatibility with extensions

maksymmalyhin · 2019-04-17T16:02:02Z

Looks like the build failure is caused by #2851 and is not related to the PR

Example/Messaging/App/iOS/NotificationService/NotificationService.h

tejasd · 2019-04-17T18:06:25Z

GoogleUtilities/AppDelegateSwizzler/Private/GULAppDelegateSwizzler.h

 *
 *  @see proxyOriginalDelegate
 */
-+ (void)proxyOriginalDelegateRemoteNotificationMethods;


You will have to make a change in Analytics as a result of this. Is this going to be a major version bump?

@paulb777 Do you think we can update the analytics as a part of Copybara CL?

Because this is blocking the release, I would vote to revert the renaming, and revisit it after the release is out

This breaks fcm test app for extension, its better to fix this if its going out for breaking change release anyway. I am fine doing it in another pr but it has to be resolved if it is going into the i/o release.

This reverts commit 2534a98.

This reverts commit cc95894.

This reverts commit d9a3555.

This reverts commit 5bbf3fd.

…iginalDelegateIncludingAPNSMethods`

tejasd

Sorry for more comments - I'm still getting used to GitHub's code review UI haha.

Implementation LGTM, just some comments for tests!

tejasd · 2019-04-17T21:07:23Z

GoogleUtilities/Example/Tests/Swizzler/GULAppDelegateSwizzlerTest.m

 - (void)skipped_testAppDelegateInstance {
  id originalDelegate = [UIApplication sharedApplication].delegate;

  GULTestAppDelegate *realAppDelegate = [[GULTestAppDelegate alloc] init];


Is skipped_testAppDelegateInstance still disabled due to flakiness? If it is, can you make sure it passes locally?

@tejasd TBH, I didn't really pay attention to the skipped test, just kept it compilable... If you have a context why it has been marked as skipped? It looks like you were the last person changing it

It was because it was flaky on Travis! (Works ok on the internal infrastructure, so should be caught when you copybara it)

cb73b2f

tejasd · 2019-04-17T21:08:09Z

GoogleUtilities/Example/Tests/Swizzler/GULAppDelegateSwizzlerTest.m

  int _arbitraryNumber;
 }

 /** A URL property that is set by the app delegate methods, which is then used to verify if the app


I might not be seeing it due to weird github diffs, but if it doesn't already exist, can you add a test that makes sure proxyOriginalDelegateIncludingAPNSMethods doesn't get swizzled when swizzling is disabled?

maksymmalyhin · 2019-04-17T21:13:28Z

GoogleUtilities/Example/Tests/Swizzler/GULAppDelegateSwizzlerTest.m

+                             handleEventsForBackgroundURLSession:completionHandler:)]);
+
+  // Remote notifications methods should be added only by
+  // -proxyOriginalDelegateIncludingAPNSMethods


I might not be seeing it due to weird github diffs, but if it doesn't already exist, can you add a test that makes sure proxyOriginalDelegateIncludingAPNSMethods doesn't get swizzled when swizzling is disabled?

@tejasd It is tested here and in other places with such a comment

This doesn't seem to test for situation where Swizzling is explicitly disabled, something like

firebase-ios-sdk/GoogleUtilities/Example/Tests/Swizzler/GULAppDelegateSwizzlerTest.m

Line 1157 in a4a94ce

* flag is not present. */

@tejasd Ah, sorry, I misread the comment. I can add it. Do you think it is critical for this PR?
proxyOriginalDelegateIncludingAPNSMethods uses proxyOriginalDelegate under the hood, worked as expected is implied with the current implementation, so it could be a separate PR

Followup PR should be ok! Thanks!

* Update versions for Release 6.0.0 * Update FirebaseSDKs.textproto with new fields. (#2818) (#2834) * Update FirebaseSDKs.textproto with new fields. * Update message about strip_32bits flag. * GULAppDelegateSwizzler - proxy APNS methods separately (#2835) (#2858) * Cherry-picks for M47 RC5 (#2882) * fix race condition checkin is deleted before writing during app start (#2860) * GoogleUtilities min ios version from 6 to 8 (#2876) * Put secret and oauth credentials into the private headers for now. (#2916) (#2919) * Add missing Messaging weak_framework dep (#2921) (#2930)

maksymmalyhin added 14 commits April 12, 2019 16:46

Rename methods, replace selectors by NSSelectorFromString

3f3fadf

NSSelectorFromString and NSInvocation

f047dcb

Pod deintegrate

d85e9ce

Reference to non-existing GoogleUtilities_IntegrationTests removed

e722d7c

Comment fix

75be06b

Reduce code duplication for swizzling of APNS method.

5db9b52

Tests for calling the original delegate method implementations added

7cf6d64

Refactor to reduce code duplication - the rest of the methods

bd2611e

GULAppDelegateSwizzlerTest - use public -proxyOriginalDelegate inst…

bf7f898

…ead of internal `-proxyAppDelegate:`

Separate remote notifications methods proxying

e662aa9

Bump GoogleUtilities version to 5.9.0

7319161

Remove assertion to improve simplify unit testing

116340d

Messaging: proxyOriginalDelegate -> `proxyOriginalDelegateRemoteNot…

8fa00e4

…ificationMethods`

Run ./scripts/style.sh

565708e

googlebot added the cla: yes label Apr 15, 2019

maksymmalyhin requested a review from tejasd April 15, 2019 21:53

maksymmalyhin self-assigned this Apr 15, 2019

paulb777 reviewed Apr 16, 2019

View reviewed changes

maksymmalyhin added 4 commits April 15, 2019 21:21

Revert Podspec bump

0267efc

Typos fixes

80c02e5

Auth - proxyOriginalDelegate -> `proxyOriginalDelegateRemoteNotific…

4da889b

…ationMethods`

GULAppDelegateSwizzler code comments

abee5ac

maksymmalyhin changed the title ~~Mm/swizzle notifications~~ GULAppDelegateSwizzler - proxy APNS methods separately Apr 16, 2019

maksymmalyhin requested review from charlotteliang, renkelvin and ryanwilson April 16, 2019 14:49

maksymmalyhin marked this pull request as ready for review April 16, 2019 14:49

maksymmalyhin added 3 commits April 16, 2019 10:49

Run ./scripts/style.sh

4b0096b

App Delegate Swizzler readme

b9684c0

Trailing whitespace

84a2ebc

maksymmalyhin added 7 commits April 17, 2019 10:01

Notification Service extension for Messaging_Example_iOS app added to…

c33e28f

… test FirebaseMessaging compatibility with extensions

NotificationService - fix licenses

4faf8c9

Trailing spaces

ba0b7c6

Method renaming

5bbf3fd

Readme updated

d9a3555

Auth, Messaging updated to use renamed methods

cc95894

Run ./scripts/style.sh

2534a98

maksymmalyhin requested a review from tejasd April 17, 2019 15:15

charlotteliang approved these changes Apr 17, 2019

View reviewed changes

Example/Messaging/App/iOS/NotificationService/NotificationService.h Show resolved Hide resolved

tejasd reviewed Apr 17, 2019

View reviewed changes

maksymmalyhin added 5 commits April 17, 2019 15:34

Revert "Run ./scripts/style.sh"

43086b6

This reverts commit 2534a98.

Revert "Auth, Messaging updated to use renamed methods"

12fa390

This reverts commit cc95894.

Revert "Readme updated"

8c8e8aa

This reverts commit d9a3555.

Revert "Method renaming"

e80f145

This reverts commit 5bbf3fd.

Renaming proxyOriginalDelegateRemoteNotificationMethods -> `proxyOr…

a4a94ce

…iginalDelegateIncludingAPNSMethods`

maksymmalyhin requested a review from tejasd April 17, 2019 19:47

tejasd requested changes Apr 17, 2019

View reviewed changes

maksymmalyhin commented Apr 17, 2019

View reviewed changes

tejasd approved these changes Apr 17, 2019

View reviewed changes

maksymmalyhin merged commit 76aa880 into master Apr 17, 2019

maksymmalyhin added a commit that referenced this pull request Apr 17, 2019

GULAppDelegateSwizzler - proxy APNS methods separately (#2835)

11a9909

maksymmalyhin mentioned this pull request Apr 17, 2019

GULAppDelegateSwizzler - proxy APNS methods separately (#2835) #2858

Merged

maksymmalyhin added a commit that referenced this pull request Apr 17, 2019

GULAppDelegateSwizzler - proxy APNS methods separately (#2835) (#2858)

23da5fe

maksymmalyhin deleted the mm/swizzle-notifications branch April 18, 2019 14:45

maksymmalyhin mentioned this pull request Apr 18, 2019

GULAppDelegateSwizzlerTest #2861

Merged

Corrob pushed a commit that referenced this pull request Apr 25, 2019

GULAppDelegateSwizzler - proxy APNS methods separately (#2835)

cb42f99

maksymmalyhin mentioned this pull request Apr 26, 2019

FIRMessaging can no longer build inside app extension. #2840

Closed

firebase locked and limited conversation to collaborators Oct 17, 2019

GULAppDelegateSwizzler - proxy APNS methods separately #2835

GULAppDelegateSwizzler - proxy APNS methods separately #2835

Uh oh!

Conversation

maksymmalyhin commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maksymmalyhin commented Apr 17, 2019

Uh oh!

Uh oh!

tejasd Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charlotteliang Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejasd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maksymmalyhin commented Apr 15, 2019 •

edited

Loading

tejasd Apr 17, 2019 •

edited

Loading

charlotteliang Apr 17, 2019 •

edited

Loading