Send debug event info on Commit requests, not just GetUpdates requests.

BUG=318439

Review URL: https://codereview.chromium.org/82303002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@236914 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 109 insertions, 13 deletions

diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc
index c7af449..a8db1a4 100644
--- a/sync/engine/commit.cc
+++ b/sync/engine/commit.cc
@@ -110,6 +110,11 @@ SyncerError Commit::PostAndProcessResponse(
   }
   session->mutable_status_controller()->set_commit_request_types(request_types);
 
+  if (session->context()->debug_info_getter()) {
+    sync_pb::DebugInfo* debug_info = message_.mutable_debug_info();
+    session->context()->debug_info_getter()->GetDebugInfo(debug_info);
+  }
+
   DVLOG(1) << "Sending commit message.";
   TRACE_EVENT_BEGIN0("sync", "PostCommit");
   const SyncerError post_result = SyncerProtoUtil::PostClientToServerMessage(
@@ -136,6 +141,12 @@ SyncerError Commit::PostAndProcessResponse(
     return SERVER_RESPONSE_VALIDATION_FAILED;
   }
 
+  if (session->context()->debug_info_getter()) {
+    // Clear debug info now that we have successfully sent it to the server.
+    DVLOG(1) << "Clearing client debug info.";
+    session->context()->debug_info_getter()->ClearDebugInfo();
+  }
+
   // Let the contributors process the responses to each of their requests.
   SyncerError processing_result = SYNCER_OK;
   for (std::map<ModelType, SyncDirectoryCommitContribution*>::iterator it =
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index e4755b6..684b404 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -461,6 +461,18 @@ class SyncerTest : public testing::Test,
     return directory()->GetCryptographer(trans);
   }
 
+  // Configures SyncSessionContext and NudgeTracker so Syncer won't call
+  // GetUpdates prior to Commit. This method can be used to ensure a Commit is
+  // not preceeded by GetUpdates.
+  void ConfigureNoGetUpdatesRequired() {
+    context_->set_server_enabled_pre_commit_update_avoidance(true);
+    nudge_tracker_.OnInvalidationsEnabled();
+    nudge_tracker_.RecordSuccessfulSyncCycle();
+
+    ASSERT_FALSE(context_->ShouldFetchUpdatesBeforeCommit());
+    ASSERT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
+  }
+
   base::MessageLoop message_loop_;
 
   // Some ids to aid tests. Only the root one's value is specific. The rest
@@ -2520,55 +2532,128 @@ TEST_F(SyncerTest, CommitManyItemsInOneGo_CommitConflict) {
 }
 
 // Tests that sending debug info events works.
-TEST_F(SyncerTest, SendDebugInfoEvents_HappyCase) {
+TEST_F(SyncerTest, SendDebugInfoEventsOnGetUpdates_HappyCase) {
   debug_info_getter_->AddDebugEvent();
   debug_info_getter_->AddDebugEvent();
 
   SyncShareNudge();
 
-  // Verify we received one request with two debug info events.
+  // Verify we received one GetUpdates request with two debug info events.
   EXPECT_EQ(1U, mock_server_->requests().size());
-  EXPECT_EQ(2, mock_server_->requests().back().debug_info().events_size());
+  ASSERT_TRUE(mock_server_->last_request().has_get_updates());
+  EXPECT_EQ(2, mock_server_->last_request().debug_info().events_size());
 
   SyncShareNudge();
 
-  // See that we received another request, but that it contains no debug info
-  // events.
+  // See that we received another GetUpdates request, but that it contains no
+  // debug info events.
   EXPECT_EQ(2U, mock_server_->requests().size());
-  EXPECT_EQ(0, mock_server_->requests().back().debug_info().events_size());
+  ASSERT_TRUE(mock_server_->last_request().has_get_updates());
+  EXPECT_EQ(0, mock_server_->last_request().debug_info().events_size());
 
   debug_info_getter_->AddDebugEvent();
 
   SyncShareNudge();
 
-  // See that we received another request and it contains one debug info event.
+  // See that we received another GetUpdates request and it contains one debug
+  // info event.
   EXPECT_EQ(3U, mock_server_->requests().size());
-  EXPECT_EQ(1, mock_server_->requests().back().debug_info().events_size());
+  ASSERT_TRUE(mock_server_->last_request().has_get_updates());
+  EXPECT_EQ(1, mock_server_->last_request().debug_info().events_size());
 }
 
 // Tests that debug info events are dropped on server error.
-TEST_F(SyncerTest, SendDebugInfoEvents_PostFailsDontDrop) {
+TEST_F(SyncerTest, SendDebugInfoEventsOnGetUpdates_PostFailsDontDrop) {
   debug_info_getter_->AddDebugEvent();
   debug_info_getter_->AddDebugEvent();
 
   mock_server_->FailNextPostBufferToPathCall();
   SyncShareNudge();
 
-  // Verify we attempted to send one request with two debug info events.
+  // Verify we attempted to send one GetUpdates request with two debug info
+  // events.
   EXPECT_EQ(1U, mock_server_->requests().size());
-  EXPECT_EQ(2, mock_server_->requests().back().debug_info().events_size());
+  ASSERT_TRUE(mock_server_->last_request().has_get_updates());
+  EXPECT_EQ(2, mock_server_->last_request().debug_info().events_size());
 
   SyncShareNudge();
 
   // See that the client resent the two debug info events.
   EXPECT_EQ(2U, mock_server_->requests().size());
-  EXPECT_EQ(2, mock_server_->requests().back().debug_info().events_size());
+  ASSERT_TRUE(mock_server_->last_request().has_get_updates());
+  EXPECT_EQ(2, mock_server_->last_request().debug_info().events_size());
 
   // The previous send was successful so this next one shouldn't generate any
   // debug info events.
   SyncShareNudge();
   EXPECT_EQ(3U, mock_server_->requests().size());
-  EXPECT_EQ(0, mock_server_->requests().back().debug_info().events_size());
+  ASSERT_TRUE(mock_server_->last_request().has_get_updates());
+  EXPECT_EQ(0, mock_server_->last_request().debug_info().events_size());
+}
+
+// Tests that sending debug info events on Commit works.
+TEST_F(SyncerTest, SendDebugInfoEventsOnCommit_HappyCase) {
+  // Make sure GetUpdate isn't call as it would "steal" debug info events before
+  // Commit has a chance to send them.
+  ConfigureNoGetUpdatesRequired();
+
+  // Generate a debug info event and trigger a commit.
+  debug_info_getter_->AddDebugEvent();
+  CreateUnsyncedDirectory("X", "id_X");
+  SyncShareNudge();
+
+  // Verify that the last request received is a Commit and that it contains a
+  // debug info event.
+  EXPECT_EQ(1U, mock_server_->requests().size());
+  ASSERT_TRUE(mock_server_->last_request().has_commit());
+  EXPECT_EQ(1, mock_server_->last_request().debug_info().events_size());
+
+  // Generate another commit, but no debug info event.
+  CreateUnsyncedDirectory("Y", "id_Y");
+  SyncShareNudge();
+
+  // See that it was received and contains no debug info events.
+  EXPECT_EQ(2U, mock_server_->requests().size());
+  ASSERT_TRUE(mock_server_->last_request().has_commit());
+  EXPECT_EQ(0, mock_server_->last_request().debug_info().events_size());
+}
+
+// Tests that debug info events are not dropped on server error.
+TEST_F(SyncerTest, SendDebugInfoEventsOnCommit_PostFailsDontDrop) {
+  // Make sure GetUpdate isn't call as it would "steal" debug info events before
+  // Commit has a chance to send them.
+  ConfigureNoGetUpdatesRequired();
+
+  mock_server_->FailNextPostBufferToPathCall();
+
+  // Generate a debug info event and trigger a commit.
+  debug_info_getter_->AddDebugEvent();
+  CreateUnsyncedDirectory("X", "id_X");
+  SyncShareNudge();
+
+  // Verify that the last request sent is a Commit and that it contains a debug
+  // info event.
+  EXPECT_EQ(1U, mock_server_->requests().size());
+  ASSERT_TRUE(mock_server_->last_request().has_commit());
+  EXPECT_EQ(1, mock_server_->last_request().debug_info().events_size());
+
+  // Try again.
+  SyncShareNudge();
+
+  // Verify that we've received another Commit and that it contains a debug info
+  // event (just like the previous one).
+  EXPECT_EQ(2U, mock_server_->requests().size());
+  ASSERT_TRUE(mock_server_->last_request().has_commit());
+  EXPECT_EQ(1, mock_server_->last_request().debug_info().events_size());
+
+  // Generate another commit and try again.
+  CreateUnsyncedDirectory("Y", "id_Y");
+  SyncShareNudge();
+
+  // See that it was received and contains no debug info events.
+  EXPECT_EQ(3U, mock_server_->requests().size());
+  ASSERT_TRUE(mock_server_->last_request().has_commit());
+  EXPECT_EQ(0, mock_server_->last_request().debug_info().events_size());
 }
 
 TEST_F(SyncerTest, HugeConflict) {