Client-to-server messages in GCMNetworkChannel

Client-to-server messages in GCMNetworkChannel

This change implements sending client-to-server messages in
GCMNetworkChannel. Implementation mimics what android implementation is
doing. To send message to server GCMNetworkChannel needs GCM
registration Id and access token. Registration Id is requested during
initialization and doesn’t change through process lifetime. Access token
is requested for every message. Errors are not handled,
cacheinvalidations will retry sending message if it doesn’t see
confirmation from server that the message was received. The only handled
error is HTTP_UNAUTHORIZED from tango server since access token needs to
be invalidated in this case.

Actual work of requesting registration id and access token is delegated
to GCMNetworkChannelDelegate interface. Implementation of this interface
will live in chrome/browser/invalidation directory and will use
GCMProfileService for registration and ProfileOAuth2TokenService for
token functions.

There are two things that are still not implemented:
- Register error handling: will need to implement retry.
- Building url for tango endpoint: this involves preparing and base64
encoding NetworkEndpointId protobuf from cacheinvalidations library.

Will address both in next CL.

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

[rlarocque, 2014-01-16 18:52:48]

Some comments, mostly style nits.  At some point I stopped commenting on
indentation style issues.  Feel free to find and fix the instances that I
missed.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
File sync/notifier/gcm_network_channel.cc (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel.cc:32: void
GCMNetworkChannel::OnRegisterComplete(const std::string& registration_id,
nit: either put this parameter on the next line, or indent the next parameter up
to the '('.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel.cc:59: weak_factory_.GetWeakPtr()));
another indentation nit: I think the second line should be aligned with the '('
that follows the 'Bind'.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel.cc:83: this));
more nits: I recommend indenting the 'this' parameter or moving all the
parameters to the 'Create(' call onto this line.

The relevant style guideline rule is here:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel.cc:85: std::string auth_header;
I think it's slightly better to declare and initialize the string at the same
time.  As a bonus, you could declare the string 'const' if you did this.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
File sync/notifier/gcm_network_channel.h (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel.h:25: class SYNC_EXPORT_PRIVATE
GCMNetworkChannel
The threading code is starting to get complicated.  Would it be possible to make
this class inherit from base::NonThreadSafe to make the rules clearer?

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
File sync/notifier/gcm_network_channel_delegate.h (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_delegate.h:26: virtual void
RequestToken(RequestTokenCallback callback) = 0;
Please add a few function comments.  A class and/or file comment would be nice,
too.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
File sync/notifier/gcm_network_channel_unittest.cc (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_unittest.cc:52: delegate_);
Can this fit on one line?

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_unittest.cc:54: delegate.Pass()));
nit: alignment again.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_unittest.cc:231: 
Does your current design still support the possibility that the callbacks
following a register or auth token refresh request will not be invoked?  Would
it make sense to have some tests for those scenarios?

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_unittest.cc:232: 
Remove extra whitespace.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/sync_system_re...
File sync/notifier/sync_system_resources.h (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/sync_system_re...
sync/notifier/sync_system_resources.h:149: void SetNetworkState(InvalidatorState
invalidator_state);
Is this change related to your efforts to make state changes be handled
differently for GCM and XMPP channels?

Looking at the code as it currently exists, I think that NotifyStateChange() is
the better name.  Were you planning to make other changes that would make this
name a bad description?

[pavely, 2014-01-17 00:44:39]

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
File sync/notifier/gcm_network_channel.cc (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel.cc:32: void
GCMNetworkChannel::OnRegisterComplete(const std::string& registration_id,
On 2014/01/16 18:52:48, rlarocque wrote:
> nit: either put this parameter on the next line, or indent the next parameter
up
> to the '('.

Done.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel.cc:59: weak_factory_.GetWeakPtr()));
On 2014/01/16 18:52:48, rlarocque wrote:
> another indentation nit: I think the second line should be aligned with the
'('
> that follows the 'Bind'.

Done.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel.cc:83: this));
On 2014/01/16 18:52:48, rlarocque wrote:
> more nits: I recommend indenting the 'this' parameter or moving all the
> parameters to the 'Create(' call onto this line.
> 
> The relevant style guideline rule is here:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel.cc:85: std::string auth_header;
On 2014/01/16 18:52:48, rlarocque wrote:
> I think it's slightly better to declare and initialize the string at the same
> time.  As a bonus, you could declare the string 'const' if you did this.

Done.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
File sync/notifier/gcm_network_channel.h (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel.h:25: class SYNC_EXPORT_PRIVATE
GCMNetworkChannel
Actually everything that happens to this class is supposed to happen on one
thread. 
I'll add NonThreadSafe and checks.
On 2014/01/16 18:52:48, rlarocque wrote:
> The threading code is starting to get complicated.  Would it be possible to
make
> this class inherit from base::NonThreadSafe to make the rules clearer?

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
File sync/notifier/gcm_network_channel_delegate.h (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_delegate.h:26: virtual void
RequestToken(RequestTokenCallback callback) = 0;
On 2014/01/16 18:52:48, rlarocque wrote:
> Please add a few function comments.  A class and/or file comment would be
nice,
> too.

Done.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
File sync/notifier/gcm_network_channel_unittest.cc (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_unittest.cc:52: delegate_);
On 2014/01/16 18:52:48, rlarocque wrote:
> Can this fit on one line?

Done.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_unittest.cc:54: delegate.Pass()));
On 2014/01/16 18:52:48, rlarocque wrote:
> nit: alignment again.

Done.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_unittest.cc:231: 
It is only possible during shutdown. Not sure what to assert in this case.

On 2014/01/16 18:52:48, rlarocque wrote:
> Does your current design still support the possibility that the callbacks
> following a register or auth token refresh request will not be invoked?  Would
> it make sense to have some tests for those scenarios?

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_unittest.cc:232: 
On 2014/01/16 18:52:48, rlarocque wrote:
> Remove extra whitespace.

Done.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/sync_system_re...
File sync/notifier/sync_system_resources.h (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/sync_system_re...
sync/notifier/sync_system_resources.h:149: void SetNetworkState(InvalidatorState
invalidator_state);
One scenario that I considered was to use this function to report network state
after each operation. This way there would be lots of calls for no state change.
In this function implementation I only notify state change if state actually
changed.
I ended up reverting change that uses this and I'm not sure I'll need this
behavior in the future so I'll revert function name and implementation change as
well.

On 2014/01/16 18:52:48, rlarocque wrote:
> Is this change related to your efforts to make state changes be handled
> differently for GCM and XMPP channels?
> 
> Looking at the code as it currently exists, I think that NotifyStateChange()
is
> the better name.  Were you planning to make other changes that would make this
> name a bad description?

[rlarocque, 2014-01-17 01:40:47]

I've got one more comment about tests.  Everything else is LGTM, so feel free to
commit if you decide you don't need more tests.

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
File sync/notifier/gcm_network_channel_unittest.cc (right):

https://codereview.chromium.org/140513002/diff/1/sync/notifier/gcm_network_ch...
sync/notifier/gcm_network_channel_unittest.cc:231: 
Are you sure that case is shutdown only?  What if the user disconnects their
account?

----

If you're worried that there could be crashes or memory leaks specific to the
shutdown scenario, then it might make sense to add the tests even if there are
no useful assertions in them.  Sometimes it makes sense to write tests knowing
that they're only useful when they're run under Valgrind or ASAN.  

If you're not worried about crashes or memory leaks, or if there are no special
risks around the shutdown scenario, then I agree there's no point in adding
extra tests.

On 2014/01/17 00:44:39, pavely wrote:
> It is only possible during shutdown. Not sure what to assert in this case.
> 
> On 2014/01/16 18:52:48, rlarocque wrote:
> > Does your current design still support the possibility that the callbacks
> > following a register or auth token refresh request will not be invoked? 
Would
> > it make sense to have some tests for those scenarios?
>

[commit-bot: I haz the power, 2014-01-17 19:23:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/140513002/140001

[tim (not reviewing), 2014-01-17 19:33:41]

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
File sync/notifier/gcm_network_channel.cc (right):

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel.cc:45: // TODO(pavely): Don't know what to do
if registration fails. Let's not do
Consider filing a bug to associate with this (and every) TODO, but particularly
ones we might easily forget about.

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel.cc:76: if (!(error ==
GoogleServiceAuthError::AuthErrorNone())) {
nit - if (error != GSAE::AuthErrorNone())

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
File sync/notifier/gcm_network_channel.h (right):

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel.h:31: explicit GCMNetworkChannel(
multi argument constructors should not be marked explicit.

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
File sync/notifier/gcm_network_channel_delegate.h (right):

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel_delegate.h:19: // registration id. This id is
used for building URL to Tango endpoint.
nit- "cache invalidation", not "Tango".

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
File sync/notifier/gcm_network_channel_unittest.cc (right):

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel_unittest.cc:34: class GCMNetworkChannelTest
nice tests!

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel_unittest.cc:39: :
url_fetchers_created_count_(0) {
nit - init |delegate_| to NULL.

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel_unittest.cc:112:
url_fetcher_factory()->SetFakeResponse(url, "", net::HTTP_OK,
nit - prefer std::string to "".

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel_unittest.cc:204: TEST_F(GCMNetworkChannelTest,
AuthErrorFromTango) {
nit - avoid the word 'Tango' in chromium.

[pavely, 2014-01-17 22:52:25]

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
File sync/notifier/gcm_network_channel.cc (right):

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel.cc:45: // TODO(pavely): Don't know what to do
if registration fails. Let's not do
On 2014/01/17 19:33:42, timsteele wrote:
> Consider filing a bug to associate with this (and every) TODO, but
particularly
> ones we might easily forget about.

Done.

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel.cc:76: if (!(error ==
GoogleServiceAuthError::AuthErrorNone())) {
On 2014/01/17 19:33:42, timsteele wrote:
> nit - if (error != GSAE::AuthErrorNone())
(error != GSAE::AuthErrorNone()) doesn't compile. Changed to (error.state() !=
GSAE::NONE)

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
File sync/notifier/gcm_network_channel.h (right):

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel.h:31: explicit GCMNetworkChannel(
On 2014/01/17 19:33:42, timsteele wrote:
> multi argument constructors should not be marked explicit.
> 

Done.

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
File sync/notifier/gcm_network_channel_delegate.h (right):

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel_delegate.h:19: // registration id. This id is
used for building URL to Tango endpoint.
On 2014/01/17 19:33:42, timsteele wrote:
> nit- "cache invalidation", not "Tango".

Done.

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
File sync/notifier/gcm_network_channel_unittest.cc (right):

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel_unittest.cc:39: :
url_fetchers_created_count_(0) {
On 2014/01/17 19:33:42, timsteele wrote:
> nit - init |delegate_| to NULL.

Done.

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel_unittest.cc:112:
url_fetcher_factory()->SetFakeResponse(url, "", net::HTTP_OK,
On 2014/01/17 19:33:42, timsteele wrote:
> nit - prefer std::string to "".

Done.

https://codereview.chromium.org/140513002/diff/140001/sync/notifier/gcm_netwo...
sync/notifier/gcm_network_channel_unittest.cc:204: TEST_F(GCMNetworkChannelTest,
AuthErrorFromTango) {
On 2014/01/17 19:33:42, timsteele wrote:
> nit - avoid the word 'Tango' in chromium.

Done.

[commit-bot: I haz the power, 2014-01-21 23:35:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/140513002/360012

[commit-bot: I haz the power, 2014-01-21 23:55:54]

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

[pavely, 2014-01-22 02:10:36]

+Ryan Hamilton for sync/notifier/DEPS: I changed DEPS file to refer
net/url_request and net/http/http_status_code.h. Should be no brainer.

[Ryan Hamilton, 2014-01-22 04:21:55]

On 2014/01/22 02:10:36, pavely wrote:
> +Ryan Hamilton for sync/notifier/DEPS: I changed DEPS file to refer
> net/url_request and net/http/http_status_code.h. Should be no brainer.

sync/notifier/DEPS: lgtm

[commit-bot: I haz the power, 2014-01-22 04:38:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/140513002/360012

[commit-bot: I haz the power, 2014-01-22 10:47:03]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-22 17:21:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/140513002/360012

[commit-bot: I haz the power, 2014-01-22 18:36:21]

Change committed as 246377