[Sync] Display the client server traffic log in about:sync.

A new 'traffic' tab is introduced and clicking the button on the tab would display the traffic.


BUG=117615
TEST=


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

[lipalani1, 2012-03-29 21:07:05]

Fred - Feel free to review as you find time. Feedback is very much appreciated.

Richard - You are the main reviewer. You can ignore the first 7 patches they are
all for self review.

[tim (not reviewing), 2012-03-29 22:00:10]

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:2193: SyncSessionContext*
context = scheduler()->session_context();
It's circuitous and somewhat of a layering violation to request the session
context from the scheduler for use from SyncManager. SyncManager creates it and
blesses it with whatever state the lower engine layer needs from SyncManager
land. Unless I'm missing a unique circumstance here, it seems like there's no
reason to not follow the pattern all other objects use with SyncSessionContext
-- if the traffic recorder is meant to be used from SyncManager, then
SyncManager should create it and own it, and pass it into the context for the
lower layers to write to. 

And then we don't expose it from SyncScheduler.

[lipalani1, 2012-03-29 22:15:33]

You are right WRT convention and I can follow it here too assuming there is
enough pushback(debug info getter which is similar to this class follows the
convention you mention).

my rationale is the following:
1. The constructor for context is getting huge.
2. If there is no need for the object to live as long as the sync manager
does(particularly important if the object could be accessed during shutdown)
then it kind of looks circuitous to create it from sync manager, pass it to
context and keep a local copy in sync manager as well. Instead have the lifetime
managed in context itself.

I wanted to bounce the idea in code and see. And more importantly this would
also give us the ability to one day expose all of context's internals in
about:sync. I am glad you jumped in on the thread, I forgot about this change in
the midst of other big things. let me know what you think.
On 2012/03/29 22:00:10, timsteele wrote:
>
http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
> File chrome/browser/sync/internal_api/sync_manager.cc (right):
> 
>
http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
> chrome/browser/sync/internal_api/sync_manager.cc:2193: SyncSessionContext*
> context = scheduler()->session_context();
> It's circuitous and somewhat of a layering violation to request the session
> context from the scheduler for use from SyncManager. SyncManager creates it
and
> blesses it with whatever state the lower engine layer needs from SyncManager
> land. Unless I'm missing a unique circumstance here, it seems like there's no
> reason to not follow the pattern all other objects use with SyncSessionContext
> -- if the traffic recorder is meant to be used from SyncManager, then
> SyncManager should create it and own it, and pass it into the context for the
> lower layers to write to. 
> 
> And then we don't expose it from SyncScheduler.

[lipalani1, 2012-03-29 22:16:56]

Also Richard and Fred you can continue reviewing. Even if I take Tim's
suggestion I would only have to change few lines(modify context's constructor to
take an extra param).
On 2012/03/29 22:15:33, lipalani1 wrote:
> You are right WRT convention and I can follow it here too assuming there is
> enough pushback(debug info getter which is similar to this class follows the
> convention you mention).
> 
> my rationale is the following:
> 1. The constructor for context is getting huge.
> 2. If there is no need for the object to live as long as the sync manager
> does(particularly important if the object could be accessed during shutdown)
> then it kind of looks circuitous to create it from sync manager, pass it to
> context and keep a local copy in sync manager as well. Instead have the
lifetime
> managed in context itself.
> 
> I wanted to bounce the idea in code and see. And more importantly this would
> also give us the ability to one day expose all of context's internals in
> about:sync. I am glad you jumped in on the thread, I forgot about this change
in
> the midst of other big things. let me know what you think.
> On 2012/03/29 22:00:10, timsteele wrote:
> >
>
http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
> > File chrome/browser/sync/internal_api/sync_manager.cc (right):
> > 
> >
>
http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
> > chrome/browser/sync/internal_api/sync_manager.cc:2193: SyncSessionContext*
> > context = scheduler()->session_context();
> > It's circuitous and somewhat of a layering violation to request the session
> > context from the scheduler for use from SyncManager. SyncManager creates it
> and
> > blesses it with whatever state the lower engine layer needs from SyncManager
> > land. Unless I'm missing a unique circumstance here, it seems like there's
no
> > reason to not follow the pattern all other objects use with
SyncSessionContext
> > -- if the traffic recorder is meant to be used from SyncManager, then
> > SyncManager should create it and own it, and pass it into the context for
the
> > lower layers to write to. 
> > 
> > And then we don't expose it from SyncScheduler.

[tim (not reviewing), 2012-03-29 22:36:09]

On 2012/03/29 22:15:33, lipalani1 wrote:
> You are right WRT convention and I can follow it here too assuming there is
> enough pushback(debug info getter which is similar to this class follows the
> convention you mention).
> 
> my rationale is the following:
> 1. The constructor for context is getting huge.
> 2. If there is no need for the object to live as long as the sync manager
> does(particularly important if the object could be accessed during shutdown)
> then it kind of looks circuitous to create it from sync manager, pass it to
> context and keep a local copy in sync manager as well. Instead have the
lifetime
> managed in context itself.
> 
> I wanted to bounce the idea in code and see. And more importantly this would
> also give us the ability to one day expose all of context's internals in
> about:sync. I am glad you jumped in on the thread, I forgot about this change
in
> the midst of other big things. let me know what you think.
> On 2012/03/29 22:00:10, timsteele wrote:
> >
>
http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
> > File chrome/browser/sync/internal_api/sync_manager.cc (right):
> > 
> >
>
http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
> > chrome/browser/sync/internal_api/sync_manager.cc:2193: SyncSessionContext*
> > context = scheduler()->session_context();
> > It's circuitous and somewhat of a layering violation to request the session
> > context from the scheduler for use from SyncManager. SyncManager creates it
> and
> > blesses it with whatever state the lower engine layer needs from SyncManager
> > land. Unless I'm missing a unique circumstance here, it seems like there's
no
> > reason to not follow the pattern all other objects use with
SyncSessionContext
> > -- if the traffic recorder is meant to be used from SyncManager, then
> > SyncManager should create it and own it, and pass it into the context for
the
> > lower layers to write to. 
> > 
> > And then we don't expose it from SyncScheduler.

Yeah, I agree the large constructor is burdensome (especially in unittests!).
The SyncSessionContext is intended to provide the full surrounding environment
for a sync session. As such, right now, it is a dumb container. To address the
large init list, I think here we should follow the pattern other similar
purposed components like net/url_request/url_request_context do- have an empty
do-nothing constructor and individual setters. This would make tests that don't
care about certain components a lot nicer to write, and would avoid the
craziness of trying to discern the ordering of NULLs we pass in when things are
removed / added. I'm hugely in favor of doing _even less_ work in this
constructor, so I think this could be a nice change!

Then, if we need to have the creation and lifetimes of things managed by the
context, we should have the SyncManager own the context, and access the various
parts through this context pointer / ownership. The SyncManager is responsible
for setting up the official top level environment for the engine, so it makes
sense that it be composed of this environment container explicitly. I've
considered doing this before, the only reason I didn't implement it that way
initially is because I didn't see a reason to.  Doing this plus the set/get
proposal above would let us have finer grained control over lifetimes, although
warning - I'm nervous about tearing things down early unless absolutely
necessary.  I think being able to have full trust in the environment always
being there (from within the scheduler) is a warm & fuzzy cushion when writing
code in there.

[lipalani1, 2012-03-29 22:47:11]

