-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[Origin API] Add more test coverage for opaque origin comparison #56922
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
[Origin API] Add more test coverage for opaque origin comparison #56922
Conversation
7b9fa1b to
fdd7cd3
Compare
fdd7cd3 to
2efdaa1
Compare
|
I pushed another commit to remove the file scheme URL test, as that has implementation defined behavior (discovered playing around making it not a tuple origin). Though actually, as I write this, maybe a test should instead be in a different file checking that: const fileSchemeIsOpaque = new URL('file:///something/').origin === 'null' ;
assert_eq(fileSchemeIsOpaque, Origin.from('file:///something/').opaque); |
annevk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great @shannonbooth, thanks for writing them! I'll see if I need to change WebKit in a minute.
For file: URLs it seems reasonable to add .tentative test expecting an opaque origin.
I was hoping at some point we could at least agree that file: URL origins would always serialize as "null", but that didn't work out, but it might be time to try again.
9fe3102 to
b9217b3
Compare
Sure, added a tentative test to check that a file URL has an opaque origin. I'm not certain if the same origin behavior would tentatively be the same as all other opaque origin schemes, but I also added tests for that. FWIW I am trying to make progress on moving towards opaque file origins in ladybird (LadybirdBrowser/ladybird#7309 makes file origins opaque by default) to try and move closer to match other browsers behavior / security etc. The main point of investigation I have for this at the moment is on how to handle file scheme URLs in fetch, so hopefully I might be able to help making progress on whatwg/fetch#1195 depending on how far I manage to progress 😄 |
annevk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good modulo nit. Thanks again!
b9217b3 to
901afb9
Compare
mikewest
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too. I have small nits to improve clarity, but neither is important.
901afb9 to
5f23986
Compare
cc @mikewest @annevk
Relating to whatwg/url#892 and whatwg/html#12058
One case which I didn't figure out a nice way of covering is opaque origin Worker which is not a data scheme URL