Skip to content

Html coop update#11

Closed
jugglinmike wants to merge 6 commits intomaster-coop-coepfrom
html-coop-update
Closed

Html coop update#11
jugglinmike wants to merge 6 commits intomaster-coop-coepfrom
html-coop-update

Conversation

@jugglinmike
Copy link
Member

@jugglinmike jugglinmike commented Dec 18, 2019

No description provided.

@jugglinmike
[html] Update COOP tests
4090a6e 
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 unsafe-allow-outgoing` has been removed and no equivalent
  value is available

Reflect these changes in the relevant tests.
@jugglinmike
[html] Test explicitly-set default COOP value
37a11fa
@jugglinmike
fixup! [html] Update COOP tests
eeea7d0
@jugglinmike
fixup! [html] Update COOP tests
93b58fb 
Add historical tests
@jugglinmike
Copy link
Member Author

jugglinmike commented Dec 18, 2019 •
edited
Loading

@zcorpan I've filed this as a pull request against bocoup/wpt so discussion is
easier to track. Really should have done this from the beginning.

same-site should not be supported.

Instead of removing tests for previous values, it would be good to verify
that they are treated the same as jibberish, as "historical" tests (and
mark such tests as historical somehow, maybe just with a JS line comment).

(via web-platform-tests@4090a6e)

I've added individual cases to the files for valid values and annotated as
you've suggested. In addition, I've revived the files dedicated to the
historical values, but I've relocated them in a new directory named
"historical."

Use same-origin instead of same-site since the latter should not be
supported.

(via web-platform-tests@bb1376b#commitcomment-36480961)

Definitely

As far as I can tell, it's possible to get duplicated headers with a
.headers file. I think that is nicer than .asis.

(via web-platform-tests@bb1376b#commitcomment-36481209)

And I think .asis is nicer :) We won't be able to move forward without more
substantive rationale, so I'll start.

The benefit of an .asis file is that all relevant information is expressed in
one place. Using .headers means that readers need to open two files to
understand the intent of a given file, and that assumes they even know to look.

@jugglinmike
Copy link
Member Author

jugglinmike commented Dec 18, 2019

I confused myself and added irrelevant commentary in my previous message. I've redacted that, and I'll include it in a forthcoming pull request. Sorry for the noise!

zcorpan
zcorpan approved these changes Dec 19, 2019
View reviewed changes
Copy link

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

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

LGTM except the relative path needs updating in historical

html/cross-origin-opener-policy/historical/popup-same-site.https.html Outdated
<script src=/resources/testharness.js></script>
<script src=/resources/testharnessreport.js></script>
<script src="/common/get-host-info.sub.js"></script>
<script src="resources/common.js"></script>
Copy link

@zcorpan zcorpan Dec 19, 2019

Choose a reason for hiding this comment

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

Suggested change
<script src="resources/common.js"></script>
<script src="../resources/common.js"></script>

html/cross-origin-opener-policy/historical/popup-same-site-unsafe-allow-outgoing.https.html Outdated
<script src=/resources/testharness.js></script>
<script src=/resources/testharnessreport.js></script>
<script src="/common/get-host-info.sub.js"></script>
<script src="resources/common.js"></script>
Copy link

@zcorpan zcorpan Dec 19, 2019

Choose a reason for hiding this comment

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

Suggested change
<script src="resources/common.js"></script>
<script src="../resources/common.js"></script>

html/cross-origin-opener-policy/historical/popup-same-origin-unsafe-allow-outgoing.https.html Outdated
<script src=/resources/testharness.js></script>
<script src=/resources/testharnessreport.js></script>
<script src="/common/get-host-info.sub.js"></script>
<script src="resources/common.js"></script>
Copy link

@zcorpan zcorpan Dec 19, 2019

Choose a reason for hiding this comment

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

Suggested change
<script src="resources/common.js"></script>
<script src="../resources/common.js"></script>

@jugglinmike
Copy link
Member Author

jugglinmike commented Dec 19, 2019

Good catch, Simon! I've fixed that, rebased, and submitted the result to WPT: web-platform-tests#20874

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@zcorpan zcorpan zcorpan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jugglinmike @zcorpan