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-13382: (ios) Added destroyWebView method for CDVViewController #342

Closed
wants to merge 0 commits into from

Conversation

surajpindoria
Copy link
Member

@surajpindoria surajpindoria commented Oct 5, 2017

Platforms affected

iOS

What does this PR do?

Adds a method to destroy CDVViewController webview object

What testing has been done on this change?

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.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #342   +/-   ##
=======================================
  Coverage   63.43%   63.43%           
=======================================
  Files          14       14           
  Lines        1690     1690           
  Branches      283      283           
=======================================
  Hits         1072     1072           
  Misses        618      618

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 7b7964f...d5d0734. Read the comment docs.

@shazron
Copy link
Member

shazron commented Oct 6, 2017

Thanks Suraj, since it's a new feature, the release that includes this should be 4.6.0? (minor version update)

@@ -767,13 +767,23 @@ - (void)onAppDidEnterBackground:(NSNotification*)notification

// ///////////////////////

- (void)destroyWebView
{
self.webViewEngine = nil;
Copy link
Member

Choose a reason for hiding this comment

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

I think this won't do anything if we don't call all what you put on the dealloc. If we are going to offer this new method, it should do everything what is on the dealloc and dealloc just call it.

Anyway, the main problem of CB-13382 is that dealloc is not doing the complete cleanup of the webview, with the new code it will do it, so we don't really need to offer this new method as dealloc will do the cleaning automatically.

Copy link
Member

Choose a reason for hiding this comment

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

Woops I forgot about the dealloc in there since I haven't thought about it since everything is ARC. I think that we should just put self.webviewEngine = nil in the dealloc method, that would free it up I think.

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.

@jcesarmobile 's comment needs to be addressed

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.

4 participants