Implement string.trim for dart2js.

Implement string.trim for dart2js.

R=ngeoffray@google.com

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

[floitsch, 2013-05-22 13:25:53]

V8 currently removes 0x85, but that seems to be a bug:
https://code.google.com/p/v8/issues/detail?id=2408
https://codereview.chromium.org/11293277/

[ngeoffray, 2013-05-22 13:34:00]

LGTM, but where I can see the motivation for not using JS's trim?

https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/js_string.dart (right):

https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:118: case 10:
Keep the hexadecimal notation?

https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:155: codeUnit == 13
|| // Newline
hexa? Also how about having two constants for those?

https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:170: codeUnit == 13
|| // Newline
Ditto.

[floitsch, 2013-05-22 17:09:13]

On 2013/05/22 13:34:00, ngeoffray wrote:
> LGTM, but where I can see the motivation for not using JS's trim?
Will add a comment.
0x85 is not trimmed by JavaScript (although V8 currently does it). We want it to
be trimmed.
> 
>
https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
> File sdk/lib/_internal/compiler/implementation/lib/js_string.dart (right):
> 
>
https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
> sdk/lib/_internal/compiler/implementation/lib/js_string.dart:118: case 10:
> Keep the hexadecimal notation?
> 
>
https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
> sdk/lib/_internal/compiler/implementation/lib/js_string.dart:155: codeUnit ==
13
> || // Newline
> hexa? Also how about having two constants for those?
> 
>
https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
> sdk/lib/_internal/compiler/implementation/lib/js_string.dart:170: codeUnit ==
13
> || // Newline
> Ditto.

[floitsch, 2013-05-22 20:45:17]

https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/js_string.dart (right):

https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:118: case 10:
On 2013/05/22 13:34:00, ngeoffray wrote:
> Keep the hexadecimal notation?

Done.

https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:155: codeUnit == 13
|| // Newline
On 2013/05/22 13:34:00, ngeoffray wrote:
> hexa? Also how about having two constants for those?

Done.

https://codereview.chromium.org/15740005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:170: codeUnit == 13
|| // Newline
On 2013/05/22 13:34:00, ngeoffray wrote:
> Ditto.

Done.

[sra1, 2013-05-22 22:39:03]

DBC

https://codereview.chromium.org/15740005/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_string.dart (right):

https://codereview.chromium.org/15740005/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:115: bool
_isWhitespace(int codeUnit) {
static?

https://codereview.chromium.org/15740005/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:151: // the
New-line character (0x85).
I believe it is called NEXT LINE, not new-line.  Comment could also say JS trim
does not trim BOM aka deprecated Zero-width no-break space.

JavaScript trim *can* be used provided the string does not contain a trimable
NEXT LINE or BOM (or see below, possibly NBSP)
You could call trim() and then fall through if the result starts or ends with
0x85 or 0xFEFF.  That would avoid >50 comparisons from the switch statement in
common cases (it fails once at each end of the string in the current
implementation).

https://codereview.chromium.org/15740005/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:152: String trim()
{
Can't this whole operation be coded concisely with a JavaScript regex match?

Example:
http://closure-library.googlecode.com/svn/docs/closure_goog_string_string.js....

goog.string.trim = function(str) {
  // Since IE doesn't include non-breaking-space (0xa0) in their \s character
  // class (as required by section 7.2 of the ECMAScript spec), we explicitly
  // include it in the regexp to enforce consistent cross-browser behavior.
  return str.replace(/^[\s\xa0]+|[\s\xa0]+$/g, '');
};

https://codereview.chromium.org/15740005/diff/6001/tests/corelib/string_trim2...
File tests/corelib/string_trim2_test.dart (right):

https://codereview.chromium.org/15740005/diff/6001/tests/corelib/string_trim2...
tests/corelib/string_trim2_test.dart:46: Expect.equals("a", (c + c + "a" + c +
c).trim());
It might be removing all characters, add e.g.
equals('a' + c + 'b',  c + 'a' + c + 'b' + c)

[floitsch, 2013-05-23 12:52:50]

https://codereview.chromium.org/15740005/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_string.dart (right):

https://codereview.chromium.org/15740005/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:115: bool
_isWhitespace(int codeUnit) {
On 2013/05/22 22:39:03, sra1 wrote:
> static?

Done.

https://codereview.chromium.org/15740005/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:151: // the
New-line character (0x85).
On 2013/05/22 22:39:03, sra1 wrote:
> I believe it is called NEXT LINE, not new-line.  Comment could also say JS
trim
> does not trim BOM aka deprecated Zero-width no-break space.
yes. NEXT LINE.
> 
> JavaScript trim *can* be used provided the string does not contain a trimable
> NEXT LINE or BOM (or see below, possibly NBSP)
> You could call trim() and then fall through if the result starts or ends with
> 0x85 or 0xFEFF.  That would avoid >50 comparisons from the switch statement in
> common cases (it fails once at each end of the string in the current
> implementation).
I don't see how the native trim function can avoid the comparisons. Also: the
switch is transformed into a JS switch. JS engines can optimize them.
Nevertheless split the _isWhitespace into <256 and >=256 parts.

https://codereview.chromium.org/15740005/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_string.dart:152: String trim()
{
On 2013/05/22 22:39:03, sra1 wrote:
> Can't this whole operation be coded concisely with a JavaScript regex match?
> 
> Example:
>
http://closure-library.googlecode.com/svn/docs/closure_goog_string_string.js....
> 
> goog.string.trim = function(str) {
>   // Since IE doesn't include non-breaking-space (0xa0) in their \s character
>   // class (as required by section 7.2 of the ECMAScript spec), we explicitly
>   // include it in the regexp to enforce consistent cross-browser behavior.
>   return str.replace(/^[\s\xa0]+|[\s\xa0]+$/g, '');
> };

The regexp will run in squared behavior for trimming the end-characters. It is
also currently not supported by dart2js (which doesn't want to see \x and \u in
JS regexps).

I tried to use the native trim function (uploaded a patch), but in the end it
wasn't really smaller.

https://codereview.chromium.org/15740005/diff/6001/tests/corelib/string_trim2...
File tests/corelib/string_trim2_test.dart (right):

https://codereview.chromium.org/15740005/diff/6001/tests/corelib/string_trim2...
tests/corelib/string_trim2_test.dart:46: Expect.equals("a", (c + c + "a" + c +
c).trim());
On 2013/05/22 22:39:03, sra1 wrote:
> It might be removing all characters, add e.g.
> equals('a' + c + 'b',  c + 'a' + c + 'b' + c)

Done in another CL.

[floitsch, 2013-05-23 12:53:17]

Committed patchset #4 manually as r23064 (presubmit successful).