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

Normalize port after updating scheme #328

Merged
merged 2 commits into from
Jun 30, 2017
Merged

Normalize port after updating scheme #328

merged 2 commits into from
Jun 30, 2017

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Jun 27, 2017 •
edited by pr-preview bot
Loading

@TimothyGu TimothyGu mentioned this pull request Jun 27, 2017
Merged
annevk
annevk approved these changes Jun 28, 2017
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

@domenic do you want to update whatwg-url and then I land this and the tests?

@domenic
Copy link
Member

domenic commented Jun 28, 2017

Can do.

@annevk
Copy link
Member

annevk commented Jun 28, 2017

It seems like we should also file a bug against Edge. @TimothyGu are you up for that or shall I do it?

rmisev
rmisev requested changes Jun 28, 2017
View reviewed changes
url.bs Outdated
@@ -1400,6 +1400,9 @@ string <var>input</var>, optionally with a <a>base URL</a> <var>base</var>, opti

<li><p>Set <var>url</var>'s <a for=url>scheme</a> to <var>buffer</var>.

<li><p>If <var>url</var>'s <a for=url>port</a> is <var>url</var>'s <a for=url>scheme</a>'s
<a>default port</a>, then set <var>url</var>'s <a for=url>port</a> to null.
Copy link
Member

@rmisev rmisev Jun 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This step is required for the protocol setter only, so it's better to place it after "4. If state override is given":

  1. Set url ’s scheme to buffer
  2. Set buffer to the empty string
  3. If state override is given, then:
    1. If url ’s port is url’s scheme’s default port , then set url’s port to null.
    2. Return.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, I think I agree with this.

And while we're in the area, is there a reason resetting buffer (3) is necessary before returning from a state override (4)?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rmisev It's sort of odd to see "Return" on its own line, which was why I refrained from this style initally, but that's fine with me.

@GPHemsley not particularly. Changed.

@TimothyGu
Copy link
Member Author

@annevk Issued filed: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/12555073/

annevk
annevk approved these changes Jun 29, 2017
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks! I'll let @domenic do the honors.

@domenic
Copy link
Member

domenic commented Jun 30, 2017

whatwg-url confirms spec and test change are in harmony and don't break anything else; merge time!

@domenic domenic merged commit 0f53958 into whatwg:master Jun 30, 2017
domenic pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 30, 2017
@TimothyGu @domenic
URL: default port set to null after scheme update (#6346)
b30abae 
For whatwg/url#328.
@TimothyGu TimothyGu deleted the scheme-port branch June 30, 2017 00:06
domenic added a commit to jsdom/whatwg-url that referenced this pull request Jun 30, 2017
@domenic
Spec update: normalize port after updating scheme
1ea1209 
Follows whatwg/url#328.
TimothyGu added a commit to TimothyGu/node that referenced this pull request Jun 30, 2017
@TimothyGu
url: normalize port on scheme change
f49c371 
Refs: whatwg/url#328
@TimothyGu TimothyGu mentioned this pull request Jun 30, 2017
Closed
3 tasks
TimothyGu added a commit to nodejs/node that referenced this pull request Jul 5, 2017
@TimothyGu
url: normalize port on scheme change
b01ac75 
PR-URL: #13997
Refs: whatwg/url#328
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit to nodejs/node that referenced this pull request Jul 11, 2017
@TimothyGu @addaleax
url: normalize port on scheme change
9e87501 
PR-URL: #13997
Refs: whatwg/url#328
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit to nodejs/node that referenced this pull request Jul 18, 2017
@TimothyGu @addaleax
url: normalize port on scheme change
cf6afe3 
PR-URL: #13997
Refs: whatwg/url#328
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Fishrock123 pushed a commit to nodejs/node that referenced this pull request Jul 19, 2017
@TimothyGu @Fishrock123
url: normalize port on scheme change
c83b3ae 
PR-URL: #13997
Refs: whatwg/url#328
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
rmisev added a commit to upa-url/upa that referenced this pull request May 24, 2020
@rmisev
Spec: normalize port after scheme change
7575329 
Follows whatwg/url#328
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GPHemsley GPHemsley GPHemsley left review comments

@rmisev rmisev rmisev approved these changes

@annevk annevk annevk approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TimothyGu @domenic @annevk @GPHemsley @rmisev