Add string.trimLeft/trimRight.

Add string.trimLeft/trimRight.

BUG= http://dartbug.com/5589
R=floitsch@google.com, sgjesse@google.com

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

[Lasse Reichstein Nielsen, 2014-03-10 13:27:49]

https://codereview.chromium.org/190853009/diff/1/sdk/lib/_internal/lib/js_str...
File sdk/lib/_internal/lib/js_string.dart (right):

https://codereview.chromium.org/190853009/diff/1/sdk/lib/_internal/lib/js_str...
sdk/lib/_internal/lib/js_string.dart:250: String result = JS('String',
'#.trimLeft()', this);
Just checked. "trimLeft" and "trimRight" are not in ES5. They are implemented by
Firefox and Blink, so we might want to use them if they are there. That would
mean changing this to:
 JS('String', '#.trimLeft ? #.trimLeft() : #", this, this, this)
and similar for trimRight.

[Søren Gjesse, 2014-03-10 16:50:03]

lgtm

https://codereview.chromium.org/190853009/diff/1/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/190853009/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:317: // Returns this string if it does not have
leading or trailing
if -> as

https://codereview.chromium.org/190853009/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:337: // Returns this string if it does not have
trailing
ditto.

https://codereview.chromium.org/190853009/diff/1/sdk/lib/_internal/lib/js_str...
File sdk/lib/_internal/lib/js_string.dart (right):

https://codereview.chromium.org/190853009/diff/1/sdk/lib/_internal/lib/js_str...
sdk/lib/_internal/lib/js_string.dart:213: const int SPACE = 0x20;
Seems like CARRIAGE_RETURN and SPACE is not used here any more (same two times
below).

https://codereview.chromium.org/190853009/diff/1/sdk/lib/_internal/lib/js_str...
sdk/lib/_internal/lib/js_string.dart:224: if (firstCode == NEL || firstCode ==
BOM) {
What if there are several BOM/NEL characters? Shouldn't we handle that as well,
or is that a completely broken string?

https://codereview.chromium.org/190853009/diff/1/sdk/lib/_internal/lib/js_str...
sdk/lib/_internal/lib/js_string.dart:250: String result = JS('String',
'#.trimLeft()', this);
On 2014/03/10 13:27:50, Lasse Reichstein Nielsen wrote:
> Just checked. "trimLeft" and "trimRight" are not in ES5. They are implemented
by
> Firefox and Blink, so we might want to use them if they are there. That would
> mean changing this to:
>  JS('String', '#.trimLeft ? #.trimLeft() : #", this, this, this)
> and similar for trimRight.

Are we even doing that for trim?

https://codereview.chromium.org/190853009/diff/1/tests/corelib/string_trimlr_...
File tests/corelib/string_trimlr_test.dart (right):

https://codereview.chromium.org/190853009/diff/1/tests/corelib/string_trimlr_...
tests/corelib/string_trimlr_test.dart:7: const WHITESPACE = const [
A comment for each of these code points would be helpful.

[Lasse Reichstein Nielsen, 2014-03-11 10:36:29]

Larger change than expected.
PTAL.

https://codereview.chromium.org/190853009/diff/1/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/190853009/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:317: // Returns this string if it does not have
leading or trailing
Rewritten.

https://codereview.chromium.org/190853009/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:337: // Returns this string if it does not have
trailing
On 2014/03/10 16:50:03, Søren Gjesse wrote:
> ditto.

Done.

https://codereview.chromium.org/190853009/diff/1/sdk/lib/_internal/lib/js_str...
File sdk/lib/_internal/lib/js_string.dart (right):

https://codereview.chromium.org/190853009/diff/1/sdk/lib/_internal/lib/js_str...
sdk/lib/_internal/lib/js_string.dart:213: const int SPACE = 0x20;
Ack, removed.

https://codereview.chromium.org/190853009/diff/1/sdk/lib/_internal/lib/js_str...
sdk/lib/_internal/lib/js_string.dart:224: if (firstCode == NEL || firstCode ==
BOM) {
Those are handled by _skipLeadingWhitespace. They are just not handled by JS
trim. We try JS trim first (because usually it's just spaces and tabs), and if
there is a NEL or BOM left, we skip to our own, probably slower, method that
handles everything.

https://codereview.chromium.org/190853009/diff/1/sdk/lib/_internal/lib/js_str...
sdk/lib/_internal/lib/js_string.dart:250: String result = JS('String',
'#.trimLeft()', this);
Plain trim is in ES5.
My simple rewrite here wouldn't work, I'll do something large.

https://codereview.chromium.org/190853009/diff/1/tests/corelib/string_trimlr_...
File tests/corelib/string_trimlr_test.dart (right):

https://codereview.chromium.org/190853009/diff/1/tests/corelib/string_trimlr_...
tests/corelib/string_trimlr_test.dart:7: const WHITESPACE = const [
On 2014/03/10 16:50:03, Søren Gjesse wrote:
> A comment for each of these code points would be helpful.

Done.

[Lasse Reichstein Nielsen, 2014-03-11 10:38:56]

Søren, can you check if Safari still trims U+200b? Just do
"\u200b".trim().length and see if it is 0 or 1.
If it is 0, we need an entry in corelib.status for safari too.

[Lasse Reichstein Nielsen, 2014-03-12 08:56:02]

Got it checked: Safari also incorrectly trims "\u200b" as if it was whitespace.
Updated status file.

[floitsch, 2014-03-12 12:34:28]

LGTM.

https://codereview.chromium.org/190853009/diff/40001/tests/corelib/corelib.st...
File tests/corelib/corelib.status (right):

https://codereview.chromium.org/190853009/diff/40001/tests/corelib/corelib.st...
tests/corelib/corelib.status:115: string_trimlr_test/none: Fail # Bug in v8.
Fixed in v8 r19222, 2014-02-10.
If it was just a v8 bug we could ignore it and wait for the fix to reach stable.
Since it is also present in safari we should change our library function to work
with it.

[Lasse Reichstein Nielsen, 2014-03-12 14:54:48]

https://codereview.chromium.org/190853009/diff/40001/tests/corelib/corelib.st...
File tests/corelib/corelib.status (right):

https://codereview.chromium.org/190853009/diff/40001/tests/corelib/corelib.st...
tests/corelib/corelib.status:115: string_trimlr_test/none: Fail # Bug in v8.
Fixed in v8 r19222, 2014-02-10.
The change would be not using the JS trim function, which would likely cost in
performance. It's not a new issue with this change, it was just not tested
before, but the existing trim function has the same problem.

I think it's a rare occurrence to actually trim U+200B, and therefore not likely
to be a problem in practice (although it is rather annoying). We generate code
that is valid for ES5, it's just that Safari (and V8 for a little while yet) are
not ES5 compliant.

V8 changing behavior could be the thing that makes JSC look at it again. I'd say
we keep it as-is for now, and see if it causes any problems.

[Lasse Reichstein Nielsen, 2014-03-12 14:57:12]

https://codereview.chromium.org/190853009/diff/40001/tests/corelib/string_tri...
File tests/corelib/string_trimlr_test.dart (right):

https://codereview.chromium.org/190853009/diff/40001/tests/corelib/string_tri...
tests/corelib/string_trimlr_test.dart:59: Expect.equals("a", (ws +
"a").trimLeft(), "L4: ${ws.codeUnitAt(0).toRadixString(16)}");
Debug-data, will remove to make line short again.

[Lasse Reichstein Nielsen, 2014-03-13 07:17:03]

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

[Søren Gjesse, 2014-03-13 13:52:50]

lgtm