Send important client side event information to the server.

chrome/browser/sync/engine/download_updates_command.cc

Index: chrome/browser/sync/engine/download_updates_command.cc
diff --git a/chrome/browser/sync/engine/download_updates_command.cc b/chrome/browser/sync/engine/download_updates_command.cc
index 7677989210cb691c5fe38a06dea474419a67a0e1..f42df83e00c57b9ea3769b7368a9a0cc067524e5 100644
--- a/chrome/browser/sync/engine/download_updates_command.cc
+++ b/chrome/browser/sync/engine/download_updates_command.cc
@@ -16,6 +16,8 @@
 
 using syncable::ScopedDirLookup;
 
+using sync_pb::DebugInfo;
+
 namespace browser_sync {
 using sessions::StatusController;
 using sessions::SyncSession;
@@ -83,6 +85,8 @@ void DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
 
   SyncerProtoUtil::AddRequestBirthday(dir, &client_to_server_message);
 
+  AppendClientDebugInfoIfNeeded(session, &client_to_server_message);

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

This method should take a DebugInfo*, not ClientToServerMessage.
It can return a bool "if needed" if it makes life easier.

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

Done.

+
   bool ok = SyncerProtoUtil::PostClientToServerMessage(
       client_to_server_message,
       &update_response,
@@ -126,4 +130,22 @@ void DownloadUpdatesCommand::SetRequestedTypes(
   DCHECK_LT(0, requested_type_count) << "Doing GetUpdates with empty filter.";
 }
 
+void DownloadUpdatesCommand::AppendClientDebugInfoIfNeeded(
+    sessions::SyncSession* session,
+    sync_pb::ClientToServerMessage* client_to_server_message) {
+  // We want to send the debug info only once per sync cycle. Check if it has
+  // already been sent.
+  if (!session->status_controller()->debug_info_sent()) {

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

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"

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

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:

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

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:

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

Done.

+    VLOG(1) << "Sending client debug info ...";
+    DebugInfo* debug_info = client_to_server_message->mutable_debug_info();
+    // could be null in some unit tests.
+    if (session->context()->debug_info_getter()) {
+      session->context()->debug_info_getter()->GetAndClearDebugInfo(
+          debug_info);
+    }
+    session->status_controller()->set_debug_info_sent(true);
+  }
+}
+
+
 }  // namespace browser_sync