Rewrite base::JSONReader to be 35-40% faster, depending on the input string.

Rewrite base::JSONReader to be 35-40% faster, depending on the input string.

This change does the following:
* Parses the input string and generates the object representation in O(n) time.
* Optimizes string decoding by using StringPiece where possible, which also
  introduces the JSON_DETACHABLE_CHILDREN parser option.
* Makes JSONReader a simpler interface by hiding the parser details in an
  internal JSONParser class.

BUG=49212, 111581, 121469
TEST=Hopefully covered by all test suites. New tests added for edge cases.

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

[Robert Sesek, 2012-04-18 21:48:53]

I apologize for the size…

[Avi (use Gerrit), 2012-04-18 21:57:54]

https://chromiumcodereview.appspot.com/10035042/diff/1/base/json/json_parser.h
File base/json/json_parser.h (right):

https://chromiumcodereview.appspot.com/10035042/diff/1/base/json/json_parser....
base/json/json_parser.h:37: // to be used directly; it encapsultes logic that
need not be exposed publicly.
spelling

[Robert Sesek, 2012-04-18 22:06:36]

https://chromiumcodereview.appspot.com/10035042/diff/1/base/json/json_parser.h
File base/json/json_parser.h (right):

https://chromiumcodereview.appspot.com/10035042/diff/1/base/json/json_parser....
base/json/json_parser.h:37: // to be used directly; it encapsultes logic that
need not be exposed publicly.
On 2012/04/18 21:57:54, Avi wrote:
> spelling

Done.

[Mark Mentovai, 2012-04-19 16:40:11]

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

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:11: #include "base/string_number_conversions.h"
'_' < 'p'

I would have stayed quiet about it, but on lines 14 and 15, you sorted '_' <
's', so this is inconsistent.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:32: CHECK(root->IsType(Value::TYPE_DICTIONARY));
Can this be a DCHECK?

Same on line 50.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:44: std::string json_;
DISALLOW_COPY_AND_ASSIGN? Maybe not. Don’t know how you’re using it yet.

