Merge the dart:uri library into dart:core and update the Uri class

Merge the dart:uri library into dart:core and update the Uri class

This merges the dart:uri library into dart:core removing the dart:uri library. Besides moving the library the Url class has been changed.

* Removed existing Uri constructor as it was equivalent with Uri.parse
* Remamed constructor Uri.fromComponents to Uri
* Moved toplevel function encodeUriComponent to static method Uri.encodeComponent
* Moved toplevel function decodeUriComponent to static method Uri.decodeComponent
* Moved toplevel function encodeUri to static method Uri.encodeFull
* Moved toplevel function decodeUri to static method Uri.decodeFull
* Rename domain to host
* Added static methods Uri.encodeQueryComponent and Uri.decodeQueryComponent
* Added support for path generation and splitting
* Added support for query generation and splitting
* Added some level of normalization

R=floitsch@google.com, lrn@google.com, nweiz@google.com, scheglov@google.com
BUG=

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

[Søren Gjesse, 2013-05-24 14:16:27]

Reviewers please divide the work

floitsch@ and lrn@ Library design

iposva@ VM changes

nweiz@ pub and pkg changes

scheglov@ editor changes

Thanks!

[nweiz, 2013-05-24 18:59:10]

lgtm

https://codereview.chromium.org/16019002/diff/2001/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_request.dart (right):

https://codereview.chromium.org/16019002/diff/2001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:140: var header = 'content-disposition:
form-data; name="${Uri.encodeFull(name)}"';
long line

[scheglov, 2013-05-24 19:01:26]

LGTM

[floitsch, 2013-05-24 20:37:26]

