Skip to content

Forwards app delegate's openURL invocations to FIRAuth. #207

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 3 commits into from
Aug 22, 2017

Conversation

XiangtianDai
Copy link
Contributor

No description provided.

@@ -312,6 +427,16 @@ - (void)testLegacyDelegateTwoHandlers {
XCTAssertEqualObjects(delegate.notificationReceived, _notification);
delegate.notificationReceived = nil;

// Verify NOT handling of `application:openURL:sourceApplication:annotation:`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Verify application:openURL:sourceApplication:annotation: is NOT handled.

Copy link
Contributor Author

@XiangtianDai XiangtianDai Aug 22, 2017

Choose a reason for hiding this comment

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

Done. Also change all instances of "Verify handling of ..." to be "Verify ... is handled" in the entire file to be consistent.

([_appDelegate respondsToSelector:openURLOptionsSelector] ||
!([_appDelegate respondsToSelector:openURLAnnotationSelector] ||
[_appDelegate respondsToSelector:handleOpenURLSelector]))) {
// Replace the modern selector which is avaliable on iOS 9 and above because this is the one
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the above condition be && instead of || ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see previous response.

if (&UIApplicationOpenURLOptionsAnnotationKey && // the constant is only available on iOS 9+
([_appDelegate respondsToSelector:openURLOptionsSelector] ||
!([_appDelegate respondsToSelector:openURLAnnotationSelector] ||
[_appDelegate respondsToSelector:handleOpenURLSelector]))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be negated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

!(a || b) == !a && !b

Changed to the right side expression as it reads better to you.

@param url The URL received by the application delegate from any of the openURL method.
@return Whether or the URL is handled. YES means the URL is for Firebase Auth
so the caller should ignore the URL from further processing, and NO means the
the URL is for the app (or another libaray) so the caller should continue handling
Copy link
Contributor

Choose a reason for hiding this comment

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

library

so the caller should ignore the URL from further processing, and NO means the
the URL is for the app (or another libaray) so the caller should continue handling
this URL as usual.
@remarks If swizzling is disabled, URLs received by application delegate must be forwarded to
Copy link
Contributor

Choose a reason for hiding this comment

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

by "the" application delegate...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@remarks If swizzling is disabled, URLs received by application delegate must be forwarded to
this method for phone number auth to work.
*/
- (BOOL)canHandleURL:(nonnull NSURL *)url;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method necessary for the diff?

Copy link
Contributor Author

@XiangtianDai XiangtianDai Aug 22, 2017

Choose a reason for hiding this comment

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

Yes, FIRAuth must conform to FIRAuthAppDelegateHandler. It does nothing yet.

@XiangtianDai XiangtianDai merged commit 6e12c50 into master Aug 22, 2017
@XiangtianDai XiangtianDai deleted the auth-handle-url branch August 22, 2017 04:14
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018
@firebase firebase locked and limited conversation to collaborators Nov 13, 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.

3 participants