Same on line 62.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:89: return other->IsType(TYPE_STRING) &&
other->GetAsString(&other_string) &&
Isn’t it cheaper to do the comparison of other and string_piece_ as StringPieces
instead of forcing everyone to be a std::string?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:95: base::StringPiece string_piece_;
DISALLOW_COPY_AND_ASSIGN? Maybe not. Don’t know how you’re using it yet.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:101: StackMarker(int* depth) : depth_(depth) {
explicit

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:108: bool IsTooDeep() {
Can be const.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:113: int* depth_;
DISALLOW_COPY_AND_ASSIGN

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:113: int* depth_;
The pointer (not value) can be const, which is nice in cases like this because
it keeps you from accidentally incrementing or decrementing the pointer itself.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:125: index_(0),
Is end_pos_ missing intentionally?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:137: Value* JSONParser::Parse(const std::string& input)
{
Perhaps this can even accept StringPiece input, potentially saving another copy
(depending on the callers).

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:158: // When the input JSON string starts with a UTF-8
Byte-Order-Mark (U+FEFF)
That’s a UTF-16 BOM.

Your comment makes it sound like this will accept a UTF-16 BOM at the beginning,
but that’s not true. (And if it were true, and it accepted a UTF-16 BOM and then
continued to parse UTF-8, it’d be pretty weird.)

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:176:
ReportError(JSONReader::JSON_UNEXPECTED_DATA_AFTER_ROOT, 1);
What’s the “, 1” doing here? The unexpected data might not be one column after
what ends the expected root.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:224: if (other->string_) {
This implementation seems wrong. What if other->string_ is set, string_ is not,
and pos_ and length_ are set?

Shouldn’t you just be swapping all three fields without regard to whether
other->string_ is set?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:238: string_->push_back(c);
The header never said what restrictions were placed on the value of “c”, but
using an int32 and not having any checks around the value, not to mention the
“else” branch of this conditional, seems to imply that you want to accept
arbitrary Unicode code points.

You can’t push a code point (int32) onto a std::string (effectively char*) like
this. Shouldn’t you be pushing one to four characters onto the string depending
on the value of c, like you do below? Shouldn’t you be UTF-8-encoding c?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:266: return !string_;
Oh, so once something is converted to a string, it can never be a StringPiece
again? That wasn’t clear to me based on the header.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:278: return *string_;
I may have said this in the other file, but if this can return a const&, that’d
be better.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.cc:281: // JSONParser private
//////////////////////////////////////////////////////////
Stopping here for lunch.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
File base/json/json_parser.h (right):

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:7: 
#pragma once

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:17: }
}  // namespace base

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:19: // Chromium and Chromium OS check out gtest to
different places, so we're
Who are “we?”

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:20: // unable to compile on both if we include
gtest_prod.h here.  Instead, include
Isn’t there some macro that’s set if you’re building for Chrome OS so that you
can #include a different path based on which product you’re building?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:45: // the parser in GetNextToken/ParseToken, that is
that tokenization operates on
The “, that is that” construction is hard to read.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:51: // of a token, such that the next loop of the parser
will be at the byte
Nit: “iteration,” not “loop.”

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:98: ~StringBuilder();
Blank line before to make it apparent than the comment about |pos| doesn’t
actually apply to the destructor.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:115: bool CanBeStringPiece();
Can this be a const function?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:118: // cannot be converted.
Lines 110-111 told me that Convert() converts from std::string to StringPiece.
Your use of “convert” in this comment implies that it’s also possible to convert
in the other direction, or something. That makes this whole thing unclear.

Maybe Convert() on line 112 should be renamed ConvertToString(), to make it
clear what the conversion is doing.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:119: StringPiece AsStringPiece();
Can this return a const ref?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:122: std::string AsString();
Can this return a const ref too?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:127: // Number of bytes in |pos_| that compose its
length.
Blank line before this.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:127: // Number of bytes in |pos_| that compose its
length.
I don’t know for sure what you mean by “compose its length”

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:129: // The copied string representation. NULL until
Convert() is called.
Blank line before this too.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:134: // Quick check that the stream has enough to
consume |length| more bytes.
The stream has enough…what?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:135: inline bool CanConsume(int length);
Get rid of the “inline”.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:140: // Performs the equivalent of NextChar N times.
Blank line before.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:147: // Consumes whitespace characters and comments
until the next non-that is
😃

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:150: // Helper function that consumes a comment,
assuming that the parser is
Blank line before.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:174: // potentially performing a UTF-16 to UTF-8
conversion. Returns true on
What’s the “potential?” When does it and when doesn’t it? Do you just mean that
this decodes \u sequences?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:178: // Helper function for ConsumeStringRaw() that
consumes the next four to 10
Blank line before.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:178: // Helper function for ConsumeStringRaw() that
consumes the next four to 10
Four OR ten, not four TO ten, right?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:188: // Helper that reads characters that are ints.
Returns true if a number was
Blank line before.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:197: bool StringsAreEqual(const char* left, const char*
right, size_t len);
This one seems like it could be static.

It may not even need to be in the class. If it’s only used in json_parser.cc, it
could just move into that file in an anonymous namespace.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:203: static std::string FormatErrorMessage(int line, int
column,
What does this do?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:206: // Options that control parsing.
What values might this have? Oh, base::JSONParserOptions from
base/json/json_reader.h! You should say so.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:211: // Pointer to the current position in the input
data. Equivalent to
Blank line before this, and 214, and 216.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:212: // |start_pos_ + index_|.
If this is equivalent to something else that’s already tracked, why is it
tracked separately?

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:224: // The last value of |index_| on the previous line.
Blank line before.

https://chromiumcodereview.appspot.com/10035042/diff/1021/base/json/json_pars...
base/json/json_parser.h:240: DISALLOW_COPY_AND_ASSIGN(JSONParser);
Blank line before.

[tfarina, 2012-04-19 22:48:18]

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h
File base/json/json_parser.h (right):

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:57: virtual ~JSONParser();
nit: I think this doesn't need to be virtual.

[tfarina, 2012-04-19 22:54:47]

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc
File base/json/json_parser.cc (right):

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:875: return base::StringPrintf(
nit: base:: here is not necessary as we are in base namespace.

[tfarina, 2012-04-19 22:56:23]

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser_unitt...
File base/json/json_parser_unittest.cc (right):

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser_unitt...
base/json/json_parser_unittest.cc:12: using base::internal::JSONParser;
nit: I don't think you need this, as you have both namespaces below.

[Robert Sesek, 2012-05-03 15:34:51]

So I "resolved" the Windows failures by forcing DETACHABLE_CHILDREN. I'll fix
that via the bug linked in the comment once my Windows box arrives.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc
File base/json/json_parser.cc (right):

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:11: #include "base/string_number_conversions.h"
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> '_' < 'p'
> 
> I would have stayed quiet about it, but on lines 14 and 15, you sorted '_' <
> 's', so this is inconsistent.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:32: CHECK(root->IsType(Value::TYPE_DICTIONARY));
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> Can this be a DCHECK?
> 
> Same on line 50.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:44: std::string json_;
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> DISALLOW_COPY_AND_ASSIGN? Maybe not. Don’t know how you’re using it yet.
> 
> Same on line 62.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:89: return other->IsType(TYPE_STRING) &&
other->GetAsString(&other_string) &&
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> Isn’t it cheaper to do the comparison of other and string_piece_ as
StringPieces
> instead of forcing everyone to be a std::string?

Yes it is. Good idea.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:95: base::StringPiece string_piece_;
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> DISALLOW_COPY_AND_ASSIGN? Maybe not. Don’t know how you’re using it yet.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:101: StackMarker(int* depth) : depth_(depth) {
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> explicit

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:108: bool IsTooDeep() {
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> Can be const.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:113: int* depth_;
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:113: int* depth_;
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> The pointer (not value) can be const, which is nice in cases like this because
> it keeps you from accidentally incrementing or decrementing the pointer
itself.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:125: index_(0),
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> Is end_pos_ missing intentionally?

Nope.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:158: // When the input JSON string starts with a UTF-8
Byte-Order-Mark (U+FEFF)
On 2012/04/19 16:40:11, Mark Mentovai wrote:
> That’s a UTF-16 BOM.
> 
> Your comment makes it sound like this will accept a UTF-16 BOM at the
beginning,
> but that’s not true. (And if it were true, and it accepted a UTF-16 BOM and
then
> continued to parse UTF-8, it’d be pretty weird.)

Isn't U+FEFF the BOM code point, which in UTF-16 is a single unit that's the
same as the point, but in UTF-8 is those three? That's what I was trying to say,
but I can see why that'd be confusing, so fixed.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:224: if (other->string_) {
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> This implementation seems wrong. What if other->string_ is set, string_ is
not,
> and pos_ and length_ are set?
> 
> Shouldn’t you just be swapping all three fields without regard to whether
> other->string_ is set?

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:266: return !string_;
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Oh, so once something is converted to a string, it can never be a StringPiece
> again? That wasn’t clear to me based on the header.

Clarified.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:278: return *string_;
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> I may have said this in the other file, but if this can return a const&,
that’d
> be better.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:281: // JSONParser private
//////////////////////////////////////////////////////////
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Stopping here for lunch.

The flatbreads were good today, yes?

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.cc#ne...
base/json/json_parser.cc:875: return base::StringPrintf(
On 2012/04/19 22:54:47, tfarina wrote:
> nit: base:: here is not necessary as we are in base namespace.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h
File base/json/json_parser.h (right):

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:7: 
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> #pragma once

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:17: }
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> }  // namespace base

I don't do this for forward declares.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:19: // Chromium and Chromium OS check out gtest to
different places, so we're
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Who are “we?”

This was copied… done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:20: // unable to compile on both if we include
gtest_prod.h here.  Instead, include
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Isn’t there some macro that’s set if you’re building for Chrome OS so that you
> can #include a different path based on which product you’re building?

No idea. I couldn't find one.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:45: // the parser in GetNextToken/ParseToken, that is
that tokenization operates on
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> The “, that is that” construction is hard to read.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:51: // of a token, such that the next loop of the parser
will be at the byte
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Nit: “iteration,” not “loop.”

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:57: virtual ~JSONParser();
On 2012/04/19 22:48:18, tfarina wrote:
> nit: I think this doesn't need to be virtual.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:98: ~StringBuilder();
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Blank line before to make it apparent than the comment about |pos| doesn’t
> actually apply to the destructor.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:115: bool CanBeStringPiece();
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Can this be a const function?

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:118: // cannot be converted.
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Lines 110-111 told me that Convert() converts from std::string to StringPiece.

I don't think that's what it says…

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:119: StringPiece AsStringPiece();
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Can this return a const ref?

No, but StringPiece is cheap, so it's okay.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:122: std::string AsString();
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Can this return a const ref too?

Yes.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:127: // Number of bytes in |pos_| that compose its
length.
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Blank line before this.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:127: // Number of bytes in |pos_| that compose its
length.
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> I don’t know for sure what you mean by “compose its length”

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:127: // Number of bytes in |pos_| that compose its
length.
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> I don’t know for sure what you mean by “compose its length”

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:129: // The copied string representation. NULL until
Convert() is called.
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Blank line before this too.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:134: // Quick check that the stream has enough to
consume |length| more bytes.
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> The stream has enough…what?

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:135: inline bool CanConsume(int length);
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Get rid of the “inline”.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:140: // Performs the equivalent of NextChar N times.
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Blank line before.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:150: // Helper function that consumes a comment,
assuming that the parser is
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Blank line before.

This and the remaining instances are done deliberately because they're
inherently related: the first is the only thing that ever calls the second.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:174: // potentially performing a UTF-16 to UTF-8
conversion. Returns true on
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> What’s the “potential?” When does it and when doesn’t it? Do you just mean
that
> this decodes \u sequences?

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:178: // Helper function for ConsumeStringRaw() that
consumes the next four to 10
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Four OR ten, not four TO ten, right?

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:178: // Helper function for ConsumeStringRaw() that
consumes the next four to 10
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Four OR ten, not four TO ten, right?

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:197: bool StringsAreEqual(const char* left, const char*
right, size_t len);
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> This one seems like it could be static.
> 
> It may not even need to be in the class. If it’s only used in json_parser.cc,
it
> could just move into that file in an anonymous namespace.

Made static. Considering this entire file is an implementation, I don't see much
benefit to putting functions in the anon namespace.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:203: static std::string FormatErrorMessage(int line, int
column,
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> What does this do?

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:206: // Options that control parsing.
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> What values might this have? Oh, base::JSONParserOptions from
> base/json/json_reader.h! You should say so.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:211: // Pointer to the current position in the input
data. Equivalent to
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Blank line before this, and 214, and 216.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:212: // |start_pos_ + index_|.
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> If this is equivalent to something else that’s already tracked, why is it
> tracked separately?

Because both are checked frequently and independently.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:224: // The last value of |index_| on the previous line.
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Blank line before.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser.h#new...
base/json/json_parser.h:240: DISALLOW_COPY_AND_ASSIGN(JSONParser);
On 2012/04/19 16:40:12, Mark Mentovai wrote:
> Blank line before.

Done.

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser_unitt...
File base/json/json_parser_unittest.cc (right):

http://codereview.chromium.org/10035042/diff/1021/base/json/json_parser_unitt...
base/json/json_parser_unittest.cc:12: using base::internal::JSONParser;
On 2012/04/19 22:56:23, tfarina wrote:
> nit: I don't think you need this, as you have both namespaces below.

Done.

[tfarina, 2012-05-04 00:25:28]

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc
File base/json/json_parser.cc (right):

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:192: pos_(0),
nit: just curious why did you choose 0 to initialize pos_ and end_pos_ instead
of NULL, but NULL for start_pos_. Seems inconsistent to my taste :/

[Mark Mentovai, 2012-05-08 20:19:40]

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc
File base/json/json_parser.cc (right):

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:41: DLOG(1) << "Swap()ing a DictionaryValue
inefficiently.";
DLOG(1) is DLOG(WARNING). You either meant that or DVLOG(1). Throughout.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:172: }
You should (D)CHECK here that depth <= kStackMaxDepth to enforce that the
callers are using this class properly. If they set a marker but never check
IsTooDeep and return, things can continue recursing unchecked.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:309: DCHECK_GE(c, 0);
Because of the stupid nature of char, you should be doing these checks with it
explicitly signed or unsigned.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:444: if (*NextChar() == '*' && *NextChar() == '/') {
This eats two characters at a time in a loop, so the comment terminator is
handled properly in /**/, improperly in /*a*/, properly again in /*ab*/, etc.,
improper handling for any odd number of characters between /* and */.

You want to seek looking just for *, and then verify that what follows it is a
/.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:451: }
If the /* is unterminated and you reach the end of input, this returns false
having eaten the comment. The parser is wound to the end of input. What’s the
intended behavior? I guess GetNextToken bails out when it sees it’s at the end,
but this is probably worth a comment.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:623: DCHECK_EQ(*pos_, *(start_pos_ + index_));
Why the *s?

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:663: DecodeUTF8(hex_digit, &string);
How is this supposed to work? Why don’t I see it in the RFC? Seems weird and bad
and udnerspecified. Is it just for compatibility with an old and terrible
interface? Is anyone actually using it? If not removed, it definitely needs to
be documented somewhere.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:774: !CBU16_IS_TRAIL(code_unit16_low)) {
CBU16_IS_TRAIL implies CBU16_IS_SURROGATE, you only need to check
CBU16_IS_TRAIL.

(Optimization might eliminate the additional check. It also might not.)

If you want to keep CBU16_IS_SURROGATE, it may be best as a DCHECK.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:777: } else if (!CBU16_IS_SINGLE(code_unit16_high)) {
CBU16_IS_SINGLE is defined as !CBU16_IS_SURROGATE, so this will never be
entered. If you want to do this check, it also might be best as a DCHECK.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:786: if (!code_unit16_low) {
Rather than rechecking this, why don’t you do it inside the if-if above,
immediately after deciding that code_unit16_low is a UTF-16 trail? Then you can
avoid having code_unit16 even being exposed at this scope.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:792: CBU8_APPEND_UNSAFE(code_point, offset,
code_unit32);
And the same for this, except you’d put it in the “I know I have a UTF-16
single” block.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:826: // The optional faction part.
fraction

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:909: case 't':
I’d be more comfortable having kTrueLiteral[] = "true" so that you can use
either arraysize(kTrueLiteral) - 1 or strlen(kTrueLiteral). A constant based on
the length of the literal string appears three times per case, and that’s asking
for trouble.

Then, you’d wrap each entire case in {} so that kTrueLiteral doesn’t exist
outside of that scope, guarding against reuse of kTrueLiteral inside the "false"
and "null" branches.

You shouldn’t have to count characters yourself, even when there are only four
of them.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:951: if (line || column) {
Do you ever have !line && column, or the other way around?

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.h
File base/json/json_parser.h (right):

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.h#ne...
base/json/json_parser.h:20: // Chromium and Chromium OS check out gtest to
different places, so this is
Why not put the #include inside #ifndef OS_CHROMEOS?

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.h#ne...
base/json/json_parser.h:21: // unable to compile on both if we include
gtest_prod.h here.  Instead, include
we

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser_unit...
File base/json/json_parser_unittest.cc (right):

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser_unit...
base/json/json_parser_unittest.cc:15: #if !defined(OS_WIN)
Cheap. What was the problem here? It deserves a comment.

http://codereview.chromium.org/10035042/diff/30002/base/string_util.cc
File base/string_util.cc (right):

http://codereview.chromium.org/10035042/diff/30002/base/string_util.cc#newcod...
base/string_util.cc:479: for (size_t i = 0; i < str.length(); ++i) {
Dump the debugging code.

[Robert Sesek, 2012-05-15 16:57:50]

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc
File base/json/json_parser.cc (right):

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:41: DLOG(1) << "Swap()ing a DictionaryValue
inefficiently.";
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> DLOG(1) is DLOG(WARNING). You either meant that or DVLOG(1). Throughout.

I've said it before, but it bears repeating: we have way too many logging
macros.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:172: }
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> You should (D)CHECK here that depth <= kStackMaxDepth to enforce that the
> callers are using this class properly. If they set a marker but never check
> IsTooDeep and return, things can continue recursing unchecked.

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:192: pos_(0),
On 2012/05/04 00:25:28, tfarina wrote:
> nit: just curious why did you choose 0 to initialize pos_ and end_pos_ instead
> of NULL, but NULL for start_pos_. Seems inconsistent to my taste :/

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:309: DCHECK_GE(c, 0);
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> Because of the stupid nature of char, you should be doing these checks with it
> explicitly signed or unsigned.

How would you do this?

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:444: if (*NextChar() == '*' && *NextChar() == '/') {
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> This eats two characters at a time in a loop, so the comment terminator is
> handled properly in /**/, improperly in /*a*/, properly again in /*ab*/, etc.,
> improper handling for any odd number of characters between /* and */.
> 
> You want to seek looking just for *, and then verify that what follows it is a
> /.

Isn't that what's happening? The operator there is &&.

Added a test, and it passes.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:451: }
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> If the /* is unterminated and you reach the end of input, this returns false
> having eaten the comment. The parser is wound to the end of input. What’s the
> intended behavior? I guess GetNextToken bails out when it sees it’s at the
end,
> but this is probably worth a comment.

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:623: DCHECK_EQ(*pos_, *(start_pos_ + index_));
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> Why the *s?

Debugging code removed.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:663: DecodeUTF8(hex_digit, &string);
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> How is this supposed to work? Why don’t I see it in the RFC? Seems weird and
bad
> and udnerspecified. Is it just for compatibility with an old and terrible
> interface? Is anyone actually using it? If not removed, it definitely needs to
> be documented somewhere.

Documented. I don't want to remove this now (without knowing what inputs could
be sending \x).

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:774: !CBU16_IS_TRAIL(code_unit16_low)) {
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> CBU16_IS_TRAIL implies CBU16_IS_SURROGATE, you only need to check
> CBU16_IS_TRAIL.
> 
> (Optimization might eliminate the additional check. It also might not.)
> 
> If you want to keep CBU16_IS_SURROGATE, it may be best as a DCHECK.

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:777: } else if (!CBU16_IS_SINGLE(code_unit16_high)) {
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> CBU16_IS_SINGLE is defined as !CBU16_IS_SURROGATE, so this will never be
> entered. If you want to do this check, it also might be best as a DCHECK.

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:786: if (!code_unit16_low) {
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> Rather than rechecking this, why don’t you do it inside the if-if above,
> immediately after deciding that code_unit16_low is a UTF-16 trail? Then you
can
> avoid having code_unit16 even being exposed at this scope.

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:792: CBU8_APPEND_UNSAFE(code_point, offset,
code_unit32);
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> And the same for this, except you’d put it in the “I know I have a UTF-16
> single”&nbsp;block.

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:826: // The optional faction part.
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> fraction

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:909: case 't':
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> I’d be more comfortable having kTrueLiteral[] = "true" so that you can use
> either arraysize(kTrueLiteral) - 1 or strlen(kTrueLiteral). A constant based
on
> the length of the literal string appears three times per case, and that’s
asking
> for trouble.
> 
> Then, you’d wrap each entire case in {} so that kTrueLiteral doesn’t exist
> outside of that scope, guarding against reuse of kTrueLiteral inside the
"false"
> and "null" branches.
> 
> You shouldn’t have to count characters yourself, even when there are only four
> of them.

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.cc#n...
base/json/json_parser.cc:951: if (line || column) {
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> Do you ever have !line && column, or the other way around?

No, but one could be zero.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.h
File base/json/json_parser.h (right):

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.h#ne...
base/json/json_parser.h:20: // Chromium and Chromium OS check out gtest to
different places, so this is
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> Why not put the #include inside #ifndef OS_CHROMEOS?

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser.h#ne...
base/json/json_parser.h:21: // unable to compile on both if we include
gtest_prod.h here.  Instead, include
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> we

Done.

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser_unit...
File base/json/json_parser_unittest.cc (right):

http://codereview.chromium.org/10035042/diff/30002/base/json/json_parser_unit...
base/json/json_parser_unittest.cc:15: #if !defined(OS_WIN)
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> Cheap. What was the problem here? It deserves a comment.

Something to do with the components build. I'll fix it now that I have a win
box.

http://codereview.chromium.org/10035042/diff/30002/base/string_util.cc
File base/string_util.cc (right):

http://codereview.chromium.org/10035042/diff/30002/base/string_util.cc#newcod...
base/string_util.cc:479: for (size_t i = 0; i < str.length(); ++i) {
On 2012/05/08 20:19:41, Mark Mentovai wrote:
> Dump the debugging code.

Done.

[Mark Mentovai, 2012-05-15 17:44:38]

LGTM. I’ve read this enough and I’m satisfied with the few changes I’ve
spot-checked.

[Robert Sesek, 2012-05-15 17:56:27]

brettw: Anything else to add?

+darin: FYI for base_export.h change

On 2012/05/15 17:44:38, Mark Mentovai wrote:
> LGTM. I’ve read this enough and I’m satisfied with the few changes I’ve
> spot-checked.

Thanks for the review!

[brettw, 2012-05-15 19:30:09]

I'm happy with Mark's review, I didn't look at anything again.