[html] Update COOP tests#20874
Conversation
The possible values of the `Cross-Origin-Opener-Policy` header have changed since these tests were authored: - `same-origin unsafe-allow-outgoing` is now `same-origin-allow-popups` - `same-site` and `same-site unsafe-allow-outgoing` have been removed and no equivalent value is available Reflect these changes in the relevant tests. Introduce cases for the removed values to ensure that they are no longer honored.
annevk
left a comment
There was a problem hiding this comment.
Thank you so much for starting this!
|
Hi Mike, I imagine you went on vacations for the past weeks. Are you still actively planning to land this patch? I'll just drop mine if that is the case, since it's pretty much a duplicate. |
|
@hemeryar I'm planning to work on this PR this week. |
annevk
left a comment
There was a problem hiding this comment.
Highlighted the other boolean flip I was worried about. I think other than that this is probably fine, though it would help a lot if @hemeryar et al. verified with Chrome's implementation.
Another thing we should look into is whether we can reduce the number of step_timeout calls as they are really problematic for CI.
| // navigate that to either a same-origin or same-site document whose COOP and COEP are set as | ||
| // per the top-most array. We then verify that this document has the correct opener for its | ||
| // specific setup. | ||
| // navigate that to either a document with same origin (site=="same-origin") or |
There was a problem hiding this comment.
I think the original sentence might be clearer, but adding the parentheticals might be even better. This sentence however reads a little strange.
|
@ParisMeuleman @hemeryar any further thoughts here? |
| t.add_cleanup(() => { | ||
| popup.close(); | ||
| }); | ||
| window.functionCalledByOpenee = () => { |
There was a problem hiding this comment.
This should use t.step_func_done.
|
In the "wpt-firefox-nightly-stability" check |
|
Probably split them if you can as that seems unlikely to be fixed on the infrastructure side. |
|
Done. These tests have some repetition. A script to generate them could make them easier to maintain. |
|
Pulled the changes locally and ran the tests, everything seems to work fine on chromium. Only difference I could spot with Firefox (that already exists) is in: |
|
The rest looks great, thanks for the work! |
|
Thank you, @hemeryar! I think we should merge this, and do the |
|
By the way, @hemeryar, would you like to be in the wpt reviewers team (so you can approve and merge PRs), and be listed as a |
|
Sure! Could you also add @ParisMeuleman who is working with me on COOP for chromium? |
|
Certainly! I've invited you both. If accepting the invite caused you to watch this repo, you probably want to unwatch it. See https://github.com/orgs/web-platform-tests/teams/reviewers for details. There is also documentation available for reviewing tests. I've updated #20843 Thanks! |
|
I am surprised by the rename: We shouldn't define an HTTP header for this test. It already defines an HTML "meta" COOP header. The test check it isn't supported. FYI: @ParisMeuleman, @hemeryar |
|
@ArthurSonzogni good catch! I probably had that file for testing and accidentally added it here. Sorry about that. I can delete it in a new PR. |
Indeed, it would explain the failure @hemeryar got in https://crrev.com/c/1985013/16/third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/popup-meta-http-equiv.https-expected.txt#2 |
The possible values of the
Cross-Origin-Opener-Policyheader havechanged since these tests were authored:
same-origin unsafe-allow-outgoingis nowsame-origin-allow-popupssame-siteandsame-site unsafe-allow-outgoinghave been removedand no equivalent value is available
Reflect these changes in the relevant tests. Introduce cases for the
removed values to ensure that they are no longer honored.
Introduce new test cases covering when the default value is specified explicitly.