Fix to the JsonStringifier in dart_json.dart to escape eols. The error messages that frog sends t...

Fix to the JsonStringifier in dart_json.dart to escape eols. The error messages that frog sends to the editor can contains eols, which is causing issues (http://code.google.com/p/dart/issues/detail?id=1064).
Committed: https://code.google.com/p/dart/source/detail?r=3256

[Jennifer Messerly, 2012-01-11 01:12:19]

lgtm

[Anton Muhin, 2012-01-11 14:33:00]

DBQ: May we have tests for this new functionality?

[devoncarew, 2012-01-11 21:29:19]

On 2012/01/11 14:33:00, antonmuhin wrote:
> DBQ: May we have tests for this new functionality?

I added a test:

expect(JSON.stringify('hi\nthere')).equals('"hi\\nthere"');

to json_tests.dart, but was not able to get the dartium tests to pass after this
change. I'm not 100% sure that the dart_json.dart code is being run. In lieu of
other ideas, I'm leaning towards just putting a patch into frog_server.dart to
strip newlines from the frog --> editor json messages, and closing this CL.

[Jennifer Messerly, 2012-01-11 21:34:47]

On 2012/01/11 21:29:19, devoncarew wrote:
> On 2012/01/11 14:33:00, antonmuhin wrote:
> > DBQ: May we have tests for this new functionality?
> 
> I added a test:
> 
> expect(JSON.stringify('hi\nthere')).equals('"hi\\nthere"');
> 
> to json_tests.dart, but was not able to get the dartium tests to pass after
this
> change. I'm not 100% sure that the dart_json.dart code is being run. In lieu
of
> other ideas, I'm leaning towards just putting a patch into frog_server.dart to
> strip newlines from the frog --> editor json messages, and closing this CL.

Oh, I think this is a good change! We want to fix it at the right layer of the
stack--that way it's fixed for everyone.

I think the reason you're seeing the dartium test fail is because we don't build
dartium locally. Your testing against a binary that has an old
(pre-your-changes) copy of JSON. The dartium bots are a few hours behind, then
you'll get a binary build that has your fix.

We should really make this JSON test be a command line test, not an in-browser
test, IMHO. In the meantime you could check this in, then add your new test in a
follow up change once the dartium bots have caught up?

[devoncarew, 2012-01-11 22:18:49]

@Anton, let me know if you'd like me to commit this as it w/o the test, or if
you'd prefer taking this change over yourself. Cheers!

[Anton Muhin, 2012-01-12 13:22:59]

Devon, thanks a lot for adding a test!

As John says, dartium tests need some time to catch up after changes to some
core libs like dart:{json,dom,html}.  So, please, commit with the test.

Thanks a lot for fixing it.

http://codereview.chromium.org/9181003/diff/3002/client/json/dart_json.dart
File client/json/dart_json.dart (right):

http://codereview.chromium.org/9181003/diff/3002/client/json/dart_json.dart#n...
client/json/dart_json.dart:514: charCode = 110; // 'n'
as an option see A_SMALL above.  Ditto for r

http://codereview.chromium.org/9181003/diff/3002/client/tests/client/json/jso...
File client/tests/client/json/json_tests.dart (right):

http://codereview.chromium.org/9181003/diff/3002/client/tests/client/json/jso...
client/tests/client/json/json_tests.dart:81:
expect(JSON.stringify('hi\nthere')).equals('"hi\\nthere"');
may I ask you to add a test for \r as well?

[devoncarew, 2012-01-12 21:28:11]

http://codereview.chromium.org/9181003/diff/3002/client/json/dart_json.dart
File client/json/dart_json.dart (right):

http://codereview.chromium.org/9181003/diff/3002/client/json/dart_json.dart#n...
client/json/dart_json.dart:514: charCode = 110; // 'n'
On 2012/01/12 13:23:00, antonmuhin wrote:
> as an option see A_SMALL above.  Ditto for r

Done.

http://codereview.chromium.org/9181003/diff/3002/client/tests/client/json/jso...
File client/tests/client/json/json_tests.dart (right):

http://codereview.chromium.org/9181003/diff/3002/client/tests/client/json/jso...
client/tests/client/json/json_tests.dart:81:
expect(JSON.stringify('hi\nthere')).equals('"hi\\nthere"');
On 2012/01/12 13:23:00, antonmuhin wrote:
> may I ask you to add a test for \r as well?

Done.