search

Cartegraph / cordova-cookie-master

MIT License
7 stars 10 forks source link

Fixed clearCookies function for WkWebView #2

Open davidestrati opened 4 years ago

davidestrati commented 4 years ago

This is using the proper method to clear the cookies when you are using WkWebview and have iOS 11 or above. Plus it will maintain compatibility for lower version of iOS using the legacy method.

KevinKelchen commented 4 years ago

Also, I'm not sure what was behind all the intermediate commits to the plugin.xml file regarding the AndroidManifest.xml path. Regarding the commit message to make it work with Cordova versions greater than 7, this plugin already works with Cordova 9 (which is what our app uses). In the end it looks like no changes were ultimately made though.

If we merge this PR we will likely squash those commits to keep the repo history clean.

davidestrati commented 4 years ago

Hi Kevin,

Sorry for the long time that this took up.

I implemented your suggestions and modified the setCookie method as long as the clearCookie one. I do believe that this method could still be useful in some rare occasions or in applications that do not take advantage of cookie retention and just need to delete them.

As regards the multiple commits for cordova 9 compatibility, it was clearly my mistake, so you can ignore and squash them if you like.

Thank you for all your suggestions.

KevinKelchen commented 4 years ago

There appear to be some discrepancies from the suggested changes.

  1. pluginResult is not declared at the top of the method.
  2. Using iOS 9 as the version check instead of being consistent with setCookie's iOS 11 check. What was the reason for this?
  3. Some indentation is off in the UIWebView-related else statement.
  4. The synchronize call is not moved into the UIWebView-related else statement.

I would have thought we could have accepted the entire suggested change in the PR which would have prevented these issues. Or at least a blanket copy paste. Were there other issues involved with the suggested changes?

I'd be happy to open a new PR to make those changes if that is helpful. 🙂

davidestrati commented 4 years ago

Hi Kevin.

I improved the PR with your suggested changes.

The reason why I used iOS 9 check instead of 11, was that the method that it is using requires iOS 9 or greater. However I replaced it with iOS 11 for consistency.

As regards the original suggested changes, I tried to merge them with my modifications in order to make clearCookies method work correctly.

If you believe that this PR is not relevant or useful anymore, feel free to close it. I just wanted to help! 😅

Regards