Add option for not adding '.0' to the end of doubles that have no fractional

Add a new option to JSONWriter, which instructs it not to append '.0' or
use exponential notation when writing doubles with no fractional part.

This is needed so we can write an integer value larger than what a 32-bit
int will hold using a double, and write the said double in such a way as
to enable external JSON readers (that support int64) to convert it to an
int64 instead of a double.

The other alternative is to add support for int64 in the Value classes and
clients. See http://codereview.chromium.org/8962042
BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125705

[Nico, 2012-01-04 16:37:41]

Looks like I'm not in base/OWNERS anymore, so I can't review this.

[brettw, 2012-01-04 21:21:03]

Can you elaborate on specifically why this is needed?

[kkania, 2012-01-05 02:07:17]

This is for ChromeDriver
(http://www.chromium.org/developers/testing/webdriver-for-chrome).
In ChromeDriver, we need to return a timestamp (in JSON) to the WebDriver client
(there are multiple clients for python, java, c#, etc.). The opensource
WebDriver project maintains these clients.

The timestamp is milliseconds since the unix epoch, so a TYPE_INTEGER Value
won't fit it. That leaves me to store the integer as a double or add int64 to
the Value classes (see http://codereview.chromium.org/8962042). The JSONWriter
writes doubles, even doubles that have no fractional part, in exponential
notation or with a '.' to ensure that when JSONReader reads the JSON the value
will always be interpreted as a double. The problem with this is that the java
JSON implementation will read the JSON and convert it into a double instead of a
long. This breaks the Java webdriver client, which expects the value to be a
long.

The new option tells JSONWriter to write integer doubles like a regular integer.
I hesitated to add this because:
1) It may be desirable to write a real and a integer double together in one JSON
object. If you pass in the option, the real at times may be written as an
integer value (if it has no fractional part), which may confuse external JSON
readers expecting a double instead of an int64. According to the JSON spec, I
think it's wrong to automatically assume that a value with no '.' or 'e' is not
a double, but the popular Java and C++ ones follow that convention.
2) I think usage of this option will be very rare.

Alternatively, I can work around the problem in the chromedriver code or go the
int64 route (in which case I'd add you as a reviewer to
http://codereview.chromium.org/8962042).

[brettw, 2012-01-05 17:56:10]

+tc who has more experience with base/value.h. Over IM:

In the past, I suggested people who want int64 support to convert to a string
and if they wanted, they could put helper functions for this in
base/value_conversions.*

[kkania, 2012-03-05 21:52:21]

ping. Just a reminder, the three options are (in order of my preference):
1) Fork base/json into chrome/test/webdriver/json and make the int64 edits.
2) Add support for int64 in the value classes
(http://codereview.chromium.org/8962042)
3) This CL

On 2012/01/05 17:56:10, brettw wrote:
> +tc who has more experience with base/value.h. Over IM:
> 
> In the past, I suggested people who want int64 support to convert to a string
> and if they wanted, they could put helper functions for this in
> base/value_conversions.*

[brettw, 2012-03-05 21:54:44]

On Mon, Mar 5, 2012 at 1:52 PM,  <kkania@chromium.org> wrote:
> ping. Just a reminder, the three options are (in order of my preference):
> 1) Fork base/json into chrome/test/webdriver/json and make the int64 edits.
> 2) Add support for int64 in the value classes
> (http://codereview.chromium.org/8962042)
> 3) This CL
>
> On 2012/01/05 17:56:10, brettw wrote:
>>
>> +tc who has more experience with base/value.h. Over IM:
>
>
>> In the past, I suggested people who want int64 support to convert to a
>> string
>> and if they wanted, they could put helper functions for this in
>> base/value_conversions.*

I sent you this suggestion that you didn't respond to.

Brett

[kkania, 2012-03-06 01:29:08]

You're right, I didn't respond. ChromeDriver receives commands from external
clients (which I don't have control over) using json over http. The WebDriver
spec dictates that timestamps be in milliseconds from unix epoch, which requires
an int64 or double. However, I can't write doubles because base/json
automatically appends a .0 to doubles to preserve type information. So, your
suggestion will not work for my case.

On 2012/03/05 21:54:44, brettw wrote:
> On Mon, Mar 5, 2012 at 1:52 PM,  <mailto:kkania@chromium.org> wrote:
> > ping. Just a reminder, the three options are (in order of my preference):
> > 1) Fork base/json into chrome/test/webdriver/json and make the int64 edits.
> > 2) Add support for int64 in the value classes
> > (http://codereview.chromium.org/8962042)
> > 3) This CL
> >
> > On 2012/01/05 17:56:10, brettw wrote:
> >>
> >> +tc who has more experience with base/value.h. Over IM:
> >
> >
> >> In the past, I suggested people who want int64 support to convert to a
> >> string
> >> and if they wanted, they could put helper functions for this in
> >> base/value_conversions.*
> 
> I sent you this suggestion that you didn't respond to.
> 
> Brett

[tony, 2012-03-06 01:37:41]

