Send GCM id to DMServer

Send GCM id to DMServer

This CL add a device management request to send GCM id after registration to
the server, so that server can maintain a mapping between GCM id and DM token.

BUG=517466
Committed: https://crrev.com/92960c61ee75a5323fc954b9819acca5b684d2aa
Cr-Commit-Position: refs/heads/master@{#342364}

Committed: https://crrev.com/3cde6e4de0c5ba3915dce583cd8dee7614e1c104
Cr-Commit-Position: refs/heads/master@{#342850}

[binjin, 2015-07-28 18:02:25]

Hi Jin,

Please review the change to protobuf file.

[jinzhang1, 2015-07-28 18:56:16]

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

[jinzhang1, 2015-07-28 18:56:17]

https://codereview.chromium.org/1258313002/diff/1/components/policy/proto/dev...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/1258313002/diff/1/components/policy/proto/dev...
components/policy/proto/device_management_backend.proto:1032: //     *
attribute_update
Please add a comment here to reflect the newly added request type.

[jinzhang1, 2015-07-28 18:57:44]

lgtm

[binjin, 2015-07-31 17:09:05]

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

[binjin, 2015-07-31 17:09:06]

Hi Drew, PTAL.

I'm not sure how to handle the case where we failed to send GCM ID mapping
request, maybe another exponential backoff.

[Andrew T Wilson (Slow), 2015-08-03 13:52:26]

Mostly LG. Good question about how to handle failures. I'm OK to ignore failures
in this CL, but put a TODO() in there to figure out how we want to handle them.

I'm possibly OK with leaving it as-is (no retries) since we currently re-send
the ID after every restart. Worst case, the device will show up as offline until
the next restart. Best case, there's no effect, if the GcmId hasn't changed at
all.

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:257:
g_browser_process->gcm_driver(), core()->client(),
can you just use client() here and not core()->client()?

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:310:
cloud_policy_client_->UpdateGcmIdMapping(
Log a bug and add a TODO() here to avoid unnecessary calls to update the GcmId
if it hasn't changed.

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:313:
base::Unretained(this)));
Use a weak pointer here instead of Unretained().

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:392:
latest_gcm_id_mapping_succeeded_ = status;
Why do we bother having a callback here at all? We don't really care if our
request succeeds or not (no need to track it in
latest_gcm_id_mapping_succeeded). At the most, we might log something, and
that's it.

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:394: LOG(WARNING) <<
"Failed to send GCM id to DM server";
Use LOG_IF() instead of if() { LOG() }

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler.h (right):

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.h:41: // Constructor.
|cloud_policy_client| will be used to send registred GCM id
nit: registered

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:255:
EXPECT_TRUE(scheduler_.latest_gcm_id_mapping_succeeded());
Don't think these checks of latest_gcm_id_mapping_succeeded() are useful - I
think we should get rid of that API/member variable entirely, and just make sure
that a completed registration calls client::UpdateGcmIdMapping().

https://codereview.chromium.org/1258313002/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/1258313002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_client.cc:698:
callback.Run(status == DM_STATUS_SUCCESS);
Should we call NotifyClientError() and update status_ as we do on things like
OnStatusUploadCompleted()? If not, why not?

[binjin, 2015-08-03 17:54:25]

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:257:
g_browser_process->gcm_driver(), core()->client(),
On 2015/08/03 13:52:26, Andrew T Wilson wrote:
> can you just use client() here and not core()->client()?

Done.

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:310:
cloud_policy_client_->UpdateGcmIdMapping(
On 2015/08/03 13:52:26, Andrew T Wilson wrote:
> Log a bug and add a TODO() here to avoid unnecessary calls to update the GcmId
> if it hasn't changed.

Done.

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:313:
base::Unretained(this)));
On 2015/08/03 13:52:26, Andrew T Wilson wrote:
> Use a weak pointer here instead of Unretained().

