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

Set CDVWebViewProcessPoolFactory to public #1031

Merged
merged 2 commits into from
Jan 7, 2021

Conversation

NiklasMerz
Copy link
Member

@NiklasMerz NiklasMerz commented Nov 25, 2020

Platforms affected

iOS

Motivation and Context

The InAppBrowser used to import the process pool from the wkwebview plugin. Now the processpool is in the iOS platform and got renamed. Because it's in the private part InAppBrowser can never import it and use the shared pool.

See this issue: apache/cordova-plugin-inappbrowser#751 and PR apache/cordova-plugin-inappbrowser#825

Description

I just set the file to public in Xcode.

From my understanding this should be public for plugins to use (like IAB to share Cookies). I am not sure if this is the right way to make this public though. Are there any other problems for having this private?

Testing

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@codecov-io
Copy link

codecov-io commented Nov 25, 2020

Codecov Report

Merging #1031 (bee6a5f) into master (e92f653) will increase coverage by 74.91%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1031       +/-   ##
===========================================
+ Coverage        0   74.91%   +74.91%     
===========================================
  Files           0       13       +13     
  Lines           0     1718     +1718     
===========================================
+ Hits            0     1287     +1287     
- Misses          0      431      +431     
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/Podfile.js 73.20% <0.00%> (ø)
bin/templates/scripts/cordova/lib/run.js 19.62% <0.00%> (ø)
bin/templates/scripts/cordova/lib/check_reqs.js 46.93% <0.00%> (ø)
bin/templates/scripts/cordova/lib/versions.js 100.00% <0.00%> (ø)
bin/templates/scripts/cordova/Api.js 71.53% <0.00%> (ø)
...ates/scripts/cordova/lib/plugman/pluginHandlers.js 90.05% <0.00%> (ø)
bin/templates/scripts/cordova/lib/projectFile.js 95.00% <0.00%> (ø)
...in/templates/scripts/cordova/lib/BridgingHeader.js 95.45% <0.00%> (ø)
bin/templates/scripts/cordova/lib/listDevices.js 100.00% <0.00%> (ø)
bin/templates/scripts/cordova/lib/build.js 51.87% <0.00%> (ø)
... and 3 more

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 e92f653...bee6a5f. Read the comment docs.

@dpogue
Copy link
Member

dpogue commented Nov 25, 2020

The problem is that this is not part of our public API, it's part of a built-in internal plugin. It lives in Classes/Private/Plugins rather than Classes/Public.

CDVWebViewProcessPoolFactory is fairly small and self-contained though, so maybe it makes sense to move that one class into the public API?

@NiklasMerz
Copy link
Member Author

By setting the checkbox it becomes public and works. But I would agree that moving it to Classes/Public would make sense since it should be part of the public API.

@dpogue dpogue added this to the 6.1.2 milestone Nov 25, 2020
@NiklasMerz NiklasMerz requested a review from dpogue November 26, 2020 07:45
@NiklasMerz
Copy link
Member Author

I did another commit and moved the class to the Classes/Public folder. From my understanding this could be public like it used to be in WKWebView engine plugin. Now the InAppBrowser can use the pool again.

Does anyone know reasons why this is private with the other plugins? Any known issues in the past?

Copy link
Member

@timbru31 timbru31 left a comment

Choose a reason for hiding this comment

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

LGTM

@NiklasMerz
Copy link
Member Author

NiklasMerz commented Jan 6, 2021

Thanks for the reviews. If nobody objects I will merge this in 24 hours together with apache/cordova-plugin-inappbrowser#825.

@NiklasMerz NiklasMerz merged commit 682ed75 into apache:master Jan 7, 2021
msmtamburro added a commit to msmtamburro/cordova-ios that referenced this pull request Oct 19, 2022
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.

5 participants