feat: add CDVWebViewEngineConfigurationDelegate

[msmtamburro]

to fully expose WKWebView configuration (#900)

Platforms affected

cordova-ios

Motivation and Context

#900

Description

Allows the consumer to extend CDVViewController as a CDVWebViewEngineConfigurationDelegate and provide a WKWebViewConfiguration. This is useful for more complex configuration cases (i.e., ones where configuration through config.xml is insufficient).

Testing

I tested both the existing behavior:

• letting cordova-ios create the WKWebViewConfiguration on your behalf

And the new behavior:

• optionally providing your own configuration, which is used instead

Checklist

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
• I've updated the documentation if necessary

[dpogue]

In concept this looks good.

I suspect CDVWebViewEngineProtocol initWithFrame:configuration: will have to be a separate method from the existing CDVWebViewEngineProtocol initWithFrame: to maintain backwards compatibility with existing webview plugins, and then we'll need to conditionally decide which one to call based on the engine class responding to it.

[msmtamburro]

Great, understanding this existing dependency is helpful. I can bring back the existing init alongside this new one.

[codecov-io]

Codecov Report

Merging #1050 (3164112) into master (7e3402c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1050   +/-   ##
=======================================
  Coverage   74.82%   74.82%           
=======================================
  Files          13       13           
  Lines        1720     1720           
=======================================
  Hits         1287     1287           
  Misses        433      433           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e3402c...3164112. Read the comment docs.

[dpogue]

Does the addition of NS_ASSUME_NONNULL_BEGIN and nullable impact the public API for plugins that implement subclasses of CDVViewController and CDVWebViewEngineProtocol?

We need to be careful with public API to ensure (as much as possible) that we don't break existing plugins, otherwise people will just stay on older versions and keep opening issues requesting fixes be backported.

[msmtamburro]

Does the addition of NS_ASSUME_NONNULL_BEGIN and nullable impact the public API for plugins that implement subclasses of CDVViewController and CDVWebViewEngineProtocol?

We need to be careful with public API to ensure (as much as possible) that we don't break existing plugins, otherwise people will just stay on older versions and keep opening issues requesting fixes be backported.

Short answer, no impact.

Longer answer: Before the changes, the original code could actually return nil, but the consumer had no way to understand this without nullability specifiers. The consumers likely assumed that these methods never return nil. Adding the NS_ASSUME_NONNULL_BEGIN is the equivalent of making no change to the nullability specifiers, however incorrect they may be. Adding nullable to the new method tells the consumer to expect that this method can return null. One could go through and update all of the previous methods with appropriate nullability specifiers at some point in the future. (Worth noting: if I had not added the NS_ASSUME_NONNULL_BEGIN, I would have been required to add nullability specifiers everywhere in this file as soon as I added one to the new method.)

[dpogue]

Adding nullable to the new method tells the consumer to expect that this method can return null. One could go through and update all of the previous methods with appropriate nullability specifiers at some point in the future.

You've changed initWithFrame: to have nullable too:

- (nullable instancetype)initWithFrame:(CGRect)frame;

Will existing plugins that implement it without nullable run into compiler errors?
i.e., https://github.com/ionic-team/cordova-plugin-ionic-webview/blob/master/src/ios/CDVWKWebViewEngine.m#L117

[msmtamburro]

Adding nullable to the new method tells the consumer to expect that this method can return null. One could go through and update all of the previous methods with appropriate nullability specifiers at some point in the future.

You've changed initWithFrame: to have nullable too:

- (nullable instancetype)initWithFrame:(CGRect)frame;

Will existing plugins that implement it without nullable run into compiler errors?
i.e., https://github.com/ionic-team/cordova-plugin-ionic-webview/blob/master/src/ios/CDVWKWebViewEngine.m#L117

They may possibly see a warning (depending on project setup, and language used), but it is a good warning regarding unchanged code. The existing code could have always returned nil here. It should not stop anyone from compiling, even if they are treating warnings as errors in their own project.

[dpogue]

This looks good to me.

@NiklasMerz @jcesarmobile Any thoughts/concerns here?

[NiklasMerz]

No concerns. This looks like a really good change for plugin authors. I am just not confident enough in ObjectiveC to review the code/find issues.

[msmtamburro]

Adding nullable to the new method tells the consumer to expect that this method can return null. One could go through and update all of the previous methods with appropriate nullability specifiers at some point in the future.

You've changed initWithFrame: to have nullable too:

- (nullable instancetype)initWithFrame:(CGRect)frame;

Will existing plugins that implement it without nullable run into compiler errors?
i.e., https://github.com/ionic-team/cordova-plugin-ionic-webview/blob/master/src/ios/CDVWKWebViewEngine.m#L117

They may possibly see a warning (depending on project setup, and language used), but it is a good warning regarding unchanged code. The existing code could have always returned nil here. It should not stop anyone from compiling, even if they are treating warnings as errors in their own project.

I took a moment to go through existing (unchanged) methods that fall under the assumed "nonnull" but can actually return nil, and explicitly marked them as "nullable," so there's less ambiguity.