Skip to content

Conversation

TimothyGu
Copy link
Member

Refs: whatwg/url#297
Refs: web-platform-tests/wpt#5944

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels May 20, 2017
const unicodeHost = unicode ? domainToUnicode(host) : host;
return `${scheme}//${unicodeHost}${port === null ? '' : `:${port}`}`;
// Refs: https://html.spec.whatwg.org/multipage/browsers.html#ascii-serialisation-of-an-origin
function serializeTupleOrigin(scheme, host, port) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe keep https://html.spec.whatwg.org/multipage/browsers.html#unicode-serialisation-of-an-origin or quote the Note: There used to also be a Unicode serialization of an origin. However, it was never widely adopted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a need to? This function is purely internal, and there is no remnant from the previous Unicode serialization that needs explanation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the standard kept a "there was something here", I thought we could emulate.
But I yield to your decision.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Lgtm. This part was always a bit sketchy to me since I knew browsers weren't consistent. Glad to see this fixed

@joyeecheung
Copy link
Member

@TimothyGu
Copy link
Member Author

Landed in 413691f.

@TimothyGu TimothyGu closed this May 25, 2017
@TimothyGu TimothyGu deleted the url-origin-ascii branch May 25, 2017 17:33
TimothyGu added a commit that referenced this pull request May 25, 2017
PR-URL: #13126
Refs: whatwg/url#297
Refs: web-platform-tests/wpt#5944
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell pushed a commit that referenced this pull request May 25, 2017
PR-URL: #13126
Refs: whatwg/url#297
Refs: web-platform-tests/wpt#5944
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell pushed a commit that referenced this pull request May 28, 2017
PR-URL: #13126
Refs: whatwg/url#297
Refs: web-platform-tests/wpt#5944
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
TimothyGu added a commit to TimothyGu/node that referenced this pull request Jul 25, 2017
Fixes: 413691f "url: expose WHATWG url.origin as ASCII"
Refs: nodejs#13126
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants