Skeleton GCMAppHandler for Push API

Skeleton GCMAppHandler for Push API

TODOs mark parts of this which need to be hooked up to Service Workers.

Depends on https://codereview.chromium.org/317823007/

BUG=350378

[fgorski, 2014-06-10 17:09:28]

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/gcm_profile_service.cc (left):

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_profile_service.cc:52: }
I think I prefer having 2 distinct method. If you worry about duplication, wrap
the last 4 lines in a method.

I'd rather see a clear picture where methods start and end.

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:25: // then we should
RemoveAppHandler all our app handlers.
Perhaps you can do that in Shutdown app handler. Unless you worry about cases
when it would never get called.

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:52: //        
"data": "BAZ",
this looks redundant... is that a work in progress still? Or is that already
part of the standard?

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:65: // Service Worker
corresponding to app_id.
Add a TODO for diagnostic observers (you likely want to add UMA and an internals
page to know that bad things happen)

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:70: //
TODO(mvanouwerkerk): Fire push error event on the Service Worker
Are you sure this deserves a semantic of an error?
onMessagesDeleted means that server sent so many messages when the device was
unreachable, that the queue size quota was exceeded.
Is that case mentioned in standard? Should it be an error?

https://codereview.chromium.org/324913004/diff/1/components/gcm_driver/gcm_dr...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/324913004/diff/1/components/gcm_driver/gcm_dr...
components/gcm_driver/gcm_driver.h:164: // to register app handlers on demand.
Please explain that scenario a bit better in response. I am not sure I follow.

[johnme, 2014-06-10 20:33:48]

Addressed some of Filip's comments

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/gcm_profile_service.cc (left):

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_profile_service.cc:52: }
On 2014/06/10 17:09:27, fgorski wrote:
> I think I prefer having 2 distinct method. If you worry about duplication,
wrap
> the last 4 lines in a method.
> 
> I'd rather see a clear picture where methods start and end.

Done, in fact I moved this code to PushMessagingServiceImpl::Init to avoid
polluting GCMProfileService.

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:25: // then we should
RemoveAppHandler all our app handlers.
On 2014/06/10 17:09:27, fgorski wrote:
> Perhaps you can do that in Shutdown app handler. Unless you worry about cases
> when it would never get called.

Not sure, I based this on ExtensionGCMAppHandler. In practice, I don't think
it's possible for GCMDriver to outlive PushMessagingServiceImpl, except perhaps
when GCMProfileService is being torn down and they get destructed in reverse
order of declaration of the member variables, but GCMDriver is unlikely to call
any app handlers from its destructor.

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:52: //        
"data": "BAZ",
On 2014/06/10 17:09:27, fgorski wrote:
> this looks redundant... is that a work in progress still? Or is that already
> part of the standard?

There are no standards around the GCM part. The Push API just says to provide a
unicode string. Unfortunately, GCM has this quirky legacy behaviour where it
only allows a map<string,string> (even though you pass it in as JSON!) because
that's all Android Bundles support. So this seems the simplest way of sending a
simple string using GCM, though the duplication of data is unfortunate.

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:65: // Service Worker
corresponding to app_id.
On 2014/06/10 17:09:27, fgorski wrote:
> Add a TODO for diagnostic observers (you likely want to add UMA and an
internals
> page to know that bad things happen)

Done.

https://codereview.chromium.org/324913004/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:70: //
TODO(mvanouwerkerk): Fire push error event on the Service Worker
On 2014/06/10 17:09:27, fgorski wrote:
> Are you sure this deserves a semantic of an error?
> onMessagesDeleted means that server sent so many messages when the device was
> unreachable, that the queue size quota was exceeded.
> Is that case mentioned in standard? Should it be an error?

Last time we discussed the spec, there seemed to be agreement to have a
PushError event for the two cases:
1. Messages were deleted
2. The registration was invalidated for some reason
with a type member to let script distinguish between the two causes.

But the spec is still pretty fluid (indeed this hasn't yet been incorporated
into the editor's draft), so I'm sure we could rename this (and/or split it up).

https://codereview.chromium.org/324913004/diff/1/components/gcm_driver/gcm_dr...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/324913004/diff/1/components/gcm_driver/gcm_dr...
components/gcm_driver/gcm_driver.h:164: // to register app handlers on demand.
On 2014/06/10 17:09:28, fgorski wrote:
> Please explain that scenario a bit better in response. I am not sure I follow.

Replied out of band by email.

[jianli, 2014-06-13 22:51:47]

https://codereview.chromium.org/324913004/diff/60001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/324913004/diff/60001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver.h:81: virtual void
AddWildcardAppHandler(GCMWildcardAppHandler* handler);
Having 2 versions of adding app handlers seems to be confusing because people
might not know which one to pick. It is also hard to maintain. How about just
using the existing version for the wildcard purpose? We can call
AddAppHandler("push:", handler) to add a handler that is going to handle the
messages for all app IDs prefixed with "push:".

[Michael van Ouwerkerk, 2014-06-18 18:32:17]

On 2014/06/13 22:51:47, jianli wrote:
>
https://codereview.chromium.org/324913004/diff/60001/components/gcm_driver/gc...
> File components/gcm_driver/gcm_driver.h (right):
> 
>
https://codereview.chromium.org/324913004/diff/60001/components/gcm_driver/gc...
> components/gcm_driver/gcm_driver.h:81: virtual void
> AddWildcardAppHandler(GCMWildcardAppHandler* handler);
> Having 2 versions of adding app handlers seems to be confusing because people
> might not know which one to pick. It is also hard to maintain. How about just
> using the existing version for the wildcard purpose? We can call
> AddAppHandler("push:", handler) to add a handler that is going to handle the
> messages for all app IDs prefixed with "push:".

I've landed this as r278087.