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-14076: (ios) Setting UIWebViewDelegate on CDVWebViewEngineProtocol doesn't work #365

Merged
merged 1 commit into from
May 15, 2018

Conversation

dpolivy
Copy link
Contributor

@dpolivy dpolivy commented May 14, 2018

Attempts to use CDVWebViewEngineProtocol::updateWithInfo to set a new UIWebViewDelegate were not working; instead of setting the passed in delegate, it was setting the CDVViewController instance as the delegate instead.

Platforms affected

cordova-ios

What does this PR do?

This commit fixes the CDVWebViewEngine class so that the proper delegate is ultimately passed in to the UIWebView when calling updateWithInfo.

What testing has been done on this change?

I used the code to create a new test app, to which I applied a test plugin which attempts to set the UIWebViewDelegate. I verified that I was able to successfully set the delegate, and receive the events from the UIWebView.

I also ran npm test and verified all tests passed.

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.
  • Added automated test coverage as appropriate for this change.

…'t work

Attempts to use `CDVWebViewEngineProtocol::updateWithInfo` to set a new `UIWebViewDelegate` were not working; instead of setting the passed in delegate, it was setting the `CDVViewController` instance as the delegate instead.

This commit fixes the code so that the proper delegate is ultimately passed in to the `UIWebView`.
@codecov-io
Copy link

Codecov Report

Merging #365 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #365   +/-   ##
=======================================
  Coverage   63.99%   63.99%           
=======================================
  Files          14       14           
  Lines        1697     1697           
  Branches      286      286           
=======================================
  Hits         1086     1086           
  Misses        611      611

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 634176b...d24fe18. Read the comment docs.

Copy link
Member

@shazron shazron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Unfortunately this doesn't cover the case when the delegate is not set. Basically set it to the self.viewController like the existing code (in an else statement)

@shazron
Copy link
Member

shazron commented May 15, 2018

Sorry I take that back, it is already set to the default in pluginInitialize

@shazron shazron merged commit 191c17e into apache:master May 15, 2018
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.

None yet

3 participants