Send important client side event information to the server.

chrome/browser/sync/engine/download_updates_command.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/engine/download_updates_command.h"
 
 #include <string>
 
 #include "base/command_line.h"
 #include "chrome/browser/sync/engine/syncer.h"
 #include "chrome/browser/sync/engine/syncer_proto_util.h"
 #include "chrome/browser/sync/engine/syncproto.h"
 #include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/syncable/model_type_payload_map.h"
 #include "chrome/common/chrome_switches.h"
 
 using syncable::ScopedDirLookup;
 
+using sync_pb::DebugInfo;
+
 namespace browser_sync {
 using sessions::StatusController;
 using sessions::SyncSession;
 using std::string;
 using syncable::FIRST_REAL_MODEL_TYPE;
 using syncable::MODEL_TYPE_COUNT;
 
 DownloadUpdatesCommand::DownloadUpdatesCommand() {}
 DownloadUpdatesCommand::~DownloadUpdatesCommand() {}
 
@@ skipping 47 matching lines @@
   get_updates->set_fetch_folders(true);
 
   // Set GetUpdatesMessage.GetUpdatesCallerInfo information.
   get_updates->mutable_caller_info()->set_source(
       session->TestAndSetSource().updates_source);
   get_updates->mutable_caller_info()->set_notifications_enabled(
       session->context()->notifications_enabled());
 
   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,
       session);
 
   VLOG(2) << SyncerProtoUtil::ClientToServerResponseDebugString(
       update_response);
 
   StatusController* status = session->status_controller();
   status->set_updates_request_types(enabled_types);
@@ skipping 23 matching lines @@
   for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
     if (target_datatypes[i]) {
       requested_type_count++;
       syncable::AddDefaultExtensionValue(syncable::ModelTypeFromInt(i),
                                          filter_protobuf);
     }
   }
   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