Make hostname state return early if a port is given

[TimothyGu]

Fixes #601.

• At least two implementers are interested (and none opposed):
  • Firefox (already implemented)
  • Safari (already implemented)
• Tests are written and can be reviewed and commented upon at:
  • url: expect hostname setter no-op if a port is given web-platform-tests/wpt#29021
• Implementation bugs are filed:
  • Chrome: https://crbug.com/1018721
  • Node.js: WHATWG URL spec change: Make hostname setter no-op if a port is given nodejs/node#38710
  • whatwg-url: Make hostname setter return failure if a port is given jsdom/whatwg-url#183

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

[annevk]

Thanks!

(I wondered if this should be a validation error, but that doesn't seem to make sense for API-specific conditions.)

[annevk]

Could you also add a test for location.hostname throwing for the same input?

[TimothyGu]

@annevk What do you mean? The setter steps don't seem to have any provision for throwing on URL parsing failure.

[annevk]

Oh, sorry, I somehow assumed Firefox's behavior was correct. It seems that Safari does navigate to the current URL. And Chrome ignores the : as per the current specification. So it does seem like something that ought to be tested, but the pass condition should probably match Safari?

[TimothyGu]

@annevk I agree that more location tests would be useful, though to be fair, Chrome doesn't exactly follow the current spec:

And if the current URL doesn't have a port while the hostname has a port, the hostname's port is used.

In fact, all the Location properties could probably use some more setter tests.

(And yes, Safari is the one we would align to.)

---

By the way, I don't have write permissions to this repo, so you'll have to merge this PR.

[annevk]

I see, I had not tested Chrome when setting from a URL with a port. Alright, I guess we can merge this and consider the Location issue somewhat distinct. Could you file an issue to track that? Either against WPT or HTML?

[annevk]

Actually, I don't see why this needs to return failure. I think we only return failure at the moment if it's relevant for the caller. Otherwise we just return. (That's also why I thought the Location setter would throw.)

[TimothyGu]

I guess we can merge this and consider the Location issue somewhat distinct. Could you file an issue to track that? Either against WPT or HTML?

There's already web-platform-tests/wpt#4103 to track Location stuff.

Actually, I don't see why this needs to return failure. I think we only return failure at the moment if it's relevant for the caller. Otherwise we just return.

Fixed.

[annevk]

Thanks!

[rmisev]

It seems a bit late now, but I think if host with port is given, then 2.2. step (added by this PR) must emit validation error.
Such practices are used in other steps (for example in 3.2.) when given value is not accepted.
If you agree, I can prepare PR.

[annevk]

@rmisev I considered that, but I thought we didn't do it for override cases?

[rmisev]

Yeah, it seems the only case, when override emits validation error, is 3.2. step in the host & hostname state.