LGTM.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/corelib_sourc...
File sdk/lib/core/corelib_sources.gypi (right):

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/corelib_sourc...
sdk/lib/core/corelib_sources.gypi:14: 'encode_decode.dart',  # Only used by
uri.dart.
I would move these 3 functions into the uri.dart file.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/corelib_sourc...
sdk/lib/core/corelib_sources.gypi:19: 'helpers.dart',  # Only used by uri.dart.
ditto.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/encode_decode...
File sdk/lib/core/encode_decode.dart (right):

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/encode_decode...
sdk/lib/core/encode_decode.dart:40: var byteToHex = (int v) => '%${hex[v >>
4]}${hex[v & 0x0f]}';
This shouldn't be a 'var' but a normal nested function.

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

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:14: * Returns the URI scheme. If there is no scheme the
empty string
All these comments are dartdocs. -> /** instead of /*

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:15: * will be returned.
No need for future.
If there is no scheme the empty string is returned.
Or (probably better):
Returns the empty string if there is no scheme.

I would also separate it from the main description:

/**
 * Returns the URI scheme
 *
 * Returns the empty string if there is no scheme.
 */

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:39: * access to the decoded path use [pathSegement]. If
there is no
pathSegments

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:87: * will be set as 0.
is set to 0.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:91: * expected to be fully percent-encoded and will be
used in its
is used

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:99: * literal form for the URI query. When
[queryParameters] is used xxx
xxx

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:132: if (path == "") return [];
const <String>[];
?

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:149: var value = element.substring(index + 1,
element.length);
substring doesn't need the second argument if it is the length.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:175: throw new ArgumentError('Illegal scheme');
Add the scheme: 'Illegal scheme: $scheme'.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:258: byte = byte * 16 + codeUnit - 0x57;
what's 0x57?

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:260: throw new ArgumentError("Invalid URI encoding");
add reason.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:288: throw new ArgumentError("Invalid Uri component");
add reason.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:380: /* absolute-URI  = scheme ":" hier-part [ "?" query ]
why is this comment here?

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:455: userInfo: targetUserInfo,
indentation.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:464: return (userInfo != "") || (host != "") || (port !=
0);
why is the authority linked to a non-zery port?

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:468: * For http/https schemes returns URI's [origin][] -
scheme://host:port.
Returns the URI's [origin][] for http/https schemes: scheme://host:port.
NL
Throws a [StateError] for all other schemes.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:474: throw new StateError("Cannot use origin without a
scheme");
give information: "Cannot use origin without a scheme: $this"

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:483: if (host == null || host == "") {
Move check up to the checks.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:493: return sb.toString();
No need for StringBuffers.
if (port == 0) return "$scheme://$host";
return "$scheme://$host:$port";

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:509: sb.write(path == null ? "null" : path);
isn't the path null-check redundant?

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:546: * [component] by replacing each instance of certain
characters by
If the set of characters is fixed, add it at the end of the comment.
Or at least add a link.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:581: * See
http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2 for more
I think there is a way to make this a link.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:589: * A javaScript-like URI component decoder.
Is it JavaScript like, or more general? In other words, is referring to
JavaScript the best way to explain it? (no problem if it really is.)

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:606: * have special meaning in the URI: [:#;,/?:@&=+\$:]
It returns the
nit (personal preference): `#;,/?:@&=+\$`.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:614: * A JavaScript-like URI decoder. Decodes a full URI
string. This is
Start with what it does: "Decodes ...".
Make one short sentence in the beginning, newline, and then more documentation.

https://codereview.chromium.org/16019002/diff/2001/tests/corelib/uri_normaliz...
File tests/corelib/uri_normalize_test.dart (right):

https://codereview.chromium.org/16019002/diff/2001/tests/corelib/uri_normaliz...
tests/corelib/uri_normalize_test.dart:36:
Expect.equals(x.toString().toUpperCase(), new Uri(path:
x.toString()).toString().toUpperCase());
long line.

https://codereview.chromium.org/16019002/diff/2001/tests/corelib/uri_path_tes...
File tests/corelib/uri_path_test.dart (right):

https://codereview.chromium.org/16019002/diff/2001/tests/corelib/uri_path_tes...
tests/corelib/uri_path_test.dart:45: test("%C8%A4/%E5%B9%B3%E4%BB%AE%E5%90%8D",
["Ȥ", "平仮名"]);
I'm not sure if we don't prefer escaping these characters (\u{..}).
Personally fine, but ask someone who knows.

https://codereview.chromium.org/16019002/diff/2001/tests/standalone/standalon...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/16019002/diff/2001/tests/standalone/standalon...
tests/standalone/standalone.status:113: io/test_runner_test: Fail # Library
dart:io removed.
dart:uri ?

[Ivan Posva, 2013-05-24 22:24:50]

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/core.dart
File sdk/lib/core/core.dart (right):

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/core.dart#new...
sdk/lib/core/core.dart:10: import "dart:utf" show codepointsToUtf8, decodeUtf8;
Any particular reason why you cannot just

import "dart:utf";

without any showing and hiding? None of the Symbols from dart:utf library should
be leaking out anyway.

[Anton Muhin, 2013-05-27 08:58:59]

Just DBC: also
http://src.chromium.org/viewvc/multivm/trunk/webkit/Source/bindings/dart/gyp/...
needs updating

[Søren Gjesse, 2013-05-27 10:47:21]

https://codereview.chromium.org/16019002/diff/2001/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_request.dart (right):

https://codereview.chromium.org/16019002/diff/2001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:140: var header = 'content-disposition:
form-data; name="${Uri.encodeFull(name)}"';
On 2013/05/24 18:59:10, nweiz wrote:
> long line

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/core.dart
File sdk/lib/core/core.dart (right):

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/core.dart#new...
sdk/lib/core/core.dart:10: import "dart:utf" show codepointsToUtf8, decodeUtf8;
On 2013/05/24 22:24:51, Ivan Posva wrote:
> Any particular reason why you cannot just
> 
> import "dart:utf";
> 
> without any showing and hiding? None of the Symbols from dart:utf library
should
> be leaking out anyway.

The only reason was to show how much of dart:utf is actually used in the core
library.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/corelib_sourc...
File sdk/lib/core/corelib_sources.gypi (right):

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/corelib_sourc...
sdk/lib/core/corelib_sources.gypi:14: 'encode_decode.dart',  # Only used by
uri.dart.
On 2013/05/24 20:37:27, floitsch wrote:
> I would move these 3 functions into the uri.dart file.

I wanted to postpone this to a separate CL to have a smaller diff. It's grown
pretty big already so I will do it here.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/corelib_sourc...
sdk/lib/core/corelib_sources.gypi:19: 'helpers.dart',  # Only used by uri.dart.
On 2013/05/24 20:37:27, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/encode_decode...
File sdk/lib/core/encode_decode.dart (right):

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/encode_decode...
sdk/lib/core/encode_decode.dart:40: var byteToHex = (int v) => '%${hex[v >>
4]}${hex[v & 0x0f]}';
On 2013/05/24 20:37:27, floitsch wrote:
> This shouldn't be a 'var' but a normal nested function.

Done.

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

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:14: * Returns the URI scheme. If there is no scheme the
empty string
On 2013/05/24 20:37:27, floitsch wrote:
> All these comments are dartdocs. -> /** instead of /*

Of cause - thanks.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:15: * will be returned.
On 2013/05/24 20:37:27, floitsch wrote:
> No need for future.
> If there is no scheme the empty string is returned.
> Or (probably better):
> Returns the empty string if there is no scheme.
> 
> I would also separate it from the main description:
> 
> /**
>  * Returns the URI scheme
>  *
>  * Returns the empty string if there is no scheme.
>  */

