Adding GCM support to the copresence component.

Adding GCM support to the copresence component. The Copresence server can push directives (and messages, though they are not yet handled) over GCM.

BUG=424253, 425681
Committed: https://crrev.com/19b4f6371c68fe1627c2b75ca5ec8a4061796e58
Cr-Commit-Position: refs/heads/master@{#303342}

[Charlie, 2014-11-07 02:31:44]

ckehoe@chromium.org changed reviewers:
+ rkc@chromium.org

[Charlie, 2014-11-07 02:31:44]

Hey Rahul,

Here's my current GCM code. Please take a look when you get a chance. The
*short* TODO list here includes:

1. Write gcm_handler_unittest.cc
2. Pull the base64 stuff out into a common place

This at least doesn't crash, but hasn't been tested with a real GCM message from
the server. It's the simplest thing I could get running that looked like it
*could* work :-). Let's get something checked in tomorrow and if it doesn't work
for realz we can fix it next week.

[Charlie, 2014-11-07 05:37:22]

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

[Charlie, 2014-11-07 05:37:23]

Added a unit test.

+fgorski for GCM review. We're hoping to get this in for M40 :-)

[Charlie, 2014-11-07 05:37:57]

CC +jianli

[fgorski, 2014-11-07 17:40:44]

Checked the GCM pieces.

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
File components/copresence/handlers/gcm_handler.cc (right):

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:37: const char
GCMHandler::kCopresenceAppId[] = "copresence";
is there a chance you use a longer app ID?
What is the package name used on Android?

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:46: : driver_(gcm_driver),
4 spaces before :

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:63: base::Unretained(this)));
use a WeakPtrFactory, that way the comment above is not necessary and you don't
run the risk you described. Avoid base::Unretained in production code.

E.g. gcm_driver.cc line 73

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:139: LOG(ERROR) << "GCM message
overflow reported";
This is definitely not an error.
If the device is offline for a long time GCM will discard messages. This is
normal.

Also, there is far too much LOG(ERROR)s in this code. Did you consider using
DVLOG instead?

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
File components/copresence/handlers/gcm_handler.h (right):

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.h:23: class GCMHandler final : public
gcm::GCMAppHandler {
Document the class and methods that are not overrides.

https://codereview.chromium.org/710513004/diff/20001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/710513004/diff/20001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:190:
base::Bind(&RpcHandler::RegisterGcmId, base::Unretained(this)));
again, consider using WeakPtrFactory

[rkc, 2014-11-07 18:39:06]

Mostly looks fine - I'll do another in depth pass after the next patch.

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
File components/copresence/handlers/gcm_handler.cc (right):

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:56: // nulled out and still crash.
This is not true. A cancelable callback will invalidate its weak pointer, hence
making a call to it will just be a no-op. Since this bug exists with cancelable
callbacks, please do the following,
a) Use a weak pointer here.
b) File a bug for the cancelable callback issue.
c) Link the bug here, not the codereview.
d) Add a TODO to move this to using cancelable callbacks once the bug is fixed.

https://codereview.chromium.org/710513004/diff/20001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/710513004/diff/20001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:190:
base::Bind(&RpcHandler::RegisterGcmId, base::Unretained(this)));
On 2014/11/07 17:40:44, fgorski wrote:
> again, consider using WeakPtrFactory

Weak pointers are heavily discouraged since they muddle up lifetime guarantees.
Is there any reason that a Weak Pointer should be used here? I think it is
pretty clear from the comment in copresence_manager_impl.h that the gcm_handler_
will die before the RpcHandler.

[Charlie, 2014-11-07 19:42:30]

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
File components/copresence/handlers/gcm_handler.cc (right):

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:37: const char
GCMHandler::kCopresenceAppId[] = "copresence";
On 2014/11/07 17:40:43, fgorski wrote:
> is there a chance you use a longer app ID?
> What is the package name used on Android?

Done.

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:46: : driver_(gcm_driver),
On 2014/11/07 17:40:44, fgorski wrote:
> 4 spaces before :

