Throw SyntaxError for non-turn/turns/stun URLs

Throw SyntaxError for non-turn/turns/stun URLs

This is a low risk change, it was previously TypeError.

Also make the error message more descriptive.

BUG=658423
Committed: https://crrev.com/8438659cb205725ac0bb3f09a27f994922d71737
Cr-Commit-Position: refs/heads/master@{#430653}

[foolip, 2016-10-25 13:17:16]

foolip@chromium.org changed reviewers:
+ hta@chromium.org

[hta - Chromium, 2016-10-25 13:43:43]

lgtm

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:315: "\".
URL scheme must be \"stun\", \"turn\" or \"turns\".");
Hyper-nit: Should one use a StringBuilder instead of string concatenation here,
just to be style-consistent?

[hta - Chromium, 2016-10-25 13:51:01]

oops:not quite lgtm

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:309: if
(!url.isValid() ||
Having to call back my LGTM: You have to throw a different message for the
"isValid = false" case, I think. Otherwise the error generated on
"turn:groovy_illegal!character" will be misleading.

[hta - Chromium, 2016-10-25 13:51:17]

not lgtm

[foolip, 2016-10-27 13:32:41]

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:309: if
(!url.isValid() ||
On 2016/10/25 13:51:01, hta - Chromium wrote:
> Having to call back my LGTM: You have to throw a different message for the
> "isValid = false" case, I think. Otherwise the error generated on
> "turn:groovy_illegal!character" will be misleading.

exceptionState.throwDOMException(SyntaxError, "'" + url + "' is not a valid
URL.") is used in a bunch of places, so I think I'll copy that.

But I can't find a string starting with "turn:" that KURL considers invalid.
What kind of groovy characters should I try? I've tried ! [ :

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:315: "\".
URL scheme must be \"stun\", \"turn\" or \"turns\".");
On 2016/10/25 13:43:43, hta - Chromium wrote:
> Hyper-nit: Should one use a StringBuilder instead of string concatenation
here,
> just to be style-consistent?

Some places use String::format(), and a bunch use the "'" + url + "..." that
I've copied. Happy to convert to any form, I guess whatever produces a smaller
binary is better given that perf doesn't matter here.

[foolip, 2016-10-27 13:34:39]

https://codereview.chromium.org/2448843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2448843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:317:
SyntaxError, "'" + url.protocol() +
This really isn't a SyntaxError, do we care? I don't care enough to ask to have
the spec changed :)

[hta - Chromium, 2016-11-08 12:12:30]

lgtm

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:309: if
(!url.isValid() ||
On 2016/10/27 13:32:41, foolip wrote:
> On 2016/10/25 13:51:01, hta - Chromium wrote:
> > Having to call back my LGTM: You have to throw a different message for the
> > "isValid = false" case, I think. Otherwise the error generated on
> > "turn:groovy_illegal!character" will be misleading.
> 
> exceptionState.throwDOMException(SyntaxError, "'" + url + "' is not a valid
> URL.") is used in a bunch of places, so I think I'll copy that.
> 
> But I can't find a string starting with "turn:" that KURL considers invalid.
> What kind of groovy characters should I try? I've tried ! [ :

:-) on the error generating problem - typical "syntax too permissive, please try
again"!

Query: Does turn:%GG (illegal hex escape) give !isValid?

https://codereview.chromium.org/2448843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2448843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:317:
SyntaxError, "'" + url.protocol() +
On 2016/10/27 13:34:38, foolip wrote:
> This really isn't a SyntaxError, do we care? I don't care enough to ask to
have
> the spec changed :)

Neither do I :-) It's an illegal parameter, as long as it fails consistently, it
seems good to me.

[foolip, 2016-11-08 15:39:07]

So, I'll go ahead and land this. Maybe there is no URL starting with 'turn:'
that's considered invalid, but even if there is (now or in the future) nothing
important will change, just the exception message.

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2448843003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:309: if
(!url.isValid() ||
On 2016/11/08 12:12:30, hta - Chromium wrote:
> On 2016/10/27 13:32:41, foolip wrote:
> > On 2016/10/25 13:51:01, hta - Chromium wrote:
> > > Having to call back my LGTM: You have to throw a different message for the
> > > "isValid = false" case, I think. Otherwise the error generated on
> > > "turn:groovy_illegal!character" will be misleading.
> > 
> > exceptionState.throwDOMException(SyntaxError, "'" + url + "' is not a valid
> > URL.") is used in a bunch of places, so I think I'll copy that.
> > 
> > But I can't find a string starting with "turn:" that KURL considers invalid.
> > What kind of groovy characters should I try? I've tried ! [ :
> 
> :-) on the error generating problem - typical "syntax too permissive, please
try
> again"!
> 
> Query: Does turn:%GG (illegal hex escape) give !isValid?

Nope, that's still considered valid.

[foolip, 2016-11-08 15:39:17]

The CQ bit was checked by foolip@chromium.org

[commit-bot: I haz the power, 2016-11-08 15:39:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 18:16:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-08 18:38:32]

Description was changed from

==========
Throw SyntaxError for non-turn/turns/stun URLs

This is a low risk change, it was previously TypeError.

Also make the error message more descriptive.

BUG=658423
==========

to

==========
Throw SyntaxError for non-turn/turns/stun URLs

This is a low risk change, it was previously TypeError.

Also make the error message more descriptive.

BUG=658423

Committed: https://crrev.com/8438659cb205725ac0bb3f09a27f994922d71737
Cr-Commit-Position: refs/heads/master@{#430653}
==========

[commit-bot: I haz the power, 2016-11-08 18:38:33]

Patchset 2 (id:??) landed as
https://crrev.com/8438659cb205725ac0bb3f09a27f994922d71737
Cr-Commit-Position: refs/heads/master@{#430653}