Signup for GCM upstream notifications

Signup for GCM upstream notifications

Also set the default heartbeats interval for GCM driver.

BUG=517466
Committed: https://crrev.com/75799d32ee85fc107ed070642cb4d2fd3181285b
Cr-Commit-Position: refs/heads/master@{#360978}

[binjin, 2015-11-14 21:52:56]

binjin@chromium.org changed reviewers:
+ atwilson@chromium.org

[binjin, 2015-11-14 21:52:57]

Hi Drew, PTAL

[jinzhang1, 2015-11-16 19:10:49]

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

[jinzhang1, 2015-11-16 19:10:49]

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:356: void
HeartbeatScheduler::SignUpUpstreamNotification() {
Hey Bin, I don't see you use AddHeartbeatInterval() defined in
https://codereview.chromium.org/1124783002/diff/80001/components/gcm_driver/g...
to set the gcm client heartbeat interval according to the device policy for
heartbeat frequency. Without it, GCM will only send heartbeat like every half an
hour.

[binjin, 2015-11-16 19:13:15]

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:356: void
HeartbeatScheduler::SignUpUpstreamNotification() {
On 2015/11/16 19:10:49, jinzhang1 wrote:
> Hey Bin, I don't see you use AddHeartbeatInterval() defined in
>
https://codereview.chromium.org/1124783002/diff/80001/components/gcm_driver/g...
> to set the gcm client heartbeat interval according to the device policy for
> heartbeat frequency. Without it, GCM will only send heartbeat like every half
an
> hour.

This will be in a separated CL.

[Andrew T Wilson (Slow), 2015-11-18 11:43:32]

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:22: const int
kMinimumSignupIntervalMs = 3 * 60 * 1000;       // 3 minutes
Would be good to add a comment for what this is.

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:239: ShutdownGCM();
Do we need to do something here to stop the upstream notifications from being
sent also? It's a bit weird for heartbeats to be disabled but for us to continue
generating upstream notifications.

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:365: if
(!last_upstream_notification_signup_time_.is_null() &&
What's this doing? Seems like you are making sure we don't ping the upstream
server too often? What's the case where we need to worry about this?

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:378:
kUpstreamNotificationSignUpDestinationID, message,
Do we need to be able to specify a different destination ID (for example, when
testing with the staging server vs prod)?

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:261:
IgnoreUpstreamNotificationMsg();
Why is this required? How many notification messages are we sending up?

[binjin, 2015-11-19 05:50:27]

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:22: const int
kMinimumSignupIntervalMs = 3 * 60 * 1000;       // 3 minutes
On 2015/11/18 11:43:32, Andrew T Wilson (Slow) wrote:
> Would be good to add a comment for what this is.

Done.

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:239: ShutdownGCM();
On 2015/11/18 11:43:32, Andrew T Wilson (Slow) wrote:
> Do we need to do something here to stop the upstream notifications from being
> sent also? It's a bit weird for heartbeats to be disabled but for us to
continue
> generating upstream notifications.

I didn't find any API to opt-out the upstream notification. It doesn't make
sense to explicitly stop upstream notification either since by design we can
just sign up and forget about it.

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:365: if
(!last_upstream_notification_signup_time_.is_null() &&
On 2015/11/18 11:43:32, Andrew T Wilson (Slow) wrote:
> What's this doing? Seems like you are making sure we don't ping the upstream
> server too often? What's the case where we need to worry about this?

The concern is over the GCM driver OnConnected event, I'm not exactly sure if
there can be a scenario that it ends up flooding the server.

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:378:
kUpstreamNotificationSignUpDestinationID, message,
On 2015/11/18 11:43:32, Andrew T Wilson (Slow) wrote:
> Do we need to be able to specify a different destination ID (for example, when
> testing with the staging server vs prod)?

The current destination ID (https://gcm.googleapis.com/gcm/gcm.event_tracker) is
public and it's just a relay service. If it's just for testing, specifying the
actual destination ID being relayed (kHeartbeatGCMDestinationID) should be
enough.

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1267983002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:261:
IgnoreUpstreamNotificationMsg();
On 2015/11/18 11:43:32, Andrew T Wilson (Slow) wrote:
> Why is this required? How many notification messages are we sending up?

I didn't consider much, I just add one call after each occurrence of
VerifyAndClearExpectations().

[Andrew T Wilson (Slow), 2015-11-20 16:37:34]

LGTM with comments

https://codereview.chromium.org/1267983002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/1267983002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:230: if
(settings->GetInteger(chromeos::kHeartbeatFrequency, &frequency)) {
So, shouldn't we call AddHeartbeatInterval() even if the default heartbeat
frequency (kDefaultheartbeatIntervalMs) is in effect?

https://codereview.chromium.org/1267983002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:371: base::TimeDelta
since_last_sign_up =
Let's just remove this logic + the member var
last_upstream_notification_signup_time_

[binjin, 2015-11-20 22:56:38]

Description was changed from

==========
Signup for GCM upstream notifications

BUG=
==========

to

==========
Signup for GCM upstream notifications

Also set the default heartbeats interval for GCM driver.

BUG=517466
==========

[binjin, 2015-11-20 22:56:49]

https://codereview.chromium.org/1267983002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/1267983002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:230: if
(settings->GetInteger(chromeos::kHeartbeatFrequency, &frequency)) {
On 2015/11/20 16:37:34, Andrew T Wilson (Slow) wrote:
> So, shouldn't we call AddHeartbeatInterval() even if the default heartbeat
> frequency (kDefaultheartbeatIntervalMs) is in effect?

Done.

https://codereview.chromium.org/1267983002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:371: base::TimeDelta
since_last_sign_up =
On 2015/11/20 16:37:34, Andrew T Wilson (Slow) wrote:
> Let's just remove this logic + the member var
> last_upstream_notification_signup_time_

Done.

[binjin, 2015-11-21 00:59:13]

The CQ bit was checked by binjin@chromium.org

[binjin, 2015-11-21 00:59:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1267983002/#ps200001
(title: "add heartbeats bits")

[commit-bot: I haz the power, 2015-11-21 01:02:36]

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

[commit-bot: I haz the power, 2015-11-21 01:28:08]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-11-21 01:28:45]

Patchset 11 (id:??) landed as
https://crrev.com/75799d32ee85fc107ed070642cb4d2fd3181285b
Cr-Commit-Position: refs/heads/master@{#360978}