Done.

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:56: // nulled out and still crash.
On 2014/11/07 18:39:05, Rahul Chaturvedi wrote:
> This is not true. A cancelable callback will invalidate its weak pointer,
hence
> making a call to it will just be a no-op. Since this bug exists with
cancelable
> callbacks, please do the following,
> a) Use a weak pointer here.
> b) File a bug for the cancelable callback issue.
> c) Link the bug here, not the codereview.
> d) Add a TODO to move this to using cancelable callbacks once the bug is
fixed.

Ah, seems this is a difference in behavior between std::bind and base::Bind.
Turns out the CancelableCallback fix was easy, so I have a CL out for that.

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:63: base::Unretained(this)));
On 2014/11/07 17:40:44, fgorski wrote:
> use a WeakPtrFactory, that way the comment above is not necessary and you
don't
> run the risk you described. Avoid base::Unretained in production code.
> 
> E.g. gcm_driver.cc line 73

Done.

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:139: LOG(ERROR) << "GCM message
overflow reported";
On 2014/11/07 17:40:44, fgorski wrote:
> This is definitely not an error.
> If the device is offline for a long time GCM will discard messages. This is
> normal.
> 
> Also, there is far too much LOG(ERROR)s in this code. Did you consider using
> DVLOG instead?

Fixed. I think we'd like to keep the other errors, though. We don't have control
here over what the server sends us, so it's good to know what's going wrong.

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
File components/copresence/handlers/gcm_handler.h (right):

https://codereview.chromium.org/710513004/diff/20001/components/copresence/ha...
components/copresence/handlers/gcm_handler.h:23: class GCMHandler final : public
gcm::GCMAppHandler {
On 2014/11/07 17:40:44, fgorski wrote:
> Document the class and methods that are not overrides.

Done.

https://codereview.chromium.org/710513004/diff/20001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/710513004/diff/20001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:190:
base::Bind(&RpcHandler::RegisterGcmId, base::Unretained(this)));
On 2014/11/07 18:39:05, Rahul Chaturvedi wrote:
> On 2014/11/07 17:40:44, fgorski wrote:
> > again, consider using WeakPtrFactory
> 
> Weak pointers are heavily discouraged since they muddle up lifetime
guarantees.
> Is there any reason that a Weak Pointer should be used here? I think it is
> pretty clear from the comment in copresence_manager_impl.h that the
gcm_handler_
> will die before the RpcHandler.

Acknowledged.

[rkc, 2014-11-07 20:24:15]

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
File components/copresence/handlers/gcm_handler.cc (right):

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:64:
registration_callback_.Cancel();
Looking at the cancelable callback code, I *think* that you don't need to do
this. The weak ptr in the object clears itself on destruction. You should ping
jhawkins@ or groby@ to confirm.

