Send important client side event information to the server.

Send important client side event information to the server. We create a class called DebugInfoEventListener which implements 2 interfaces. one being the syncmanager::observer to observe events from syncmanager.
The other being DebugInfoGetter so that syncer can call this class to give the debug information in protobuf format.
The implementation of this class uses a queue. And it limits the number of events to 6. if more than 6 events are in the queue we delete the oldest event.
Also we send this information to the server only once per sync cycle during the first getudpdates command.

also includes the unit test.

TBR=cmp@chromium.org for the SI relaxation.
BUG=100058
TEST=

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

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

[lipalani1, 2011-10-13 00:34:44]

Please review.

[tim (not reviewing), 2011-10-13 00:41:30]

Looking.  Have you seen base/memory/mru_cache.h ?

[lipalani, 2011-10-13 00:45:52]

we dont need mru at least for now. we dont care about most recently used
event. (well in future may be we can do some optimization like, if this
particular event has been used several times make a note of that to the
server). but currently we want first in first out. i.e., send the last 'x'
events to the server(and 'x' is about 6 which is large enough for now to
make sure most of the time no event is dicarded as we send this once per
sync cycle.). for which a simple (circular) queue is sufficient.

Thanks!
regards,
Lingesh

On Wed, Oct 12, 2011 at 5:41 PM, <tim@chromium.org> wrote:

> Looking.  Have you seen base/memory/mru_cache.h ?
>
>
http://codereview.chromium.**org/8189003/<http://codereview.chromium.org/8189...
>

