[GCM] Fixing the client interval interaction with heartbeat ack

[GCM] Fixing the client interval interaction with heartbeat ack

This patch fixes the case, where heartbeat interval is lowered to
1 minute after a heartbeat is initiated by the gcm client.

It also handles the case, where custom interval was set to a higher
value, but because of no reconnection, we are keeping the lowest
known value. (Caught by existing unit test).

BUG=591490
R=zea@chromium.org

Committed: https://crrev.com/a565abf9111511c13ebd0809656e12323956a4b2
Cr-Commit-Position: refs/heads/master@{#379095}

[fgorski, 2016-03-02 23:16:31]

Description was changed from

==========
[GCM] Fixing the client interval interaction with heartbeat ack

This patch fixes the case, where heartbeat interval is lowered to
1 minute after a heartbeat is initiated by the gcm client.

It also handles the case, where custom interval was set to a higher
value, but because of no reconnection, we are keeping the lowest
known value. (Caught by existing unit test).

BUG=591490
R=zea@chromium.org
==========

to

==========
[GCM] Fixing the client interval interaction with heartbeat ack

This patch fixes the case, where heartbeat interval is lowered to
1 minute after a heartbeat is initiated by the gcm client.

It also handles the case, where custom interval was set to a higher
value, but because of no reconnection, we are keeping the lowest
known value. (Caught by existing unit test).

BUG=591490
R=zea@chromium.org
==========

[fgorski, 2016-03-02 23:17:35]

Please take a look.

[sjoe, 2016-03-02 23:46:07]

sjoe@chromium.org changed reviewers:
+ sjoe@chromium.org

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

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
google_apis/gcm/engine/heartbeat_manager.cc:192: // Not updating the value of
heartbeat_interval_ms_ when waitign for ack.
waitign -> waiting

[Nicolas Zea, 2016-03-03 00:19:44]

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
google_apis/gcm/engine/heartbeat_manager.cc:170: int interval_ms =
std::numeric_limits<int>::max();
Why not initialize to heartbeat_interval_ms_?

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
google_apis/gcm/engine/heartbeat_manager.cc:172: // Recalculate the timer
interval based network type.
nit: based network type -> based on network type

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
google_apis/gcm/engine/heartbeat_manager.cc:177: heartbeat_interval_ms_ =
server_interval_ms_;
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:36]

Updated. PTAL

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
google_apis/gcm/engine/heartbeat_manager.cc:170: int interval_ms =
std::numeric_limits<int>::max();
On 2016/03/03 00:19:43, Nicolas Zea wrote:
> Why not initialize to heartbeat_interval_ms_?

Done.

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
google_apis/gcm/engine/heartbeat_manager.cc:172: // Recalculate the timer
interval based network type.
On 2016/03/03 00:19:43, Nicolas Zea wrote:
> nit: based network type -> based on network type

Done.

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
google_apis/gcm/engine/heartbeat_manager.cc:177: heartbeat_interval_ms_ =
server_interval_ms_;
On 2016/03/03 00:19:43, Nicolas Zea wrote:
> 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

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.

https://codereview.chromium.org/1758573004/diff/1/google_apis/gcm/engine/hear...
google_apis/gcm/engine/heartbeat_manager.cc:192: // Not updating the value of
heartbeat_interval_ms_ when waitign for ack.
On 2016/03/02 23:46:08, sjoe wrote:
> waitign -> waiting

Done.

https://codereview.chromium.org/1758573004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/1758573004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:242: }
zea@ perhaps we can have a DCHECK(heartbeat_interval_ms_ > 0) here?

[Nicolas Zea, 2016-03-03 19:09:41]

LGTM

https://codereview.chromium.org/1758573004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/1758573004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:7: #include <limits>
nit: not needed anymore

https://codereview.chromium.org/1758573004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:178: if (!waiting_for_ack_) {
nit: for readability, would be better to do:
if (waiting for ack)
   ...
} else {
  ...
}

(negative conditions are less readable in general)

https://codereview.chromium.org/1758573004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:242: }
On 2016/03/03 17:10:35, fgorski wrote:
> zea@ perhaps we can have a DCHECK(heartbeat_interval_ms_ > 0) here?

DCHECK_GT, but yes SGTM :)

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

All done. thanks for review.

https://codereview.chromium.org/1758573004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/1758573004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:7: #include <limits>
On 2016/03/03 19:09:40, Nicolas Zea wrote:
> nit: not needed anymore

Done.

https://codereview.chromium.org/1758573004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:178: if (!waiting_for_ack_) {
On 2016/03/03 19:09:40, Nicolas Zea wrote:
> nit: for readability, would be better to do:
> if (waiting for ack)
>    ...
> } else {
>   ...
> }
> 
> (negative conditions are less readable in general)

Done.

[fgorski, 2016-03-03 20:31:38]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-03-03 20:31:38]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1758573004/#ps40001
(title: "Addressing final feedback")

[commit-bot: I haz the power, 2016-03-03 20:32:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1758573004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1758573004/40001

[commit-bot: I haz the power, 2016-03-03 21:35:27]

Description was changed from

==========
[GCM] Fixing the client interval interaction with heartbeat ack

This patch fixes the case, where heartbeat interval is lowered to
1 minute after a heartbeat is initiated by the gcm client.

It also handles the case, where custom interval was set to a higher
value, but because of no reconnection, we are keeping the lowest
known value. (Caught by existing unit test).

BUG=591490
R=zea@chromium.org
==========

to

==========
[GCM] Fixing the client interval interaction with heartbeat ack

This patch fixes the case, where heartbeat interval is lowered to
1 minute after a heartbeat is initiated by the gcm client.

It also handles the case, where custom interval was set to a higher
value, but because of no reconnection, we are keeping the lowest
known value. (Caught by existing unit test).

BUG=591490
R=zea@chromium.org
==========

[commit-bot: I haz the power, 2016-03-03 21:35:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-03 21:36:35]

Description was changed from

==========
[GCM] Fixing the client interval interaction with heartbeat ack

This patch fixes the case, where heartbeat interval is lowered to
1 minute after a heartbeat is initiated by the gcm client.

It also handles the case, where custom interval was set to a higher
value, but because of no reconnection, we are keeping the lowest
known value. (Caught by existing unit test).

BUG=591490
R=zea@chromium.org
==========

to

==========
[GCM] Fixing the client interval interaction with heartbeat ack

This patch fixes the case, where heartbeat interval is lowered to
1 minute after a heartbeat is initiated by the gcm client.

It also handles the case, where custom interval was set to a higher
value, but because of no reconnection, we are keeping the lowest
known value. (Caught by existing unit test).

BUG=591490
R=zea@chromium.org

Committed: https://crrev.com/a565abf9111511c13ebd0809656e12323956a4b2
Cr-Commit-Position: refs/heads/master@{#379095}
==========

[commit-bot: I haz the power, 2016-03-03 21:36:36]

Patchset 3 (id:??) landed as
https://crrev.com/a565abf9111511c13ebd0809656e12323956a4b2
Cr-Commit-Position: refs/heads/master@{#379095}