Done.

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:392:
latest_gcm_id_mapping_succeeded_ = status;
On 2015/08/03 13:52:26, Andrew T Wilson wrote:
> Why do we bother having a callback here at all? We don't really care if our
> request succeeds or not (no need to track it in
> latest_gcm_id_mapping_succeeded). At the most, we might log something, and
> that's it.

Okay, removed the latest_gcm_id_mapping_succeeded in this CL, leaving the
callback for logging purpose.

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:394: LOG(WARNING) <<
"Failed to send GCM id to DM server";
On 2015/08/03 13:52:26, Andrew T Wilson wrote:
> Use LOG_IF() instead of if() { LOG() }

Done.

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler.h (right):

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.h:41: // Constructor.
|cloud_policy_client| will be used to send registred GCM id
On 2015/08/03 13:52:26, Andrew T Wilson wrote:
> nit: registered

Done.

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1258313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:255:
EXPECT_TRUE(scheduler_.latest_gcm_id_mapping_succeeded());
On 2015/08/03 13:52:26, Andrew T Wilson wrote:
> Don't think these checks of latest_gcm_id_mapping_succeeded() are useful - I
> think we should get rid of that API/member variable entirely, and just make
sure
> that a completed registration calls client::UpdateGcmIdMapping().

Done.

https://codereview.chromium.org/1258313002/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/1258313002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_client.cc:698:
callback.Run(status == DM_STATUS_SUCCESS);
On 2015/08/03 13:52:26, Andrew T Wilson wrote:
> Should we call NotifyClientError() and update status_ as we do on things like
> OnStatusUploadCompleted()? If not, why not?

I thought it's not essential. Both onClientError() and status() are policy fetch
failure oriented as I understood. Probably only user visible error should be
notified here. Anyhow, there is no harm adding the notification of failure here.

Done.

[jinzhang1, 2015-08-05 22:08:04]

https://codereview.chromium.org/1258313002/diff/60001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/1258313002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:1024: // Sent by the
client to server to update the mapping from GCM id to DM token
change to: "the mapping from GCM id to device_id on the server side"?

https://codereview.chromium.org/1258313002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:1137: // Update the GCM
id to DM token mapping.
change to "to device_id mapping"?

[Andrew T Wilson (Slow), 2015-08-06 09:40:03]

