[GCM] Fixing the client interval interaction with heartbeat ack

google_apis/gcm/engine/heartbeat_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "google_apis/gcm/engine/heartbeat_manager.h"
 
+#include <limits>
 #include <utility>
 
 #include "base/callback.h"
 #include "base/location.h"
 #include "base/metrics/histogram.h"
 #include "base/power_monitor/power_monitor.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "build/build_config.h"
@@ skipping 142 matching lines @@
     ResetConnection(ConnectionFactory::HEARTBEAT_FAILURE);
     return;
   }
 
   waiting_for_ack_ = true;
   RestartTimer();
   send_heartbeat_callback_.Run();
 }
 
 void HeartbeatManager::RestartTimer() {
+  int interval_ms = std::numeric_limits<int>::max();

[Nicolas Zea, 2016/03/03 00:19:43]

Why not initialize to heartbeat_interval_ms_?

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

Done.

   if (!waiting_for_ack_) {
     // Recalculate the timer interval based network type.

[Nicolas Zea, 2016/03/03 00:19:43]

nit: based network type -> based on network type

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

Done.

     // 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_;

[Nicolas Zea, 2016/03/03 00:19:43]

It seems like at this point heartbeat_interval_ms_ always stores the current
non-ack interval. Does it make sense to only update heartbeat_interval_ms_ in
SetClientHeartbeatIntervalMs or UpdateHeartbeatConfig methods?

At that point you can get rid of all this logic (it'll happen in the respective
methods), and the code could just be
int interval_ms = heartbeat_interval_ms_;
if (waiting_for_ack)
  interval_ms =  ack default

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

We are not able to do calculation in respective methods, as it is a bit more
complex than updating the value. I did however extract the code and called it
before timer starts (which is also triggered when new, smaller custom interval
is requested by the client) and upon server's request.

     } 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_ ||
+      if (client_interval_ms_ <= heartbeat_interval_ms_ ||
           heartbeat_interval_ms_ == 0) {
         heartbeat_interval_ms_ = client_interval_ms_;
       }
     } else {
       heartbeat_interval_ms_ = GetDefaultHeartbeatInterval();
     }
+    interval_ms = heartbeat_interval_ms_;
     DVLOG(1) << "Sending next heartbeat in "
-             << heartbeat_interval_ms_ << " ms.";
+             << interval_ms << " ms.";
   } else {
-    heartbeat_interval_ms_ = kHeartbeatAckDefaultMs;
+    // Not updating the value of heartbeat_interval_ms_ when waitign for ack.

[sjoe, 2016/03/02 23:46:08]

waitign -> waiting

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

Done.

+    interval_ms = kHeartbeatAckDefaultMs;
     DVLOG(1) << "Resetting timer for ack with "
-             << heartbeat_interval_ms_ << " ms interval.";
+             << interval_ms << " ms interval.";
   }
 
   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
   // when the system is suspending or resuming.  The only one that does not is
   // Linux so we need to poll to check for missed heartbeats.
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&HeartbeatManager::CheckForMissedHeartbeat,
                  weak_ptr_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(kHeartbeatMissedCheckMs));
@@ skipping 74 matching lines @@
       interval <= max_heartbeat_interval;
 }
 
 void HeartbeatManager::ResetConnection(
     ConnectionFactory::ConnectionResetReason reason) {
   Stop();
   trigger_reconnect_callback_.Run(reason);
 }
 
 }  // namespace gcm