Support getting and deleting token for Instance ID.

Support getting and deleting token for Instance ID.

GCM's registration and unregistration request have been refactored to
support token requests for Instance ID.

BUG=477084
TEST=new tests

Committed: https://crrev.com/7a0c9b6d322d0a501fc114d3854df944fc059f0f
Cr-Commit-Position: refs/heads/master@{#331475}

[jianli, 2015-05-13 01:17:58]

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

[jianli, 2015-05-13 01:24:57]

Patchset #2 (id:20001) has been deleted

[fgorski, 2015-05-13 18:32:41]

https://codereview.chromium.org/1137463003/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js
(right):

https://codereview.chromium.org/1137463003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js:113:
var oldToken;
put this inside of the test.

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

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:359: if (registration.get())
else?

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:785: else if (status ==
RegistrationRequest::INVALID_SENDER)
Are there any new cases on the server side? Could you check with costin?

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:947:
GCMClientImpl::BuildUnregistrationRequestInfo(
Looks like this code belongs in registration_info (and it's specializations)

applies above as well.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1140: registered = true;
surround with {} because of multi-line condition

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1197:
!ExistsGCMRegistrationInMap(registrations_, kGCMAccountMapperAppId);
Where is that defined?

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop.cc:883: const std::string&
registration_id,
nit: we could just call it token

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop.cc:1102: const TokenTuple& a, const
TokenTuple& b) const {
pretty cool

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_driver_unittest.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_driver_unittest.cc:318: 
add second token and verify that it is still there, unchanged, when deleting the
first one.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:22: switch (result) {
ASYNC_OPERATION_PENDING?

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
File components/gcm_driver/registration_info.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:32: registration.reset();
surround with {}

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:92: DCHECK(registration_id.find('=')
== std::string::npos);
are we sure that is the case?

Even if it had =, when deserializing we only care about the first one, the rest
don't need to be considered as separators.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:98: bool
GCMRegistrationInfo::Deserialize(
Will this deserialize what we have in store today?

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:181: std::string* registration_id) {
nit: token?

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:194: return false;
{}

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:225: // !iid_a && iid_b => true.
add comment: GCM registrationID sort before instance IDs.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:238: return iid_a->scope >
iid_b->scope;
why not <, which would be natural way of doing it?

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
File components/gcm_driver/registration_info.h (right):

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.h:40: // registration info are written
as key. The remaining of the registration
is written as key. or change part to parts

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.h:41: // info plus the registration ID
are writtem as value.
written

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.h:45: // |registration_id| can be NULL
if no interest to it.
I don't understand the comment.

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:94: // GCM token request.
Instance ID token request

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/registration_request_unittest.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request_unittest.cc:105: void
RegistrationRequestTest::CreateRequest(const std::string& sender_ids) {
How about adding tests for Instance ID specific code?

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.cc:123:
BuildFormEncoding(kAppIdKey, app_id, body);
does app id need to be present in both cases?
I thought it is replaced by instance ID when it is available. Please confirm and
check the same for registration request

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.h (right):

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.h:85: // GCM token request.
Instance ID token request

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request_unittest.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request_unittest.cc:93: void
UnregistrationRequestTest::CreateRequest() {
add tests for instance id path.

[jianli, 2015-05-13 22:42:57]

https://codereview.chromium.org/1137463003/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js
(right):

https://codereview.chromium.org/1137463003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js:113:
var oldToken;
On 2015/05/13 18:32:38, fgorski wrote:
> put this inside of the test.

I can't do this because oldToken are referred from 2 different places.

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

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:359: if (registration.get())
On 2015/05/13 18:32:38, fgorski wrote:
> else?

If we can get registration, it means that either the store data were corrupted
or something wrong with our parsing code. Added a TODO for UMA.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:785: else if (status ==
RegistrationRequest::INVALID_SENDER)
On 2015/05/13 18:32:38, fgorski wrote:
> Are there any new cases on the server side? Could you check with costin?

Confirmed that no new cases.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:947:
GCMClientImpl::BuildUnregistrationRequestInfo(
On 2015/05/13 18:32:39, fgorski wrote:
> Looks like this code belongs in registration_info (and it's specializations)
> 
> applies above as well.

To move these codes into registration_info, we will have to include
registration_request.h and unregistration_request.h that are located under
google_apis/gcm. Since registration_info now lives under components/gcm_driver
and though it can include header file from google_apis/gcm, I still want to
avoid doing this in order not to pollute clean definition in
registration_info.h. I rather include all these google_apis header files in
gcm_client_impl layer.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1140: registered = true;
On 2015/05/13 18:32:38, fgorski wrote:
> surround with {} because of multi-line condition

Done.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1197:
!ExistsGCMRegistrationInMap(registrations_, kGCMAccountMapperAppId);
On 2015/05/13 18:32:39, fgorski wrote:
> Where is that defined?

In registration_info.h

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop.cc:883: const std::string&
registration_id,
On 2015/05/13 18:32:39, fgorski wrote:
> nit: we could just call it token

Done.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop.cc:1102: const TokenTuple& a, const
TokenTuple& b) const {
On 2015/05/13 18:32:39, fgorski wrote:
> pretty cool

Acknowledged.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_driver_unittest.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_driver_unittest.cc:318: 
On 2015/05/13 18:32:39, fgorski wrote:
> add second token and verify that it is still there, unchanged, when deleting
the
> first one.

Done.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:22: switch (result) {
On 2015/05/13 18:32:39, fgorski wrote:
> ASYNC_OPERATION_PENDING?

Done.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
File components/gcm_driver/registration_info.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/05/13 18:32:39, fgorski wrote:
> 2015

Done.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:32: registration.reset();
On 2015/05/13 18:32:40, fgorski wrote:
> surround with {}

Done.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:92: DCHECK(registration_id.find('=')
== std::string::npos);
On 2015/05/13 18:32:39, fgorski wrote:
> are we sure that is the case?
> 
> Even if it had =, when deserializing we only care about the first one, the
rest
> don't need to be considered as separators.

I don't think registration ID will contain '='. But you're right, we probably
don't need to care about this. Removed the check.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:98: bool
GCMRegistrationInfo::Deserialize(
On 2015/05/13 18:32:40, fgorski wrote:
> Will this deserialize what we have in store today?

Yes. We still serialize and deserialize GCM registration record same as before:
app_id => (sender1,sender2,...=reg_id).

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:181: std::string* registration_id) {
On 2015/05/13 18:32:39, fgorski wrote:
> nit: token?

I rather still call it registration_id since it is named like this in the base
class.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:194: return false;
On 2015/05/13 18:32:40, fgorski wrote:
> {}

Done.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:225: // !iid_a && iid_b => true.
On 2015/05/13 18:32:40, fgorski wrote:
> add comment: GCM registrationID sort before instance IDs.

Done.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.cc:238: return iid_a->scope >
iid_b->scope;
On 2015/05/13 18:32:40, fgorski wrote:
> why not <, which would be natural way of doing it?

It should be <. Sorry for my typo.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
File components/gcm_driver/registration_info.h (right):

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.h:40: // registration info are written
as key. The remaining of the registration
On 2015/05/13 18:32:40, fgorski wrote:
> is written as key. or change part to parts

Done.

https://codereview.chromium.org/1137463003/diff/40001/components/gcm_driver/r...
components/gcm_driver/registration_info.h:41: // info plus the registration ID
are writtem as value.
On 2015/05/13 18:32:40, fgorski wrote:
> written

Done.

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:94: // GCM token request.
On 2015/05/13 18:32:41, fgorski wrote:
> Instance ID token request

Done.

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/registration_request_unittest.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request_unittest.cc:105: void
RegistrationRequestTest::CreateRequest(const std::string& sender_ids) {
On 2015/05/13 18:32:41, fgorski wrote:
> How about adding tests for Instance ID specific code?

Done.

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.cc:123:
BuildFormEncoding(kAppIdKey, app_id, body);
On 2015/05/13 18:32:41, fgorski wrote:
> does app id need to be present in both cases?
> I thought it is replaced by instance ID when it is available. Please confirm
and
> check the same for registration request

I confirmed with Costin that this was still required since it was used for
routing purpose.

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.h (right):

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.h:85: // GCM token request.
On 2015/05/13 18:32:41, fgorski wrote:
> Instance ID token request

Done.

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request_unittest.cc (right):

https://codereview.chromium.org/1137463003/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request_unittest.cc:93: void
UnregistrationRequestTest::CreateRequest() {
On 2015/05/13 18:32:41, fgorski wrote:
> add tests for instance id path.

Done.

[jianli, 2015-05-13 22:48:04]

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

[jianli, 2015-05-13 22:48:05]

zea, could you please also take a look? thanks.

[Nicolas Zea, 2015-05-14 05:02:47]

First pass

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store.h (right):

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store.h:85: virtual void AddRegistration(const
std::string& serialized_key,
Is registration id being completely deprecated, or should we keep it around in
addition to the registration id? What about existing apps with a registration
id?

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store_impl.cc:432: std::string key =
MakeRegistrationKey(serialized_key);
Should this be using a new leveldb key prefix?

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store_impl.cc:435:
MakeSlice(MakeRegistrationKey(serialized_key)),
reuse |key|?

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:63: struct GCM_EXPORT RequestInfo
{
style: Structs should only be for passive data containers. Does this need to
have virtual methods? Can they just be helpers defined outside the struct?
Otherwise this should be a class (same with the method below)

[jianli, 2015-05-14 19:18:14]

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store.h (right):

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store.h:85: virtual void AddRegistration(const
std::string& serialized_key,
On 2015/05/14 05:02:47, Nicolas Zea wrote:
> Is registration id being completely deprecated, or should we keep it around in
> addition to the registration id? What about existing apps with a registration
> id?

The GCM registration IDs are still around, side by side with InstanceID tokens.
Both GCM registration records and InstanceID token records are saved together
because the latter is indeed a new way of registration. The GCM registrations
are still persisted same as before without the format change: app_id as key and
sender1,...=registration_id as value. For new InstanceID tokens, they will be
saved with iid-app_id,authorized_entity,scope as key and token as value, where
"iid-" is the special prefix to differentiate from the GCM registrations.

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store_impl.cc:432: std::string key =
MakeRegistrationKey(serialized_key);
On 2015/05/14 05:02:47, Nicolas Zea wrote:
> Should this be using a new leveldb key prefix?

We don't want to do that since we still want to make GCM registrations work same
as before. Here registrations mean both GCM registrations and InstanceID tokens.

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store_impl.cc:435:
MakeSlice(MakeRegistrationKey(serialized_key)),
On 2015/05/14 05:02:47, Nicolas Zea wrote:
> reuse |key|?

Done.

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/80001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:63: struct GCM_EXPORT RequestInfo
{
On 2015/05/14 05:02:47, Nicolas Zea wrote:
> style: Structs should only be for passive data containers. Does this need to
> have virtual methods? Can they just be helpers defined outside the struct?
> Otherwise this should be a class (same with the method below)

I think virtual methods are better to understand and more easily to maintain
than helper functions. I converted this to class.

[Nicolas Zea, 2015-05-15 20:37:01]

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
File google_apis/gcm/engine/gcm_store.h (right):

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
google_apis/gcm/engine/gcm_store.h:85: virtual void AddRegistration(const
std::string& serialized_key,
Does it make sense to just pass in something equivalent to a RegistrationInfo,
which encapsulate both the app id/sender id or instance id information? Having
this be an arbitrary key/value string pair seems too generic.

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:68: virtual void
BuildRequestHeaders(std::string* extra_headers);
Style guide prefers either doing pure inheritance (e.g. interfaces) or using
composition instead.

How about having the RequestInfo contain a the android id, security token,
chrome version, and app id, and then also a RequestId member. The RequestId
class would be a purely virtual class with two implementations like you have
below (and also any id implementation specific helper methods).

In some sense though, I think it might make sense to keep a RegistrationId class
around that can encapsulate ID information in general. See my comment about how
we're persisting it into the gcm store for example.

[fgorski, 2015-05-18 14:30:44]

LGTM with one comment. Please address comments from Nicolas.

https://codereview.chromium.org/1137463003/diff/140001/components/gcm_driver/...
File components/gcm_driver/registration_info.h (right):

https://codereview.chromium.org/1137463003/diff/140001/components/gcm_driver/...
components/gcm_driver/registration_info.h:45: // |registration_id| can be NULL
if no interest to it.
if it is of no interest to the caller.

[jianli, 2015-05-21 00:41:15]

https://codereview.chromium.org/1137463003/diff/140001/components/gcm_driver/...
File components/gcm_driver/registration_info.h (right):

https://codereview.chromium.org/1137463003/diff/140001/components/gcm_driver/...
components/gcm_driver/registration_info.h:45: // |registration_id| can be NULL
if no interest to it.
On 2015/05/18 14:30:44, fgorski wrote:
> if it is of no interest to the caller.

Done.

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
File google_apis/gcm/engine/gcm_store.h (right):

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
google_apis/gcm/engine/gcm_store.h:85: virtual void AddRegistration(const
std::string& serialized_key,
On 2015/05/15 20:37:01, Nicolas Zea wrote:
> Does it make sense to just pass in something equivalent to a RegistrationInfo,
> which encapsulate both the app id/sender id or instance id information? Having
> this be an arbitrary key/value string pair seems too generic.

We've already passed a serialized string from store front-end to back-end for
registrations to workaround the bug that linked_ptr is not thread-safe.  We
could try to bring something equivalent to RegistrationInfo, but we need to make
a couple copies which are not efficient. So it seems to me that it might be
better to keep serialization/deserliazation logic in GCMClient layer, instead of
making GCMStore more complicated.

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:68: virtual void
BuildRequestHeaders(std::string* extra_headers);
On 2015/05/15 20:37:01, Nicolas Zea wrote:
> Style guide prefers either doing pure inheritance (e.g. interfaces) or using
> composition instead.
> 
> How about having the RequestInfo contain a the android id, security token,
> chrome version, and app id, and then also a RequestId member. The RequestId
> class would be a purely virtual class with two implementations like you have
> below (and also any id implementation specific helper methods).
> 
> In some sense though, I think it might make sense to keep a RegistrationId
class
> around that can encapsulate ID information in general. See my comment about
how
> we're persisting it into the gcm store for example.

With more thought, I decided to make RequestInfo still defined as struct without
introducing any virtual methods. All Build*** methods have been moved into
RegistrationRequest. We now have GCMRegistrationRequest and
InstanceIDGetTokenRequest that derive from RegistrationRequest, which will
implement the different request building and response parsing logic.

[Nicolas Zea, 2015-05-21 21:07:52]

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
File google_apis/gcm/engine/gcm_store.h (right):

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
google_apis/gcm/engine/gcm_store.h:85: virtual void AddRegistration(const
std::string& serialized_key,
On 2015/05/21 00:41:15, jianli wrote:
> On 2015/05/15 20:37:01, Nicolas Zea wrote:
> > Does it make sense to just pass in something equivalent to a
RegistrationInfo,
> > which encapsulate both the app id/sender id or instance id information?
Having
> > this be an arbitrary key/value string pair seems too generic.
> 
> We've already passed a serialized string from store front-end to back-end for
> registrations to workaround the bug that linked_ptr is not thread-safe.  We
> could try to bring something equivalent to RegistrationInfo, but we need to
make
> a couple copies which are not efficient. So it seems to me that it might be
> better to keep serialization/deserliazation logic in GCMClient layer, instead
of
> making GCMStore more complicated.

Fair enough. Could we give these more precise names though? For example keep
app_id and make serialized_value serialized_registration?

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/base/g...
File google_apis/gcm/base/gcm_util.h (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/base/g...
google_apis/gcm/base/gcm_util.h:12: void BuildFormEncoding(const std::string&
key,
Comment what this is for

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
File google_apis/gcm/engine/instance_id_get_token_request.cc (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/instance_id_get_token_request.cc:62:
DCHECK(!extra_request_info_.instance_id.empty() &&
These should probably be in ExtraRequestInfo constructor, rather than here or in
addition to (and same with other registration requests).

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.cc:81: app_id(app_id) {
Maybe DCHECK that the values are set?

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:62: // Encapsulates the common
info about a registration/token request.
Are all parameters mandatory?

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:93: virtual void
BuildRequestHeaders(std::string* extra_headers);
Although I like the approach of injecting extra request headers that the request
consumes, I still think it would be cleaner to build the request such that you
don't need to overload these Build/Parse/Get methods and use a composition
approach instead.

How about just having callback plumbed in for the Build method here (it's always
called after the native build method I think), and having the source be a string
that's passed in at construction time? It doesn't need to be calculated
repeatedly right?

A similar approach could I think be taken for UnregistrationRequest.

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
File google_apis/gcm/engine/unregistration_request.h (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.h:89: virtual void
BuildRequestHeaders(std::string* extra_headers);
Is anything overriding this?

[jianli, 2015-05-21 23:11:25]

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
File google_apis/gcm/engine/gcm_store.h (right):

https://codereview.chromium.org/1137463003/diff/140001/google_apis/gcm/engine...
google_apis/gcm/engine/gcm_store.h:85: virtual void AddRegistration(const
std::string& serialized_key,
On 2015/05/21 21:07:51, Nicolas Zea wrote:
> On 2015/05/21 00:41:15, jianli wrote:
> > On 2015/05/15 20:37:01, Nicolas Zea wrote:
> > > Does it make sense to just pass in something equivalent to a
> RegistrationInfo,
> > > which encapsulate both the app id/sender id or instance id information?
> Having
> > > this be an arbitrary key/value string pair seems too generic.
> > 
> > We've already passed a serialized string from store front-end to back-end
for
> > registrations to workaround the bug that linked_ptr is not thread-safe.  We
> > could try to bring something equivalent to RegistrationInfo, but we need to
> make
> > a couple copies which are not efficient. So it seems to me that it might be
> > better to keep serialization/deserliazation logic in GCMClient layer,
instead
> of
> > making GCMStore more complicated.
> 
> Fair enough. Could we give these more precise names though? For example keep
> app_id and make serialized_value serialized_registration?

I can't just make app_id key for registrations since it does not work for
persisting InstanceID token records. For each app, there could be multiple token
records that are identified by pair of authorized entity and scope. So the key
to store InstanceID token records is indeed <app_id, authorized_entity, scope>.
I've updated the comment.

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/base/g...
File google_apis/gcm/base/gcm_util.h (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/base/g...
google_apis/gcm/base/gcm_util.h:12: void BuildFormEncoding(const std::string&
key,
On 2015/05/21 21:07:51, Nicolas Zea wrote:
> Comment what this is for

Done.

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
File google_apis/gcm/engine/instance_id_get_token_request.cc (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/instance_id_get_token_request.cc:62:
DCHECK(!extra_request_info_.instance_id.empty() &&
On 2015/05/21 21:07:51, Nicolas Zea wrote:
> These should probably be in ExtraRequestInfo constructor, rather than here or
in
> addition to (and same with other registration requests).

Done.

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.cc:81: app_id(app_id) {
On 2015/05/21 21:07:51, Nicolas Zea wrote:
> Maybe DCHECK that the values are set?

Done.

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:62: // Encapsulates the common
info about a registration/token request.
On 2015/05/21 21:07:51, Nicolas Zea wrote:
> Are all parameters mandatory?

Yes. Added to comment.

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:93: virtual void
BuildRequestHeaders(std::string* extra_headers);
On 2015/05/21 21:07:51, Nicolas Zea wrote:
> Although I like the approach of injecting extra request headers that the
request
> consumes, I still think it would be cleaner to build the request such that you
> don't need to overload these Build/Parse/Get methods and use a composition
> approach instead.
> 
> How about just having callback plumbed in for the Build method here (it's
always
> called after the native build method I think), and having the source be a
string
> that's passed in at construction time? It doesn't need to be calculated
> repeatedly right?
> 
> A similar approach could I think be taken for UnregistrationRequest.

I don't fully understand the callback proposal. Could you please illustrate
more?

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
File google_apis/gcm/engine/unregistration_request.h (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.h:89: virtual void
BuildRequestHeaders(std::string* extra_headers);
On 2015/05/21 21:07:51, Nicolas Zea wrote:
> Is anything overriding this?

No. Removed virtual.

[Nicolas Zea, 2015-05-22 14:53:14]

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/180001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:93: virtual void
BuildRequestHeaders(std::string* extra_headers);
On 2015/05/21 23:11:24, jianli wrote:
> On 2015/05/21 21:07:51, Nicolas Zea wrote:
> > Although I like the approach of injecting extra request headers that the
> request
> > consumes, I still think it would be cleaner to build the request such that
you
> > don't need to overload these Build/Parse/Get methods and use a composition
> > approach instead.
> > 
> > How about just having callback plumbed in for the Build method here (it's
> always
> > called after the native build method I think), and having the source be a
> string
> > that's passed in at construction time? It doesn't need to be calculated
> > repeatedly right?
> > 
> > A similar approach could I think be taken for UnregistrationRequest.
> 
> I don't fully understand the callback proposal. Could you please illustrate
> more?

So there's be three new constructor parameters, two are callbacks
(build_body_callback, build_header_callback), and the other is a string
(source_for_recorder).

[jianli, 2015-05-22 21:59:14]

I've updated to pass the request handler per discussion. Please review again.
Thanks.

[Nicolas Zea, 2015-05-26 16:39:15]

Thanks for making those changes, I think it looks much cleaner. gcm LGTM with
some nits

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/gcm_unregistration_request_handler.h (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/gcm_unregistration_request_handler.h:8: #include
"google_apis/gcm/engine/unregistration_request.h"
#include string

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/instance_id_delete_token_request_handler.cc (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/instance_id_delete_token_request_handler.cc:22: //
Response constants.
nit: Combine with previous group? (considering the comment is the same)

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/instance_id_delete_token_request_handler.h (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/instance_id_delete_token_request_handler.h:8: #include
"google_apis/gcm/engine/unregistration_request.h"
#include string

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/instance_id_get_token_request_handler.cc (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/instance_id_get_token_request_handler.cc:36:
DCHECK(!instance_id.empty() && !authorized_entity.empty() && !scope.empty());
nit: I think it's cleaner to split this up into multiple dchecks (that way if it
fires, you know which one failed)

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.cc:156:
custom_request_handler_->BuildRequestBody(body);
Check if it exists first?

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:9: #include <vector>
#include string

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:85: virtual void
BuildRequestBody(std::string* body) = 0;
Comment describing what this method does and when its called?

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.cc:44: DCHECK(android_id != 0UL &&
security_token != 0UL);
nit: have as two separate DCHECKS

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.cc:114:
custom_request_handler_->BuildRequestBody(body);
Check if the pointer is valid first? (here and below)

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.cc:165:
UnregistrationRequest::Status status= ParseResponse(source);
nit: space between "status" and "="

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/unregistration_request.h (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.h:81: virtual void
BuildRequestBody(std::string* body) = 0;
Comment when these are called? (i.e. this is called after the
UnregistrationRequest BuildRequestBody)

[jianli, 2015-05-26 20:49:39]

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/gcm_unregistration_request_handler.h (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/gcm_unregistration_request_handler.h:8: #include
"google_apis/gcm/engine/unregistration_request.h"
On 2015/05/26 16:39:14, Nicolas Zea wrote:
> #include string

Done.

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/instance_id_delete_token_request_handler.cc (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/instance_id_delete_token_request_handler.cc:22: //
Response constants.
On 2015/05/26 16:39:14, Nicolas Zea wrote:
> nit: Combine with previous group? (considering the comment is the same)

The comment is indeed not the same, one for request while the other is for
response.

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/instance_id_delete_token_request_handler.h (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/instance_id_delete_token_request_handler.h:8: #include
"google_apis/gcm/engine/unregistration_request.h"
On 2015/05/26 16:39:14, Nicolas Zea wrote:
> #include string

Done.

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/instance_id_get_token_request_handler.cc (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/instance_id_get_token_request_handler.cc:36:
DCHECK(!instance_id.empty() && !authorized_entity.empty() && !scope.empty());
On 2015/05/26 16:39:14, Nicolas Zea wrote:
> nit: I think it's cleaner to split this up into multiple dchecks (that way if
it
> fires, you know which one failed)

Done.

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.cc:156:
custom_request_handler_->BuildRequestBody(body);
On 2015/05/26 16:39:15, Nicolas Zea wrote:
> Check if it exists first?

Done.

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:9: #include <vector>
On 2015/05/26 16:39:15, Nicolas Zea wrote:
> #include string

Done.

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:85: virtual void
BuildRequestBody(std::string* body) = 0;
On 2015/05/26 16:39:15, Nicolas Zea wrote:
> Comment describing what this method does and when its called?

Done.

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.cc:44: DCHECK(android_id != 0UL &&
security_token != 0UL);
On 2015/05/26 16:39:15, Nicolas Zea wrote:
> nit: have as two separate DCHECKS

Done.

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.cc:114:
custom_request_handler_->BuildRequestBody(body);
On 2015/05/26 16:39:15, Nicolas Zea wrote:
> Check if the pointer is valid first? (here and below)

Done.

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.cc:165:
UnregistrationRequest::Status status= ParseResponse(source);
On 2015/05/26 16:39:15, Nicolas Zea wrote:
> nit: space between "status" and "="

Done.

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
File google_apis/gcm/engine/unregistration_request.h (right):

https://codereview.chromium.org/1137463003/diff/220001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.h:81: virtual void
BuildRequestBody(std::string* body) = 0;
On 2015/05/26 16:39:15, Nicolas Zea wrote:
> Comment when these are called? (i.e. this is called after the
> UnregistrationRequest BuildRequestBody)

Done.

[jianli, 2015-05-26 20:49:46]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-05-26 20:49:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1137463003/#ps240001
(title: "Patch to land")

[commit-bot: I haz the power, 2015-05-26 20:52:20]

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

[commit-bot: I haz the power, 2015-05-26 23:24:58]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2015-05-26 23:25:57]

Patchset 12 (id:??) landed as
https://crrev.com/7a0c9b6d322d0a501fc114d3854df944fc059f0f
Cr-Commit-Position: refs/heads/master@{#331475}