Write custom URI-parser, to avoid using regexp.

Write custom URI-parser, to avoid using regexp.

BUG=
R=sgjesse@google.com

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

[Søren Gjesse, 2014-02-24 09:12:41]

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

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:121: static Uri parse(String uri) {
I think it could be a good idea to include the regexp from the spec in a
documentation comment here.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:143: if (codeUnits[0] != '/'.codeUnitAt(0)) {
Use _SLASH (see frequently used character codes at line 1483.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:145: while (index < length) {
Please provide some more comments, e.g. "Look for valid scheme ending in ':'. If
not found assume schema-less URI."

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:149: if (codeUnit == ':'.codeUnitAt(0)) {
_COLON, and more below.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:152: // Back up one char, as we meet a special char.
meet -> met?

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:163: // If we see '//', it must be a authority.
it must -> there must
a -> an

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:175: // IPv6. Skip to '['.
'[' -> ']'

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:200: if (codeUnit == '['.codeUnitAt(0)) {
Refactor next 5 lines to a local function

authorityEndIndex = ipV6Address(codeUnits, authorityEndIndex);

and use above as well

https://codereview.chromium.org/167703010/diff/1/tests/corelib/uri_file_test....
File tests/corelib/uri_file_test.dart (right):

https://codereview.chromium.org/167703010/diff/1/tests/corelib/uri_file_test....
tests/corelib/uri_file_test.dart:261: check("file://user:password@host/path",
windowsOk: true);
Please add the previous URI to a failing test somewhere.

[Anders Johnsen, 2014-02-24 12:44:32]

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

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:121: static Uri parse(String uri) {
On 2014/02/24 09:12:41, Søren Gjesse wrote:
> I think it could be a good idea to include the regexp from the spec in a
> documentation comment here.

Done.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:143: if (codeUnits[0] != '/'.codeUnitAt(0)) {
On 2014/02/24 09:12:41, Søren Gjesse wrote:
> Use _SLASH (see frequently used character codes at line 1483.

Done.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:145: while (index < length) {
On 2014/02/24 09:12:41, Søren Gjesse wrote:
> Please provide some more comments, e.g. "Look for valid scheme ending in ':'.
If
> not found assume schema-less URI."

Done.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:149: if (codeUnit == ':'.codeUnitAt(0)) {
On 2014/02/24 09:12:41, Søren Gjesse wrote:
> _COLON, and more below.

Done.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:152: // Back up one char, as we meet a special char.
On 2014/02/24 09:12:41, Søren Gjesse wrote:
> meet -> met?

Done.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:163: // If we see '//', it must be a authority.
On 2014/02/24 09:12:41, Søren Gjesse wrote:
> it must -> there must
> a -> an

Done.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:175: // IPv6. Skip to '['.
On 2014/02/24 09:12:41, Søren Gjesse wrote:
> '[' -> ']'

Done.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:200: if (codeUnit == '['.codeUnitAt(0)) {
On 2014/02/24 09:12:41, Søren Gjesse wrote:
> Refactor next 5 lines to a local function
> 
> authorityEndIndex = ipV6Address(codeUnits, authorityEndIndex);
> 
> and use above as well

Done.

https://codereview.chromium.org/167703010/diff/1/tests/corelib/uri_file_test....
File tests/corelib/uri_file_test.dart (right):

https://codereview.chromium.org/167703010/diff/1/tests/corelib/uri_file_test....
tests/corelib/uri_file_test.dart:261: check("file://user:password@host/path",
windowsOk: true);
On 2014/02/24 09:12:41, Søren Gjesse wrote:
> Please add the previous URI to a failing test somewhere.

Done.

[Lasse Reichstein Nielsen, 2014-02-24 13:02:27]

Looks good, but I'll only trust the algorithm with more tests.

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

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:123: // it'll call `new Uri(...)` that will perform these
validations. This is
it'll -> it will
that -> which

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:124: // purely splitting up the uri string into
components.
uri -> URI.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:125: bool isSchemeCharacter(int ch) {
Consider making this a static private method instead.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:143: if (codeUnits[0] != '/'.codeUnitAt(0)) {
Unless you *know* that this is optimized away by all compilers (and it isn't),
replace it with a constant ("const CHAR_SLASH = 0x..;", which may be local to
function or static and private).

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:152: // Back up one char, as we meet a special char.
as -> since

https://codereview.chromium.org/167703010/diff/1/tests/corelib/uri_file_test....
File tests/corelib/uri_file_test.dart (right):

https://codereview.chromium.org/167703010/diff/1/tests/corelib/uri_file_test....
tests/corelib/uri_file_test.dart:261: check("file://user:password@host/path",
windowsOk: true);
We need tests. Lots of tests.

Something like:

for (var fragment in ["", ,"#", "#f", "#fragment", "#l:?/"]) {
  for (var query in ["", "?", "?q", "?query", "?q:/"]) {
    for (var path in ["", "x", "x/y", "x/y/", "x:y"]) {
      for (var user in ["", "x", "xxx", "x:4", "xxx:444", "x:4:x"] {
        for (var host in ["", "h", "hhh", "h:4", "hhh:444",
                          "[1.2.3.4]","<some ipv6addresses>"]) {
          var auth = host;
          if (user.isNotEmpty) auth = "$user@$auth";
          if (auth.isNotEmpty) auth = "//$auth";
          check("scheme:$auth/$path$query$fragment");
        }
      }         
    }
  }

and something similar for non-scheme URIs, where I believe the rules are
slightly different.

[Søren Gjesse, 2014-02-24 13:59:57]

LGTM!

But please add more tests as Lasse suggested.

[Anders Johnsen, 2014-02-24 14:26:49]

Added test. Landing.

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

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:123: // it'll call `new Uri(...)` that will perform these
validations. This is
On 2014/02/24 13:02:27, Lasse Reichstein Nielsen wrote:
> it'll -> it will
> that -> which

Done.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:124: // purely splitting up the uri string into
components.
On 2014/02/24 13:02:27, Lasse Reichstein Nielsen wrote:
> uri -> URI.

Done.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:125: bool isSchemeCharacter(int ch) {
On 2014/02/24 13:02:27, Lasse Reichstein Nielsen wrote:
> Consider making this a static private method instead.

Done.

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:143: if (codeUnits[0] != '/'.codeUnitAt(0)) {
On 2014/02/24 13:02:27, Lasse Reichstein Nielsen wrote:
> Unless you *know* that this is optimized away by all compilers (and it isn't),
> replace it with a constant ("const CHAR_SLASH = 0x..;", which may be local to
> function or static and private).

Any const static members should be compiled away?

https://codereview.chromium.org/167703010/diff/1/sdk/lib/core/uri.dart#newcod...
sdk/lib/core/uri.dart:152: // Back up one char, as we meet a special char.
On 2014/02/24 13:02:27, Lasse Reichstein Nielsen wrote:
> as -> since

Done.

https://codereview.chromium.org/167703010/diff/1/tests/corelib/uri_file_test....
File tests/corelib/uri_file_test.dart (right):

https://codereview.chromium.org/167703010/diff/1/tests/corelib/uri_file_test....
tests/corelib/uri_file_test.dart:261: check("file://user:password@host/path",
windowsOk: true);
On 2014/02/24 13:02:27, Lasse Reichstein Nielsen wrote:
> We need tests. Lots of tests.
> 
> Something like:
> 
> for (var fragment in ["", ,"#", "#f", "#fragment", "#l:?/"]) {
>   for (var query in ["", "?", "?q", "?query", "?q:/"]) {
>     for (var path in ["", "x", "x/y", "x/y/", "x:y"]) {
>       for (var user in ["", "x", "xxx", "x:4", "xxx:444", "x:4:x"] {
>         for (var host in ["", "h", "hhh", "h:4", "hhh:444",
>                           "[1.2.3.4]","<some ipv6addresses>"]) {
>           var auth = host;
>           if (user.isNotEmpty) auth = "$user@$auth";
>           if (auth.isNotEmpty) auth = "//$auth";
>           check("scheme:$auth/$path$query$fragment");
>         }
>       }         
>     }
>   }
> 
> and something similar for non-scheme URIs, where I believe the rules are
> slightly different.

Done.

[Anders Johnsen, 2014-02-24 14:36:39]

Committed patchset #5 manually as r32966 (presubmit successful).