Recognize "\r\n" line endings in base::JSONParser to report correct line numbers in errors

Recognize "\r\n" (Windows) line endings in base::JSONParser so that line numbers in error reports won't confuse users.

BUG=167424
TEST=Unit tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180625

[方觉(Fang Jue), 2013-01-19 13:16:45]

Please review.
Thanks!

[Robert Sesek, 2013-01-22 18:38:01]

https://chromiumcodereview.appspot.com/12026035/diff/6001/base/json/json_pars...
File base/json/json_parser.cc (right):

https://chromiumcodereview.appspot.com/12026035/diff/6001/base/json/json_pars...
base/json/json_parser.cc:408: if ( !(*pos_ == '\n' && pos_ > start_pos_ &&
*(pos_ - 1) == '\r') )
nit: no spaces around the condition of an if

Also, doesn't the index_last_line_ piece above also need to be in the body? It's
used to calculate the column in errors.

[not at google - send to devlin, 2013-01-23 01:16:59]

Have you signed the CLA?

[Robert Sesek, 2013-01-23 03:52:02]

On Jan 22, 2013 8:16 PM, <kalman@chromium.org> wrote:
>
> Have you signed the CLA?

He has.

> https://chromiumcodereview.appspot.com/12026035/

[方觉(Fang Jue), 2013-01-23 04:10:04]

Thank you for your review!

On 2013/01/22 18:38:01, rsesek wrote:

> base/json/json_parser.cc:408: if ( !(*pos_ == '\n' && pos_ > start_pos_ &&
> *(pos_ - 1) == '\r') )
> nit: no spaces around the condition of an if
> 
Done.

> Also, doesn't the index_last_line_ piece above also need to be in the body?
It's
> used to calculate the column in errors.
The comment of index_last_line_ says it is 'The last value of |index_| on the
previous line.' The '\n' in the "\r\n" line ending still belongs to the previous
line. And column calculating code assumes index_last_line_ to point to one
character before the first character of the current line. Therefore I think it
should be out of the body.
I tried to put it inside the body and ran base_unittests and it failed.

[not at google - send to devlin, 2013-01-23 05:12:20]

Cool, sorry for the interruption.

[Robert Sesek, 2013-01-23 19:34:32]

LGTM

On 2013/01/23 04:10:04, 方觉 wrote:
> Thank you for your review!
> 
> On 2013/01/22 18:38:01, rsesek wrote:
> 
> > base/json/json_parser.cc:408: if ( !(*pos_ == '\n' && pos_ > start_pos_ &&
> > *(pos_ - 1) == '\r') )
> > nit: no spaces around the condition of an if
> > 
> Done.
> 
> > Also, doesn't the index_last_line_ piece above also need to be in the body?
> It's
> > used to calculate the column in errors.
> The comment of index_last_line_ says it is 'The last value of |index_| on the
> previous line.' The '\n' in the "\r\n" line ending still belongs to the
previous
> line. And column calculating code assumes index_last_line_ to point to one
> character before the first character of the current line. Therefore I think it
> should be out of the body.
> I tried to put it inside the body and ran base_unittests and it failed.

Good point. You don't want the \n to count as column 1.

[方觉(Fang Jue), 2013-01-31 13:35:46]

darin: friendly ping.

[Robert Sesek, 2013-02-04 16:06:19]

+mark for OWNERS

[Mark Mentovai, 2013-02-04 17:41:00]

https://chromiumcodereview.appspot.com/12026035/diff/12001/AUTHORS
File AUTHORS (right):

https://chromiumcodereview.appspot.com/12026035/diff/12001/AUTHORS#newcode222
AUTHORS:222: 方觉 <fangjue23303@gmail.com>
Can you add an ASCII-friendly version of your name in parentheses after the
Chinese characters to aid non-Chinese-readers/speakers in pronouncing your name,
and giving them something to type when referring to you?

https://chromiumcodereview.appspot.com/12026035/diff/12001/base/json/json_par...
File base/json/json_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/12026035/diff/12001/base/json/json_par...
base/json/json_parser_unittest.cc:223: const char* big_json_crlf =
This and line 212 should both be “const char xxx[]” and not “const char* xxx”.

https://chromiumcodereview.appspot.com/12026035/diff/12001/base/json/json_par...
base/json/json_parser_unittest.cc:224: "[\r\n0,\r\n1,\r\n2,\r\n3,4,5,6
7,\r\n8,\r\n9\r\n]";
This file uses four (not two) spaces of extra indentation for continuation lines
of this form, as on line 307.

