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

feat: enable NodeIntegrationInSubFrames for webview #17226

Merged
merged 5 commits into from Mar 15, 2019

Conversation

brenca
Copy link
Contributor

@brenca brenca commented Mar 5, 2019

Description of Change

This was previously not done because of a crash. electron/node#94 fixes that crash, and allow us to enable this option for the webview. The node PR also fixes #16733.

Fixes #16780.

Checklist

Release Notes

Notes: Enabled NodeIntegrationInSubFrames option usage for webview tags.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Mar 5, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Mar 6, 2019
@brenca brenca force-pushed the brenca/node-integration-subframes-webview branch from e37b7f9 to b66755d Compare March 6, 2019 08:42
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM, can you fix the conflicts. Thanks!

@brenca
Copy link
Contributor Author

brenca commented Mar 7, 2019

@deepak1556 I'm on it

@brenca brenca force-pushed the brenca/node-integration-subframes-webview branch from b66755d to ef4c3da Compare March 7, 2019 19:42
@MarshallOfSound
Copy link
Member

This is a feature that is targeting 5, cc @electron/wg-releases

@brenca brenca force-pushed the brenca/node-integration-subframes-webview branch 3 times, most recently from 2bd6def to bf1ff75 Compare March 8, 2019 12:21
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM, approving for the native changes. Would love review on tests.

Its just a personal style, better to duplicate the tests with their own configs rather than automating different configs with single variation. The latter seems hard on review.

@brenca
Copy link
Contributor Author

brenca commented Mar 8, 2019

@deepak1556 I followed the existing code's style with the test generation, and I think it makes sense here to not duplicate code just for those flipped flags, but I'm happy to change it up. I'll ask some folks for their opinion.

@MarshallOfSound
Copy link
Member

Personally I'd rather not duplicate the tests because the intention here is to test that the behavior is identical for different configurations.

If we duplicate then they will diverge or it will become a pain to update in the future. We've used this test generation pattern in quite a few places now

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

Approving on behalf of Releases WG, and will do the same for the 5-0-x bp, which unfortunately is going to need to be manual.

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

For the record, @electron/wg-releases approved this for 5-0-x in the 2019-03-13 meeting

@brenca
Copy link
Contributor Author

brenca commented Mar 13, 2019

Thanks for the update guys! Fortunately the backport only failed because of the ongoing typescript conversion, should be straightforward to migrate the conflicting changes back to JS. I'll get to it once this is merged and I have internet again (probably tomorrow at the soonest)

@MarshallOfSound MarshallOfSound merged commit 43ef561 into master Mar 15, 2019
@release-clerk
Copy link

release-clerk bot commented Mar 15, 2019

Release Notes Persisted

Enabled NodeIntegrationInSubFrames option usage for webview tags.

@MarshallOfSound MarshallOfSound deleted the brenca/node-integration-subframes-webview branch March 15, 2019 17:39
@trop
Copy link
Contributor

trop bot commented Mar 15, 2019

I was unable to backport this PR to "5-0-x" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Mar 15, 2019

A maintainer has manually backported this PR to "5-0-x", please check out #17398

1 similar comment
@trop
Copy link
Contributor

trop bot commented Mar 19, 2019

A maintainer has manually backported this PR to "5-0-x", please check out #17398

@sofianguy sofianguy added this to Fixed in 5.0.0-beta.6 in 5.0.x Mar 20, 2019
@sofianguy sofianguy removed this from Fixed in 5.0.0-beta.6 in 5.0.x Mar 20, 2019
kiku-jw pushed a commit to kiku-jw/electron that referenced this pull request May 16, 2019
* feat: enable nodeIntegrationInSubFrames for webview

* test: add tests

* docs: document webview's nodeintegrationinsubframes

* lint: fix indent

* fix: resolve some merge bloopers
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.

Support new nodeIntegrationInSubFrames in webviews Issue with new nodeIntegrationInSubFrames config
5 participants