Allow JSONWriter and JSONValueSerializer to omit binary values when instructed to do so.

Allow JSONWriter and JSONValueSerializer to ignore binary values when instructed to do so.

Design discussion is available here: http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/967eb64325c24f9c

BUG=None
TEST=base_unittests


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

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

[Jói, 2011-11-10 11:08:16]

Cool, I have a couple of nits.

Note that Evan is on leave for the rest of 2011 so you'll need to find another
base/OWNER.

Cheers,
Jói

http://codereview.chromium.org/8505033/diff/2001/base/json/json_value_seriali...
File base/json/json_value_serializer.h (right):

http://codereview.chromium.org/8505033/diff/2001/base/json/json_value_seriali...
base/json/json_value_serializer.h:41: // into the string passed into the
constructor. In the overloaded method,
Please document this separately rather than together.  It might make most sense
to document the first Serialize as "equialent to calling Serialize(root, false)"

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

http://codereview.chromium.org/8505033/diff/2001/base/json/json_writer.h#newc...
base/json/json_writer.h:26: // Ignore values of binary type. They will be
omitted from the output.
This is slightly vague:  _values_ ... will be omitted.

I think you want something like "For values of binary type, the value (and key
if within a dictionary) will be omitted from the output."

[Eric Dingle, 2011-11-10 15:05:25]

http://codereview.chromium.org/8505033/diff/2001/base/json/json_value_seriali...
File base/json/json_value_serializer.h (right):

http://codereview.chromium.org/8505033/diff/2001/base/json/json_value_seriali...
base/json/json_value_serializer.h:41: // into the string passed into the
constructor. In the overloaded method,
On 2011/11/10 11:08:16, Jói wrote:
> Please document this separately rather than together.  It might make most
sense
> to document the first Serialize as "equialent to calling Serialize(root,
false)"

Done.

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

http://codereview.chromium.org/8505033/diff/2001/base/json/json_writer.h#newc...
base/json/json_writer.h:26: // Ignore values of binary type. They will be
omitted from the output.
On 2011/11/10 11:08:16, Jói wrote:
> This is slightly vague:  _values_ ... will be omitted.
> 
> I think you want something like "For values of binary type, the value (and key
> if within a dictionary) will be omitted from the output."

Done.

[Jói, 2011-11-10 15:07:14]

LGTM

[willchan no longer on Chromium, 2011-11-10 18:33:07]

http://codereview.chromium.org/8505033/diff/2002/base/json/json_value_seriali...
File base/json/json_value_serializer.h (right):

http://codereview.chromium.org/8505033/diff/2002/base/json/json_value_seriali...
base/json/json_value_serializer.h:47: virtual bool Serialize(const Value& root,
bool ignore_binary_values);
No overloading, and why is this virtual, it's not overriding anything. Please
apply OVERRIDE where you are overriding a virtual.

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

