sync: Report GetUpdate triggers to the server

sync/sessions/nudge_tracker.cc

Index: sync/sessions/nudge_tracker.cc
diff --git a/sync/sessions/nudge_tracker.cc b/sync/sessions/nudge_tracker.cc
index 714b67d13f14582e427fdf544c40bc44a067dab8..b5ab158b06d332c4822a6547c208403eb83d75b7 100644
--- a/sync/sessions/nudge_tracker.cc
+++ b/sync/sessions/nudge_tracker.cc
@@ -3,47 +3,189 @@
 // found in the LICENSE file.
 
 #include "sync/sessions/nudge_tracker.h"
+
+#include "sync/internal_api/public/base/invalidation.h"
+#include "sync/internal_api/public/sessions/sync_source_info.h"
 #include "sync/protocol/sync.pb.h"
 
 namespace syncer {
 namespace sessions {
 
-NudgeTracker::NudgeTracker() { }
+size_t NudgeTracker::kMaxPayloadsPerType = 10;

[tim (not reviewing), 2013/05/06 21:29:04]

Might this be worth adding to client command as something we can override via
cdd?

[rlarocque, 2013/05/07 18:45:39]

I don't see much benefit in fiddling with this knob.  

If the value is too small, then we risk unnecessarily asking the server to
perform a "full" sync.  If the value is too large, we risk almost nothing.  It
will cost us a few bytes of in-memory storage and network transmit.  I think we
should just set this to a large value and be done with it.  

That's the theory, anyway.  I'd like to start with this and see how it turns
out.  I'd be willing to add a client command if there is a need for it.  

[tim (not reviewing), 2013/05/07 20:34:13]

I agree with your logic, but the reason I ask is because we rarely ever know we
need to tweak it it until we need to tweak it, at which point it's useless to
add because of the sea of clients that won't understand the request anyway. 
Having a server switch might let us learn the impact of changing the value more
easily via experiment, would be more flexible to changes in the Tango trickles
design, and may come in handy firefighting a prod inicident. On the flip side,
there isn't a big downside, and it's easy to add since there's already a path to
plumb ClientCommand values through. So I encourage you to consider it once more
:p

[rlarocque, 2013/05/07 21:50:57]

I still don't think it's necessary, but I added it.  

+
+NudgeTracker::NudgeTracker()
+    : updates_source_(sync_pb::GetUpdatesCallerInfo::UNKNOWN),
+      invalidations_enabled_(false),
+      invalidations_out_of_sync_(true) { }
 
 NudgeTracker::~NudgeTracker() { }
 
-void NudgeTracker::CoalesceSources(const SyncSourceInfo& source) {
-  CoalesceStates(source.types, &source_info_.types);
-  source_info_.updates_source = source.updates_source;
+bool NudgeTracker::SyncRequired() {
+  if (!local_nudge_counts_.empty()) {
+    return true;
+  } else if (!refresh_requested_counts_.empty()) {
+    return true;
+  } else if (!payload_list_map_.empty()) {
+    return true;
+  } else {
+    return false;
+  }
 }
 
-bool NudgeTracker::IsEmpty() {
-  return source_info_.types.empty();
+void NudgeTracker::RecordSuccessfulSyncCycle() {
+  updates_source_ = sync_pb::GetUpdatesCallerInfo::UNKNOWN;
+  local_nudge_counts_.clear();
+  refresh_requested_counts_.clear();
+  payload_list_map_.clear();
+  locally_dropped_payload_types_.Clear();
+  server_dropped_payload_types_.Clear();
+
+  // A successful cycle while invalidations are enabled puts us back into sync.
+  invalidations_out_of_sync_ = !invalidations_enabled_;
 }
 
-void NudgeTracker::Reset() {
-  source_info_ = SyncSourceInfo();
+void NudgeTracker::RecordLocalChange(ModelTypeSet types) {
+  // Don't overwrite an NOTIFICATION or DATATYPE_REFRESH source.  The server
+  // makes some assumptions about the source; overriding these sources with
+  // LOCAL could lead to incorrect behaviour.  This is part of the reason why
+  // we're deprecating 'source' in favor of 'origin'.
+  if (updates_source_ != sync_pb::GetUpdatesCallerInfo::NOTIFICATION
+      && updates_source_ != sync_pb::GetUpdatesCallerInfo::DATATYPE_REFRESH) {
+    updates_source_ = sync_pb::GetUpdatesCallerInfo::LOCAL;
+  }
+
+  for (ModelTypeSet::Iterator it = types.First(); it.Good(); it.Inc()) {
+    local_nudge_counts_[it.Get()]++;
+  }
+}
+
+void NudgeTracker::RecordLocalRefreshRequest(ModelTypeSet types) {
+  // Don't overwrite an NOTIFICATION source.  The server makes some assumptions
+  // about the source.  Overriding this source with LOCAL could lead to
+  // incorrect behaviour.  This is part of the reason why we're deprecating
+  // 'source' in favor of 'origin'.
+  if (updates_source_ != sync_pb::GetUpdatesCallerInfo::NOTIFICATION) {
+    updates_source_ = sync_pb::GetUpdatesCallerInfo::DATATYPE_REFRESH;
+  }
+
+  for (ModelTypeSet::Iterator it = types.First(); it.Good(); it.Inc()) {
+    refresh_requested_counts_[it.Get()]++;
+  }
+}
+
+void NudgeTracker::RecordRemoteInvalidation(
+    const ModelTypeInvalidationMap& invalidation_map) {
+  updates_source_ = sync_pb::GetUpdatesCallerInfo::NOTIFICATION;
+
+  for (ModelTypeInvalidationMap::const_iterator i = invalidation_map.begin();
+       i != invalidation_map.end(); ++i) {
+    const ModelType type = i->first;
+    const std::string& payload = i->second.payload;
+
+    payload_list_map_[type].push_back(payload);
+
+    // Respect the size limits on locally queued invalidation hints.
+    if (payload_list_map_[type].size() > kMaxPayloadsPerType) {
+      payload_list_map_[type].pop_front();
+      locally_dropped_payload_types_.Put(type);
+    }
+  }
+}
+
+void NudgeTracker::InvalidationsEnabled() {
+  invalidations_enabled_ = true;
+}
+
+void NudgeTracker::InvalidationsDisabled() {
+  invalidations_enabled_ = false;
+  invalidations_out_of_sync_ = true;
+}
+
+SyncSourceInfo NudgeTracker::source_info() const {

[tim (not reviewing), 2013/05/06 21:29:04]

This isn't such a trivial method.  GetSourceInfo() is a better name as per style
guide.

[rlarocque, 2013/05/07 18:45:39]

Done.

+  // The old-style source added an empty hint for all locally nudge types.
+  // This will be overwritten with the proper hint for any types that were both
+  // locally nudged and invalidated.
+  ModelTypeSet nudged_types;
+  for (NudgeMap::const_iterator it = local_nudge_counts_.begin();
+       it != local_nudge_counts_.end(); ++it) {
+    nudged_types.Put(it->first);
+  }
+  ModelTypeInvalidationMap invalidation_map =
+      ModelTypeSetToInvalidationMap(nudged_types, "");

[tim (not reviewing), 2013/05/06 21:29:04]

prefer std::string() to "" for efficiency & explicitness (vs implicit
constructor).

[rlarocque, 2013/05/07 18:45:39]

Done.

+
+  // The old-style source info can contain only one hint per type.  We grab the
+  // most recent, to mimic the old coalescing behaviour.
+  for (PayloadListMap::const_iterator it = payload_list_map_.begin();
+       it != payload_list_map_.end(); ++it) {
+    ModelType type = it->first;
+    const PayloadList& list = it->second;
+
+    if (!list.empty()) {
+      Invalidation invalidation;
+      invalidation.payload = list.back();
+      if (invalidation_map.find(type) != invalidation_map.end()) {
+        invalidation_map[type] = invalidation;
+      } else {
+        invalidation_map.insert(std::make_pair(type, invalidation));
+      }
+    }
+  }
+
+  return SyncSourceInfo(updates_source_, invalidation_map);
 }
 
-// TODO(rlarocque): This function often reports incorrect results.  However, it
-// is compatible with the "classic" behaviour.  We would need to make the nudge
-// tracker stop overwriting its own information (ie. fix crbug.com/231693)
-// before we could even try to report correct results.  The main issue is that
-// an notifications and local modifications nudges may overlap with each other
-// in sych a way that we lose track of which types were or were not locally
-// modified.
 ModelTypeSet NudgeTracker::GetLocallyModifiedTypes() const {
-  ModelTypeSet locally_modified;
+  ModelTypeSet nudged_types;
+  for (NudgeMap::const_iterator it = local_nudge_counts_.begin();
+       it != local_nudge_counts_.end(); ++it) {
+    nudged_types.Put(it->first);
+  }
+  return nudged_types;
+}
+
+sync_pb::GetUpdatesCallerInfo::GetUpdatesSource NudgeTracker::updates_source()
+    const {
+  return updates_source_;
+}
+
+void NudgeTracker::FillProtoMessage(
+    ModelType type,
+    sync_pb::GetUpdateTriggers* msg) const {
+  // Fill the list of payloads, if applicable.  The payloads must be ordered
+  // oldest to newest, so we insert them in the same order as we've been storing
+  // them internally.
+  PayloadListMap::const_iterator type_it = payload_list_map_.find(type);
+  if (type_it != payload_list_map_.end()) {
+    const PayloadList& payload_list = type_it->second;
+    for (PayloadList::const_iterator payload_it = payload_list.begin();
+         payload_it != payload_list.end(); ++payload_it) {
+      msg->add_notification_hint(*payload_it);
+    }
+  }
+
+  // TODO(rlarocque): Support Tango trickles.  See crbug.com/223437.
+  // msg->set_server_dropped_hints(server_dropped_payload_types_.Has(type));
+
+  msg->set_client_dropped_hints(locally_dropped_payload_types_.Has(type));
+  msg->set_invalidations_out_of_sync(invalidations_out_of_sync_);
 
-  if (source_info_.updates_source != sync_pb::GetUpdatesCallerInfo::LOCAL) {
-    return locally_modified;
+  {
+    NudgeMap::const_iterator it = local_nudge_counts_.find(type);
+    if (it == local_nudge_counts_.end()) {
+      msg->set_local_modification_nudges(0);
+    } else {
+      msg->set_local_modification_nudges(it->second);
+    }
   }
 
-  for (ModelTypeInvalidationMap::const_iterator i = source_info_.types.begin();
-       i != source_info().types.end(); ++i) {
-    locally_modified.Put(i->first);
+  {
+    NudgeMap::const_iterator it = refresh_requested_counts_.find(type);
+    if (it == refresh_requested_counts_.end()) {
+      msg->set_datatype_refresh_nudges(0);
+    } else {
+      msg->set_datatype_refresh_nudges(it->second);
+    }
   }
-  return locally_modified;
 }
 
 }  // namespace sessions