Done.

Similar change for all Uri component properties.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:39: * access to the decoded path use [pathSegement]. If
there is no
On 2013/05/24 20:37:27, floitsch wrote:
> pathSegments

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:87: * will be set as 0.
On 2013/05/24 20:37:27, floitsch wrote:
> is set to 0.

This was a broken sentence. Removed the "as 0" from here and described the
default values in a separate paragraph.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:91: * expected to be fully percent-encoded and will be
used in its
On 2013/05/24 20:37:27, floitsch wrote:
> is used

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:99: * literal form for the URI query. When
[queryParameters] is used xxx
On 2013/05/24 20:37:27, floitsch wrote:
> xxx

My mark of unfinished comment - sorry.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:132: if (path == "") return [];
On 2013/05/24 20:37:27, floitsch wrote:
> const <String>[];
> ?

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:149: var value = element.substring(index + 1,
element.length);
On 2013/05/24 20:37:27, floitsch wrote:
> substring doesn't need the second argument if it is the length.

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:175: throw new ArgumentError('Illegal scheme');
On 2013/05/24 20:37:27, floitsch wrote:
> Add the scheme: 'Illegal scheme: $scheme'.

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:258: byte = byte * 16 + codeUnit - 0x57;
On 2013/05/24 20:37:27, floitsch wrote:
> what's 0x57?

  LOWERCASE_A - 10