[tim (not reviewing), 2011-10-13 16:04:14]

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/engine/d...
File chrome/browser/sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:88:
AppendClientDebugInfoIfNeeded(session, &client_to_server_message);
This method should take a DebugInfo*, not ClientToServerMessage.
It can return a bool "if needed" if it makes life easier.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:138: if
(!session->status_controller()->debug_info_sent()) {
The getter should track this.  If we're building that special object to track
debug_info, every extra bit of control we have to sprinkle elsewhere is
unfortunate.  At the very least, this method (AppendClientDebugInfoIfNeeded)
should exist solely on the getter itself (instead of GetAndClear) and not on
DownloadUpdatesCommand.

DebugInfoGetter::FillClientDebugInfoIfNeeded(Session*, DebugInfo*) {
  if (!session->status_controller()->sent_debug_info())
    ... do whatever GetAndClearDebugInfo does
  set_sent_debug_info()
}

And instead of sent_debug_info (more extraneous sprinkling :/), can't we simply
check has_updates_response()?  If we have to add something (if it happens to be
that we can't infer whether we tried to GU from anything in the controller) we
should call it out for what it is versus what we're using it for in this case
"sent_get_updates()".

Lastly I don't think we need the true/false param, right?  Just
set_sent_get_updates() and sent_get_updates() should suffice (and be preferable
since it's more in line with "never more than once per session"

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/debug_info_event_listener.cc (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:23:
event_info.mutable_sync_cycle_completed_event_info();
indent

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:26:
snapshot->syncer_status.syncer_stuck);
indent

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:28:
snapshot->num_conflicting_updates);
indent

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:30:
snapshot->num_blocking_conflicting_updates);
indent

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:65: DCHECK(false);
NOTREACHED

Although how are we guaranteed this won't happen? Comment?

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:69: DCHECK(false);
NOTREACHED

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/debug_info_event_listener.h (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.h:26: virtual
~DebugInfoEventListener();
newline after

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.h:51:
FRIEND_TEST_ALL_PREFIXES(DebugInfoEventListenerTest, VerifyEventsAdded);
goes in private: section (if you need it)

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/client_debug_info.proto:24: // these events dont
have any extra info associated with them.
Comments should start with a capital.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/client_debug_info.proto:40: message DebugInfo {
We had discussed time deltas for events.  I still think that's useful. 

We're populating |events| using the queue, right?  Do we want a bit tracking
whether we overwrote data?

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/status_controller.h (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/status_controller.h:278: bool debug_info_sent_;
Do not add state like this here.  This belongs in SyncCycleControlParams.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:146: DebugInfoGetter* const
debug_info_getter_;
Comment. e.g. see the one around ExtensionsActivityMonitor.

[tim (not reviewing), 2011-10-13 16:08:22]

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/engine/d...
File chrome/browser/sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:138: if
(!session->status_controller()->debug_info_sent()) {
It occurred to me that maybe you were mimicing what ExtensionsActivityMonitor
did with BuildUpdatesCommand since I guess having the method on DUC is analogous
and more of a simple data object.  I can go either way on this one and leave it
to you.  But the second two paragraphs apply either way :)

On 2011/10/13 16:04:14, timsteele wrote:
> The getter should track this.  If we're building that special object to track
> debug_info, every extra bit of control we have to sprinkle elsewhere is
> unfortunate.  At the very least, this method (AppendClientDebugInfoIfNeeded)
> should exist solely on the getter itself (instead of GetAndClear) and not on
> DownloadUpdatesCommand.
> 
> DebugInfoGetter::FillClientDebugInfoIfNeeded(Session*, DebugInfo*) {
>   if (!session->status_controller()->sent_debug_info())
>     ... do whatever GetAndClearDebugInfo does
>   set_sent_debug_info()
> }
> 
> And instead of sent_debug_info (more extraneous sprinkling :/), can't we
simply
> check has_updates_response()?  If we have to add something (if it happens to
be
> that we can't infer whether we tried to GU from anything in the controller) we
> should call it out for what it is versus what we're using it for in this case
> "sent_get_updates()".
> 
> Lastly I don't think we need the true/false param, right?  Just
> set_sent_get_updates() and sent_get_updates() should suffice (and be
preferable
> since it's more in line with "never more than once per session"

[lipalani1, 2011-10-13 21:39:18]

PTAL

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/engine/d...
File chrome/browser/sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:88:
AppendClientDebugInfoIfNeeded(session, &client_to_server_message);
On 2011/10/13 16:04:14, timsteele wrote:
> This method should take a DebugInfo*, not ClientToServerMessage.
> It can return a bool "if needed" if it makes life easier.

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:138: if
(!session->status_controller()->debug_info_sent()) {
Keeping the getter as the data only object. After doing this in PSS in my last
patch (for failed datatypes handler) I am becoming a fan of this. Particularly
for scalability. If we need to fix some bug in download_updates_command we
shouldnot have to touch debug_info_getter. Otherwise DUC's state would be
checked in too many places.
Removed the true/false.
Regarding sent_get_updates that was deliberate too. Reason being there is talk
of commits with out getupdates etc. The code as written could be called from any
where(download or commit) and would work. The other methods cant guarantee it. 
On 2011/10/13 16:04:14, timsteele wrote:
> The getter should track this.  If we're building that special object to track
> debug_info, every extra bit of control we have to sprinkle elsewhere is
> unfortunate.  At the very least, this method (AppendClientDebugInfoIfNeeded)
> should exist solely on the getter itself (instead of GetAndClear) and not on
> DownloadUpdatesCommand.
> 
> DebugInfoGetter::FillClientDebugInfoIfNeeded(Session*, DebugInfo*) {
>   if (!session->status_controller()->sent_debug_info())
>     ... do whatever GetAndClearDebugInfo does
>   set_sent_debug_info()
> }
> 
> And instead of sent_debug_info (more extraneous sprinkling :/), can't we
simply
> check has_updates_response()?  If we have to add something (if it happens to
be
> that we can't infer whether we tried to GU from anything in the controller) we
> should call it out for what it is versus what we're using it for in this case
> "sent_get_updates()".
> 
> Lastly I don't think we need the true/false param, right?  Just
> set_sent_get_updates() and sent_get_updates() should suffice (and be
preferable
> since it's more in line with "never more than once per session"

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:138: if
(!session->status_controller()->debug_info_sent()) {
On 2011/10/13 16:08:22, timsteele wrote:
> It occurred to me that maybe you were mimicing what ExtensionsActivityMonitor
> did with BuildUpdatesCommand since I guess having the method on DUC is
analogous
> and more of a simple data object.  I can go either way on this one and leave
it
> to you.  But the second two paragraphs apply either way :)
> 
> On 2011/10/13 16:04:14, timsteele wrote:
> > The getter should track this.  If we're building that special object to
track
> > debug_info, every extra bit of control we have to sprinkle elsewhere is
> > unfortunate.  At the very least, this method (AppendClientDebugInfoIfNeeded)
> > should exist solely on the getter itself (instead of GetAndClear) and not on
> > DownloadUpdatesCommand.
> > 
> > DebugInfoGetter::FillClientDebugInfoIfNeeded(Session*, DebugInfo*) {
> >   if (!session->status_controller()->sent_debug_info())
> >     ... do whatever GetAndClearDebugInfo does
> >   set_sent_debug_info()
> > }
> > 
> > And instead of sent_debug_info (more extraneous sprinkling :/), can't we
> simply
> > check has_updates_response()?  If we have to add something (if it happens to
> be
> > that we can't infer whether we tried to GU from anything in the controller)
we
> > should call it out for what it is versus what we're using it for in this
case
> > "sent_get_updates()".
> > 
> > Lastly I don't think we need the true/false param, right?  Just
> > set_sent_get_updates() and sent_get_updates() should suffice (and be
> preferable
> > since it's more in line with "never more than once per session"
> 

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/debug_info_event_listener.cc (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:23:
event_info.mutable_sync_cycle_completed_event_info();
On 2011/10/13 16:04:14, timsteele wrote:
> indent

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:26:
snapshot->syncer_status.syncer_stuck);
On 2011/10/13 16:04:14, timsteele wrote:
> indent

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:28:
snapshot->num_conflicting_updates);
On 2011/10/13 16:04:14, timsteele wrote:
> indent

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:30:
snapshot->num_blocking_conflicting_updates);
On 2011/10/13 16:04:14, timsteele wrote:
> indent

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:65: DCHECK(false);
On 2011/10/13 16:04:14, timsteele wrote:
> NOTREACHED
> 
> Although how are we guaranteed this won't happen? Comment?

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.cc:69: DCHECK(false);
On 2011/10/13 16:04:14, timsteele wrote:
> NOTREACHED

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/debug_info_event_listener.h (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.h:26: virtual
~DebugInfoEventListener();
On 2011/10/13 16:04:14, timsteele wrote:
> newline after

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/debug_info_event_listener.h:51:
FRIEND_TEST_ALL_PREFIXES(DebugInfoEventListenerTest, VerifyEventsAdded);
On 2011/10/13 16:04:14, timsteele wrote:
> goes in private: section (if you need it)

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/client_debug_info.proto:24: // these events dont
have any extra info associated with them.
On 2011/10/13 16:04:14, timsteele wrote:
> Comments should start with a capital.

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/client_debug_info.proto:40: message DebugInfo {
TimeDeltas need some thinking through to send in a bandwidth optimized way. And
I would be happy to consider it as the next evolution of this feature. Added a
bit to track if events were dropped. Good idea!
On 2011/10/13 16:04:14, timsteele wrote:
> We had discussed time deltas for events.  I still think that's useful. 
> 
> We're populating |events| using the queue, right?  Do we want a bit tracking
> whether we overwrote data?

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/status_controller.h (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/status_controller.h:278: bool debug_info_sent_;
On 2011/10/13 16:04:14, timsteele wrote:
> Do not add state like this here.  This belongs in SyncCycleControlParams.

Done.

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/8189003/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:146: DebugInfoGetter* const
debug_info_getter_;
On 2011/10/13 16:04:14, timsteele wrote:
> Comment. e.g. see the one around ExtensionsActivityMonitor.

Done.

[tim (not reviewing), 2011-10-13 21:48:14]

Will take quick look to wrap up when I get off the bus!

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/debug_info_getter.h (right):

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/session...
chrome/browser/sync/sessions/debug_info_getter.h:16: public:
declare a virtual destructor 
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Interf...)

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/status_controller.h (right):

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/session...
chrome/browser/sync/sessions/status_controller.h:276: // True indicates we have
sent the debug info already once this session.
remove from here and add this comment to session_state.h

[lipalani1, 2011-10-13 22:08:06]

Cool will do so. agree on both.

Since the power is out on seattle office I cannot upload the patch with those
changes. So please continue reviewing if you are looking for more things!! that
way i can address and check in once the power is back.

[tim (not reviewing), 2011-10-14 01:23:20]

WIth comments from previous review and this one addressed, and a healthy try
run, this LGTM

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/debug_info_event_listener.h (right):

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/debug_info_event_listener.h:23: public
browser_sync::sessions::DebugInfoGetter {
can you line this up with public sync_api:: above?

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/debug_info_event_listener.h:61: bool
events_dropped_;
comment

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

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:54: #include
"chrome/browser/sync/internal_api/debug_info_event_listener.cc"
yikes... pretty sure you meant to remove this :)

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:143:
debug_info_event_listener_() {
this initializer isn't needed, right?

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/client_debug_info.proto:18: optional bool
syncer_stuck = 1;
perhaps add a comment at the message level here saying that these relate to
SyncerStatus / etc.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/client_debug_info.proto:24: // These events dont
have any extra info associated with them.
What's extra info?  Might be more helpful to write what these are rather than
try and explain what they aren't

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/client_debug_info.proto:26: AUTH_ERROR = 1; // Auth
error
add period.  this comment isn't that useful either...  could just remove it or
improve it (i.e does this error cause the syncer to wait until the user does
something?)

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/client_debug_info.proto:31: // This event should
never be seen by the server in the absence of bugs.
Which event does this pertain to?

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/client_debug_info.proto:43: optional bool
cryptographer_has_pending_keys = 3;
comments on these

[lipalani1, 2011-10-14 18:31:10]

Fixed all feedback. running try jobs now before commit.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/debug_info_event_listener.h (right):

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/debug_info_event_listener.h:23: public
browser_sync::sessions::DebugInfoGetter {
On 2011/10/14 01:23:21, timsteele wrote:
> can you line this up with public sync_api:: above?

Done.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/debug_info_event_listener.h:61: bool
events_dropped_;
On 2011/10/14 01:23:21, timsteele wrote:
> comment

Done.

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

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:54: #include
"chrome/browser/sync/internal_api/debug_info_event_listener.cc"
On 2011/10/14 01:23:21, timsteele wrote:
> yikes... pretty sure you meant to remove this :)

Done.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:143:
debug_info_event_listener_() {
On 2011/10/14 01:23:21, timsteele wrote:
> this initializer isn't needed, right?

Done.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/client_debug_info.proto:18: optional bool
syncer_stuck = 1;
On 2011/10/14 01:23:21, timsteele wrote:
> perhaps add a comment at the message level here saying that these relate to
> SyncerStatus / etc.

Done.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/client_debug_info.proto:24: // These events dont
have any extra info associated with them.
On 2011/10/14 01:23:21, timsteele wrote:
> What's extra info?  Might be more helpful to write what these are rather than
> try and explain what they aren't

Done.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/client_debug_info.proto:26: AUTH_ERROR = 1; // Auth
error
On 2011/10/14 01:23:21, timsteele wrote:
> add period.  this comment isn't that useful either...  could just remove it or
> improve it (i.e does this error cause the syncer to wait until the user does
> something?)

Done.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/client_debug_info.proto:31: // This event should
never be seen by the server in the absence of bugs.
On 2011/10/14 01:23:21, timsteele wrote:
> Which event does this pertain to?

Done.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/client_debug_info.proto:43: optional bool
cryptographer_has_pending_keys = 3;
On 2011/10/14 01:23:21, timsteele wrote:
> comments on these 

Done.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/debug_info_getter.h (right):

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/session...
chrome/browser/sync/sessions/debug_info_getter.h:16: public:
On 2011/10/13 21:48:14, timsteele wrote:
> declare a virtual destructor 
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Interf...)

Done.

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/status_controller.h (right):

http://codereview.chromium.org/8189003/diff/14001/chrome/browser/sync/session...
chrome/browser/sync/sessions/status_controller.h:276: // True indicates we have
sent the debug info already once this session.
On 2011/10/13 21:48:14, timsteele wrote:
> remove from here and add this comment to session_state.h

Done.

[commit-bot: I haz the power, 2011-10-15 04:37:01]

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

[commit-bot: I haz the power, 2011-10-15 07:53:41]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 105651, job name
was 8189003-30001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-10-15 08:27:51]

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

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

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 105659, job name
was 8189003-30001 (retry) (retry) (retry) (retry).

[cmp, 2011-10-15 22:25:26]

lgtm for sizes/chrome-si expectations change