[GCM] Fixing the client interval interaction with heartbeat ack

google_apis/gcm/engine/heartbeat_manager.cc

Index: google_apis/gcm/engine/heartbeat_manager.cc
diff --git a/google_apis/gcm/engine/heartbeat_manager.cc b/google_apis/gcm/engine/heartbeat_manager.cc
index 8ac70b841b90f5a200f28b8a82fba47f674e4674..0d1ce0e2ba937664401a8153078edb0c91bb6ea9 100644
--- a/google_apis/gcm/engine/heartbeat_manager.cc
+++ b/google_apis/gcm/engine/heartbeat_manager.cc
@@ -4,6 +4,7 @@
 
 #include "google_apis/gcm/engine/heartbeat_manager.h"
 
+#include <limits>

[Nicolas Zea, 2016/03/03 19:09:40]

nit: not needed anymore

[fgorski, 2016/03/03 20:31:32]

Done.

 #include <utility>
 
 #include "base/callback.h"
@@ -71,6 +72,9 @@ void HeartbeatManager::Start(
   if (monitor)
     monitor->AddObserver(this);
 
+  // Calculated the heartbeat interval just before we start the timer.
+  UpdateHeartbeatInterval();
+
   // Kicks off the timer.
   waiting_for_ack_ = false;
   RestartTimer();
@@ -106,6 +110,10 @@ void HeartbeatManager::UpdateHeartbeatConfig(
   }
   DVLOG(1) << "Updating server heartbeat interval to " << config.interval_ms();
   server_interval_ms_ = config.interval_ms();
+
+  // Make sure heartbeat interval is recalculated when new server interval is
+  // available.
+  UpdateHeartbeatInterval();
 }
 
 base::TimeTicks HeartbeatManager::GetNextHeartbeatTime() const {
@@ -166,39 +174,20 @@ void HeartbeatManager::OnHeartbeatTriggered() {
 }
 
 void HeartbeatManager::RestartTimer() {
+  int interval_ms = heartbeat_interval_ms_;
   if (!waiting_for_ack_) {

[Nicolas Zea, 2016/03/03 19:09:40]

nit: for readability, would be better to do:
if (waiting for ack)
   ...
} else {
  ...
}

(negative conditions are less readable in general)

[fgorski, 2016/03/03 20:31:32]

Done.

-    // Recalculate the timer interval based network type.
-    // Server interval takes precedence over client interval, even if the latter
-    // is less.
-    if (server_interval_ms_ != 0) {
-      // If a server interval is set, it overrides any local one.
-      heartbeat_interval_ms_ = server_interval_ms_;
-    } else if (HasClientHeartbeatInterval()) {
-      // Client interval might have been adjusted up, which should only take
-      // effect during a reconnection.
-      if (client_interval_ms_ < heartbeat_interval_ms_ ||
-          heartbeat_interval_ms_ == 0) {
-        heartbeat_interval_ms_ = client_interval_ms_;
-      }
-    } else {
-      heartbeat_interval_ms_ = GetDefaultHeartbeatInterval();
-    }
-    DVLOG(1) << "Sending next heartbeat in "
-             << heartbeat_interval_ms_ << " ms.";
+    DVLOG(1) << "Sending next heartbeat in " << interval_ms << " ms.";
   } else {
-    heartbeat_interval_ms_ = kHeartbeatAckDefaultMs;
-    DVLOG(1) << "Resetting timer for ack with "
-             << heartbeat_interval_ms_ << " ms interval.";
+    interval_ms = kHeartbeatAckDefaultMs;
+    DVLOG(1) << "Resetting timer for ack within " << interval_ms << " ms.";
   }
 
   heartbeat_expected_time_ =
-      base::Time::Now() +
-      base::TimeDelta::FromMilliseconds(heartbeat_interval_ms_);
+      base::Time::Now() + base::TimeDelta::FromMilliseconds(interval_ms);
   heartbeat_timer_->Start(FROM_HERE,
-                         base::TimeDelta::FromMilliseconds(
-                             heartbeat_interval_ms_),
-                         base::Bind(&HeartbeatManager::OnHeartbeatTriggered,
-                                    weak_ptr_factory_.GetWeakPtr()));
+                          base::TimeDelta::FromMilliseconds(interval_ms),
+                          base::Bind(&HeartbeatManager::OnHeartbeatTriggered,
+                                     weak_ptr_factory_.GetWeakPtr()));
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   // Windows, Mac, Android, iOS, and Chrome OS all provide a way to be notified
@@ -235,6 +224,24 @@ void HeartbeatManager::CheckForMissedHeartbeat() {
 #endif  // defined(OS_LINUX) && !defined(OS_CHROMEOS)
 }
 
+void HeartbeatManager::UpdateHeartbeatInterval() {
+  // Server interval takes precedence over client interval, even if the latter
+  // is less.
+  if (server_interval_ms_ != 0) {
+    // If a server interval is set, it overrides any local one.
+    heartbeat_interval_ms_ = server_interval_ms_;
+  } else if (HasClientHeartbeatInterval() &&
+             (client_interval_ms_ < heartbeat_interval_ms_ ||
+              heartbeat_interval_ms_ == 0)) {
+    // Client interval might have been adjusted up, which should only take
+    // effect during a reconnection.
+    heartbeat_interval_ms_ = client_interval_ms_;
+  } else if (heartbeat_interval_ms_ == 0) {
+    // If interval is still 0, recalculate it based on network type.
+    heartbeat_interval_ms_ = GetDefaultHeartbeatInterval();
+  }

[fgorski, 2016/03/03 17:10:35]

zea@ perhaps we can have a DCHECK(heartbeat_interval_ms_ > 0) here?

[Nicolas Zea, 2016/03/03 19:09:40]

DCHECK_GT, but yes SGTM :)

+}
+
 int HeartbeatManager::GetDefaultHeartbeatInterval() {
   // For unknown connections, use the longer cellular heartbeat interval.
   int heartbeat_interval_ms = kCellHeartbeatDefaultMs;