LGTM, if you can get rid of SetCloudPolicyClientForTesting()

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:393: void
HeartbeatScheduler::OnGcmIdMappingRequestSent(bool status) {
nit: rename status -> success to make it clear that |true| means "success"

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:239:
scheduler_.SetCloudPolicyClientForTesting(&cloud_policy_client);
Why do we need SetCloudPolicyClientForTesting()? Why can't we pass this in the
constructor for HeartbeatScheduler instead of nullptr?

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:247: 
nit:get->got

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:251:
EXPECT_FALSE(callback.is_null());
Should you invoke the callback to make sure the HeartbeatScheduler doesn't do
anything weird like crash here?

[binjin, 2015-08-06 10:19:15]

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:393: void
HeartbeatScheduler::OnGcmIdMappingRequestSent(bool status) {
On 2015/08/06 09:40:02, Andrew T Wilson wrote:
> nit: rename status -> success to make it clear that |true| means "success"

Done.

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:239:
scheduler_.SetCloudPolicyClientForTesting(&cloud_policy_client);
On 2015/08/06 09:40:02, Andrew T Wilson wrote:
> Why do we need SetCloudPolicyClientForTesting()? Why can't we pass this in the
> constructor for HeartbeatScheduler instead of nullptr?

Done. Use scoped_ptr for scheduler_ here.

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:247: 
On 2015/08/06 09:40:02, Andrew T Wilson wrote:
> nit:get->got

Done.

https://codereview.chromium.org/1258313002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:251:
EXPECT_FALSE(callback.is_null());
On 2015/08/06 09:40:02, Andrew T Wilson wrote:
> Should you invoke the callback to make sure the HeartbeatScheduler doesn't do
> anything weird like crash here?

Done.

https://codereview.chromium.org/1258313002/diff/60001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/1258313002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:1024: // Sent by the
client to server to update the mapping from GCM id to DM token
On 2015/08/05 22:08:04, jinzhang1 wrote:
> change to: "the mapping from GCM id to device_id on the server side"?

Done.

https://codereview.chromium.org/1258313002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:1137: // Update the GCM
id to DM token mapping.
On 2015/08/05 22:08:04, jinzhang1 wrote:
> change to "to device_id mapping"?

Done.

[Andrew T Wilson (Slow), 2015-08-06 11:17:06]

https://codereview.chromium.org/1258313002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1258313002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:64:
InitializeScheduler(nullptr);
why not set a mock cloud policy client here, then you don't have to reset it
down in SendGcmIdMappingUpdate()?

Better to have consistent setup for all tests.

[binjin, 2015-08-06 13:33:02]

https://codereview.chromium.org/1258313002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1258313002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:64:
InitializeScheduler(nullptr);
On 2015/08/06 11:17:06, Andrew T Wilson wrote:
> why not set a mock cloud policy client here, then you don't have to reset it
> down in SendGcmIdMappingUpdate()?
> 
> Better to have consistent setup for all tests.

Done.

[Andrew T Wilson (Slow), 2015-08-06 14:23:50]

lgtm

[binjin, 2015-08-06 14:26:36]

The CQ bit was checked by binjin@chromium.org

[binjin, 2015-08-06 14:26:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
jinzhang@chromium.org
Link to the patchset: https://codereview.chromium.org/1258313002/#ps140001
(title: "add accidentally removed comment")

[commit-bot: I haz the power, 2015-08-06 14:26:44]

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

[binjin, 2015-08-06 14:27:24]

The CQ bit was unchecked by binjin@chromium.org

[binjin, 2015-08-06 16:08:50]

The CQ bit was checked by binjin@chromium.org

[binjin, 2015-08-06 16:08:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
jinzhang@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1258313002/#ps160001
(title: "fix typo")

[commit-bot: I haz the power, 2015-08-06 16:09:08]

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

[commit-bot: I haz the power, 2015-08-06 17:00:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 17:00:56]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[binjin, 2015-08-06 17:05:02]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2015-08-06 17:05:24]

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

[commit-bot: I haz the power, 2015-08-06 18:30:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 18:30:11]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[binjin, 2015-08-06 18:33:22]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2015-08-06 18:35:22]

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

[commit-bot: I haz the power, 2015-08-06 20:59:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 20:59:02]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[binjin, 2015-08-06 21:03:01]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2015-08-06 21:04:09]

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

[commit-bot: I haz the power, 2015-08-06 22:59:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 22:59:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[binjin, 2015-08-07 00:17:19]

The CQ bit was checked by binjin@chromium.org

[binjin, 2015-08-07 00:17:56]

The CQ bit was unchecked by binjin@chromium.org

[binjin, 2015-08-07 13:18:52]

The CQ bit was checked by binjin@chromium.org

[binjin, 2015-08-07 13:18:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
jinzhang@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1258313002/#ps180001
(title: "adopt upstream protobuf changes")

[commit-bot: I haz the power, 2015-08-07 13:19:11]

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

[commit-bot: I haz the power, 2015-08-07 14:37:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 14:37:04]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[binjin, 2015-08-07 14:40:41]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2015-08-07 14:40:55]

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

[commit-bot: I haz the power, 2015-08-07 16:15:46]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-08-07 16:16:55]

Patchset 10 (id:??) landed as
https://crrev.com/92960c61ee75a5323fc954b9819acca5b684d2aa
Cr-Commit-Position: refs/heads/master@{#342364}

[binjin, 2015-08-07 20:44:00]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1276853003/ by binjin@chromium.org.

The reason for reverting is: protobuf change on server side..

[binjin, 2015-08-11 17:04:31]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2015-08-11 17:05:14]

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

[commit-bot: I haz the power, 2015-08-11 18:27:53]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-08-11 18:28:38]

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