Fixed compile time checks for backwards compatibility

[conradev]

We compile this library into some shared code that has a deployment target of iOS 7.0, and then use it with a WKWebView on iOS 8.0+. Before these changes, the OnePassword extension code would crash.

I made a few changes to fix this:

1. Checking #ifdef __IPHONE_8_0 is not directly checking if the base SDK is greater than or equal to 8.0. I replaced these checks with __IPHONE_OS_VERSION_MAX_ALLOWED >= 80000, which is the standard for doing this.
2. I replaced NSClassFromString(@"NSExtensionItem") with [NSExtensionItem class], because it is marked with NS_CLASS_AVAILABLE, which automatically marks the symbol as weak if the deployment target is less than 8.0
3. I replaced the compile time check for the deployment version to be greater than 8.0, due to the issue we were experiencing above. This will not cause any crashes because WebKit is marked as a weak_framework in the Podspec, so that [WKWebView class] will just evaluate to nil on iOS 7.0.

[radazzouz]

Hi @conradev,

Thank you for taking the time to PR this change 👍

If we were to merge this PR as it currently is, does this mean that developers who use -[OnePasswordExtension findLoginForURLString:forViewController:sender:completion:completion] with iOS 7.1 or earlier as a the iOS Deployment Target will need to add WebKit.framework to their project?

If so, this doesn’t make any sense to other developers. The WebKit.framework is not necessary when no web view is present.

I am pretty sure that the crash that the crash you see is caused by us throwing the following exception:

[NSExceptionraise:@"Invalid argument: web view must be an instance of WKWebView or UIWebView."format:@"”];

Can you please confirm this? To do this you need to create an exception breakpoint in your app.

Assuming that the crash that you are experiencing is caused by the exception, we strongly recommend the DIY approach: to make these changes in your code only.

I do like the [NSExtensionItem class] != nil; change very much. Can you please update the PR so we can keep this change only.

I look forward to your reply 😃

Cheers!

[conradev]

The crash is indeed caused by that exception, and I did make this change in our code, I just figured it might be useful to the developer community as a whole.

Correct, if developers have iOS 7.1 or earlier as a the iOS Deployment Target, they will have to include WebKit.framework and mark it as weak (if they don't use CocoaPods). The Podspec does this already. Currently, if the deployment target is iOS 8.0 or greater, WebKit.framework is implicitly linked regardless of whether or not the developer uses a webview.

I can still split out the changes if you'd like, but I think 1 and 2 are no brainers.

[radazzouz]

Hi @conradev,

Thanks for replying so quickly 👍

Correct, if developers have iOS 7.1 or earlier as a the iOS Deployment Target, they will have to include WebKit.framework and mark it as weak (if they don't use CocoaPods). The Podspec does this already.

Indeed, if developers use CocoaPods WebKit.framework is automatically weak linked. However, the recommended steps to use our API in the README are to include the files. While CocoaPods is awesome, not all developers use it. We want to make sure that we aren't going to break any previous integrations that do not use CocoaPods. That's why we can't merge this PR as it is right now.

I can still split out the changes if you'd like, but I think 1 and 2 are no brainers.

I am not too certain about 1 yet. I need to test it some more. IIRC, we had replaced some #if __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_8_0 with #ifdef __IPHONE_8_0 on purpose.

As far as 2 is concerned, by all means, PR it separately or updated this PR (it's up to you). I will merge it as soon as you make the change 😉

Thanks!

[radazzouz]

Hi @conradev,

I created issue #212 and PR'd #214 to address it.

Developers want to use WKWebViews on iOS 8 devices (with runtime checks) while their Deployment Target is iOS 7.1 or earlier should now be able to use WKWebViews. Please make sure that you read the instructions to enable it in the REAME.

I am closing this PR for now. Can you please PR the [NSExtensionItem class] change in a separate PR?

Have a nice weekend 😉