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.

Sign up for GitHub

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

Jump to bottom

Html coop update #11

Closed
wants to merge 6 commits into from
Closed

Html coop update #11

wants to merge 6 commits into from

Conversation

jugglinmike
Copy link
Member

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

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

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

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

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

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

@zcorpan zcorpan zcorpan approved these changes

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