You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@cordova.apache.org by GitBox <gi...@apache.org> on 2021/01/05 15:27:23 UTC
[GitHub] [cordova-ios] msmtamburro opened a new pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
msmtamburro opened a new pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050
to fully expose WKWebView configuration (#900)
<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist is intended as a quick reference, for complete details please see our Contributor Guidelines:
http://cordova.apache.org/contribute/contribute_guidelines.html
Thanks!
-->
### Platforms affected
cordova-ios
### Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
<!-- If it fixes an open issue, please link to the issue here. -->
https://github.com/apache/cordova-ios/issues/900
### Description
<!-- Describe your changes in detail -->
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
<!-- Please describe in detail how you tested your changes. -->
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
- [x] 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)`)
- [x] 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](https://help.github.com/articles/closing-issues-using-keywords/))
- [ ] I've updated the documentation if necessary
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] dpogue commented on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
dpogue commented on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-755541976
> 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:
```objectivec
- (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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] msmtamburro commented on a change in pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
msmtamburro commented on a change in pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#discussion_r552981520
##########
File path: CordovaLib/Classes/Public/CDVViewController.m
##########
@@ -505,9 +505,10 @@ - (UIView*)newCordovaViewWithFrame:(CGRect)bounds
self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds];
}
} else {
- self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds];
+ WKWebViewConfiguration *config = [self respondsToSelector:@selector(configuration)] ? [self configuration] : nil;
+ self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds configuration:config];
Review comment:
Since the change was more extensive than expected, I raised it in another branch on my fork: https://github.com/msmtamburro/cordova-ios/pull/1/files
If it makes sense, I'll merge it in to this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io edited a comment on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-754784800
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=h1) Report
> Merging [#1050](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=desc) (3cd8bda) into [master](https://codecov.io/gh/apache/cordova-ios/commit/7e3402c565c2e34eae2bb954c65c989f71e20df1?el=desc) (7e3402c) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=tree)
```diff
@@ 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](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=footer). Last update [7e3402c...3cd8bda](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] msmtamburro commented on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
msmtamburro commented on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-755438573
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] msmtamburro commented on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
msmtamburro commented on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-755741653
> > 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:
>
> ```objc
> - (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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] msmtamburro commented on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
msmtamburro commented on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-754759489
Great, understanding this existing dependency is helpful. I can bring back the existing init alongside this new one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] dpogue commented on a change in pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
dpogue commented on a change in pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#discussion_r552788845
##########
File path: CordovaLib/Classes/Public/CDVViewController.m
##########
@@ -505,9 +505,10 @@ - (UIView*)newCordovaViewWithFrame:(CGRect)bounds
self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds];
}
} else {
- self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds];
+ WKWebViewConfiguration *config = [self respondsToSelector:@selector(configuration)] ? [self configuration] : nil;
+ self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds configuration:config];
Review comment:
It looks like you're only applying this in the case when an engine is not specified.
What might be best is to make a private `initWebViewEngine:(NSClass)engineClass` method that takes in the engine class, and checks if it responds to `initWithFrame:configuration:` and calls that, falling back to `initWithFrame:`.
Then we'd call `initWebViewEngine:` on lines 502, 505, and 509 here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io edited a comment on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-754784800
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=h1) Report
> Merging [#1050](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=desc) (3cd8bda) into [master](https://codecov.io/gh/apache/cordova-ios/commit/7e3402c565c2e34eae2bb954c65c989f71e20df1?el=desc) (7e3402c) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=tree)
```diff
@@ 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](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=footer). Last update [7e3402c...3cd8bda](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] NiklasMerz commented on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
NiklasMerz commented on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-756301545
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] dpogue commented on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
dpogue commented on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-754726896
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](https://github.com/ionic-team/cordova-plugin-ionic-webview/blob/master/src/ios/CDVWKWebViewEngine.m#L117) [webview](https://github.com/bpresles/cordova-plugin-uiwebview-engine/blob/master/src/ios/BPRUIWebViewEngine.m#L39) plugins, and then we'll need to conditionally decide which one to call based on the engine class responding to it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] msmtamburro commented on a change in pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
msmtamburro commented on a change in pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#discussion_r552929486
##########
File path: CordovaLib/Classes/Public/CDVViewController.m
##########
@@ -505,9 +505,10 @@ - (UIView*)newCordovaViewWithFrame:(CGRect)bounds
self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds];
}
} else {
- self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds];
+ WKWebViewConfiguration *config = [self respondsToSelector:@selector(configuration)] ? [self configuration] : nil;
+ self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds configuration:config];
Review comment:
This is a great suggestion... I didn't imagine custom web engines using this, but of course, that makes sense. I'll tidy this up.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io commented on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-754784800
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=h1) Report
> Merging [#1050](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=desc) (3cd8bda) into [master](https://codecov.io/gh/apache/cordova-ios/commit/7e3402c565c2e34eae2bb954c65c989f71e20df1?el=desc) (7e3402c) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=tree)
```diff
@@ 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](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=footer). Last update [7e3402c...3cd8bda](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] msmtamburro edited a comment on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
msmtamburro edited a comment on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-755438573
> 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.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io edited a comment on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-754784800
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=h1) Report
> Merging [#1050](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=desc) (3164112) into [master](https://codecov.io/gh/apache/cordova-ios/commit/7e3402c565c2e34eae2bb954c65c989f71e20df1?el=desc) (7e3402c) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=tree)
```diff
@@ 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](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=footer). Last update [7e3402c...3164112](https://codecov.io/gh/apache/cordova-ios/pull/1050?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] msmtamburro commented on pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
msmtamburro commented on pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050#issuecomment-758135910
> > > 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:
> > ```objc
> > - (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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-ios] dpogue merged pull request #1050: feat: add CDVWebViewEngineConfigurationDelegate
Posted by GitBox <gi...@apache.org>.
dpogue merged pull request #1050:
URL: https://github.com/apache/cordova-ios/pull/1050
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org