Change the code to which should be easier to read.

  byte = byte * 16 + codeUnit - _LOWER_CASE_A + 10

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:260: throw new ArgumentError("Invalid URI encoding");
On 2013/05/24 20:37:27, floitsch wrote:
> add reason.

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:288: throw new ArgumentError("Invalid Uri component");
On 2013/05/24 20:37:27, floitsch wrote:
> add reason.

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:380: /* absolute-URI  = scheme ":" hier-part [ "?" query ]
On 2013/05/24 20:37:27, floitsch wrote:
> why is this comment here?

It was there in the dart:uri library. Removed it. One can consult the spec. to
find it.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:455: userInfo: targetUserInfo,
On 2013/05/24 20:37:27, floitsch wrote:
> indentation.

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:464: return (userInfo != "") || (host != "") || (port !=
0);
On 2013/05/24 20:37:27, floitsch wrote:
> why is the authority linked to a non-zery port?

Good catch!

An authority always have a host. Changed to only check for host. Also added an
authority getter.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:468: * For http/https schemes returns URI's [origin][] -
scheme://host:port.
On 2013/05/24 20:37:27, floitsch wrote:
> Returns the URI's [origin][] for http/https schemes: scheme://host:port.
> NL
> Throws a [StateError] for all other schemes.

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:474: throw new StateError("Cannot use origin without a
scheme");
On 2013/05/24 20:37:27, floitsch wrote:
> give information: "Cannot use origin without a scheme: $this"

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:483: if (host == null || host == "") {
On 2013/05/24 20:37:27, floitsch wrote:
> Move check up to the checks.

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:493: return sb.toString();
On 2013/05/24 20:37:27, floitsch wrote:
> No need for StringBuffers.
> if (port == 0) return "$scheme://$host";
> return "$scheme://$host:$port";

Good point. Changed.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:509: sb.write(path == null ? "null" : path);
On 2013/05/24 20:37:27, floitsch wrote:
> isn't the path null-check redundant?

It is - removed.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:546: * [component] by replacing each instance of certain
characters by
On 2013/05/24 20:37:27, floitsch wrote:
> If the set of characters is fixed, add it at the end of the comment.
> Or at least add a link.

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:581: * See
http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2 for more
On 2013/05/24 20:37:27, floitsch wrote:
> I think there is a way to make this a link.

I will postpone the adding of proper links to a separate CL.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:589: * A javaScript-like URI component decoder.
On 2013/05/24 20:37:27, floitsch wrote:
> Is it JavaScript like, or more general? In other words, is referring to
> JavaScript the best way to explain it? (no problem if it really is.)

Rephrased the comment and explained more precisely what it does.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:606: * have special meaning in the URI: [:#;,/?:@&=+\$:]
It returns the
On 2013/05/24 20:37:27, floitsch wrote:
> nit (personal preference): `#;,/?:@&=+\$`.

Done.

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:614: * A JavaScript-like URI decoder. Decodes a full URI
string. This is
On 2013/05/24 20:37:27, floitsch wrote:
> Start with what it does: "Decodes ...".
> Make one short sentence in the beginning, newline, and then more
documentation.

Rephrased the comment and explained more precisely what it does.

https://codereview.chromium.org/16019002/diff/2001/tests/corelib/uri_normaliz...
File tests/corelib/uri_normalize_test.dart (right):

https://codereview.chromium.org/16019002/diff/2001/tests/corelib/uri_normaliz...
tests/corelib/uri_normalize_test.dart:36:
Expect.equals(x.toString().toUpperCase(), new Uri(path:
x.toString()).toString().toUpperCase());
On 2013/05/24 20:37:27, floitsch wrote:
> long line.

Done.

https://codereview.chromium.org/16019002/diff/2001/tests/corelib/uri_path_tes...
File tests/corelib/uri_path_test.dart (right):

https://codereview.chromium.org/16019002/diff/2001/tests/corelib/uri_path_tes...
tests/corelib/uri_path_test.dart:45: test("%C8%A4/%E5%B9%B3%E4%BB%AE%E5%90%8D",
["Ȥ", "平仮名"]);
On 2013/05/24 20:37:27, floitsch wrote:
> I'm not sure if we don't prefer escaping these characters (\u{..}).
> Personally fine, but ask someone who knows.

We already have quite a few tests with literal non-us-ascii characters.

https://codereview.chromium.org/16019002/diff/2001/tests/standalone/standalon...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/16019002/diff/2001/tests/standalone/standalon...
tests/standalone/standalone.status:113: io/test_runner_test: Fail # Library
dart:io removed.
On 2013/05/24 20:37:27, floitsch wrote:
> dart:uri ?

Done.

[Lasse Reichstein Nielsen, 2013-05-28 07:26:23]

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/encode_decode...
File sdk/lib/core/encode_decode.dart (right):

https://codereview.chromium.org/16019002/diff/2001/sdk/lib/core/encode_decode...
sdk/lib/core/encode_decode.dart:40: var byteToHex = (int v) => '%${hex[v >>
4]}${hex[v & 0x0f]}';
Seems inefficient to create a string, and then write that string to a
StringBuffer. Just write each character code independently:
 void writeHexByte(StringBuffer buf, int byte) {
   const digits = '0123456789ABCDEF';
   buf.writeCharCode(digits.codeUnitAt(byte >> 4));
   buf.writeCharCode(digits.codeUnitAt(byte & 0x0f));
 }
You can make this a static/top-level function.

[Lasse Reichstein Nielsen, 2013-05-28 08:45:33]

Only comments on uri.dart.
LGTM

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

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:8: * A parsed URI, as specified by RFC-3986,
http://tools.ietf.org/html/rfc3986.
More documentation here, please.
Describe the parts (scheme, authority (I don't know what that means), etc).

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:14: * Returns the scheme.
Returns the scheme component of this URI.

Generally, consider adding "component" after the component parts (and I think
it's event "fragment identifier component").

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:26: * Returns the empty string if there is no authority.
Why empty string and not, e.g., null?
(I assume it is because then you can just concatenate the parts to get the
whole, but maybe state that in the class documentation: we chose to return empty
strings because ...).

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:30: var sb =  new StringBuffer();
Two spaces after "=".

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:33: }
Add empty line.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:51: * Returns 0 if there is no port in the authority.
I can see "port zero is a wildcard" as precedented by TCP/UDP bind function, but
I don't see it making a lot of sense as a URI default - it's never the correct
port at the other end. 
Are we sure there is no schemes where port zero may be a valid port?
How about using null?

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:58: * The returned path is encoded. To get direct access
to the decoded
Encoded how, and encoded from what?
The version that is not encoded is "uncoded" (or raw?), not "decoded". Referring
to both the "encoded" and the "decoded" in the same context makes it not obvious
what they are relative to, and if there is a third "normal" version as well.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:66: * Returns the URI query. The returned query is
encoded. To get
Ditto for encoded: What has been encoded and how?

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:76: * Returns the empty string if there is no fragment.
Is there a way to distinguish:
  foo:bar#
from
  foo:bar
?
Is it relevant? :)

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:388: // escapes, and unicode characters.
Why are we restricting more than the RegExp in the RFC?

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:416: return true;
just:
  return scheme != "" && fragment == "";
More readable!

I don't think adding a fragment identifier will make an otherwise absolute URI
not absolute. Fragments are orthogonal to URIs (they don't specify the resource,
they refer to the content of the resource). I.e., change this to just:
  return scheme != "";

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:427: for (String segment in path.split("/")) {
Seems like overkill to split and rebuild every time.
How about checking for the occurrence of '/.' in the string first, 
and check if it is followed by '/' or './', and just return path
if nothing is found?

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:506: * Throws StateError if the scheme is not http or
https.
Just say "It is an error if ...".
Quote "http" and "https".

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:569: sb.write(first == null ? "null" : first);
Since null.toString() == "null", just write
  sb.write(first); 
  sb.write(second);

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:575: * Encode the string [component] using
percent-encoding to make it
I prefer "URL encoding" to "percent-encoding".
Is it worth writing that this is the URL-encoding of the UTF-8 encoded character
data?

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:592: * a Uri.
Uri -> [Uri].

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:749: String ch = text[i];
Consider using text.codeUnitAt(i) and work with code units instead of singleton
strings.
I know the VM will optimize singleton strings, but I'd still want to see if it's
faster to work on integers.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:766: break;
break on line above, or add braces.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (left):

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/io/http_impl.dart...
sdk/lib/io/http_impl.dart:1500: scheme: scheme != null ? scheme : uri.scheme,
What if domain is "", will it break anything?

[Søren Gjesse, 2013-05-28 13:33:25]

Thanks for the comments Lasse. Lets discuss the documention further after this
is landed.

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

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:8: * A parsed URI, as specified by RFC-3986,
http://tools.ietf.org/html/rfc3986.
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> More documentation here, please.
> Describe the parts (scheme, authority (I don't know what that means), etc).

I absolutely agree. However I will like to postpone updating the comments to a
separate CL.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:14: * Returns the scheme.
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Returns the scheme component of this URI.
> 
> Generally, consider adding "component" after the component parts (and I think
> it's event "fragment identifier component").

Done.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:26: * Returns the empty string if there is no authority.
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Why empty string and not, e.g., null?
> (I assume it is because then you can just concatenate the parts to get the
> whole, but maybe state that in the class documentation: we chose to return
empty
> strings because ...).

This is not changed from the current behavior in dart:uri. I was tempted to
change it from the empty string to null. When this is landed I will try to see
if it is feasible.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:30: var sb =  new StringBuffer();
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Two spaces after "=".

Done.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:33: }
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Add empty line.

Done.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:51: * Returns 0 if there is no port in the authority.
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> I can see "port zero is a wildcard" as precedented by TCP/UDP bind function,
but
> I don't see it making a lot of sense as a URI default - it's never the correct
> port at the other end. 
> Are we sure there is no schemes where port zero may be a valid port?
> How about using null?

Again this is the behavior from the current dart:uir, and again it makes sense
to change it.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:58: * The returned path is encoded. To get direct access
to the decoded
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Encoded how, and encoded from what?
> The version that is not encoded is "uncoded" (or raw?), not "decoded".
Referring
> to both the "encoded" and the "decoded" in the same context makes it not
obvious
> what they are relative to, and if there is a third "normal" version as well.

I agree. I think adding a general comment above about when components are
encoded/decoded and that the accessors (except for pathSegments and
queryParameters) return the literal URI component value.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:66: * Returns the URI query. The returned query is
encoded. To get
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Ditto for encoded: What has been encoded and how?

See above.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:76: * Returns the empty string if there is no fragment.
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Is there a way to distinguish:
>   foo:bar#
> from
>   foo:bar
> ?
> Is it relevant? :)

No and I don't think so. We could use null vs. the empty string if we really
want, but that will be pretty subtle.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:388: // escapes, and unicode characters.
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Why are we restricting more than the RegExp in the RFC?

Currently RegExp is also parsing the user info, host and port parts from the
authority.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:416: return true;
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> just:
>   return scheme != "" && fragment == "";
> More readable!
> 
> I don't think adding a fragment identifier will make an otherwise absolute URI
> not absolute. Fragments are orthogonal to URIs (they don't specify the
resource,
> they refer to the content of the resource). I.e., change this to just:
>   return scheme != "";

Change the formatting.

The spec says "...calls for an absolute-URI syntax rule that does not allow a
fragment."

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:427: for (String segment in path.split("/")) {
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Seems like overkill to split and rebuild every time.
> How about checking for the occurrence of '/.' in the string first, 
> and check if it is followed by '/' or './', and just return path
> if nothing is found?

Done.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:506: * Throws StateError if the scheme is not http or
https.
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Just say "It is an error if ...".
> Quote "http" and "https".

Done.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:569: sb.write(first == null ? "null" : first);
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Since null.toString() == "null", just write
>   sb.write(first); 
>   sb.write(second);

Done.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:575: * Encode the string [component] using
percent-encoding to make it
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> I prefer "URL encoding" to "percent-encoding".
> Is it worth writing that this is the URL-encoding of the UTF-8 encoded
character
> data?

I would like to address the comments in a separate CL. Btw. the spec. calls it
percent-encoding.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:592: * a Uri.
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Uri -> [Uri].

Done.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:749: String ch = text[i];
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> Consider using text.codeUnitAt(i) and work with code units instead of
singleton
> strings.
> I know the VM will optimize singleton strings, but I'd still want to see if
it's
> faster to work on integers.

Done.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:766: break;
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> break on line above, or add braces.

Done.

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (left):

https://codereview.chromium.org/16019002/diff/18001/sdk/lib/io/http_impl.dart...
sdk/lib/io/http_impl.dart:1500: scheme: scheme != null ? scheme : uri.scheme,
On 2013/05/28 08:45:33, Lasse Reichstein Nielsen wrote:
> What if domain is "", will it break anything?

I don't think so - nothing changed here.

[Søren Gjesse, 2013-05-28 13:36:50]

Committed patchset #7 manually as r23266 (presubmit successful).