On 2012/03/06 01:29:08, kkania wrote:
> You're right, I didn't respond. ChromeDriver receives commands from external
> clients (which I don't have control over) using json over http. The WebDriver
> spec dictates that timestamps be in milliseconds from unix epoch, which
requires
> an int64 or double. However, I can't write doubles because base/json
> automatically appends a .0 to doubles to preserve type information. So, your
> suggestion will not work for my case.

Can you elaborate a bit on this?  You're saying that we receive json with large
numbers?  When do we need to write large numbers that get read by external
clients?

[kkania, 2012-03-06 02:31:53]

On 2012/03/06 01:37:41, tony wrote:
> On 2012/03/06 01:29:08, kkania wrote:
> > You're right, I didn't respond. ChromeDriver receives commands from external
> > clients (which I don't have control over) using json over http. The
WebDriver
> > spec dictates that timestamps be in milliseconds from unix epoch, which
> requires
> > an int64 or double. However, I can't write doubles because base/json
> > automatically appends a .0 to doubles to preserve type information. So, your
> > suggestion will not work for my case.
> 
> Can you elaborate a bit on this?  You're saying that we receive json with
large
> numbers?  When do we need to write large numbers that get read by external
> clients?

We actually don't need to receive json with large numbers yet. We just need to
send json with large numbers. I suppose I need to explain ChromeDriver, the code
that needs this, a bit more.

WebDriver (also called selenium 2.0) is a popular tool for automating website
testing across multiple browsers. It has a server and client component. There
are several clients written in various languages (C#,java,ruby,python,etc),
which can communicate with the server according to the WebDriver wire protocol
(see the current spec: http://code.google.com/p/selenium/wiki/JsonWireProtocol).
The wire protocol uses json over http. ChromeDriver is the server component that
clients can talk to to automate chrome.

The WebDriver open source team maintains the various clients, while members from
the chromium team have maintained the chromedriver server. I could ask the
WebDriver team to change all their clients to not use milliseconds since unix
epoch for their timestamp, but I think that's a bit unreasonable. Currently, the
only part of the WebDriver spec that needs timestamps is their new
logging-related functionality. I imagine in the future we'd need to be able to
read big integers too. It would be nice to do this without having to cast
between doubles too.

[tony, 2012-03-06 17:57:25]

Does sending numbers with .0 break existing clients?  JSON doesn't differentiate
between doubles and ints, there's only a number type so the client should be
able to handle both.

http://codereview.chromium.org/9016024/diff/8001/base/json/json_writer.cc
File base/json/json_writer.cc (right):

http://codereview.chromium.org/9016024/diff/8001/base/json/json_writer.cc#new...
base/json/json_writer.cc:97: break;
Does it work to just not append the .0 below?

http://codereview.chromium.org/9016024/diff/8001/base/json/json_writer.h
File base/json/json_writer.h (right):

http://codereview.chromium.org/9016024/diff/8001/base/json/json_writer.h#newc...
base/json/json_writer.h:41: OPTIONS_ALLOW_DOUBLES_AS_INTEGERS = 1 << 2
The name of this is confusing because JSON doesn't have a notion of double or
int.  Maybe something like OPTIONS_OMIT_DECIMAL_HINT?

[kkania, 2012-03-06 21:26:09]

The JSON spec doesn't differentiate, but most implementations automatically
assume a number with . or e is a double. See
https://github.com/douglascrockford/JSON-java/blob/master/JSONObject.java#L1255
and line 560 of http://jsoncpp.sourceforge.net/json__reader_8cpp_source.html

http://codereview.chromium.org/9016024/diff/8001/base/json/json_writer.cc
File base/json/json_writer.cc (right):

http://codereview.chromium.org/9016024/diff/8001/base/json/json_writer.cc#new...
base/json/json_writer.cc:97: break;
On 2012/03/06 17:57:25, tony wrote:
> Does it work to just not append the .0 below?

No, because DoubleToString sometimes uses exponential notation

http://codereview.chromium.org/9016024/diff/8001/base/json/json_writer.h
File base/json/json_writer.h (right):

http://codereview.chromium.org/9016024/diff/8001/base/json/json_writer.h#newc...
base/json/json_writer.h:41: OPTIONS_ALLOW_DOUBLES_AS_INTEGERS = 1 << 2
On 2012/03/06 17:57:25, tony wrote:
> The name of this is confusing because JSON doesn't have a notion of double or
> int.  Maybe something like OPTIONS_OMIT_DECIMAL_HINT?

That's a much better name, but the option also prevents exponential notation in
addition to no decimal points. How about OPTIONS_OMIT_DOUBLE_TYPE_PRESERVATION?

[tony, 2012-03-06 21:50:13]

On 2012/03/06 21:26:09, kkania wrote:
> The JSON spec doesn't differentiate, but most implementations automatically
> assume a number with . or e is a double. See
>
https://github.com/douglascrockford/JSON-java/blob/master/JSONObject.java#L1255
> and line 560 of http://jsoncpp.sourceforge.net/json__reader_8cpp_source.html

Right, but I'm saying the client code that uses JSON-java or jsoncpp (or
Chromium's jsonreader) should read the number and if it's a double but wants an
int, the client code needs to do that conversion.  There's no way for a json
reader to know what type of number you want.

That said, I think this is a reasonable option to add to our writer since adding
the .0 is kind of a hack.

LGTM

http://codereview.chromium.org/9016024/diff/27001/base/json/json_writer.h
File base/json/json_writer.h (right):

http://codereview.chromium.org/9016024/diff/27001/base/json/json_writer.h#new...
base/json/json_writer.h:36: // 64-bit int.
I would remove the text from line 30-36.  It's sufficient to just describe the
behavior.

[kkania, 2012-03-06 22:41:57]

http://codereview.chromium.org/9016024/diff/27001/base/json/json_writer.h
File base/json/json_writer.h (right):

http://codereview.chromium.org/9016024/diff/27001/base/json/json_writer.h#new...
base/json/json_writer.h:36: // 64-bit int.
On 2012/03/06 21:50:13, tony wrote:
> I would remove the text from line 30-36.  It's sufficient to just describe the
> behavior.

Done.

[kkania, 2012-03-06 23:18:02]

Just waiting for owner's approval now. No rush.

[brettw, 2012-03-06 23:24:05]

Rubber stamp LGTM based on Tony's review.

[commit-bot: I haz the power, 2012-03-08 19:39:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/9016024/30001

[commit-bot: I haz the power, 2012-03-08 22:55:33]

Change committed as 125705