I hear your concern and fair enough. I will follow the convention for now. What
you are suggesting will probably need to be a change by itself.(and we have to
think carefully about shutdown scenarios as it is a little fragile!)
On 2012/03/29 22:36:09, timsteele wrote:
> On 2012/03/29 22:15:33, lipalani1 wrote:
> > You are right WRT convention and I can follow it here too assuming there is
> > enough pushback(debug info getter which is similar to this class follows the
> > convention you mention).
> > 
> > my rationale is the following:
> > 1. The constructor for context is getting huge.
> > 2. If there is no need for the object to live as long as the sync manager
> > does(particularly important if the object could be accessed during shutdown)
> > then it kind of looks circuitous to create it from sync manager, pass it to
> > context and keep a local copy in sync manager as well. Instead have the
> lifetime
> > managed in context itself.
> > 
> > I wanted to bounce the idea in code and see. And more importantly this would
> > also give us the ability to one day expose all of context's internals in
> > about:sync. I am glad you jumped in on the thread, I forgot about this
change
> in
> > the midst of other big things. let me know what you think.
> > On 2012/03/29 22:00:10, timsteele wrote:
> > >
> >
>
http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
> > > File chrome/browser/sync/internal_api/sync_manager.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
> > > chrome/browser/sync/internal_api/sync_manager.cc:2193: SyncSessionContext*
> > > context = scheduler()->session_context();
> > > It's circuitous and somewhat of a layering violation to request the
session
> > > context from the scheduler for use from SyncManager. SyncManager creates
it
> > and
> > > blesses it with whatever state the lower engine layer needs from
SyncManager
> > > land. Unless I'm missing a unique circumstance here, it seems like there's
> no
> > > reason to not follow the pattern all other objects use with
> SyncSessionContext
> > > -- if the traffic recorder is meant to be used from SyncManager, then
> > > SyncManager should create it and own it, and pass it into the context for
> the
> > > lower layers to write to. 
> > > 
> > > And then we don't expose it from SyncScheduler.
> 
> Yeah, I agree the large constructor is burdensome (especially in unittests!).
> The SyncSessionContext is intended to provide the full surrounding environment
> for a sync session. As such, right now, it is a dumb container. To address the
> large init list, I think here we should follow the pattern other similar
> purposed components like net/url_request/url_request_context do- have an empty
> do-nothing constructor and individual setters. This would make tests that
don't
> care about certain components a lot nicer to write, and would avoid the
> craziness of trying to discern the ordering of NULLs we pass in when things
are
> removed / added. I'm hugely in favor of doing _even less_ work in this
> constructor, so I think this could be a nice change!
> 
> Then, if we need to have the creation and lifetimes of things managed by the
> context, we should have the SyncManager own the context, and access the
various
> parts through this context pointer / ownership. The SyncManager is responsible
> for setting up the official top level environment for the engine, so it makes
> sense that it be composed of this environment container explicitly. I've
> considered doing this before, the only reason I didn't implement it that way
> initially is because I didn't see a reason to.  Doing this plus the set/get
> proposal above would let us have finer grained control over lifetimes,
although
> warning - I'm nervous about tearing things down early unless absolutely
> necessary.  I think being able to have full trust in the environment always
> being there (from within the scheduler) is a warm & fuzzy cushion when writing
> code in there.

[rlarocque, 2012-03-29 22:54:26]

