Add encodimg argument to Uri.encodeQueryString

Add encodimg argument to Uri.encodeQueryString

R=lrn@google.com
BUG=http://dartbug.com/12780

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

[Søren Gjesse, 2013-11-29 09:04:35]

ptal

As discussed offline separate the encoding of string to bytes and the encoding
of bytes to percent encoding.

The percent-encoding could event be changed into a codec now.

[Lasse Reichstein Nielsen, 2013-11-29 12:34:10]

LGTM

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

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1119: * characters `-._~` are percent encoded. Note that
the set of
"percent-encoded"

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1125: * character. The default encoding is UTF-8.
Comment is no longer correct - it's not just the unreserved characters that are
encoded, but everything. So, replace the parts above by something like:

 Encode the string [component] according to the HTML 4.01 rules
 for encoding the posting of a HTML form as a query string	 
 component.

 The component is first encoded to bytes using [encoding]. 
 The default is to use [UTF8] encoding, which preserves all 
 the characters that don't need encoding.

 Then the resulting bytes are "percent-encoded". This transforms
 spaces (U+0020) to a plus sign ('+') and all bytes that are not
 the ASCII decimal digits, letters or one of '-._~' are written as 
 a percent sign '%' followed by the two-digit hexadecimal
 representation of the byte.

 Note that the set of characters which are percent-encoded is a 
 superset of what HTML 4.01 requires, since it refers to RFC 1738
 for reserved characters.

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1166: * UTF-8.
And this is now the inverse of encodeQueryComponent! Yey!

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1362: static const int _SPACE = 0x20;
Sigh. We really, really should have character code constants in the language!

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1404: bytes.forEach((byte) {
I'd do 
 for (int i = 0; i < bytes.length; i++) {
   int byte = bytes[i];
   ...
 }
No need for an extra function call per byte.

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1409: result.write("+");
  result.writeCharCode(_PLUS);  
Better to not mix String writes and code writes too much (I think, depends on
implementation, but I'd optimize for sequences of the same kind of writes).

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1411: result.write(byteToHex(byte));
Pass 'result' to the converter and let it put the bytes into it, instead of
creating an intermediate string. I.e.
  writeByteAsHex(byte, result);

with 
  void writeByteAsHex(byte, buffer) {
    const String hex = "0123456789ABCDEF";
    buffer.writeCharCode(hex.codeUnitAt(byte >> 4));
    buffer.writeCharCode(hex.codeUnitAt(byte & 0x0f));
  }

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1455: List<int> bytes = new List<int>();
Consider using a new Uint8List(text.length) as buffer.
It should be smaller than one smi per three characters, and even better if the
string isn't all percent-encodings.

Also consider checking first if there are any plusses, percent-signs or
non-ASCII chars, and only build the buffer if there is. If not, you can use
text.codeUnits directly as the bytes.

(but that's just optimizations).

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1458: if (codeUnit > 128) {
codeUnit >= 128  
or
codeUnit > 127

[Søren Gjesse, 2013-12-02 09:00:54]

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

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1119: * characters `-._~` are percent encoded. Note that
the set of
On 2013/11/29 12:34:10, Lasse Reichstein Nielsen wrote:
> "percent-encoded"

Done.

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1125: * character. The default encoding is UTF-8.
On 2013/11/29 12:34:10, Lasse Reichstein Nielsen wrote:
> Comment is no longer correct - it's not just the unreserved characters that
are
> encoded, but everything. So, replace the parts above by something like:
> 
>  Encode the string [component] according to the HTML 4.01 rules
>  for encoding the posting of a HTML form as a query string	 
>  component.
> 
>  The component is first encoded to bytes using [encoding]. 
>  The default is to use [UTF8] encoding, which preserves all 
>  the characters that don't need encoding.
> 
>  Then the resulting bytes are "percent-encoded". This transforms
>  spaces (U+0020) to a plus sign ('+') and all bytes that are not
>  the ASCII decimal digits, letters or one of '-._~' are written as 
>  a percent sign '%' followed by the two-digit hexadecimal
>  representation of the byte.
> 
>  Note that the set of characters which are percent-encoded is a 
>  superset of what HTML 4.01 requires, since it refers to RFC 1738
>  for reserved characters.
>  

Thanks for these suggestions. Comment updated accordingly.

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1362: static const int _SPACE = 0x20;
On 2013/11/29 12:34:10, Lasse Reichstein Nielsen wrote:
> Sigh. We really, really should have character code constants in the language!

Is there a bug to star?

Is it time to add the "chars" package?

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1404: bytes.forEach((byte) {
On 2013/11/29 12:34:10, Lasse Reichstein Nielsen wrote:
> I'd do 
>  for (int i = 0; i < bytes.length; i++) {
>    int byte = bytes[i];
>    ...
>  }
> No need for an extra function call per byte.

Done.

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1409: result.write("+");
On 2013/11/29 12:34:10, Lasse Reichstein Nielsen wrote:
>   result.writeCharCode(_PLUS);  
> Better to not mix String writes and code writes too much (I think, depends on
> implementation, but I'd optimize for sequences of the same kind of writes).

Done.

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1411: result.write(byteToHex(byte));
On 2013/11/29 12:34:10, Lasse Reichstein Nielsen wrote:
> Pass 'result' to the converter and let it put the bytes into it, instead of
> creating an intermediate string. I.e.
>   writeByteAsHex(byte, result);
> 
> with 
>   void writeByteAsHex(byte, buffer) {
>     const String hex = "0123456789ABCDEF";
>     buffer.writeCharCode(hex.codeUnitAt(byte >> 4));
>     buffer.writeCharCode(hex.codeUnitAt(byte & 0x0f));
>   }

Done.

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1455: List<int> bytes = new List<int>();
On 2013/11/29 12:34:10, Lasse Reichstein Nielsen wrote:
> Consider using a new Uint8List(text.length) as buffer.
> It should be smaller than one smi per three characters, and even better if the
> string isn't all percent-encodings.
> 
> Also consider checking first if there are any plusses, percent-signs or
> non-ASCII chars, and only build the buffer if there is. If not, you can use
> text.codeUnits directly as the bytes.
> 
> (but that's just optimizations).

Added the test for simple strings. Not sure about Uint8List - aren't we still
trying to keep IE9 working?

https://codereview.chromium.org/94733003/diff/20001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1458: if (codeUnit > 128) {
On 2013/11/29 12:34:10, Lasse Reichstein Nielsen wrote:
> codeUnit >= 128  
> or
> codeUnit > 127

Ups. Good catch.

[Søren Gjesse, 2013-12-02 09:01:31]

Committed patchset #3 manually as r30791 (presubmit successful).