Log the sync communication that happens between client and server.

This would allow the developers to view the client/server sync traffic. In retail builds this code would be compiled out. To view the traffic you have to run chrome with the flag:
--vmodule=traffic_logger=1.


BUG=117615
TEST=git-cl try --email=foo


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

[lipalani1, 2012-03-10 00:26:56]

Please review.

bug 117612 was found while developing the feature and I found it easier to
figure out which datatype was changing just looking at the traffic.

The next step would be to store this in memory and then display it in
about:sync.

[akalin, 2012-03-13 00:20:43]

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
File chrome/browser/sync/engine/traffic_logger.cc (right):

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:18: template<class T>
space before <

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:20:
DictionaryValue*(to_dictionary_value)(const T&, bool),
the right syntax is:

DictionaryValue* (*to_dictionary_value)(const T&, bool)

(declaration follows use)

(It probably just happens to work because of function->pointer-to-function
decay, but it's clearer not to rely on it, I think)

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:23: scoped_ptr<DictionaryValue>
value(to_dictionary_value(data, true));
(*to_dictionary_value)(data, true)

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:23: scoped_ptr<DictionaryValue>
value(to_dictionary_value(data, true));
comment what "true" means, like

(data, true /* parameter_name */)

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:25:
base::JSONWriter::Write(value.get(), true, &message);
here too

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:33:
LogData((sync_pb::ClientToServerMessage)msg, ClientToServerMessageToValue,
this is rendered moot by the comment in the header, but this is wrong; should be
a cast to a const ref (unless you're deliberately slicing the object)

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:33:
LogData((sync_pb::ClientToServerMessage)msg, ClientToServerMessageToValue,
&ClientToServerMessageToValue

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:41: LogData(response,
ClientToServerResponseToValue,
&ClientToServerResponseToValue

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
File chrome/browser/sync/engine/traffic_logger.h (right):

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.h:14: class ClientToServerMessage;
probably more consistent to use sync_pb::ClientToServerMessage instead

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/proto_enum_conversions.h (right):

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_enum_conversions.h:35: const char*
ResponseTypeString(
Like the other functions in this file, prefix with 'Get'

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/proto_value_conversions.cc (right):

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:402: DictionaryValue*
SyncEntityToValue(const sync_pb::SyncEntity& proto,
declarations for these functions in the header file?  Or wrap them in an anon
namespace

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:420: // Let us not go
through the specifics if we dont have to.
Seems like premature optimization, since this code is only hit during debugging.

In any case, this comment belongs more with the caller of this function

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.h:144: bool
decode_specifics);
use an enum, something like WITH_SPECIFICS / WITHOUT_SPECIFICS

[lipalani1, 2012-03-13 21:32:39]

PTAL.

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
File chrome/browser/sync/engine/traffic_logger.cc (right):

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:18: template<class T>
On 2012/03/13 00:20:43, akalin wrote:
> space before <

Done.

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:20:
DictionaryValue*(to_dictionary_value)(const T&, bool),
Yeah that is correct. I have seen this convention used in my previous job.
However I am OK converting to prepending *.
On 2012/03/13 00:20:43, akalin wrote:
> the right syntax is:
> 
> DictionaryValue* (*to_dictionary_value)(const T&, bool)
> 
> (declaration follows use)
> 
> (It probably just happens to work because of function->pointer-to-function
> decay, but it's clearer not to rely on it, I think)

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:23: scoped_ptr<DictionaryValue>
value(to_dictionary_value(data, true));
On 2012/03/13 00:20:43, akalin wrote:
> (*to_dictionary_value)(data, true)

Done.

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:25:
base::JSONWriter::Write(value.get(), true, &message);
On 2012/03/13 00:20:43, akalin wrote:
> here too

Done.

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:33:
LogData((sync_pb::ClientToServerMessage)msg, ClientToServerMessageToValue,
On 2012/03/13 00:20:43, akalin wrote:
> this is rendered moot by the comment in the header, but this is wrong; should
be
> a cast to a const ref (unless you're deliberately slicing the object)

Done.

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:33:
LogData((sync_pb::ClientToServerMessage)msg, ClientToServerMessageToValue,
The necessity for the cast was to have the template code pick it up. Otherwise
it is getting confused.
On 2012/03/13 00:20:43, akalin wrote:
> this is rendered moot by the comment in the header, but this is wrong; should
be
> a cast to a const ref (unless you're deliberately slicing the object)

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:33:
LogData((sync_pb::ClientToServerMessage)msg, ClientToServerMessageToValue,
On 2012/03/13 00:20:43, akalin wrote:
> &ClientToServerMessageToValue

Done.

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:41: LogData(response,
ClientToServerResponseToValue,
On 2012/03/13 00:20:43, akalin wrote:
> &ClientToServerResponseToValue

Done.

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
File chrome/browser/sync/engine/traffic_logger.h (right):

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.h:14: class ClientToServerMessage;
On 2012/03/13 00:20:43, akalin wrote:
> probably more consistent to use sync_pb::ClientToServerMessage instead

Done.

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/proto_enum_conversions.h (right):

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_enum_conversions.h:35: const char*
ResponseTypeString(
On 2012/03/13 00:20:43, akalin wrote:
> Like the other functions in this file, prefix with 'Get'

Done.

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/proto_value_conversions.cc (right):

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:402: DictionaryValue*
SyncEntityToValue(const sync_pb::SyncEntity& proto,
Put them on a namespace. They are not used elsewhere.
On 2012/03/13 00:20:43, akalin wrote:
> declarations for these functions in the header file?  Or wrap them in an anon
> namespace

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:420: // Let us not go
through the specifics if we dont have to.
removed the comment. The optimization is needed for the second part. We plan to
decode and save the last x traffic in memory. That would run on retail builds. 
On 2012/03/13 00:20:43, akalin wrote:
> Seems like premature optimization, since this code is only hit during
debugging.
> 
> In any case, this comment belongs more with the caller of this function

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/9663023/diff/2009/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.h:144: bool
decode_specifics);
I think true/false is a better choice. The code would read "if
(decode_specifics)". And the caller has a comment indicating the parameter name.
I have seen chromium code that uses both enums and bool. If you are passionate I
can change this. 
On 2012/03/13 00:20:43, akalin wrote:
> use an enum, something like WITH_SPECIFICS / WITHOUT_SPECIFICS

[akalin, 2012-03-14 00:17:30]

okay, few more

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
File chrome/browser/sync/engine/traffic_logger.cc (right):

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:29: VLOG(1) << "\n" << message;
may want to make it a single line, like:

VLOG(1) << "\n" << description << "\n" << message;

Since the message contains newlines anyway, may as well just print the VLOG
prefix stuff once

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:37: // TODO(lipalani) : Store the
data(minus specifics)
space before '(minus'

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:45: // TODO(lipalani) : Store the
data(minus specifics)
here too

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
File chrome/browser/sync/engine/traffic_logger.h (right):

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.h:8: 
Add a comment about this file, specifically that it's here so that one can do
--enable-logging --vmodule='traffic_logger=1' to see sync HTTP traffic

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/proto_value_conversions.cc (right):

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:405: bool
decode_specifics) {
replace decode_specifics with include_specifics everywhere in this file

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:429: void
ConvertSyncEntitiesToValue(DictionaryValue* value,
make this return a ListValue*, and have the caller put it in the dictionary
(similar to the other functions)

rename to SyncEntitiesToValue to be consistent

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:429: void
ConvertSyncEntitiesToValue(DictionaryValue* value,
newline after ( and indent

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:430:
::google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> entities,
const ref?

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:451: DictionaryValue*
CommitMessageToValue(const sync_pb::CommitMessage& proto,
newline and indent after (

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:568:
value->Set("commit", CommitMessageToValue(proto.commit(),
newline after , and indent

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:569: decode_specifics));
line up after CommitMessageToValue

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.h:144: bool
decode_specifics);
i think include_specifics is a better name (the structure in memory is already
decoded)

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.h:148: bool
decode_specifics);
here too.  don't forget to change the reference from traffic_logger

[lipalani1, 2012-03-15 00:02:13]

PTAL.
the next patch is in flight. it would store this in memory.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
File chrome/browser/sync/engine/traffic_logger.cc (right):

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:29: VLOG(1) << "\n" << message;
On 2012/03/14 00:17:30, akalin wrote:
> may want to make it a single line, like:
> 
> VLOG(1) << "\n" << description << "\n" << message;
> 
> Since the message contains newlines anyway, may as well just print the VLOG
> prefix stuff once

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:37: // TODO(lipalani) : Store the
data(minus specifics)
On 2012/03/14 00:17:30, akalin wrote:
> space before '(minus'

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.cc:45: // TODO(lipalani) : Store the
data(minus specifics)
On 2012/03/14 00:17:30, akalin wrote:
> here too

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
File chrome/browser/sync/engine/traffic_logger.h (right):

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
chrome/browser/sync/engine/traffic_logger.h:8: 
On 2012/03/14 00:17:30, akalin wrote:
> Add a comment about this file, specifically that it's here so that one can do
> --enable-logging --vmodule='traffic_logger=1' to see sync HTTP traffic

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/proto_value_conversions.cc (right):

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:405: bool
decode_specifics) {
On 2012/03/14 00:17:30, akalin wrote:
> replace decode_specifics with include_specifics everywhere in this file

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:429: void
ConvertSyncEntitiesToValue(DictionaryValue* value,
On 2012/03/14 00:17:30, akalin wrote:
> make this return a ListValue*, and have the caller put it in the dictionary
> (similar to the other functions)
> 
> rename to SyncEntitiesToValue to be consistent

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:429: void
ConvertSyncEntitiesToValue(DictionaryValue* value,
On 2012/03/14 00:17:30, akalin wrote:
> make this return a ListValue*, and have the caller put it in the dictionary
> (similar to the other functions)
> 
> rename to SyncEntitiesToValue to be consistent

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:430:
::google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> entities,
On 2012/03/14 00:17:30, akalin wrote:
> const ref?

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:451: DictionaryValue*
CommitMessageToValue(const sync_pb::CommitMessage& proto,
On 2012/03/14 00:17:30, akalin wrote:
> newline and indent after (

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:568:
value->Set("commit", CommitMessageToValue(proto.commit(),
On 2012/03/14 00:17:30, akalin wrote:
> newline after , and indent

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.cc:569: decode_specifics));
On 2012/03/14 00:17:30, akalin wrote:
> line up after CommitMessageToValue

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.h:144: bool
decode_specifics);
On 2012/03/14 00:17:30, akalin wrote:
> i think include_specifics is a better name (the structure in memory is already
> decoded)

Done.

http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/proto_value_conversions.h:148: bool
decode_specifics);
On 2012/03/14 00:17:30, akalin wrote:
> here too.  don't forget to change the reference from traffic_logger

Done.

[akalin, 2012-03-15 00:30:09]

btw is this intended to be compiled out in release builds?  If so, you should
use DVLOG instead of VLOG

On 2012/03/15 00:02:13, lipalani1 wrote:
> PTAL.
> the next patch is in flight. it would store this in memory.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
> File chrome/browser/sync/engine/traffic_logger.cc (right):
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
> chrome/browser/sync/engine/traffic_logger.cc:29: VLOG(1) << "\n" << message;
> On 2012/03/14 00:17:30, akalin wrote:
> > may want to make it a single line, like:
> > 
> > VLOG(1) << "\n" << description << "\n" << message;
> > 
> > Since the message contains newlines anyway, may as well just print the VLOG
> > prefix stuff once
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
> chrome/browser/sync/engine/traffic_logger.cc:37: // TODO(lipalani) : Store the
> data(minus specifics)
> On 2012/03/14 00:17:30, akalin wrote:
> > space before '(minus'
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
> chrome/browser/sync/engine/traffic_logger.cc:45: // TODO(lipalani) : Store the
> data(minus specifics)
> On 2012/03/14 00:17:30, akalin wrote:
> > here too
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
> File chrome/browser/sync/engine/traffic_logger.h (right):
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/engine/t...
> chrome/browser/sync/engine/traffic_logger.h:8: 
> On 2012/03/14 00:17:30, akalin wrote:
> > Add a comment about this file, specifically that it's here so that one can
do
> > --enable-logging --vmodule='traffic_logger=1' to see sync HTTP traffic
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> File chrome/browser/sync/protocol/proto_value_conversions.cc (right):
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> chrome/browser/sync/protocol/proto_value_conversions.cc:405: bool
> decode_specifics) {
> On 2012/03/14 00:17:30, akalin wrote:
> > replace decode_specifics with include_specifics everywhere in this file
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> chrome/browser/sync/protocol/proto_value_conversions.cc:429: void
> ConvertSyncEntitiesToValue(DictionaryValue* value,
> On 2012/03/14 00:17:30, akalin wrote:
> > make this return a ListValue*, and have the caller put it in the dictionary
> > (similar to the other functions)
> > 
> > rename to SyncEntitiesToValue to be consistent
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> chrome/browser/sync/protocol/proto_value_conversions.cc:429: void
> ConvertSyncEntitiesToValue(DictionaryValue* value,
> On 2012/03/14 00:17:30, akalin wrote:
> > make this return a ListValue*, and have the caller put it in the dictionary
> > (similar to the other functions)
> > 
> > rename to SyncEntitiesToValue to be consistent
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> chrome/browser/sync/protocol/proto_value_conversions.cc:430:
> ::google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> entities,
> On 2012/03/14 00:17:30, akalin wrote:
> > const ref?
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> chrome/browser/sync/protocol/proto_value_conversions.cc:451: DictionaryValue*
> CommitMessageToValue(const sync_pb::CommitMessage& proto,
> On 2012/03/14 00:17:30, akalin wrote:
> > newline and indent after (
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> chrome/browser/sync/protocol/proto_value_conversions.cc:568:
> value->Set("commit", CommitMessageToValue(proto.commit(),
> On 2012/03/14 00:17:30, akalin wrote:
> > newline after , and indent
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> chrome/browser/sync/protocol/proto_value_conversions.cc:569:
decode_specifics));
> On 2012/03/14 00:17:30, akalin wrote:
> > line up after CommitMessageToValue
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> File chrome/browser/sync/protocol/proto_value_conversions.h (right):
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> chrome/browser/sync/protocol/proto_value_conversions.h:144: bool
> decode_specifics);
> On 2012/03/14 00:17:30, akalin wrote:
> > i think include_specifics is a better name (the structure in memory is
already
> > decoded)
> 
> Done.
> 
>
http://codereview.chromium.org/9663023/diff/7002/chrome/browser/sync/protocol...
> chrome/browser/sync/protocol/proto_value_conversions.h:148: bool
> decode_specifics);
> On 2012/03/14 00:17:30, akalin wrote:
> > here too.  don't forget to change the reference from traffic_logger
> 
> Done.

[lipalani1, 2012-03-15 01:23:26]

Fixed to use DVLOG. PTAL.

[tim (not reviewing), 2012-03-15 04:10:30]

(drive-by comment, not reviewing)

I like the addition of logging.

I think everyone here is a slightly weary of the idea of storing these in a
circular buffer in memory for practical reasons. The benefit to having better
tools is clear. As we've discussed together already, it's a matter of balancing
that with the notion that sometimes we trade off debug-ability for raw
efficiency. If we can keep it truly lean and ultra restricted to a particular
use case, then this has legs.

I think it's really important that the benefits are made clear, though.  We have
this, client debug info, backstreet, and other tools. At least for the parts
that have code living in chromium, having a go-to guide (preferably in code)
that outlines the benefits of each, where they do and do not overlap, and
provides example scenarios of where each excels would be useful. Useful now, and
when the question of "why do[es] we/sync have this" comes up, because it will. 

condition_variable_win.cc has a FAQ (!), you probably don't have to go that
wild, but a how-to of sorts would be a big win, I think.)

[akalin, 2012-03-15 19:18:20]

Can you rebase and reupload?  Thanks!

[lipalani1, 2012-03-16 00:09:52]

PTAL.

Tim - This patch only adds logging to debug builds and will not show up on
retail.

The circular buffer idea is the next patch and I will make sure to address your
comments.

[akalin, 2012-03-16 21:28:27]

few more comments -- sorry, forgot about unit tests

http://codereview.chromium.org/9663023/diff/19001/sync/engine/traffic_logger.cc
File sync/engine/traffic_logger.cc (right):

http://codereview.chromium.org/9663023/diff/19001/sync/engine/traffic_logger....
sync/engine/traffic_logger.cc:28: DVLOG(1) << "\n" << description << message;
you forgot the newline after description:

DVLOG(1) << "\n" << description << "\n" << message;

http://codereview.chromium.org/9663023/diff/19001/sync/engine/traffic_logger.h
File sync/engine/traffic_logger.h (right):

http://codereview.chromium.org/9663023/diff/19001/sync/engine/traffic_logger....
sync/engine/traffic_logger.h:6: // To get the log run chrome with the flag
--vmodule=traffic_logger=1.
chrome -> a debug build of chrome

http://codereview.chromium.org/9663023/diff/19001/sync/protocol/proto_enum_co...
File sync/protocol/proto_enum_conversions.cc (right):

http://codereview.chromium.org/9663023/diff/19001/sync/protocol/proto_enum_co...
sync/protocol/proto_enum_conversions.cc:93: const char* GetResponseTypeString(
i forgot: add unit tests for these new functions (should be pretty easy)

http://codereview.chromium.org/9663023/diff/19001/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/9663023/diff/19001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions.h:142: base::DictionaryValue*
ClientToServerMessageToValue(
add unit tests for these two functions.  Should be pretty easy, although you may
also want to include tests for include_specifics

[lipalani1, 2012-03-17 01:29:50]

PTAL.

http://codereview.chromium.org/9663023/diff/19001/sync/engine/traffic_logger.cc
File sync/engine/traffic_logger.cc (right):

http://codereview.chromium.org/9663023/diff/19001/sync/engine/traffic_logger....
sync/engine/traffic_logger.cc:28: DVLOG(1) << "\n" << description << message;
On 2012/03/16 21:28:27, akalin wrote:
> you forgot the newline after description:
> 
> DVLOG(1) << "\n" << description << "\n" << message;

Done.

http://codereview.chromium.org/9663023/diff/19001/sync/engine/traffic_logger.h
File sync/engine/traffic_logger.h (right):

http://codereview.chromium.org/9663023/diff/19001/sync/engine/traffic_logger....
sync/engine/traffic_logger.h:6: // To get the log run chrome with the flag
--vmodule=traffic_logger=1.
On 2012/03/16 21:28:27, akalin wrote:
> chrome -> a debug build of chrome

Done.

http://codereview.chromium.org/9663023/diff/19001/sync/protocol/proto_enum_co...
File sync/protocol/proto_enum_conversions.cc (right):

http://codereview.chromium.org/9663023/diff/19001/sync/protocol/proto_enum_co...
sync/protocol/proto_enum_conversions.cc:93: const char* GetResponseTypeString(
On 2012/03/16 21:28:27, akalin wrote:
> i forgot: add unit tests for these new functions (should be pretty easy)

Done.

http://codereview.chromium.org/9663023/diff/19001/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/9663023/diff/19001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions.h:142: base::DictionaryValue*
ClientToServerMessageToValue(
On 2012/03/16 21:28:27, akalin wrote:
> add unit tests for these two functions.  Should be pretty easy, although you
may
> also want to include tests for include_specifics

Done.

[akalin, 2012-03-17 01:55:05]

Few more

http://codereview.chromium.org/9663023/diff/26001/sync/engine/syncer_proto_ut...
File sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/9663023/diff/26001/sync/engine/syncer_proto_ut...
sync/engine/syncer_proto_util.cc:8: #include "base/json/json_writer.h"
remove this unneeded include

http://codereview.chromium.org/9663023/diff/26001/sync/engine/traffic_logger.cc
File sync/engine/traffic_logger.cc (right):

http://codereview.chromium.org/9663023/diff/26001/sync/engine/traffic_logger....
sync/engine/traffic_logger.cc:28: DVLOG(1) << "\n" << description << message <<
"\n";
This wasn't what I meant.  Here's the line I pasted again:

DVLOG(1) << "\n" << description << "\n" << message;

This is basically what you had before, just put on one line

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_enum_co...
File sync/protocol/proto_enum_conversions_unittest.cc (right):

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_enum_co...
sync/protocol/proto_enum_conversions_unittest.cc:74: // ErrorType enum is
sequential only upto MIGRATION_DONE.
upto -> up to

Although for consistency it would be better to use the same comment/formatting
as the GetPageTransitionString test above

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_enum_co...
sync/protocol/proto_enum_conversions_unittest.cc:78:
sync_pb::SyncEnums::UNKNOWN);
replace the last UNKNOWN with ErrorType_MAX

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions_unittest.cc (right):

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:191:
sync_pb::ClientToServerMessage message;
You should put a specifics in this message, and test that the converted
dictionary contains an entry for the specifics if you pass in true, and doesn't
if you pass in false.

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:203:
scoped_ptr<DictionaryValue> value_with_specifics(
same here

[lipalani1, 2012-03-19 19:24:37]

PTAL.

http://codereview.chromium.org/9663023/diff/26001/sync/engine/syncer_proto_ut...
File sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/9663023/diff/26001/sync/engine/syncer_proto_ut...
sync/engine/syncer_proto_util.cc:8: #include "base/json/json_writer.h"
On 2012/03/17 01:55:05, akalin wrote:
> remove this unneeded include

Done.

http://codereview.chromium.org/9663023/diff/26001/sync/engine/traffic_logger.cc
File sync/engine/traffic_logger.cc (right):

http://codereview.chromium.org/9663023/diff/26001/sync/engine/traffic_logger....
sync/engine/traffic_logger.cc:28: DVLOG(1) << "\n" << description << message <<
"\n";
On 2012/03/17 01:55:05, akalin wrote:
> This wasn't what I meant.  Here's the line I pasted again:
> 
> DVLOG(1) << "\n" << description << "\n" << message;
> 
> This is basically what you had before, just put on one line

Done.

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_enum_co...
File sync/protocol/proto_enum_conversions_unittest.cc (right):

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_enum_co...
sync/protocol/proto_enum_conversions_unittest.cc:74: // ErrorType enum is
sequential only upto MIGRATION_DONE.
On 2012/03/17 01:55:05, akalin wrote:
> upto -> up to
> 
> Although for consistency it would be better to use the same comment/formatting
> as the GetPageTransitionString test above

Done.

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_enum_co...
sync/protocol/proto_enum_conversions_unittest.cc:78:
sync_pb::SyncEnums::UNKNOWN);
On 2012/03/17 01:55:05, akalin wrote:
> replace the last UNKNOWN with ErrorType_MAX

Done.

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions_unittest.cc (right):

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:191:
sync_pb::ClientToServerMessage message;
On 2012/03/17 01:55:05, akalin wrote:
> You should put a specifics in this message, and test that the converted
> dictionary contains an entry for the specifics if you pass in true, and
doesn't
> if you pass in false.

Done.

http://codereview.chromium.org/9663023/diff/26001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:203:
scoped_ptr<DictionaryValue> value_with_specifics(
On 2012/03/17 01:55:05, akalin wrote:
> same here

Done.

[akalin, 2012-03-19 21:31:38]

Few more test comments.  Almost there!

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions_unittest.cc (right):

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:191: void
VerifySpecifics(DictionaryValue* value,
I think it would be cleaner to have this something like:

// Returns whether the given value has specifics under the entries in the given
path.
bool ValueHasSpecifics(const DictionaryValue& value, const std::string&
entries_path) {
  ...

  return entry_dictionary->GetDictionary(...);
}

and move the asserts to the tests themselves

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:194: ListValue* entities_list;
initialize to NULL

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:197:
EXPECT_TRUE(value->GetList(path, &entities_list));
use

if(!...) {
  return false;
}

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:205:
TEST_F(ProtoValueConversionsTest, ClientToServerMessageToValue) {
add test-level comment.  Something like

// Create a ClientToServerMessage with an EntitySpecifics.  Converting it to a
value should respect the |include_specifics| flag.

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:214:
VerifySpecifics(value_with_specifics.get(), "commit.entries", true);
true -> true /* include_specifics */

(everywhere else you call VerifySpecifics, too)

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:222:
TEST_F(ProtoValueConversionsTest, ClientToServerResponseToValue) {
add test-level comment

[lipalani1, 2012-03-20 19:24:40]

PTAL.

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions_unittest.cc (right):

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:191: void
VerifySpecifics(DictionaryValue* value,
On 2012/03/19 21:31:38, akalin wrote:
> I think it would be cleaner to have this something like:
> 
> // Returns whether the given value has specifics under the entries in the
given
> path.
> bool ValueHasSpecifics(const DictionaryValue& value, const std::string&
> entries_path) {
>   ...
> 
>   return entry_dictionary->GetDictionary(...);
> }
> 
> and move the asserts to the tests themselves

Done.

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:194: ListValue* entities_list;
On 2012/03/19 21:31:38, akalin wrote:
> initialize to NULL

Done.

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:197:
EXPECT_TRUE(value->GetList(path, &entities_list));
On 2012/03/19 21:31:38, akalin wrote:
> use
> 
> if(!...) {
>   return false;
> }

Done.

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:205:
TEST_F(ProtoValueConversionsTest, ClientToServerMessageToValue) {
On 2012/03/19 21:31:38, akalin wrote:
> add test-level comment.  Something like
> 
> // Create a ClientToServerMessage with an EntitySpecifics.  Converting it to a
> value should respect the |include_specifics| flag.

Done.

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:214:
VerifySpecifics(value_with_specifics.get(), "commit.entries", true);
The param is removed.
On 2012/03/19 21:31:38, akalin wrote:
> true -> true /* include_specifics */
> 
> (everywhere else you call VerifySpecifics, too)

Done.

http://codereview.chromium.org/9663023/diff/31001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:222:
TEST_F(ProtoValueConversionsTest, ClientToServerResponseToValue) {
On 2012/03/19 21:31:38, akalin wrote:
> add test-level comment

Done.

[akalin, 2012-03-20 19:38:34]

LGTM after nits

http://codereview.chromium.org/9663023/diff/37001/sync/engine/traffic_logger.cc
File sync/engine/traffic_logger.cc (right):

http://codereview.chromium.org/9663023/diff/37001/sync/engine/traffic_logger....
sync/engine/traffic_logger.cc:24: scoped_ptr<DictionaryValue>
value((*to_dictionary_value)(data,
better to indent like:

scoped_ptr<...> value(
    (*to_dictionary_value)(data, true /* include_specifics */));

http://codereview.chromium.org/9663023/diff/37001/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions.cc (right):

http://codereview.chromium.org/9663023/diff/37001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions.cc:11: #include "base/bind.h"
I don't think you need these two includes anymore, right?

http://codereview.chromium.org/9663023/diff/37001/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions_unittest.cc (right):

http://codereview.chromium.org/9663023/diff/37001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:193: bool
ValueHasSpecifics(DictionaryValue* value,
use a const DictionaryValue&

http://codereview.chromium.org/9663023/diff/37001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:211: // Create a
ClientToServerMessage with an EntitySpecifics.  Converting it to
move comment to above TEST_F

http://codereview.chromium.org/9663023/diff/37001/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions_unittest.cc:231: // Create a
ClientToServerResponse with an EntitySpecifics.  Converting it to
here, too

[commit-bot: I haz the power, 2012-03-20 20:04:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/9663023/39004

[commit-bot: I haz the power, 2012-03-20 20:31:50]

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

[commit-bot: I haz the power, 2012-03-21 17:34:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/9663023/37012

[commit-bot: I haz the power, 2012-03-21 18:01:23]

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

[commit-bot: I haz the power, 2012-03-21 20:47:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/9663023/62001

[commit-bot: I haz the power, 2012-03-22 18:48:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/9663023/56002

[commit-bot: I haz the power, 2012-03-22 19:56:23]

Change committed as 128277