Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CB-7179 (iOS): Finish off WKWebView implementation #245

Closed
wants to merge 256 commits into from

Conversation

dpa99c
Copy link
Contributor

@dpa99c dpa99c commented Sep 27, 2017

Platforms affected

iOS

What does this PR do?

This PR builds on the initial work done by Shazron in https://github.com/apache/cordova-plugin-inappbrowser/tree/wkwebview:

  • Adds shared cookie pool via CDVWKProcessPoolFactory, enabling app Webview to share cookies with IAB Webviews (pulled from Niklas Merz)
  • Implements WKWebView delegates to make event handlers and script injection work
  • Replaces iframe bridge with WKScriptMessageHandler bridge
  • Adds a dependency on cordova-plugin-wkwebview-engine, since the new implementation relies on it to provide a WKWebView.
  • Updates other platforms of the wkwebview branch to align with master at 9024275

What testing has been done on this change?

Run automated tests as outlined here.
All tests pass for iOS platform:

Note: other platforms not tested because they have not been changed.

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
    • CB-7179 (iOS): Rework iOS implementation to use WKWebView instead of UIWebView. Adds dependency on cordova-plugin-wkwebview-engine.
  • Added automated test coverage as appropriate for this change.
    • No additional test coverage needed as no functionality has changed, just the implementation.

SomaticIT and others added 30 commits July 11, 2014 21:43
also added resources and fixed file paths, renamed test dir, added nested plugin.xml

github: close 55
- move the resources from /resources to /cdvtests/iab-resources
- change the url of the resources from absolute to relative
- explicitly set the background color of local.html to be white because it was inheriting black
- add the js to display the user-agent on the tests menu
- change the www.google.com URL from http to https to avoid an unexpected redirect
- fix the paths to the injected resources
- update the urls to Google home page to use https to avoid redirects
- get the user agent to display on the inject.html page
This change supports Android and Amazon Fire OS
Added logs and corrected indentation according to 81161eb
dpa99c referenced this pull request in dpa99c/cordova-plugin-inappbrowser-wkwebview Sep 28, 2017
…UIWebView. Adds dependency on cordova-plugin-wkwebview-engine.

Fix content sizing bug caused by 4573c87d0b74c087ef35e40f4311674bc92e7947
@infil00p
Copy link
Member

This needs to be cleaned up, since there's too many commits from other people in this PR. Can you do this PR again, this time with just the code that you authored in the PR?

@infil00p infil00p closed this Jan 17, 2018
@dpa99c
Copy link
Contributor Author

dpa99c commented Jan 17, 2018

@infil00p The original apache/wkwebview branch that I made as the merge target was last updated by @shazron in 2014, so is majorly out of date with respect to apache/master. So my dpa99c/wkwebview fork is based on apache/master at the time the PR was created.

Would you rather I fork the current apache/master and apply my WKWebView changes to that?
Because applying them to the 2014 apache/wkwebview doesn't seem that useful...

@dpa99c
Copy link
Contributor Author

dpa99c commented Jan 18, 2018

@infil00p @stevengill @agrieve @shazron @purplecabbage How should I proceed with this?

I have a fork of this plugin as cordova-plugin-inappbrowser-wkwebview which implements WKWebview for the InappBrowser for iOS.
It's fairly battle-hardened now as it's being used by around 1/4 million users per month (both Android & iOS) in production apps.

The implementation is based on @shazron's apache/wkwebview branch from 2014, but I've committed it against a fork of apache/master since this is much more update-to-date with respect to other platforms than apache/wkwebview.

The current implementation drops support for the UIWebView implementation in favour of WKWebView, so relies on cordova-plugin-wkwebview-engine to be present in the project.
Should I attempt to modify it to conditionally switch between the UIWebView and WKWebView implementations based on the presence of cordova-plugin-wkwebview-engine?
I think it may be a fair amount of work to do this though...

@infil00p
Copy link
Member

infil00p commented Apr 18, 2018

@dpa99c We didn't see this comment on the closed commit until someone pointed this out on the vote thread. If you could modify it to conditionally switch, that would make it easier, but I'm also not the authority on iOS (I've only recently been starting to triage the iOS issues). I would send the PR as is for discussion, or start a discussion on the dev list about this.

@dpa99c
Copy link
Contributor Author

dpa99c commented Jun 5, 2018

@infil00p UIWebView deprecated on iOS 12 - OK, time to revive this.

I'll see if I can put together a dual implementation which enables you to switch between UIWebView and WKWebView in IAB. I should be able to make that PR against master since it will be suitable for mainstream release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.