sync: GU retry with less explicit TimeTicks logic

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 f96d3464826e86653fb04d515c7d9575d891f2be..8ad215b26a73c90d20b00595d6bdc4d56fdb46a6 100644
--- a/sync/sessions/nudge_tracker.cc
+++ b/sync/sessions/nudge_tracker.cc
@@ -18,7 +18,8 @@ size_t NudgeTracker::kDefaultMaxPayloadsPerType = 10;
 NudgeTracker::NudgeTracker()
     : updates_source_(sync_pb::GetUpdatesCallerInfo::UNKNOWN),
       invalidations_enabled_(false),
-      invalidations_out_of_sync_(true) {
+      invalidations_out_of_sync_(true),
+      is_retry_required_(false) {
   ModelTypeSet protocol_types = ProtocolTypes();
   // Default initialize all the type trackers.
   for (ModelTypeSet::Iterator it = protocol_types.First(); it.Good();
@@ -44,11 +45,11 @@ bool NudgeTracker::IsSyncRequired() const {
   return false;
 }
 
-bool NudgeTracker::IsGetUpdatesRequired(base::TimeTicks now) const {
+bool NudgeTracker::IsGetUpdatesRequired() const {
   if (invalidations_out_of_sync_)
     return true;
 
-  if (IsRetryRequired(now))
+  if (IsRetryRequired())
     return true;
 
   for (TypeTrackerMap::const_iterator it = type_trackers_.begin();
@@ -60,16 +61,18 @@ bool NudgeTracker::IsGetUpdatesRequired(base::TimeTicks now) const {
   return false;
 }
 
-bool NudgeTracker::IsRetryRequired(base::TimeTicks now) const {
-  return !next_retry_time_.is_null() && next_retry_time_ < now;
+bool NudgeTracker::IsRetryRequired() const {
+  return is_retry_required_;
 }
 
-void NudgeTracker::RecordSuccessfulSyncCycle(base::TimeTicks now) {
+void NudgeTracker::RecordSuccessfulSyncCycle() {
   updates_source_ = sync_pb::GetUpdatesCallerInfo::UNKNOWN;
-  last_successful_sync_time_ = now;
 
-  if (next_retry_time_ < now)
+  // If a retry was required, we've just serviced it.  Unset the flag.
+  if (is_retry_required_) {
+    is_retry_required_ = false;
     next_retry_time_ = base::TimeTicks();
+  }
 
   // A successful cycle while invalidations are enabled puts us back into sync.
   invalidations_out_of_sync_ = !invalidations_enabled_;
@@ -239,6 +242,18 @@ void NudgeTracker::FillProtoMessage(
   type_trackers_.find(type)->second.FillGetUpdatesTriggersMessage(msg);
 }
 
+void NudgeTracker::ToggleRetryFlagIfRequired(base::TimeTicks now) {

[haitaol1, 2014/01/27 23:17:07]

Seems redundant to keep both is_retry_required and next_retry_time. How about
changing this to SetSyncCycleStartTime() and compare start time with
next_retry_time in IsRetryRequired and RecordSuccessfulSyncCycle? In this way,
next_retry_time_ is also not lost if GU failed.

[rlarocque, 2014/01/28 01:06:34]

I wanted to avoid saving the value of 'now' inside the NudgeTracker.  My reasons
for this are subjective.  I think that 'now' is a bit too much information for
the NudgeTracker.  It's easy to justify why the NudgeTracker should have an
'is_retry_required_' member.  That's less true with 'sync_cycle_start_time_'.

Anyway, it seems that trying to prevent storing sync_cycle_start_time_ here has
led to significantly worse code in other places, so it's getting very hard
justify that decision.  I might need to take your suggestion and store the
timestamp here.

+  // Nothing to do if no retries are scheduled for the future.
+  if (next_retry_time_.is_null())
+    return;
+
+  if (next_retry_time_ < now) {
+    // The time has come.  We should perform a GU retry as soon as possible.
+    is_retry_required_ = true;
+    next_retry_time_ = base::TimeTicks();
+  }
+}
+
 void NudgeTracker::SetHintBufferSize(size_t size) {
   for (TypeTrackerMap::iterator it = type_trackers_.begin();
        it != type_trackers_.end(); ++it) {
@@ -246,5 +261,9 @@ void NudgeTracker::SetHintBufferSize(size_t size) {
   }
 }
 
+void NudgeTracker::SetNextRetryTime(base::TimeTicks retry_time) {
+  next_retry_time_ = retry_time;
+}
+
 }  // namespace sessions
 }  // namespace syncer