Initial comments.

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/traffic.js (right):

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/traffic.js:5: (function() {
I don't think this is good style, though I'm not sure what good style is. 
Should this be hooked up to an onLoad() event?  I guess it's better than the
current practice on about:sync of declaring global functions then directly
calling them. 

I'd feel better about this review if we included a JavaScript expert.

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:2193: SyncSessionContext*
context = scheduler()->session_context();
On 2012/03/29 22:00:10, timsteele wrote:
> It's circuitous and somewhat of a layering violation to request the session
> context from the scheduler for use from SyncManager. SyncManager creates it
and
> blesses it with whatever state the lower engine layer needs from SyncManager
> land. Unless I'm missing a unique circumstance here, it seems like there's no
> reason to not follow the pattern all other objects use with SyncSessionContext
> -- if the traffic recorder is meant to be used from SyncManager, then
> SyncManager should create it and own it, and pass it into the context for the
> lower layers to write to. 
> 
> And then we don't expose it from SyncScheduler.

+1

http://codereview.chromium.org/9826035/diff/13020/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/9826035/diff/13020/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:131: sessions::SyncSessionContext*
session_context() {
Don't forget to remove this if you're willing to accept Tim's comment.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:46: const char*
GetMessageTypeString(TrafficRecorder::TrafficMessageType type) {
I'd like to take this opportunity to hide some unusual terminology.  Could we
translate these as either {"To client","To server"} or {"Request","Response"}? 
I think the current naming, where both types are called "CLIENT_TO_SERVER" is
confusing.  

While there is a cost to not having the enums match the strings exactly, in this
case the enums exist only inside the TrafficRecorder so I don't feel too bad
about it.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:91: std::string message_id =
StringPrintf("message%d", message_count);
Couldn't this be an array instead?  I don't see the point of enumerating each
message with a string key.

[rlarocque, 2012-03-29 22:58:49]

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:69: ClientToServerMessageToValue(message_proto,
Because the JavaScript isn't processing this in any way, would it be easier to
expose this as one big string?  Our serialization format doesn't necessarily
have to be JSON.

[lipalani1, 2012-03-29 23:58:37]

Drew - would you mind reviewing the JS file?
Thanks!

Tim - the dependancy is removed.

Richard - comments addressed. Please take a look.

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/traffic.js (right):

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/traffic.js:5: (function() {
ha ha I dont know either. I just copied from other places in about:sync that
does similar things.(like about.js etc. I will include Drew for review.)
On 2012/03/29 22:54:27, rlarocque wrote:
> I don't think this is good style, though I'm not sure what good style is. 
> Should this be hooked up to an onLoad() event?  I guess it's better than the
> current practice on about:sync of declaring global functions then directly
> calling them. 
> 
> I'd feel better about this review if we included a JavaScript expert.

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:2193: SyncSessionContext*
context = scheduler()->session_context();
On 2012/03/29 22:00:10, timsteele wrote:
> It's circuitous and somewhat of a layering violation to request the session
> context from the scheduler for use from SyncManager. SyncManager creates it
and
> blesses it with whatever state the lower engine layer needs from SyncManager
> land. Unless I'm missing a unique circumstance here, it seems like there's no
> reason to not follow the pattern all other objects use with SyncSessionContext
> -- if the traffic recorder is meant to be used from SyncManager, then
> SyncManager should create it and own it, and pass it into the context for the
> lower layers to write to. 
> 
> And then we don't expose it from SyncScheduler.

Done.

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:2193: SyncSessionContext*
context = scheduler()->session_context();
On 2012/03/29 22:54:27, rlarocque wrote:
> On 2012/03/29 22:00:10, timsteele wrote:
> > It's circuitous and somewhat of a layering violation to request the session
> > context from the scheduler for use from SyncManager. SyncManager creates it
> and
> > blesses it with whatever state the lower engine layer needs from SyncManager
> > land. Unless I'm missing a unique circumstance here, it seems like there's
no
> > reason to not follow the pattern all other objects use with
SyncSessionContext
> > -- if the traffic recorder is meant to be used from SyncManager, then
> > SyncManager should create it and own it, and pass it into the context for
the
> > lower layers to write to. 
> > 
> > And then we don't expose it from SyncScheduler.
> 
> +1

Done.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/9826035/diff/13020/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:131: sessions::SyncSessionContext*
session_context() {
On 2012/03/29 22:54:27, rlarocque wrote:
> Don't forget to remove this if you're willing to accept Tim's comment.

Done.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:46: const char*
GetMessageTypeString(TrafficRecorder::TrafficMessageType type) {
On 2012/03/29 22:54:27, rlarocque wrote:
> I'd like to take this opportunity to hide some unusual terminology.  Could we
> translate these as either {"To client","To server"} or {"Request","Response"}?

> I think the current naming, where both types are called "CLIENT_TO_SERVER" is
> confusing.  
> 
> While there is a cost to not having the enums match the strings exactly, in
this
> case the enums exist only inside the TrafficRecorder so I don't feel too bad
> about it.  

Done.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:69: ClientToServerMessageToValue(message_proto,
JS converts this into a formatted string using JSON. this is the convention so
far. Also this makes it easier to wiring up the function to sync manager
etc(which expects dictionary)
On 2012/03/29 22:58:49, rlarocque wrote:
> Because the JavaScript isn't processing this in any way, would it be easier to
> expose this as one big string?  Our serialization format doesn't necessarily
> have to be JSON.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:91: std::string message_id =
StringPrintf("message%d", message_count);
We have to put this in a dictionary. Having it as message1, message2 etc in
about:sync page looked to me. let me know if you have some other idea.
On 2012/03/29 22:54:27, rlarocque wrote:
> Couldn't this be an array instead?  I don't see the point of enumerating each
> message with a string key.

[rlarocque, 2012-03-30 00:20:51]

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:67: if (message_proto.ParseFromString(message))
By the way, do we really need to round-trip the message protos through a
serialization to string and back?  Why do we serialize the message only to
ParseFromString() later on?

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:69: ClientToServerMessageToValue(message_proto,
On 2012/03/29 23:58:37, lipalani1 wrote:
> JS converts this into a formatted string using JSON. this is the convention so
> far. Also this makes it easier to wiring up the function to sync manager
> etc(which expects dictionary)

Right, but that dictionary could consist solely of { "value" : <custom
serialization traffic logs> }.  We can serialize and format this however we want
from C++, rather than trying to stuff this information into a hierarchy of Value
objects.

Besides, I thought this function was only called from
TrafficRecorder::ToValue()?  I realize you need to return a dictionary
eventually, but I don't think that constraint applies here.  

The current approach gets the job done, so I'm not against it.  I'm just
pointing out that there might be other options.  An automatically generated
protobuf-to-string method, for example, would probably be easier to maintain. 
Though I think we lost that option by using the "lite" version of protobufs.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:91: std::string message_id =
StringPrintf("message%d", message_count);
On 2012/03/29 23:58:37, lipalani1 wrote:
> We have to put this in a dictionary. Having it as message1, message2 etc in
> about:sync page looked to me. let me know if you have some other idea.

Same idea as above.  A single-attribute dictionary will let you return anything
you want.  I'd prefer that form of dictionary abuse rather than generating keys
to pretend it's an array.  At least that way you can still grab the array and
iterate over it without having to do strange things like construct the set of
keys.  

Cleaner solutions, like modifying the SyncManager to not necessarily require a
dictionary, would be acceptable, too.

[akalin, 2012-03-30 01:45:35]

Looks pretty good, some comments

http://codereview.chromium.org/9826035/diff/21002/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/traffic.html (right):

http://codereview.chromium.org/9826035/diff/21002/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/traffic.html:1: <p><strong>Some personal
info may be in the data dump.  An option to
Shorten this to something like:

Some personal info may be in the traffic dump.

Feel free to modify the place you copied this from (but change 'traffic' to
'data')

It might be worthwhile to include the traffic dump in the data dump, too.

http://codereview.chromium.org/9826035/diff/21002/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/21002/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:44: #define ENUM_CASE(x) case
TrafficRecorder::##x: return #x; break;
looks like you don't use this macro after all

http://codereview.chromium.org/9826035/diff/21002/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:65: GetMessageTypeString(message_type));
line up with (

http://codereview.chromium.org/9826035/diff/21002/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:93: std::string message_id =
StringPrintf("message%d", message_count);
is it necessary to have a string ID?  Why not fill a ListValue and return that
instead?  As a bonus you preserve order.

[akalin, 2012-03-30 01:55:04]

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/traffic.js (right):

http://codereview.chromium.org/9826035/diff/13020/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/traffic.js:5: (function() {
On 2012/03/29 22:54:27, rlarocque wrote:
> I don't think this is good style, though I'm not sure what good style is. 
> Should this be hooked up to an onLoad() event?  I guess it's better than the
> current practice on about:sync of declaring global functions then directly
> calling them. 
> 
> I'd feel better about this review if we included a JavaScript expert. 

This is the 'Javascript way' of avoiding pollution of the global namespace. 
FWIW, the places this is copied from was reviewed by arv@ (Javascript guru) and
he let it stand.  I defer to Drew on this, though.

onLoad isn't necessary since this .js file is included after the elements it
references.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:67: if (message_proto.ParseFromString(message))
On 2012/03/30 00:20:51, rlarocque wrote:
> By the way, do we really need to round-trip the message protos through a
> serialization to string and back?  Why do we serialize the message only to
> ParseFromString() later on?

Yeah, I wanted it to be stored serialized to save memory (since this is always
logged, regardless of whether about:sync is open).  We already bound memory
usage, but storing it serialized will enable storing bigger messages.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:69: ClientToServerMessageToValue(message_proto,
On 2012/03/30 00:20:51, rlarocque wrote:
> Right, but that dictionary could consist solely of { "value" : <custom
> serialization traffic logs> }.  We can serialize and format this however we
want
> from C++, rather than trying to stuff this information into a hierarchy of
Value
> objects.
> 
> Besides, I thought this function was only called from
> TrafficRecorder::ToValue()?  I realize you need to return a dictionary
> eventually, but I don't think that constraint applies here.  
> 
> The current approach gets the job done, so I'm not against it.  I'm just
> pointing out that there might be other options.  An automatically generated
> protobuf-to-string method, for example, would probably be easier to maintain. 
> Though I think we lost that option by using the "lite" version of protobufs.

I'm not sure I follow; why would serializing to a string be better than
(essentially) the current JSON format?  Ideally, this would be displayed in JS
as something other than a raw string dump (a tree, etc.), and keeping the JSON
format would avoid the need for the JS code to try and parse the string to split
it up, etc.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:91: std::string message_id =
StringPrintf("message%d", message_count);
On 2012/03/30 00:20:51, rlarocque wrote:
> On 2012/03/29 23:58:37, lipalani1 wrote:
> > We have to put this in a dictionary. Having it as message1, message2 etc in
> > about:sync page looked to me. let me know if you have some other idea.
> 
> Same idea as above.  A single-attribute dictionary will let you return
anything
> you want.  I'd prefer that form of dictionary abuse rather than generating
keys
> to pretend it's an array.  At least that way you can still grab the array and
> iterate over it without having to do strange things like construct the set of
> keys.  
> 
> Cleaner solutions, like modifying the SyncManager to not necessarily require a
> dictionary, would be acceptable, too.

See my comment re. using ListValue for this.  The returned dictionary value
should look something like:

{
  trafficRecords: [
    { (traffic record 0) },
    { (traffic record 1) },
    ....
  ]
}

[rlarocque, 2012-03-30 17:49:01]

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:69: ClientToServerMessageToValue(message_proto,
On 2012/03/30 01:55:04, akalin wrote:
> On 2012/03/30 00:20:51, rlarocque wrote:
> > Right, but that dictionary could consist solely of { "value" : <custom
> > serialization traffic logs> }.  We can serialize and format this however we
> want
> > from C++, rather than trying to stuff this information into a hierarchy of
> Value
> > objects.
> > 
> > Besides, I thought this function was only called from
> > TrafficRecorder::ToValue()?  I realize you need to return a dictionary
> > eventually, but I don't think that constraint applies here.  
> > 
> > The current approach gets the job done, so I'm not against it.  I'm just
> > pointing out that there might be other options.  An automatically generated
> > protobuf-to-string method, for example, would probably be easier to
maintain. 
> > Though I think we lost that option by using the "lite" version of protobufs.
> 
> I'm not sure I follow; why would serializing to a string be better than
> (essentially) the current JSON format?  Ideally, this would be displayed in JS
> as something other than a raw string dump (a tree, etc.), and keeping the JSON
> format would avoid the need for the JS code to try and parse the string to
split
> it up, etc.

I don't mean to argue that it's necessarily better, only to point out that it
should be considered.

Serializing to JSON is only useful if we intend to slice and dice this data in
JavaScript.

The drawback to converting everything to JSON/Value* is that we have to maintain
the conversion functions.  It's going to be hard to keep
ClientToServerMessageToValue up to date.  I was hoping protobufs could help us
with this, but I'm not sure that's possible, due to other technical limitations.

Using JSON also makes it harder to use C++ code to process this data before
presenting it.  Though it seems unlikely that we'll be doing much formatting in
any language, so I'm not too concerned about that at this point.

[lipalani1, 2012-03-30 20:35:16]

fixed feedback.
Also spoke with Richard offline to make sure I understand the concerns and it is
fixed here as well.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:67: if (message_proto.ParseFromString(message))
We had a discussion about the best format for storing the message(as the
protobuf struct, string or dictionary). string gave the most bang for the buck
because it was very compressed(probably because it is the same text that is sent
across the wire) and serializing is not too heavy too. I am not too sure about
deserializing but in that case performance delay is OK.
On 2012/03/30 00:20:51, rlarocque wrote:
> By the way, do we really need to round-trip the message protos through a
> serialization to string and back?  Why do we serialize the message only to
> ParseFromString() later on?

http://codereview.chromium.org/9826035/diff/21002/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/traffic.html (right):

http://codereview.chromium.org/9826035/diff/21002/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/traffic.html:1: <p><strong>Some personal
info may be in the data dump.  An option to
I had it initially hooked up to data dump but the amount of data got little
huge. However I am open to it if there is interest. I can do it in the future
too if we want to wait and watch.
On 2012/03/30 01:45:35, akalin wrote:
> Shorten this to something like:
> 
> Some personal info may be in the traffic dump.
> 
> Feel free to modify the place you copied this from (but change 'traffic' to
> 'data')
> 
> It might be worthwhile to include the traffic dump in the data dump, too.

Done.

http://codereview.chromium.org/9826035/diff/21002/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/21002/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:44: #define ENUM_CASE(x) case
TrafficRecorder::##x: return #x; break;
On 2012/03/30 01:45:35, akalin wrote:
> looks like you don't use this macro after all

Done.

http://codereview.chromium.org/9826035/diff/21002/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:65: GetMessageTypeString(message_type));
On 2012/03/30 01:45:35, akalin wrote:
> line up with (

Done.

http://codereview.chromium.org/9826035/diff/21002/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:93: std::string message_id =
StringPrintf("message%d", message_count);
On 2012/03/30 01:45:35, akalin wrote:
> is it necessary to have a string ID?  Why not fill a ListValue and return that
> instead?  As a bonus you preserve order.

Done.

[rlarocque, 2012-03-30 21:01:11]

One small comment, but otherwise LGTM from me.  I think you should wait to hear
from other reviewers before committing, though.

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/13020/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:67: if (message_proto.ParseFromString(message))
I'm not entirely convinced that this will be a significant performance win (when
compared against a more straightforward solution), but I do agree that it will
do little or no harm.  That's good enough for me.

http://codereview.chromium.org/9826035/diff/28003/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/28003/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:112: if ((unsigned int)msg.ByteSize() >=
max_message_size_) {
This line changed a few times throughout the review so I may have missed some of
the comments that led to this.  

The diff from trunk to version 12 of this patch set shows that we're going from
a static_cast<unsigned int>() to a C style cast here.  I thought the usual style
was to use C++ casts?

[Andrew T Wilson (Slow), 2012-03-30 21:18:56]

CSS/JS LGTM with a couple of nits.

http://codereview.chromium.org/9826035/diff/28003/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/sync_index.html (right):

http://codereview.chromium.org/9826035/diff/28003/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_index.html:47: pre {
I'm not sure we want to apply this style to all <pre> sections. Perhaps you
should just apply this to #traffic-dump elements instead?

http://codereview.chromium.org/9826035/diff/28003/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/traffic.js (right):

http://codereview.chromium.org/9826035/diff/28003/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/traffic.js:6: var trafficToTextButton =
document.getElementById('traffic-to-text');
I think you should indent this by two spaces - or does the style guide say this
is not necessary for anonymous namespace-esque functions?

[lipalani1, 2012-03-30 22:04:57]

Including jhawkins for owners approval of the changes under webui.

[James Hawkins, 2012-03-30 22:19:17]

sync_internals_ui.cc LGTM

[lipalani1, 2012-03-30 23:34:05]

Fixed the nits pointed out by Drew and Richard.

I will wait and see if Fred has any comments otherwise will submit sometime
tomorrow.

Fred - Even if you get to this at a later point of time(be it later next week or
after vacation) I am happy to take the feedback and do the necessary revisions.

http://codereview.chromium.org/9826035/diff/28003/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/sync_index.html (right):

http://codereview.chromium.org/9826035/diff/28003/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_index.html:47: pre {
On 2012/03/30 21:18:56, Andrew T Wilson wrote:
> I'm not sure we want to apply this style to all <pre> sections. Perhaps you
> should just apply this to #traffic-dump elements instead?

Done.

http://codereview.chromium.org/9826035/diff/28003/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/traffic.js (right):

http://codereview.chromium.org/9826035/diff/28003/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/traffic.js:6: var trafficToTextButton =
document.getElementById('traffic-to-text');
On 2012/03/30 21:18:56, Andrew T Wilson wrote:
> I think you should indent this by two spaces - or does the style guide say
this
> is not necessary for anonymous namespace-esque functions?

Done.

http://codereview.chromium.org/9826035/diff/28003/sync/engine/traffic_recorde...
File sync/engine/traffic_recorder.cc (right):

http://codereview.chromium.org/9826035/diff/28003/sync/engine/traffic_recorde...
sync/engine/traffic_recorder.cc:112: if ((unsigned int)msg.ByteSize() >=
max_message_size_) {
Bad merge. It is static_cast after the latest rebase.
On 2012/03/30 21:01:11, rlarocque wrote:
> This line changed a few times throughout the review so I may have missed some
of
> the comments that led to this.  
> 
> The diff from trunk to version 12 of this patch set shows that we're going
from
> a static_cast<unsigned int>() to a C style cast here.  I thought the usual
style
> was to use C++ casts?

[commit-bot: I haz the power, 2012-03-31 17:31:28]

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

[commit-bot: I haz the power, 2012-03-31 17:40:34]

Try job failure for 9826035-30015 (retry) on android for steps "Compile, build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

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

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

[commit-bot: I haz the power, 2012-03-31 18:03:40]

Try job failure for 9826035-32012 (retry) on android for steps "Compile, build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-03-31 22:18:30]

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

[commit-bot: I haz the power, 2012-04-01 20:22:47]

Change committed as 130081