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

--site-isolation isn't running tests in a cross-origin iframe #25126

Conversation

charliewolfe
Copy link
Member

@charliewolfe charliewolfe commented Feb 26, 2024

be13d89

`--site-isolation` isn't running tests in a cross-origin iframe
https://bugs.webkit.org/show_bug.cgi?id=270114
rdar://123646865

Reviewed by Alex Christensen.

A typo was preventing it from being enabled.

Most tests actually fail, so only enable a few for now.

* LayoutTests/platform/mac-site-isolation/TestExpectations:
* Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:
(_set_up_derived_options):
* Tools/Scripts/webkitpy/port/driver.py:
(Driver.is_http_test):
* Tools/Scripts/webkitpy/port/driver_unittest.py:
* Tools/WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::invoke):
* Tools/WebKitTestRunner/TestOptions.cpp:
(WTR::TestOptions::defaults):
(WTR::TestOptions::keyTypeMapping):
* Tools/WebKitTestRunner/TestOptions.h:
(WTR::TestOptions::runInCrossOriginFrame const):
(WTR::TestOptions::runInCrossOriginIFrame const): Deleted.

Canonical link: https://commits.webkit.org/275837@main

67d618e

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 webkitpy ✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-skia
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv-sim ✅ 🧪 api-gtk
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@charliewolfe charliewolfe self-assigned this Feb 26, 2024
@charliewolfe charliewolfe added Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases skip-ews Applied to prevent a change from being run on EWS labels Feb 26, 2024
@charliewolfe charliewolfe marked this pull request as draft February 26, 2024 21:12
@@ -478,7 +478,7 @@ def _set_up_derived_options(port, options):
options.additional_platform_directory.insert(0, port.host.filesystem.join(host.scm().checkout_root, 'LayoutTests/platform/mac-gpup'))

if port.port_name == "mac" and options.site_isolation:
options.self_compare_with_header = 'SiteIsolationEnabled=true runInCrossOriginIframe=true'
options.self_compare_with_header = 'SiteIsolationEnabled=true runInCrossOriginIFrame=true'
Copy link
Member

Choose a reason for hiding this comment

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

Can we just make this directive runInCrossOriginFrame (no I) so we don't make this typo again? I only worked on site isolation for about a week and I messed up the capitalization almost every time I tried to use the directive in an HTML comment.

Copy link
Member

Choose a reason for hiding this comment

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

I should have probably made the fix myself but I thought I must be the only one making the mistake at the time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should do that before merging this.

@charliewolfe charliewolfe removed the skip-ews Applied to prevent a change from being run on EWS label Mar 8, 2024
@charliewolfe charliewolfe force-pushed the eng/--site-isolation-isnt-running-tests-in-a-cross-origin-iframe branch from c6dd252 to 67d618e Compare March 8, 2024 01:32
@charliewolfe charliewolfe requested a review from rreno March 8, 2024 01:33
@charliewolfe charliewolfe marked this pull request as ready for review March 8, 2024 01:33
@rreno
Copy link
Member

rreno commented Mar 8, 2024

LGTM thanks for making that change to help avoid typos. (don't have r+ authority yet)

@achristensen07 achristensen07 added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Mar 8, 2024
https://bugs.webkit.org/show_bug.cgi?id=270114
rdar://123646865

Reviewed by Alex Christensen.

A typo was preventing it from being enabled.

Most tests actually fail, so only enable a few for now.

* LayoutTests/platform/mac-site-isolation/TestExpectations:
* Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:
(_set_up_derived_options):
* Tools/Scripts/webkitpy/port/driver.py:
(Driver.is_http_test):
* Tools/Scripts/webkitpy/port/driver_unittest.py:
* Tools/WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::invoke):
* Tools/WebKitTestRunner/TestOptions.cpp:
(WTR::TestOptions::defaults):
(WTR::TestOptions::keyTypeMapping):
* Tools/WebKitTestRunner/TestOptions.h:
(WTR::TestOptions::runInCrossOriginFrame const):
(WTR::TestOptions::runInCrossOriginIFrame const): Deleted.

Canonical link: https://commits.webkit.org/275837@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/--site-isolation-isnt-running-tests-in-a-cross-origin-iframe branch from 67d618e to be13d89 Compare March 8, 2024 16:14
@webkit-commit-queue
Copy link
Collaborator

Committed 275837@main (be13d89): https://commits.webkit.org/275837@main

Reviewed commits have been landed. Closing PR #25126 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit be13d89 into WebKit:main Mar 8, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases
Projects
None yet
5 participants