GCM Checkin implementation with unit tests and protobufs

GCM Checkin implementation with unit tests.

BUG=284553
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243729

[fgorski, 2014-01-04 00:54:07]

There are a few includes and fields that I need to clean up, but I wanted to get
your opinion early.

Thanks.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:7: #include
"base/values.h"
I probably don't need that.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:45:
GCMClient::CheckInInfo user_checkin_info_;
I'll remove that.

[jianli, 2014-01-04 01:34:09]

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request.cc (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.cc:28: const char
kRequestDataFormat[] = "application/x-protobuf";
nit: kRequestContentType

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.cc:98: response_proto.android_id() <=
0) {
android_id is uint64. So the check should be == 0.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.h:37: base::Callback<void(uint64,
uint64)> callback_;
Please comment on the callback parameters.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:23: const int
kExpectedDeviceType = 4;
Can we use the enum defined in the proto file?

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:42: int64 android_id_;
Should we use uint64?

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:89:
TEST_F(GCMCheckinRequestTest, EmptyResponseBody) {
Do we also need to have the test coverage for the following cases:
1) Non-200 HTTP response code, say 401.
2) Corrupted response data.
3) Empty android id or security token.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:138:
TEST_F(GCMCheckinRequestTest, SuccessfulCheckin) {
Is this to test subsequent checkin? If so, better call it as
SuccessfulSubsequentCheckIn.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/gcm_client.h
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/gcm_clien...
google_apis/gcm/gcm_client.h:81: struct GCM_EXPORT ChromeCheckInInfo {
ChromeCheckInInfo can be easily confused with CheckInInfo. This is indeed the
Chrome info passed to the check-in request. Why not just simply using
ChromeBuildProto to pass the info?

[Nicolas Zea, 2014-01-04 01:40:19]

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request.cc (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.cc:63: new
checkin_proto::AndroidCheckinProto();
you can just do request->mutable_checkin_proto and
mutable_allocated_chrome_build instead of allocating new ones.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.cc:92: callback_.Run(0, 0);
I wonder if we should have the retry logic built in to this class, such that
calling start won't end until it succeeds or you call Stop.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.cc:99: LOG(ERROR) << "Badly formatted
checkin response.";
Will these always be there on non-first time checkins?

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.h:22: // TODO(zea): support non-first
time checkin.
is this todo still needed?

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.h:24: net::URLRequestContextGetter*
request_context_getter,
nit: make request context getter last param (input/output ordering)

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:97: request.Start();
nit: newline after (to match other tests)

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:148: request.Start();
nit: newline after (to match other tests)

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
File google_apis/gcm/protocol/android_checkin.proto (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
google_apis/gcm/protocol/android_checkin.proto:16: // option
(datapol.msg_semantic_type) = ST_SOFTWARE_ID;
remove this?

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
google_apis/gcm/protocol/android_checkin.proto:49: // option
(logs_proto.msg_details) = {
remove this, and other internal/unused fields or comments too

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
google_apis/gcm/protocol/android_checkin.proto:61: optional
checkin_proto.ChromeBuildProto chrome_build = 13;
nit: have fields ordered numerically? so chrome_build and ios_build should be
further down and reversed in order

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
google_apis/gcm/protocol/android_checkin.proto:68: // Time (Java epoch) of the
device's last successful checkin.
s/Java epoch/Millseconds since the Unix epoch

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
google_apis/gcm/protocol/android_checkin.proto:110: 
nit: remove newline

[fgorski, 2014-01-06 20:34:12]

PTAL.

Nicolas, I think android_id should be of type int64. Is there a reason we are
using uint64 right now, except for an accident?

I am ready to go and clean the situation up a bit if we decide to do that.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request.cc (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.cc:28: const char
kRequestDataFormat[] = "application/x-protobuf";
On 2014/01/04 01:34:10, jianli wrote:
> nit: kRequestContentType

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.cc:63: new
checkin_proto::AndroidCheckinProto();
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> you can just do request->mutable_checkin_proto and
> mutable_allocated_chrome_build instead of allocating new ones.

Yep. That is my first time manipulating complex protobufs :)
Updated.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.cc:92: callback_.Run(0, 0);
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> I wonder if we should have the retry logic built in to this class, such that
> calling start won't end until it succeeds or you call Stop.

Let's add that in a separate step.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.cc:98: response_proto.android_id() <=
0) {
On 2014/01/04 01:34:10, jianli wrote:
> android_id is uint64. So the check should be == 0.

As mentioned elsewhere, I suspect it actually is int64. I'll check with Nicolas
and update everywhere, if needed.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.cc:99: LOG(ERROR) << "Badly formatted
checkin response.";
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> Will these always be there on non-first time checkins?

What do you mean. Even if it is the first time check in, the response should
carry a non-zero id and security token, shouldn't it?

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.h:22: // TODO(zea): support non-first
time checkin.
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> is this todo still needed?

Done. Dropped the TODO.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.h:24: net::URLRequestContextGetter*
request_context_getter,
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> nit: make request context getter last param (input/output ordering)

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request.h:37: base::Callback<void(uint64,
uint64)> callback_;
On 2014/01/04 01:34:10, jianli wrote:
> Please comment on the callback parameters.

Done. That is one of the places where we need an update.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:7: #include
"base/values.h"
On 2014/01/04 00:54:07, Filip Gorski wrote:
> I probably don't need that.

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:23: const int
kExpectedDeviceType = 4;
On 2014/01/04 01:34:10, jianli wrote:
> Can we use the enum defined in the proto file?

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:42: int64 android_id_;
On 2014/01/04 01:34:10, jianli wrote:
> Should we use uint64?

No, actually I've not found a protobuf that would make that value uint64 instead
of int64. I'll gladly update all of the other usages... Thoughts?

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:45:
GCMClient::CheckInInfo user_checkin_info_;
On 2014/01/04 00:54:07, Filip Gorski wrote:
> I'll remove that.

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:97: request.Start();
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> nit: newline after (to match other tests)

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:138:
TEST_F(GCMCheckinRequestTest, SuccessfulCheckin) {
On 2014/01/04 01:34:10, jianli wrote:
> Is this to test subsequent checkin? If so, better call it as
> SuccessfulSubsequentCheckIn.

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:148: request.Start();
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> nit: newline after (to match other tests)

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/gcm_client.h
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/gcm_clien...
google_apis/gcm/gcm_client.h:81: struct GCM_EXPORT ChromeCheckInInfo {
On 2014/01/04 01:34:10, jianli wrote:
> ChromeCheckInInfo can be easily confused with CheckInInfo. This is indeed the
> Chrome info passed to the check-in request. Why not just simply using
> ChromeBuildProto to pass the info?

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
File google_apis/gcm/protocol/android_checkin.proto (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
google_apis/gcm/protocol/android_checkin.proto:16: // option
(datapol.msg_semantic_type) = ST_SOFTWARE_ID;
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> remove this?

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
google_apis/gcm/protocol/android_checkin.proto:49: // option
(logs_proto.msg_details) = {
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> remove this, and other internal/unused fields or comments too

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
google_apis/gcm/protocol/android_checkin.proto:61: optional
checkin_proto.ChromeBuildProto chrome_build = 13;
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> nit: have fields ordered numerically? so chrome_build and ios_build should be
> further down and reversed in order

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
google_apis/gcm/protocol/android_checkin.proto:68: // Time (Java epoch) of the
device's last successful checkin.
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> s/Java epoch/Millseconds since the Unix epoch

Done.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/protocol/...
google_apis/gcm/protocol/android_checkin.proto:110: 
On 2014/01/04 01:40:19, Nicolas Zea wrote:
> nit: remove newline

Done.

[jianli, 2014-01-06 21:00:00]

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:89:
TEST_F(GCMCheckinRequestTest, EmptyResponseBody) {
On 2014/01/04 01:34:10, jianli wrote:
> Do we also need to have the test coverage for the following cases:
> 1) Non-200 HTTP response code, say 401.
> 2) Corrupted response data.
> 3) Empty android id or security token.

???

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:138:
TEST_F(GCMCheckinRequestTest, SuccessfulCheckin) {
On 2014/01/06 20:34:12, Filip Gorski wrote:
> On 2014/01/04 01:34:10, jianli wrote:
> > Is this to test subsequent checkin? If so, better call it as
> > SuccessfulSubsequentCheckIn.
> 
> Done.

It is still the old name in your new patch.

https://codereview.chromium.org/98173009/diff/200001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_checkin_request.cc (right):

https://codereview.chromium.org/98173009/diff/200001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_checkin_request.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/98173009/diff/200001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/200001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_checkin_request.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
2014

[fgorski, 2014-01-06 21:58:03]

Added tests.

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
File google_apis/gcm/engine/gcm_checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/70001/google_apis/gcm/engine/gc...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:89:
TEST_F(GCMCheckinRequestTest, EmptyResponseBody) {
On 2014/01/06 21:00:01, jianli wrote:
> On 2014/01/04 01:34:10, jianli wrote:
> > Do we also need to have the test coverage for the following cases:
> > 1) Non-200 HTTP response code, say 401.
Done.
> > 2) Corrupted response data.
Done.
> > 3) Empty android id or security token.
Tested with both missing values and with values equal to 0, assuming that that
should not happen.
> 
> ???

https://codereview.chromium.org/98173009/diff/200001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_checkin_request.cc (right):

https://codereview.chromium.org/98173009/diff/200001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_checkin_request.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2014/01/06 21:00:01, jianli wrote:
> 2014

Done.

https://codereview.chromium.org/98173009/diff/200001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/200001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_checkin_request.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2014/01/06 21:00:01, jianli wrote:
> 2014

Done.

[jianli, 2014-01-06 22:26:33]

https://codereview.chromium.org/98173009/diff/310001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/310001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_checkin_request.h:21: class GCM_EXPORT
GCMCheckinRequest : public net::URLFetcherDelegate {
Name the class as GCMCheckInRequest to be consistent with CheckIn used in
GCMClient.

Also, it might be simpler to call it CheckInRequest.

https://codereview.chromium.org/98173009/diff/310001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/310001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:49:
net::TestURLFetcherFactory factory_;
nit: url_factory_factory_

https://codereview.chromium.org/98173009/diff/310001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:50:
scoped_refptr<net::TestURLRequestContextGetter> getter_;
nit: getter_ is too short to convey the meaning. Prefer something like
url_request_context_getter_.

[fgorski, 2014-01-06 23:15:54]

PTAL.

android_id issue solved;
classes renamed.

https://codereview.chromium.org/98173009/diff/310001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/310001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_checkin_request.h:21: class GCM_EXPORT
GCMCheckinRequest : public net::URLFetcherDelegate {
On 2014/01/06 22:26:33, jianli wrote:
> Name the class as GCMCheckInRequest to be consistent with CheckIn used in
> GCMClient.
> 
> Also, it might be simpler to call it CheckInRequest.

Done. By updating the name to CheckinRequest, and naming of CheckInInfo to
CheckinInfo.

As we agreed, CheckIn function and CheckInFinished stay with capitalized In.

https://codereview.chromium.org/98173009/diff/310001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/310001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:49:
net::TestURLFetcherFactory factory_;
On 2014/01/06 22:26:33, jianli wrote:
> nit: url_factory_factory_

Done.

https://codereview.chromium.org/98173009/diff/310001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_checkin_request_unittest.cc:50:
scoped_refptr<net::TestURLRequestContextGetter> getter_;
On 2014/01/06 22:26:33, jianli wrote:
> nit: getter_ is too short to convey the meaning. Prefer something like
> url_request_context_getter_.

Done.

[jianli, 2014-01-06 23:28:43]

lgtm

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request_unittest.cc:235:
TEST_F(CheckinRequestTest, ResponseValueOfAndroidIdIsZero) {
nit: ResponseContainingZeroAndroidId

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request_unittest.cc:254:
TEST_F(CheckinRequestTest, ResponseValueOfSecurityTokenIsZero) {
nit: ditto

[Nicolas Zea, 2014-01-07 00:41:51]

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_client_mock.h (right):

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_client_mock.h:58: void
CheckInFinished(std::string username, CheckinInfo checkin_info);
nit: this, and all these other params should be passed by const ref. I should
have caught that the first time around.

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.cc:192: void
SetCheckinInfo(GCMClient::CheckinInfo checkin_info);
const ref

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.h:143: void
CheckInFinished(GCMClient::CheckinInfo checkin_info,
const ref

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request.cc (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.cc:55: checkin->mutable_chrome_build();
can't you actually just do
checkin->mutable_chrome_build()->CopyFrom(chrome_build_proto)?

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.h:21: class GCM_EXPORT CheckinRequest :
public net::URLFetcherDelegate {
Add comment describing the purpose of this class

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.h:30: const GCMClient::CheckinInfo&
checkin_info,
It's a bit weird that a CheckinRequest takes a CheckinInfo as an input.
Considering that CheckinInfo might eventually be expanded to include the various
settings/fields the checkin response returns, and we're really only using it to
pass the id/token, perhaps just pass those explicitly?

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request_unittest.cc:40: void
SetResponseMissingValue(net::TestURLFetcher* fetcher,
Can you just have a single SetResponse(ValueState value_state), where ValueStat
is one of MISSING_ANDROID_ID, MISSING_SECURITY_TOKEN, MISSING_NONE? It seems
like fetcher_ can be a protected member variable, and the same android id and
security token are used everywhere.

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:160: // |username|: the username (email address)
passed in |CheckIn|.
nit: typically the ||'s are used for variables/expressions. In this case it's
just a function name, so maybe right it as "pass in CheckIn(..)".

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/protocol...
File google_apis/gcm/protocol/android_checkin.proto (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/protocol...
google_apis/gcm/protocol/android_checkin.proto:48: // optional
android.AndroidBuildProto build = 1;
I think we should go ahead and clean up all these commented out fields (possibly
just with a comment saying they're not used).

[fgorski, 2014-01-07 23:46:43]

Applied feedback, PTAL

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_client_mock.h (right):

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_client_mock.h:58: void
CheckInFinished(std::string username, CheckinInfo checkin_info);
On 2014/01/07 00:41:52, Nicolas Zea wrote:
> nit: this, and all these other params should be passed by const ref. I should
> have caught that the first time around.

Added a TODO

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.cc:192: void
SetCheckinInfo(GCMClient::CheckinInfo checkin_info);
On 2014/01/07 00:41:52, Nicolas Zea wrote:
> const ref

Added TODO.

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/98173009/diff/400001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.h:143: void
CheckInFinished(GCMClient::CheckinInfo checkin_info,
On 2014/01/07 00:41:52, Nicolas Zea wrote:
> const ref

Added a TODO.

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request.cc (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.cc:55: checkin->mutable_chrome_build();
On 2014/01/07 00:41:52, Nicolas Zea wrote:
> can't you actually just do
> checkin->mutable_chrome_build()->CopyFrom(chrome_build_proto)?

Done.

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.h:21: class GCM_EXPORT CheckinRequest :
public net::URLFetcherDelegate {
On 2014/01/07 00:41:52, Nicolas Zea wrote:
> Add comment describing the purpose of this class

Done.

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.h:30: const GCMClient::CheckinInfo&
checkin_info,
On 2014/01/07 00:41:52, Nicolas Zea wrote:
> It's a bit weird that a CheckinRequest takes a CheckinInfo as an input.
> Considering that CheckinInfo might eventually be expanded to include the
various
> settings/fields the checkin response returns, and we're really only using it
to
> pass the id/token, perhaps just pass those explicitly?

Done.

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request_unittest.cc (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request_unittest.cc:40: void
SetResponseMissingValue(net::TestURLFetcher* fetcher,
On 2014/01/07 00:41:52, Nicolas Zea wrote:
> Can you just have a single SetResponse(ValueState value_state), where
ValueStat
> is one of MISSING_ANDROID_ID, MISSING_SECURITY_TOKEN, MISSING_NONE? It seems
> like fetcher_ can be a protected member variable, and the same android id and
> security token are used everywhere.

I actually dealt without ever storing the fetcher. I can query the factory to
get it every time. I did extend the enum with response to cover 5 options.
(Called it ResponseScenario)

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request_unittest.cc:235:
TEST_F(CheckinRequestTest, ResponseValueOfAndroidIdIsZero) {
On 2014/01/06 23:28:43, jianli wrote:
> nit: ResponseContainingZeroAndroidId

Done.

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request_unittest.cc:254:
TEST_F(CheckinRequestTest, ResponseValueOfSecurityTokenIsZero) {
On 2014/01/06 23:28:43, jianli wrote:
> nit: ditto

Done.

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:160: // |username|: the username (email address)
passed in |CheckIn|.
On 2014/01/07 00:41:52, Nicolas Zea wrote:
> nit: typically the ||'s are used for variables/expressions. In this case it's
> just a function name, so maybe right it as "pass in CheckIn(..)".

Done.

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/protocol...
File google_apis/gcm/protocol/android_checkin.proto (right):

https://codereview.chromium.org/98173009/diff/400001/google_apis/gcm/protocol...
google_apis/gcm/protocol/android_checkin.proto:48: // optional
android.AndroidBuildProto build = 1;
On 2014/01/07 00:41:52, Nicolas Zea wrote:
> I think we should go ahead and clean up all these commented out fields
(possibly
> just with a comment saying they're not used).

Done.

[Nicolas Zea, 2014-01-08 19:53:36]

LGTM with nits

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.h:20: // Enables making check-in requests
with the GCM infrastructure. When call with
nit: when call -> when called

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.h:28: typedef base::Callback<void(uint64,
uint64)> CheckinRequestCallback;
nit: I find it useful to include parameter names in callback definition like
this, so it's clear the first uint64 is the android id, and the second is the
security token.

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/protocol...
File google_apis/gcm/protocol/checkin.proto (right):

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/protocol...
google_apis/gcm/protocol/checkin.proto:6: //
nit: remove this line

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/protocol...
google_apis/gcm/protocol/checkin.proto:16: // import
"wireless/android/proto/device_configuration.proto";
this too

[fgorski, 2014-01-08 21:26:39]

Updated all. Thanks for review. CQ will try the patch.

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request.h (right):

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.h:20: // Enables making check-in requests
with the GCM infrastructure. When call with
On 2014/01/08 19:53:36, Nicolas Zea wrote:
> nit: when call -> when called

Done.

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.h:28: typedef base::Callback<void(uint64,
uint64)> CheckinRequestCallback;
On 2014/01/08 19:53:36, Nicolas Zea wrote:
> nit: I find it useful to include parameter names in callback definition like
> this, so it's clear the first uint64 is the android id, and the second is the
> security token.

Done.

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/protocol...
File google_apis/gcm/protocol/checkin.proto (right):

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/protocol...
google_apis/gcm/protocol/checkin.proto:6: //
On 2014/01/08 19:53:36, Nicolas Zea wrote:
> nit: remove this line

Done.

https://codereview.chromium.org/98173009/diff/700001/google_apis/gcm/protocol...
google_apis/gcm/protocol/checkin.proto:16: // import
"wireless/android/proto/device_configuration.proto";
On 2014/01/08 19:53:36, Nicolas Zea wrote:
> this too

Done.

[commit-bot: I haz the power, 2014-01-08 21:31:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/98173009/790001

[commit-bot: I haz the power, 2014-01-08 22:44:27]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-08 23:40:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/98173009/790001

[commit-bot: I haz the power, 2014-01-09 01:37:13]

Change committed as 243729