http://codereview.chromium.org/8505033/diff/2002/base/json/json_writer.h#newc...
base/json/json_writer.h:43: static void Write(const Value* const node, bool
pretty_print,
Style guide prohibits overloading. You should probably rename this to
WriteWithOptions() or something.

[willchan no longer on Chromium, 2011-11-10 18:52:27]

http://codereview.chromium.org/8505033/diff/2002/base/json/json_value_seriali...
File base/json/json_value_serializer.cc (right):

http://codereview.chromium.org/8505033/diff/2002/base/json/json_value_seriali...
base/json/json_value_serializer.cc:29: &root,
Should be 4 space indent. See the example in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls.

http://codereview.chromium.org/8505033/diff/2002/base/json/json_writer_unitte...
File base/json/json_writer_unittest.cc (right):

http://codereview.chromium.org/8505033/diff/2002/base/json/json_writer_unitte...
base/json/json_writer_unittest.cc:101: ASSERT_EQ("", output_js);
Microoptimization that doesn't matter individually, but I like pointing out:
I prefer output_js.empty() here, so we don't have to construct a std::string out
of "", and leads to less code bloat.

[Eric Dingle, 2011-11-10 23:19:57]

http://codereview.chromium.org/8505033/diff/2002/base/json/json_value_seriali...
File base/json/json_value_serializer.cc (right):

http://codereview.chromium.org/8505033/diff/2002/base/json/json_value_seriali...
base/json/json_value_serializer.cc:29: &root,
On 2011/11/10 18:52:27, willchan wrote:
> Should be 4 space indent. See the example in
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls.

Done.

http://codereview.chromium.org/8505033/diff/2002/base/json/json_value_seriali...
File base/json/json_value_serializer.h (right):

http://codereview.chromium.org/8505033/diff/2002/base/json/json_value_seriali...
base/json/json_value_serializer.h:47: virtual bool Serialize(const Value& root,
bool ignore_binary_values);
On 2011/11/10 18:33:07, willchan wrote:
> No overloading, and why is this virtual, it's not overriding anything. Please
> apply OVERRIDE where you are overriding a virtual.

Done.

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

http://codereview.chromium.org/8505033/diff/2002/base/json/json_writer.h#newc...
base/json/json_writer.h:43: static void Write(const Value* const node, bool
pretty_print,
On 2011/11/10 18:33:07, willchan wrote:
> Style guide prohibits overloading. You should probably rename this to
> WriteWithOptions() or something.

Done.

http://codereview.chromium.org/8505033/diff/2002/base/json/json_writer_unitte...
File base/json/json_writer_unittest.cc (right):

http://codereview.chromium.org/8505033/diff/2002/base/json/json_writer_unitte...
base/json/json_writer_unittest.cc:101: ASSERT_EQ("", output_js);
On 2011/11/10 18:52:27, willchan wrote:
> Microoptimization that doesn't matter individually, but I like pointing out:
> I prefer output_js.empty() here, so we don't have to construct a std::string
out
> of "", and leads to less code bloat.

Done.

[willchan no longer on Chromium, 2011-11-10 23:46:29]

http://codereview.chromium.org/8505033/diff/11004/base/json/json_value_serial...
File base/json/json_value_serializer.h (right):

http://codereview.chromium.org/8505033/diff/11004/base/json/json_value_serial...
base/json/json_value_serializer.h:39: // Equivalent to calling Serialize(root,
false).
Comments are off.

http://codereview.chromium.org/8505033/diff/11004/base/json/json_value_serial...
base/json/json_value_serializer.h:47: bool
SerializeWithOptionalBinaryValues(const Value& root,
There's no reason to use this except when we want to omit the binary values,
right? How about renaming to SerializeAndOmitBinaryValues() and including a
private SerializeInternal() function?

http://codereview.chromium.org/8505033/diff/11004/base/json/json_value_serial...
base/json/json_value_serializer.h:88: // Equivalent to calling Serialize(root,
false).
Comments are off.

[Eric Dingle, 2011-11-14 19:03:56]

http://codereview.chromium.org/8505033/diff/11004/base/json/json_value_serial...
File base/json/json_value_serializer.h (right):

http://codereview.chromium.org/8505033/diff/11004/base/json/json_value_serial...
base/json/json_value_serializer.h:39: // Equivalent to calling Serialize(root,
false).
On 2011/11/10 23:46:29, willchan wrote:
> Comments are off.

Done.

http://codereview.chromium.org/8505033/diff/11004/base/json/json_value_serial...
base/json/json_value_serializer.h:47: bool
SerializeWithOptionalBinaryValues(const Value& root,
On 2011/11/10 23:46:29, willchan wrote:
> There's no reason to use this except when we want to omit the binary values,
> right? How about renaming to SerializeAndOmitBinaryValues() and including a
> private SerializeInternal() function?

Done.

http://codereview.chromium.org/8505033/diff/11004/base/json/json_value_serial...
base/json/json_value_serializer.h:88: // Equivalent to calling Serialize(root,
false).
On 2011/11/10 23:46:29, willchan wrote:
> Comments are off.

Done.

[willchan no longer on Chromium, 2011-11-14 22:37:31]

LGTM

[commit-bot: I haz the power, 2011-11-15 00:00:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericdingle@chromium.org/8505033/18001

[commit-bot: I haz the power, 2011-11-15 00:34:05]

Try job failure for 8505033-18001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-15 01:25:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericdingle@chromium.org/8505033/22002

[commit-bot: I haz the power, 2011-11-15 02:50:51]

Change committed as 110021

[Eric Dingle, 2011-11-16 18:32:48]

+mnissler as reviewer

Mattias, can you please review the change to
chrome/browser/policy/configuration_policy_handler_chromeos.cc?

[Mattias Nissler (ping if slow), 2011-11-16 18:45:53]

chrome/browser/policy LGTM, but I wonder if stripping the binary values in a
preprocessing step before calling the serializer would be simpler? Admittedly
that'd mean lots of copying.

[commit-bot: I haz the power, 2011-11-18 00:52:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericdingle@chromium.org/8505033/25001

[commit-bot: I haz the power, 2011-11-18 02:29:59]

Change committed as 110640