Cleanup origin serialization #870
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
@SimonSapin, you might want to review this. |
source
Outdated
| part of the <span>origin</span> tuple, and append the results — each component, in the same | ||
| order, separated by U+002E FULL STOP characters (.) — to <var>result</var>. <ref spec=URL></p></li> | ||
| <li><p>Let <var>unicodeHost</var> be <var>host</var> if <var>host</var> is not a | ||
| <span>domain</span>, and the result of applying <span>domain to Unicode</span> to <var>host</var> |
Contributor
|
(As a side note, I find it hard to find the one thing that changed when a paragraph is almost entirely red/green in the diff because it was re-wrapped. Please consider http://rhodesmill.org/brandon/2012/one-sentence-per-line/. Maybe not to rewrap everything at once, but for paragraphs that you otherwise edit.) |
Contributor
|
Looks good to me. |
annevk
added
the
do not merge yet
Member
Author
|
FWIW, I added the do not merge yet label because it needs to land after #868 and I need to fix the nit above. |
|
|
||
|
|
||
| <dt><dfn data-x="concept-origin-opaque-identifiers">Opaque identifiers</dfn></dt> | ||
| <dt><dfn data-x="concept-origin-opaque-identifier">Opaque identifiers</dfn></dt> |
Member
There was a problem hiding this comment.
Why change these and break incoming links?
annevk
force-pushed
the
origin-serialization
branch
from
e3dffca to
de54b41
Compare
Member
|
LGTM, land at will |
Origin serialization didn’t properly serialize hosts and ports and used the domain to Unicode and domain to ASCII algorithms incorrectly, passing domain labels rather than domains (not accounting for the fact that the host might not be a domain either). This depends on #868 landing since it assumes aliasing is no longer a thing. This also depends on the URL Standard changing to no longer pass the default port when computing the origin of a URL. Instead, an origin and a URL should share the same data model subset, where port can be null. Since they serialise the same way, that makes things easier to understand. See whatwg/url#102 for that. This fixes #611, https://www.w3.org/Bugs/Public/show_bug.cgi?id=28788, and https://www.w3.org/Bugs/Public/show_bug.cgi?id=29056.
annevk
force-pushed
the
origin-serialization
branch
from
de54b41 to
5dcc1ee
Compare
domenic
added a commit
to jsdom/whatwg-url
that referenced
this pull request
Apr 15, 2016
See whatwg/html#870 and whatwg/url@b0e4def. I don't believe this changes any observable output; it is just a simplification.
3 tasks
mrobinson
added a commit
to servo/rust-url
that referenced
this pull request
Oct 31, 2024
The URL specification changed in 2016 [1] [2] such that now the origin's port should be the same as the URL's port. This means that the port can `None` in the case that it is the default port for a scheme. 1. whatwg/url@b0e4def 2. whatwg/html#870 Fixes #821.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Origin serialization didn’t properly serialize hosts and ports and
used the domain to Unicode and domain to ASCII algorithms
incorrectly, passing domain labels rather than domains (not
accounting for the fact that the host might not be a domain either).
This depends on #868 landing since it assumes aliasing is no longer a
thing.
This also depends on the URL Standard changing to no longer pass the
default port when computing the origin of a URL. Instead, an origin
and a URL should share the same data model subset, where port can be
null. Since they serialise the same way, that makes things easier to
understand.
This fixes #611,
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28788, and
https://www.w3.org/Bugs/Public/show_bug.cgi?id=29056.