search

apache / cordova-ios

Apache Cordova iOS
https://cordova.apache.org/
Apache License 2.0
2.15k stars 988 forks source link

feat: add CDVWebViewEngineConfigurationDelegate #1050

Closed msmtamburro closed 2 years ago

msmtamburro commented 3 years ago

to fully expose WKWebView configuration (#900)

Platforms affected

cordova-ios

Motivation and Context

https://github.com/apache/cordova-ios/issues/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:

And the new behavior:

Checklist

dpogue commented 3 years ago

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 commented 3 years ago

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

codecov-io commented 3 years ago

Codecov Report

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

Impacted file tree graph

@@           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.

msmtamburro commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

NiklasMerz commented 3 years ago

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 commented 3 years ago

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.