New Uri.parse and validation.

New Uri.parse and validation.

Ensures that the resulting URI is valid.
Escapes invalid characters except the gen-delim group of delimites, where
escapes are allowed.

R=ajohnsen@google.com, sgjesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37479

[Anders Johnsen, 2014-06-17 13:07:04]

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:204: break authEnd;
Consider alternative without authEnd block.

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:253: toPath: {
Consider doing:

bool hasPath = true;
while(...) {...}

if (hasPath) {
  // enter 'toPath' code
}

// enter 'endPath' code

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:306: if (char== _QUESTION || char == _NUMBER_SIGN) break
endPath;
Add missing space.

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:917: static String _makeScheme(String scheme, int start,
int end) {
start is redundant, as it's always 0.

https://codereview.chromium.org/335373003/diff/10003/tests/corelib/uri_test.dart
File tests/corelib/uri_test.dart (right):

https://codereview.chromium.org/335373003/diff/10003/tests/corelib/uri_test.d...
tests/corelib/uri_test.dart:330: // TODO(lrn): Also do path normalization: /./
-> / and /x/../ -> /
Is this TODO a test or Uri todo?

https://codereview.chromium.org/335373003/diff/10003/tests/corelib/uri_test.d...
tests/corelib/uri_test.dart:472: String dump(Uri uri) {
Debug stuff?

[Lasse Reichstein Nielsen, 2014-06-17 14:31:45]

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:204: break authEnd;
Considered. See suggestion.

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:253: toPath: {
Ack, my precious flow control!!!

See alternative version.

I don't think it's prettier!

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:306: if (char== _QUESTION || char == _NUMBER_SIGN) break
endPath;
On 2014/06/17 13:07:03, Anders Johnsen wrote:
> Add missing space.

Done.

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:917: static String _makeScheme(String scheme, int start,
int end) {
True. And I know.

I have a dream of making Uri.parse take start and end too, but I guess I can
reintroduce the start if that ever happens.
Removed.

(Arguably, the end of fragment is also always uri.length, but let's not go
there).

https://codereview.chromium.org/335373003/diff/10003/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:1670: static List<int> parseIPv6Address(String host, [int
start = 0, int end]) {
This is a public method that I added start/end to.

I could make a private method with the start/end arguments, and have the public
just take one argument and forward to the private one.

WDYT?

https://codereview.chromium.org/335373003/diff/10003/tests/corelib/uri_test.dart
File tests/corelib/uri_test.dart (right):

https://codereview.chromium.org/335373003/diff/10003/tests/corelib/uri_test.d...
tests/corelib/uri_test.dart:330: // TODO(lrn): Also do path normalization: /./
-> / and /x/../ -> /
It's a Uri-TODO, so I'll remove it from here.

https://codereview.chromium.org/335373003/diff/10003/tests/corelib/uri_test.d...
tests/corelib/uri_test.dart:472: String dump(Uri uri) {
Actually used in the error messages of some of the Expect.equals calls above.
Makes debugging much easier when a test fails :)

[Søren Gjesse, 2014-06-19 07:39:44]

LGTM!

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:119: * Creates a new URI object by parsing a URI string.
This comment needs to be updated...

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:207: if (char == _AT_SIGN) {
So you can have @ and : in the username?

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:267: int state = STATE_PATH_END;
It is somewhat confusing that this state value does not reflect the actual
initial state. As far as I can see below you always set it anyway. How about a
STATE_BEFORE_PATH?

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:275: if (index == 0) {
Conditional expression?

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:1096: if (char > 0xffff) {
Are we sure that we cannot go beyond 4-byte encoding?

https://codereview.chromium.org/335373003/diff/50001/tests/corelib/uri_test.dart
File tests/corelib/uri_test.dart (right):

https://codereview.chromium.org/335373003/diff/50001/tests/corelib/uri_test.d...
tests/corelib/uri_test.dart:259: checkInvalid("s://x/x#foo#bar");
So we do not escape the second #?

[Lasse Reichstein Nielsen, 2014-06-19 08:44:58]

ptal

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:119: * Creates a new URI object by parsing a URI string.
On 2014/06/19 07:39:43, Søren Gjesse wrote:
> This comment needs to be updated...

Done.

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:207: if (char == _AT_SIGN) {
You can have :, which you always could.
You can't have @. It will be tokenized into the userInfo, but the _makeUserInfo
function will recognize it as invalid, and since its a gen-delim, it will not be
escaped, but instead cause a failure to parse. 

We have the test: checkInvalid("s://x@x@x.x/");

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:267: int state = STATE_PATH_END;
I only set the state on a break. This is the state it will have when the while
loop exits normally. This is attempted explained in the comment.

I've renamed the states. Please check if it's more readable.

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:275: if (index == 0) {
Ok. 
I don't really like them, but I guess it's ok here.

https://codereview.chromium.org/335373003/diff/50001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:1096: if (char > 0xffff) {
Yes. We get at most 21 bits from a surrogate pair.
I'll add an assert(char <= 0x10ffff);

https://codereview.chromium.org/335373003/diff/50001/tests/corelib/uri_test.dart
File tests/corelib/uri_test.dart (right):

https://codereview.chromium.org/335373003/diff/50001/tests/corelib/uri_test.d...
tests/corelib/uri_test.dart:259: checkInvalid("s://x/x#foo#bar");
No. The '#' is a gen-delim, so we don't escape it. You can't have more than one
"#" in a URI.

[Anders Johnsen, 2014-06-19 09:03:46]

lgtm

[Lasse Reichstein Nielsen, 2014-06-19 09:05:18]

Committed patchset #6 manually as r37479 (presubmit successful).

[Søren Gjesse, 2014-06-19 10:25:54]

lgtm

https://codereview.chromium.org/335373003/diff/90001/tests/corelib/uri_test.dart
File tests/corelib/uri_test.dart (right):

https://codereview.chromium.org/335373003/diff/90001/tests/corelib/uri_test.d...
tests/corelib/uri_test.dart:265: // It is allowed un-enclosed as argument to
Uri(hist: ) because we don't
hist? (hist og her? hist hvor vejen slog en bugt?)