You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by akofman <gi...@git.apache.org> on 2016/04/13 11:51:25 UTC
[GitHub] cordova-plugin-wkwebview-engine pull request: Fixes CB-11074: WKWe...
GitHub user akofman opened a pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
Fixes CB-11074: WKWebView configuration is not considered
I updated the configuration before the WKWebView initialisation in order to consider it.
It implies that the updateWithInfo method doesn't update it anymore. I'm not sure it is really a problem but I'd prefer @shazron to confirm.
Thanks for the reviews !
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/akofman/cordova-plugin-wkwebview-engine master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #7
----
commit 18219a0fe66208cc3ff1a794908510194cf86668
Author: Alexis Kofman <al...@gmail.com>
Date: 2016-04-13T09:46:45Z
Fixes CB-11074: WKWebView configuration is not considered
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine pull request #7: Fixes CB-11074: WKWebView c...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine issue #7: Fixes CB-11074: WKWebView configur...
Posted by shazron <gi...@git.apache.org>.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
I'm creating failing unit tests for this first, I think we have passed the point where we can argue about the implementation, but we need objective verification: https://issues.apache.org/jira/browse/CB-11496
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine issue #7: Fixes CB-11074: WKWebView configur...
Posted by macrozone <gi...@git.apache.org>.
Github user macrozone commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
It seems that this bug prevents autoplay in <video> and <audio> elements on IOS, because the config "MediaPlaybackRequiresUserAction" is never passed to the WkWebview instance and therefore always true (default in the code).
I needed to switch back to UiWebview in my meteor application because of this.
See https://github.com/meteor/meteor/issues/7173
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine pull request: Fixes CB-11074: WKWe...
Posted by shazron <gi...@git.apache.org>.
Github user shazron commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7#issuecomment-211493781
It still needs to be able to update it dynamically. I would refactor it to not remove the existing code, but create a new `- (WKWebViewConfiguration*) createConfigurationFromSettings:(NSDictionary*)settings;`
That way it can be used in both updateSettings and initWithFrame
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine issue #7: Fixes CB-11074: WKWebView configur...
Posted by macrozone <gi...@git.apache.org>.
Github user macrozone commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
@shazron : cool, thanks. Is there any short-time workaround known?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine pull request: Fixes CB-11074: WKWe...
Posted by akofman <gi...@git.apache.org>.
Github user akofman commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7#issuecomment-211276741
Any feedback ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine pull request: Fixes CB-11074: WKWe...
Posted by kenichi-fukushima <gi...@git.apache.org>.
Github user kenichi-fukushima commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7#issuecomment-216118441
A drive-by comment:
In our app, we ended up subclassing CDVWKWebViewEngine and creating another instance of WKWebView in -pluginInizialize so that we can use configuration values that we want. I'd suggest factoring out the code to create WKWebViewConfiguration to another method so that Cordova users can customize the logic by code rather than static configurations in a file. This means you can't create a web view instance in the initializer any more. This is OK because CDVWKWebViewEngine doesn't have to conform to the usual plugin semantics and you can add a factory method like -webView.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine issue #7: Fixes CB-11074: WKWebView configur...
Posted by shazron <gi...@git.apache.org>.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
I have failing unit tests that do objectively verify that as others have shown, yes, we can't update the configuration dynamically (I've tried all sorts of clever runtime tricks, to no avail).
The WKWebViewConfiguration.preferences (WKPreferences) *can* however be updated dynamically. I will update https://issues.apache.org/jira/browse/CB-11496 with the unit tests to reflect this, as well as document this. After doing so, I will re-review this pull request with that in mind.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine pull request: Fixes CB-11074: WKWe...
Posted by ephemer <gi...@git.apache.org>.
Github user ephemer commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7#issuecomment-215453411
As I commented in the base repo, this PR doesn't do what it says it does yet.
@shazron is it normal to change the settings after the WebView has already been inited? When? It seems like this feature (of changing settings after init) is creating unnecessary work for us.
My preferred approach would be to init the CDVPlugin/WebViewEngine instance, but not the WKWebView immediately, because for that to work, we need the commandDelegate to be set. The problem with that approach is that this check in `CDVViewController.m` will fail, causing `self.webViewEngine` to fallback to UIWebView:
if (!self.webViewEngine || ![self.webViewEngine conformsToProtocol:@protocol(CDVWebViewEngineProtocol)] || ![self.webViewEngine canLoadRequest:[NSURLRequest requestWithURL:self.appUrl]]) {
self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds];
}
The only thing I can image that would work in the current `CDVViewController.m` structure is that we init a WKWebView with default options on `init`, then re-init the WKWebView in `pluginInitialize` with the user's actual settings, because it's only then that `commandDelegate.settings` is finally available.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine issue #7: Fixes CB-11074: WKWebView configur...
Posted by macrozone <gi...@git.apache.org>.
Github user macrozone commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
Any update on this? In my opinion, this is a very severe issue with WKWebview.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine pull request: Fixes CB-11074: WKWe...
Posted by shazron <gi...@git.apache.org>.
Github user shazron commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7#issuecomment-215584737
@ephemer not just settings (which as you said will probably not a frequent scenario), but setting the other delegates. updateWithInfo will be used for certain apps that need to set their own delegates on the webViewEngines -- a minor case, but I encountered this just last week.
I don't think self.webViewEngine will fail in the case of your 'preferred approach' you mentioned since that is just the CDVPlugin object wrapper that will always be instantiated unless WKWebView is not available: https://github.com/apache/cordova-plugin-wkwebview-engine/blob/33a75172a1450e8922788cd23382cc6ec33845c7/src/ios/CDVWKWebViewEngine.m#L47-L49
I'll comment on your #8 pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine issue #7: Fixes CB-11074: WKWebView configur...
Posted by shazron <gi...@git.apache.org>.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
Please provide feedback on #13, I will be pulling that in soon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-wkwebview-engine issue #7: Fixes CB-11074: WKWebView configur...
Posted by shazron <gi...@git.apache.org>.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
Please see #13, and let's continue discussion there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org