(If this is true, we probably should add this info to the cancelable callback
documentation - not in this CL though, that'll require a //base owners review).

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:119: DCHECK_EQ(kCopresenceAppId,
app_id);
app_id will become an unused argument on release builds.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:125: const
GCMClient::SendErrorDetails& send_error_details) {
/* app_id */
/* send_error_details */
here and all the functions below where the arguments aren't being used.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
File components/copresence/handlers/gcm_handler.h (right):

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
components/copresence/handlers/gcm_handler.h:45: 
Remove all the newlines between these functions. Makes it easier to identify the
overrides. Also, change the comment to GCMAppHandler overrides: to keep it
consistent with the use everywhere else (unless you prefer to change everywhere
else to be "XXXX implementation.").

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:181: // |gcm_handler_| is optional.
Unnecessary comment.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:195: // Do not use |directive_handler_|
here.
Update this comment to also indicate that now we cannot reference |gcm_handler_|
here either.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:400: if (!auth_token.empty() &&
!device_id.empty())
If the device_id is not empty, doesn't that mean that we already are registered
for this auth token? Why are we registering for this auth token again then?

[Charlie, 2014-11-07 20:50:13]

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
File components/copresence/handlers/gcm_handler.cc (right):

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:64:
registration_callback_.Cancel();
On 2014/11/07 20:24:15, Rahul Chaturvedi wrote:
> Looking at the cancelable callback code, I *think* that you don't need to do
> this. The weak ptr in the object clears itself on destruction. You should ping
> jhawkins@ or groby@ to confirm.
> 
> (If this is true, we probably should add this info to the cancelable callback
> documentation - not in this CL though, that'll require a //base owners
review).

Done.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:119: DCHECK_EQ(kCopresenceAppId,
app_id);
On 2014/11/07 20:24:15, Rahul Chaturvedi wrote:
> app_id will become an unused argument on release builds.

I think this is ok, since it's an argument as opposed to a local variable. We
can let the bots decide.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
components/copresence/handlers/gcm_handler.cc:125: const
GCMClient::SendErrorDetails& send_error_details) {
On 2014/11/07 20:24:15, Rahul Chaturvedi wrote:
> /* app_id */
> /* send_error_details */
> here and all the functions below where the arguments aren't being used.

Done.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
File components/copresence/handlers/gcm_handler.h (right):

https://codereview.chromium.org/710513004/diff/40001/components/copresence/ha...
components/copresence/handlers/gcm_handler.h:45: 
On 2014/11/07 20:24:15, Rahul Chaturvedi wrote:
> Remove all the newlines between these functions. Makes it easier to identify
the
> overrides. Also, change the comment to GCMAppHandler overrides: to keep it
> consistent with the use everywhere else (unless you prefer to change
everywhere
> else to be "XXXX implementation.").

Done.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:181: // |gcm_handler_| is optional.
On 2014/11/07 20:24:15, Rahul Chaturvedi wrote:
> Unnecessary comment.

Do you mean confusing? This is to explain why we're not DCHECKing it.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:195: // Do not use |directive_handler_|
here.
On 2014/11/07 20:24:15, Rahul Chaturvedi wrote:
> Update this comment to also indicate that now we cannot reference
|gcm_handler_|
> here either.

Done.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:400: if (!auth_token.empty() &&
!device_id.empty())
On 2014/11/07 20:24:15, Rahul Chaturvedi wrote:
> If the device_id is not empty, doesn't that mean that we already are
registered
> for this auth token? Why are we registering for this auth token again then?

To pass the GCM ID up to the server. The new registration uses the existing
device ID, and just updates the GCM data.

Did you read the comment? :-)

[rkc, 2014-11-07 21:05:11]

lgtm

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:181: // |gcm_handler_| is optional.
On 2014/11/07 20:50:12, Charlie wrote:
> On 2014/11/07 20:24:15, Rahul Chaturvedi wrote:
> > Unnecessary comment.
> 
> Do you mean confusing? This is to explain why we're not DCHECKing it.

No, just unnecessary. Not DCHECK'ing something isn't really something that needs
a comment. I don't feel too strongly about it, this is just a nit.

https://codereview.chromium.org/710513004/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:400: if (!auth_token.empty() &&
!device_id.empty())
On 2014/11/07 20:50:13, Charlie wrote:
> On 2014/11/07 20:24:15, Rahul Chaturvedi wrote:
> > If the device_id is not empty, doesn't that mean that we already are
> registered
> > for this auth token? Why are we registering for this auth token again then?
> 
> To pass the GCM ID up to the server. The new registration uses the existing
> device ID, and just updates the GCM data.
> 
> Did you read the comment? :-)

Ah right, case 2.

[fgorski, 2014-11-07 22:28:12]

gcm related code lgtm

please work on tests as soon as you can.

[Charlie, 2014-11-07 23:01:18]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-11-07 23:03:11]

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

[commit-bot: I haz the power, 2014-11-07 23:09:07]

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

[commit-bot: I haz the power, 2014-11-07 23:09:08]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie, 2014-11-07 23:12:38]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-11-07 23:14:20]

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

[commit-bot: I haz the power, 2014-11-07 23:26:28]

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

[commit-bot: I haz the power, 2014-11-07 23:26:29]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie, 2014-11-07 23:29:02]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-11-07 23:30:16]

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

[commit-bot: I haz the power, 2014-11-08 01:01:34]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-08 01:02:18]

Patchset 8 (id:??) landed as
https://crrev.com/19b4f6371c68fe1627c2b75ca5ec8a4061796e58
Cr-Commit-Position: refs/heads/master@{#303342}