GCM Registration Request implementation

GCM Registration Request
* implementation of request creation
* implementation of response parsing
* tests for both.

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

[fgorski, 2014-01-10 01:16:11]

Please take a look. I will go through the server code once again tomorrow (I've
just found I missed user's android id in the first run) to double check, but
essentially the pack and tests are ready.

[Nicolas Zea, 2014-01-10 19:45:12]

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:21: namespace {
nit: newline after

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:124: LOG(ERROR) << "Failed to get
registration response.";
nit: add todo to have retry logic. It'll probably make sense to reuse the same
retry logic/schedule here as in the checkin code and the connection code. We can
discuss further offline.

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

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.h:30: uint64 android_id,
this list of params is getting pretty long, encapsulate in a
RegistrationRequestInfo struct perhaps (could have it's own serialize to post
encoding method).

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request_unittest.cc (right):

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request_unittest.cc:23: const char
kNoRegistrationIdReturned[] = "No Regisrtation Id returned";
spelling nit: registration

[jianli, 2014-01-10 20:17:42]

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:43: out->append("&" + key + "=" +
net::EscapeUrlEncodedData(value, true));
It seems to be more efficient to write like:
  if (!out->empty())
    out->append("&");
  out->append(key + "=" + net::EscapeUrlEncodedData(value, true));

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

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.h:64: scoped_ptr<net::URLFetcher>
url_fetcher_;
nit: better to add explicit includes for scoped_refptr and scoped_ptr.

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request_unittest.cc (right):

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request_unittest.cc:119: // Verify data was
formated properly.
nit: formatted

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request_unittest.cc:123: EXPECT_EQ("app",
data_tokenizer.token());
These checks rely on a specific order. If the key-value pairs in the source code
have been rearranged, you will have to update the test codes here. Is it
possible to extract all pairs into a map, keep a list of expected key-value
pairs and iterate through this list to do the check?

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request_unittest.cc:156: // Verify data was
formated properly.
ditto for typo

[fgorski, 2014-01-10 22:46:58]

PTAL. All comments addressed. I also added some documentation in the header.

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:21: namespace {
On 2014/01/10 19:45:12, Nicolas Zea wrote:
> nit: newline after

Done.

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:43: out->append("&" + key + "=" +
net::EscapeUrlEncodedData(value, true));
On 2014/01/10 20:17:43, jianli wrote:
> It seems to be more efficient to write like:
>   if (!out->empty())
>     out->append("&");
>   out->append(key + "=" + net::EscapeUrlEncodedData(value, true));

Done.

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:124: LOG(ERROR) << "Failed to get
registration response.";
On 2014/01/10 19:45:12, Nicolas Zea wrote:
> nit: add todo to have retry logic. It'll probably make sense to reuse the same
> retry logic/schedule here as in the checkin code and the connection code. We
can
> discuss further offline.

Done.

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

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.h:30: uint64 android_id,
On 2014/01/10 19:45:12, Nicolas Zea wrote:
> this list of params is getting pretty long, encapsulate in a
> RegistrationRequestInfo struct perhaps (could have it's own serialize to post
> encoding method).

Done.

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.h:64: scoped_ptr<net::URLFetcher>
url_fetcher_;
On 2014/01/10 20:17:43, jianli wrote:
> nit: better to add explicit includes for scoped_refptr and scoped_ptr.

Done.

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request_unittest.cc (right):

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request_unittest.cc:23: const char
kNoRegistrationIdReturned[] = "No Regisrtation Id returned";
On 2014/01/10 19:45:12, Nicolas Zea wrote:
> spelling nit: registration

Done.

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request_unittest.cc:119: // Verify data was
formated properly.
On 2014/01/10 20:17:43, jianli wrote:
> nit: formatted

Done.

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request_unittest.cc:123: EXPECT_EQ("app",
data_tokenizer.token());
On 2014/01/10 20:17:43, jianli wrote:
> These checks rely on a specific order. If the key-value pairs in the source
code
> have been rearranged, you will have to update the test codes here. Is it
> possible to extract all pairs into a map, keep a list of expected key-value
> pairs and iterate through this list to do the check?

Done.

https://codereview.chromium.org/126513007/diff/80001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request_unittest.cc:156: // Verify data was
formated properly.
On 2014/01/10 20:17:43, jianli wrote:
> ditto for typo

Done.

[jianli, 2014-01-10 23:23:47]

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

https://codereview.chromium.org/126513007/diff/160001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.cc:25: const char
kRegistrationDataFormat[] = "application/x-www-form-urlencoded";
nit: kRegistrationRequestContentType

https://codereview.chromium.org/126513007/diff/160001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.cc:110: senders.append(",");
What if one sender is empty or ","? Do we need so have some sort of validation
in GcmRegisterFunction::DoWork?

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

https://codereview.chromium.org/126513007/diff/160001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:37: struct Parameters {
RequestInfo?

https://codereview.chromium.org/126513007/diff/160001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:48: RegistrationCallback callback;
I think RegistrationCallback might be better to pass as a separate argument to
the constructor. All other fields defined here are all related to registration
request.

[fgorski, 2014-01-11 00:10:36]

PTAL

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

https://codereview.chromium.org/126513007/diff/160001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.cc:25: const char
kRegistrationDataFormat[] = "application/x-www-form-urlencoded";
On 2014/01/10 23:23:47, jianli wrote:
> nit: kRegistrationRequestContentType

Done.

https://codereview.chromium.org/126513007/diff/160001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.cc:110: senders.append(",");
On 2014/01/10 23:23:47, jianli wrote:
> What if one sender is empty or ","? Do we need so have some sort of validation
> in GcmRegisterFunction::DoWork?

We already have validation upstream, but I see no reason not to add a DCHECK
here.

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

https://codereview.chromium.org/126513007/diff/160001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:37: struct Parameters {
On 2014/01/10 23:23:47, jianli wrote:
> RequestInfo?

Done.

https://codereview.chromium.org/126513007/diff/160001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:48: RegistrationCallback callback;
On 2014/01/10 23:23:47, jianli wrote:
> I think RegistrationCallback might be better to pass as a separate argument to
> the constructor. All other fields defined here are all related to registration
> request.

Done.

[Nicolas Zea, 2014-01-11 00:25:56]

lgtm

[jianli, 2014-01-11 00:33:39]

lgtm

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

https://codereview.chromium.org/126513007/diff/240001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:64: const RegistrationCallback&
callback,
nit: it might be better to put RequestInfo as 1st parameter

https://codereview.chromium.org/126513007/diff/240001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:65: const RequestInfo& parameters,
nit: request_info

https://codereview.chromium.org/126513007/diff/240001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:76: RequestInfo parameters_;
nit: request_info_

[fgorski, 2014-01-11 01:07:28]

Fixed all. Checking CQ now.

Thanks for review.

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

https://codereview.chromium.org/126513007/diff/240001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:64: const RegistrationCallback&
callback,
On 2014/01/11 00:33:39, jianli wrote:
> nit: it might be better to put RequestInfo as 1st parameter 

Done.

https://codereview.chromium.org/126513007/diff/240001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:65: const RequestInfo& parameters,
On 2014/01/11 00:33:39, jianli wrote:
> nit: request_info

Done.

https://codereview.chromium.org/126513007/diff/240001/google_apis/gcm/engine/...
google_apis/gcm/engine/registration_request.h:76: RequestInfo parameters_;
On 2014/01/11 00:33:39, jianli wrote:
> nit: request_info_

Done.

[commit-bot: I haz the power, 2014-01-11 02:00:38]

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

[commit-bot: I haz the power, 2014-01-11 04:02:07]

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

[commit-bot: I haz the power, 2014-01-13 05:42:47]

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

[commit-bot: I haz the power, 2014-01-13 07:42:50]

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

[commit-bot: I haz the power, 2014-01-13 15:26:40]

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

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

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

[commit-bot: I haz the power, 2014-01-13 18:38:44]

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

[commit-bot: I haz the power, 2014-01-13 19:51:27]

Change committed as 244561