Don't double-escape in strings in the VM Service, and don't use \u0000 to determine the string lengt

Don't double-escape strings in the VM Service, and don't use \u0000 to determine the string length.

This makes characters above ASCII to display as themselves in Observatory, strings with \u0000 in the middle not to be truncated, and avoids a bug in the previous escaping where a non-ASCII character in the last position would have its escaped representation cut off.

R=johnmccutchan@google.com

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

[rmacnak, 2014-09-09 00:09:45]

rmacnak@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2014-09-09 15:55:29]

lgtm and thanks! This is an important fix.

[turnidge, 2014-09-09 16:12:30]

turnidge@google.com changed reviewers:
+ turnidge@google.com

[turnidge, 2014-09-09 16:12:31]

I really like this change.

Some comments...

https://codereview.chromium.org/542363003/diff/60001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/lib/src/elements/instance_ref.html
(right):

https://codereview.chromium.org/542363003/diff/60001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/src/elements/instance_ref.html:30: <a
on-click="{{ goto }}" _href="{{ url }}">{{ asStringLiteral(ref.valueAsString,
ref.valueAsStringIsTruncated) }}</a>
I suppose we could actually make the "..." be a link which fetches the
non-truncated string and re-displays it inline, much like the curly-block
expansion.

https://codereview.chromium.org/542363003/diff/60001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/lib/src/elements/observatory_element.dart
(right):

https://codereview.chromium.org/542363003/diff/60001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/src/elements/observatory_element.dart:117:
result.add("'".codeUnitAt(0));
I like the change to single quotes.

https://codereview.chromium.org/542363003/diff/60001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/src/elements/observatory_element.dart:129:
else result.add(codeUnit);
A switch statement might be more idiomatic here.

https://codereview.chromium.org/542363003/diff/60001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/test/string_escaping_test.dart (right):

https://codereview.chromium.org/542363003/diff/60001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/test/string_escaping_test.dart:27: void
script() {
Nice.

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17127: intptr_t length = 0;
Do you like these names?  I've always felt 'valueAsString' was a bit long, but
at least it is descriptive.  Do you have any other suggestions?

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17130: jsobj.AddProperty("valueAsStringIsTruncated",
did_truncate);
I think we should have a convention that if "valueAsStringIsTruncated" is false,
it is omitted.

This will make String valueAsString consistent with int valueAsString, which
currently doesn't provide a valueAsStringIsTruncated property.

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object_test.c...
runtime/vm/object_test.cc:4113: 
Could you add a direct unit test for ToCStringTruncated somewhere in this file,
which exercises both the truncate and no-truncate case?

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object_test.c...
runtime/vm/object_test.cc:4310: "\"valueAsStringIsTruncated\":false}",
Omit if false

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/report_test.cc
File runtime/vm/report_test.cc (right):

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/report_test.c...
runtime/vm/report_test.cc:39: EXPECT_SUBSTRING("\"valueAsString\":\"High
Voltage\"",
Much nicer.

[rmacnak, 2014-09-09 21:00:23]

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17127: intptr_t length = 0;
On 2014/09/09 16:12:31, turnidge wrote:
> Do you like these names?  I've always felt 'valueAsString' was a bit long, but
> at least it is descriptive.  Do you have any other suggestions?

I'm not fond of them. Perhaps just 'toString'?

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17130: jsobj.AddProperty("valueAsStringIsTruncated",
did_truncate);
On 2014/09/09 16:12:31, turnidge wrote:
> I think we should have a convention that if "valueAsStringIsTruncated" is
false,
> it is omitted.
> 
> This will make String valueAsString consistent with int valueAsString, which
> currently doesn't provide a valueAsStringIsTruncated property.

I like that.

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/542363003/diff/60001/runtime/vm/object_test.c...
runtime/vm/object_test.cc:4113: 
On 2014/09/09 16:12:31, turnidge wrote:
> Could you add a direct unit test for ToCStringTruncated somewhere in this
file,
> which exercises both the truncate and no-truncate case?

Added.

[rmacnak, 2014-09-09 21:34:21]

Committed patchset #6 (id:100001) manually as 40060 (presubmit successful).

[koda, 2014-09-10 03:12:45]

koda@google.com changed reviewers:
+ koda@google.com

[koda, 2014-09-10 03:12:46]

This seems like a partial fix to this bug:

  https://code.google.com/p/dart/issues/detail?id=20583

right?

Would be great to also fix the remaining places where we use this broken format.