https://chromiumcodereview.appspot.com/12026035/diff/12001/base/json/json_par...
base/json/json_parser_unittest.cc:227: &error_code, &error_message));
You haven’t cleared error_message or error_code from their last use, so this
test isn’t really fully testing everything that you intend.

[方觉(Fang Jue), 2013-02-05 01:56:00]

Thanks for your review!

On 2013/02/04 17:41:00, Mark Mentovai wrote:
> https://chromiumcodereview.appspot.com/12026035/diff/12001/AUTHORS
> File AUTHORS (right):
> 
> https://chromiumcodereview.appspot.com/12026035/diff/12001/AUTHORS#newcode222
> AUTHORS:222: 方觉 <mailto:fangjue23303@gmail.com>
> Can you add an ASCII-friendly version of your name in parentheses after the
> Chinese characters to aid non-Chinese-readers/speakers in pronouncing your
name,
> and giving them something to type when referring to you?
> 
Done. Sorry for not considering this before.

>
https://chromiumcodereview.appspot.com/12026035/diff/12001/base/json/json_par...
> File base/json/json_parser_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12026035/diff/12001/base/json/json_par...
> base/json/json_parser_unittest.cc:223: const char* big_json_crlf =
> This and line 212 should both be “const char xxx[]” and not “const char* xxx”.
> 
Done.

>
https://chromiumcodereview.appspot.com/12026035/diff/12001/base/json/json_par...
> base/json/json_parser_unittest.cc:224: "[\r\n0,\r\n1,\r\n2,\r\n3,4,5,6
> 7,\r\n8,\r\n9\r\n]";
> This file uses four (not two) spaces of extra indentation for continuation
lines
> of this form, as on line 307.
> 
Done.

>
https://chromiumcodereview.appspot.com/12026035/diff/12001/base/json/json_par...
> base/json/json_parser_unittest.cc:227: &error_code, &error_message));
> You haven’t cleared error_message or error_code from their last use, so this
> test isn’t really fully testing everything that you intend.
Done.

[Mark Mentovai, 2013-02-05 02:13:38]

The next one should be the winner.

https://chromiumcodereview.appspot.com/12026035/diff/22001/AUTHORS
File AUTHORS (right):

https://chromiumcodereview.appspot.com/12026035/diff/22001/AUTHORS#newcode222
AUTHORS:222: 方觉(Fang Jue) <fangjue23303@gmail.com>
Nit: space before (.

https://chromiumcodereview.appspot.com/12026035/diff/22001/base/json/json_par...
File base/json/json_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/12026035/diff/22001/base/json/json_par...
base/json/json_parser_unittest.cc:214: // error here
---------------------------------^
You need to fix this now—it’s supposed to point to the 7.

https://chromiumcodereview.appspot.com/12026035/diff/22001/base/json/json_par...
base/json/json_parser_unittest.cc:227: // error here --------------------^
This too.

[方觉(Fang Jue), 2013-02-05 02:28:40]

On 2013/02/05 02:13:38, Mark Mentovai wrote:
> The next one should be the winner.
> 
> https://chromiumcodereview.appspot.com/12026035/diff/22001/AUTHORS
> File AUTHORS (right):
> 
> https://chromiumcodereview.appspot.com/12026035/diff/22001/AUTHORS#newcode222
> AUTHORS:222: 方觉(Fang Jue) <mailto:fangjue23303@gmail.com>
> Nit: space before (.
> 
Done.

>
https://chromiumcodereview.appspot.com/12026035/diff/22001/base/json/json_par...
> File base/json/json_parser_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12026035/diff/22001/base/json/json_par...
> base/json/json_parser_unittest.cc:214: // error here
> ---------------------------------^
> You need to fix this now—it’s supposed to point to the 7.
> 
Done.

>
https://chromiumcodereview.appspot.com/12026035/diff/22001/base/json/json_par...
> base/json/json_parser_unittest.cc:227: // error here --------------------^
> This too.
Done.

[Mark Mentovai, 2013-02-05 02:41:36]

LGTM

[方觉(Fang Jue), 2013-02-05 03:37:04]

On 2013/02/05 02:41:36, Mark Mentovai wrote:
> LGTM

Could you please pass it to the try server for me?

[Mark Mentovai, 2013-02-05 04:05:24]

When you’re ready to commit, just hit the “commit” checkbox on the code review
site. That will pass it to the try bots, and if it passes, commit it when the
tree is open.

[commit-bot: I haz the power, 2013-02-05 04:08:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fangjue23303@gmail.com/12026035/29001

[commit-bot: I haz the power, 2013-02-05 07:31:51]

Change committed as 180625