[GCM] Wiring heartbeat interval calls to GCMDriver

[GCM] Wiring heartbeat interval calls to GCMDriver

Wires the call to Add/RemoveHeartbeatInterval through:
* GCMDriver/Desktop/Android/Fake
* GCMDriverDesktop::IOWorker
* GCMClient/Impl to MCSClient
* Adding loading of persisted interval in MCSClient

BUG=481174
Committed: https://crrev.com/2275446a45e50b79540953969bd98f0fb3fb49b6
Cr-Commit-Position: refs/heads/master@{#329750}

[fgorski, 2015-05-06 17:53:39]

fgorski@chromium.org changed reviewers:
+ zea@chromium.org

[fgorski, 2015-05-06 17:53:40]

PTAL.

I've added one test focused on storing and restoring the heartbeats. Other than
that it is simply wiring, which we discussed.

[Nicolas Zea, 2015-05-07 18:13:52]

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:313: // Gets and sets custom heartbeat
interval for the MCS connection.
Describe what scope corresponds to?

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:536:
mcs_client_->AddHeartbeatInterval(scope, interval_ms);
This can be called from any arbitrary component right? What prevents them from
calling this before GCM has created the mcs client?

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:157: virtual void AddHeartbeatInterval(const
std::string& scope,
nit: describe what scope is and how it should be set.

Also, any rules as to when this is allowed to be called?

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

https://codereview.chromium.org/1124783002/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:394:
heartbeat_manager_.SetClientHeartbeatIntervalMs(min_interval_ms);
Will this ever trigger a connection reset before we've attempted the initial
connection, or while we're in the middle of a connection? Is that something we
need to handle?

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

https://codereview.chromium.org/1124783002/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client_unittest.cc:1004: TEST_F(MCSClientTest,
StoringCustomHeartbeatInterval) {
Can we break this up into multiple test cases?

Also, add test case for setting heartbeat interval before and during the initial
connection?

[fgorski, 2015-05-10 06:44:34]

Addressed. PTAL

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:313: // Gets and sets custom heartbeat
interval for the MCS connection.
On 2015/05/07 18:13:52, Nicolas Zea wrote:
> Describe what scope corresponds to?

Done.

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:536:
mcs_client_->AddHeartbeatInterval(scope, interval_ms);
On 2015/05/07 18:13:52, Nicolas Zea wrote:
> This can be called from any arbitrary component right? What prevents them from
> calling this before GCM has created the mcs client?

Good point. I've added a check against delayed task controller in
GCMDriverDesktop to address the issue. Components are not meant to call stuff on
GCMClient directly, but go through the driver.

I realize that this may cause a situation that upon setting the interval to the
new value right after starting Chrome, we will reset the connection (which might
be pretty fresh), but I think this is a low price. After all next time around
the setting will come from the store.

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1124783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:157: virtual void AddHeartbeatInterval(const
std::string& scope,
On 2015/05/07 18:13:52, Nicolas Zea wrote:
> nit: describe what scope is and how it should be set.
> 
> Also, any rules as to when this is allowed to be called?

Done.

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

https://codereview.chromium.org/1124783002/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:394:
heartbeat_manager_.SetClientHeartbeatIntervalMs(min_interval_ms);
On 2015/05/07 18:13:52, Nicolas Zea wrote:
> Will this ever trigger a connection reset before we've attempted the initial
> connection, or while we're in the middle of a connection? Is that something we
> need to handle?

This is not going to trigger before the initial connection is started, per the
delayed task controller added in response to earlier comment.

Setting lower connection interval than the current one will trigger a connection
reset.

I'd ignore the problem and let the connection reset quickly. This only happens
to be the case when a shorter interval is initiated by a component after a reset
(unlikely given changes like that would rather be carried in updates like
enterprise policy changes)... this is of course unless we decided to have a
command line flag, or someone goes rouge with consumption of the API, by setting
it to 2 minutes after reset and removing after connection was reestablished)

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

https://codereview.chromium.org/1124783002/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client_unittest.cc:1004: TEST_F(MCSClientTest,
StoringCustomHeartbeatInterval) {
On 2015/05/07 18:13:52, Nicolas Zea wrote:
> Can we break this up into multiple test cases?
> 
> Also, add test case for setting heartbeat interval before and during the
initial
> connection?

Done.

[Nicolas Zea, 2015-05-13 17:57:37]

LGTM with some nits

https://codereview.chromium.org/1124783002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/1124783002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:325: // |scope| is used to identify the
component that requests a custom interval
nit: I'd explicitly call out that it should be a unique identifier for the
component

https://codereview.chromium.org/1124783002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1124783002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:187: // |interval_ms| should be between 2 and
15 (28 on cellular networks). For
2 and 15 are in units of minutes right? I'd call that out.

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:229: int GetMinHeartbeatInterval();
nit: maybe call this GetHeartbeatIntervalMs (and mention in the comment that the
unit is milliseconds)

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client_unittest.cc:938: \
remove \

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client_unittest.cc:1017: // Tests adding custom
heartbeat interval before connection is initilaized.
nit: initialized

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client_unittest.cc:1025:
LoginClientWithHeartbeat(std::vector<std::string>(), interval_ms);
verify HasClientHeartbeatInterval (here and in the other test cases)?

[fgorski, 2015-05-13 18:59:16]

Addressed. Thanks for review.

https://codereview.chromium.org/1124783002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/1124783002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:325: // |scope| is used to identify the
component that requests a custom interval
On 2015/05/13 17:57:36, Nicolas Zea wrote:
> nit: I'd explicitly call out that it should be a unique identifier for the
> component

Done.

https://codereview.chromium.org/1124783002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1124783002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:187: // |interval_ms| should be between 2 and
15 (28 on cellular networks). For
On 2015/05/13 17:57:36, Nicolas Zea wrote:
> 2 and 15 are in units of minutes right? I'd call that out.

Done.

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:229: int GetMinHeartbeatInterval();
On 2015/05/13 17:57:37, Nicolas Zea wrote:
> nit: maybe call this GetHeartbeatIntervalMs (and mention in the comment that
the
> unit is milliseconds)

Done.

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client_unittest.cc:938: \
On 2015/05/13 17:57:37, Nicolas Zea wrote:
> remove \

Done.

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client_unittest.cc:1017: // Tests adding custom
heartbeat interval before connection is initilaized.
On 2015/05/13 17:57:37, Nicolas Zea wrote:
> nit: initialized

Done.

https://codereview.chromium.org/1124783002/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client_unittest.cc:1025:
LoginClientWithHeartbeat(std::vector<std::string>(), interval_ms);
On 2015/05/13 17:57:37, Nicolas Zea wrote:
> verify HasClientHeartbeatInterval (here and in the other test cases)?

Done. Added here and below. Other tests already have that.

[fgorski, 2015-05-13 21:53:52]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2015-05-13 21:53:52]

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/1124783002/#ps80001
(title: "Addressing final feedback and fixing formatting")

[commit-bot: I haz the power, 2015-05-13 21:54:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124783002/80001

[commit-bot: I haz the power, 2015-05-14 00:05:28]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-14 00:06:52]

Patchset 5 (id:??) landed as
https://crrev.com/2275446a45e50b79540953969bd98f0fb3fb49b6
Cr-Commit-Position: refs/heads/master@{#329750}