sync: union model type nudge source before dropping coalesced nudge.

Also, clear nudge_types in UpdateNudgeSource along with the other pending_nudge state, so that we don't accidentally bleed data pertaining to a nudge that comes in while a sync is in progress into the histogram record for that sync.
BUG=none
TEST=SyncerThreadWithSyncerTest

Review URL: http://codereview.chromium.org/5145006

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

3 files changed, 68 insertions, 16 deletions

diff --git a/chrome/browser/sync/engine/syncer_thread.cc b/chrome/browser/sync/engine/syncer_thread.cc
index f5c2f80..88cee21 100644
--- a/chrome/browser/sync/engine/syncer_thread.cc
+++ b/chrome/browser/sync/engine/syncer_thread.cc
@@ -332,6 +332,7 @@ void SyncerThread::ThreadMainLoop() {
 
     // Handle a nudge, caused by either a notification or a local bookmark
     // event.  This will also update the source of the following SyncMain call.
+    syncable::ModelTypeBitSet last_nudge_types(vault_.pending_nudge_types_);
     bool nudged = UpdateNudgeSource(throttled, continue_sync_cycle,
                                     &initial_sync_for_thread);
 
@@ -342,18 +343,14 @@ void SyncerThread::ThreadMainLoop() {
     // nudges.
     base::TimeTicks now = TimeTicks::Now();
     for (size_t i = syncable::FIRST_REAL_MODEL_TYPE;
-         i < vault_.pending_nudge_types_.size();
+         i < last_nudge_types.size();
          ++i) {
-      if (vault_.pending_nudge_types_[i]) {
+      if (last_nudge_types[i]) {
         syncable::PostTimeToTypeHistogram(syncable::ModelType(i),
                                           now - last_sync_time);
       }
     }
 
-    // Now that the nudge has been handled, we can reset our tracking of the
-    // datatypes triggering a nudge.
-    vault_.pending_nudge_types_.reset();
-
     last_sync_time = now;
 
     VLOG(1) << "Updating the next polling time after SyncMain";
@@ -540,27 +537,30 @@ bool SyncerThread::UpdateNudgeSource(bool was_throttled,
                                      bool* initial_sync) {
   bool nudged = false;
   NudgeSource nudge_source = kUnknown;
+  syncable::ModelTypeBitSet model_types;
   // Has the previous sync cycle completed?
   if (continue_sync_cycle)
     nudge_source = kContinuation;
   // Update the nudge source if a new nudge has come through during the
   // previous sync cycle.
   if (!vault_.pending_nudge_time_.is_null()) {
-    if (!was_throttled && !nudged) {
+    if (!was_throttled) {
       nudge_source = vault_.pending_nudge_source_;
+      model_types = vault_.pending_nudge_types_;
       nudged = true;
     }
     VLOG(1) << "Clearing pending nudge from " << vault_.pending_nudge_source_
             << " at tick " << vault_.pending_nudge_time_.ToInternalValue();
     vault_.pending_nudge_source_ = kUnknown;
+    vault_.pending_nudge_types_.reset();
     vault_.pending_nudge_time_ = base::TimeTicks();
   }
-  SetUpdatesSource(nudged, nudge_source, initial_sync);
+  SetUpdatesSource(nudged, nudge_source, model_types, initial_sync);
   return nudged;
 }
 
 void SyncerThread::SetUpdatesSource(bool nudged, NudgeSource nudge_source,
-                                    bool* initial_sync) {
+    const syncable::ModelTypeBitSet& nudge_types, bool* initial_sync) {
   sync_pb::GetUpdatesCallerInfo::GetUpdatesSource updates_source =
       sync_pb::GetUpdatesCallerInfo::UNKNOWN;
   if (*initial_sync) {
@@ -588,8 +588,7 @@ void SyncerThread::SetUpdatesSource(bool nudged, NudgeSource nudge_source,
         break;
     }
   }
-  vault_.syncer_->set_updates_source(
-      updates_source, vault_.pending_nudge_types_);
+  vault_.syncer_->set_updates_source(updates_source, nudge_types);
 }
 
 void SyncerThread::CreateSyncer(const std::string& dirname) {
@@ -704,6 +703,12 @@ void SyncerThread::NudgeSyncImpl(int milliseconds_from_now,
     return;
   }
 
+  // Union the current bitset with any from nudges that may have already
+  // posted (coalesce the nudge datatype information).
+  // TODO(tim): It seems weird to do this if the sources don't match up (e.g.
+  // if pending_source is kLocal and |source| is kClearPrivateData).
+  vault_.pending_nudge_types_ |= model_types;
+
   const TimeTicks nudge_time = TimeTicks::Now() +
       TimeDelta::FromMilliseconds(milliseconds_from_now);
   if (nudge_time <= vault_.pending_nudge_time_) {
@@ -712,12 +717,9 @@ void SyncerThread::NudgeSyncImpl(int milliseconds_from_now,
     return;
   }
 
-  // Union the current bitset with any from nudges that may have already
-  // posted (coalesce the nudge datatype information).
-  vault_.pending_nudge_types_ |= model_types;
-
   VLOG(1) << "Replacing pending nudge for source " << source
           << " at " << nudge_time.ToInternalValue();
+
   vault_.pending_nudge_source_ = source;
   vault_.pending_nudge_time_ = nudge_time;
   vault_field_changed_.Broadcast();
diff --git a/chrome/browser/sync/engine/syncer_thread.h b/chrome/browser/sync/engine/syncer_thread.h
index a7322be..79f4a6c 100644
--- a/chrome/browser/sync/engine/syncer_thread.h
+++ b/chrome/browser/sync/engine/syncer_thread.h
@@ -46,6 +46,8 @@ class SyncerThread : public base::RefCountedThreadSafe<SyncerThread>,
   FRIEND_TEST_ALL_PREFIXES(SyncerThreadWithSyncerTest, Polling);
   FRIEND_TEST_ALL_PREFIXES(SyncerThreadWithSyncerTest, Nudge);
   FRIEND_TEST_ALL_PREFIXES(SyncerThreadWithSyncerTest, NudgeWithDataTypes);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThreadWithSyncerTest,
+                           NudgeWithDataTypesCoalesced);
   FRIEND_TEST_ALL_PREFIXES(SyncerThreadWithSyncerTest, Throttling);
   FRIEND_TEST_ALL_PREFIXES(SyncerThreadWithSyncerTest, AuthInvalid);
   FRIEND_TEST_ALL_PREFIXES(SyncerThreadWithSyncerTest, Pause);
@@ -271,7 +273,9 @@ class SyncerThread : public base::RefCountedThreadSafe<SyncerThread>,
   // Returns true if it determines a nudge actually occurred.
   bool UpdateNudgeSource(bool was_throttled, bool continue_sync_cycle,
                          bool* initial_sync);
-  void SetUpdatesSource(bool nudged, NudgeSource nudge_source,
+  void SetUpdatesSource(bool nudged,
+                        NudgeSource nudge_source,
+                        const syncable::ModelTypeBitSet& nudge_types,
                         bool* initial_sync);
 
   int UserIdleTime();
diff --git a/chrome/browser/sync/engine/syncer_thread_unittest.cc b/chrome/browser/sync/engine/syncer_thread_unittest.cc
index b2238eb..521b292 100644
--- a/chrome/browser/sync/engine/syncer_thread_unittest.cc
+++ b/chrome/browser/sync/engine/syncer_thread_unittest.cc
@@ -732,6 +732,52 @@ TEST_F(SyncerThreadWithSyncerTest, NudgeWithDataTypes) {
   EXPECT_TRUE(syncer_thread()->vault_.pending_nudge_types_.none());
 }
 
+TEST_F(SyncerThreadWithSyncerTest, NudgeWithDataTypesCoalesced) {
+  SyncShareIntercept interceptor;
+  connection()->SetMidCommitObserver(&interceptor);
+  // We don't want a poll to happen during this test (except the first one).
+  PreventThreadFromPolling();
+  EXPECT_TRUE(syncer_thread()->Start());
+  metadb()->Open();
+  syncer_thread()->CreateSyncer(metadb()->name());
+  const TimeDelta poll_interval = TimeDelta::FromMinutes(5);
+  interceptor.WaitForSyncShare(1, poll_interval + poll_interval);
+  EXPECT_EQ(static_cast<unsigned int>(1),
+    interceptor.times_sync_occured().size());
+
+  // The SyncerThread should be waiting for the poll now.  Nudge it to sync
+  // immediately (5ms).
+  syncable::ModelTypeBitSet model_types;
+  model_types[syncable::BOOKMARKS] = true;
+
+  // Paused so we can verify the nudge types safely.
+  syncer_thread()->RequestPause();
+  syncer_thread()->NudgeSyncerWithDataTypes(100,
+      SyncerThread::kUnknown,
+      model_types);
+  EXPECT_EQ(model_types, syncer_thread()->vault_.pending_nudge_types_);
+
+  model_types[syncable::BOOKMARKS] = false;
+  model_types[syncable::AUTOFILL] = true;
+  syncer_thread()->NudgeSyncerWithDataTypes(0,
+      SyncerThread::kUnknown,
+      model_types);
+
+  // Reset BOOKMARKS for expectations.
+  model_types[syncable::BOOKMARKS] = true;
+  EXPECT_EQ(model_types, syncer_thread()->vault_.pending_nudge_types_);
+
+  syncer_thread()->RequestResume();
+
+  interceptor.WaitForSyncShare(1, TimeDelta::FromSeconds(1));
+  EXPECT_EQ(static_cast<unsigned int>(2),
+      interceptor.times_sync_occured().size());
+
+  // SyncerThread should be waiting again.  Signal it to stop.
+  EXPECT_TRUE(syncer_thread()->Stop(2000));
+  EXPECT_TRUE(syncer_thread()->vault_.pending_nudge_types_.none());
+}
+
 TEST_F(SyncerThreadWithSyncerTest, Throttling) {
   SyncShareIntercept interceptor;
   connection()->SetMidCommitObserver(&interceptor);