Push API: register PushMessagingServiceImpl as GCMAppHandler.

Push API: register PushMessagingServiceImpl as GCMAppHandler.

Based on patch 324913004 by johnme@chromium.org who is on holiday now.

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

[Michael van Ouwerkerk, 2014-06-17 16:53:52]

Hi Filip, Jian, as John is away on holiday for the week, I took over this patch.
I've taken out the wildcard methods but preserved the CanHandle method as I
think it's cleaner to contains the app_id parsing in the class that will
generate them.

Please take a look.

[fgorski, 2014-06-17 17:24:00]

lgtm.

Thanks for being persistent about addressing the comments. I really like the
CanHandle solution. Just a bunch of nits to address.

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

https://codereview.chromium.org/338363003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:37: void
PushMessagingServiceImpl::InitializeForProfile(Profile* profile) {
Is there any chance that this function gets called multiple times with number of
registrations > 0 triggering the DCHECK in AddAppHandler?

Switching in and out of incognito comes to mind, or similar cases.

It's just a thought, perhaps worth exploring.

https://codereview.chromium.org/338363003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:41:
switches::kEnableExperimentalWebPlatformFeatures)) {
nit: fix alignment

https://codereview.chromium.org/338363003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:51: // Create the
GCMProfileService, and hence intiantiate this class.
typo in instantiate

https://codereview.chromium.org/338363003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:132:
DidRegister(app_id, callback, "", GCMClient::UNKNOWN_ERROR);
Perhaps GCMClient::GCM_DISABLED is more suitable, depending how you want to
handle that to the user. It will definitely be more useful for figuring out what
might have gone wrong,  when you consider monitoring the API usage.

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

https://codereview.chromium.org/338363003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.h:64: Profile* profile_;
// It owns our owner.
nit: 2 spaces before //

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

https://codereview.chromium.org/338363003/diff/1/components/gcm_driver/gcm_dr...
components/gcm_driver/gcm_driver.cc:167: iter = app_handlers_.begin();
nit: any reason not to put it in the for header?

[jianli, 2014-06-17 17:59:30]

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

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:24: const std::string
kAppIdPrefix = "push:";
nit: use char array.

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:67: //
TODO(mvanouwerkerk): Automatically unregister Service Workers from GCM when
Not sure I understand the comment here. Do we want to unregister from the
constructor?

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:132:
DidRegister(app_id, callback, "", GCMClient::UNKNOWN_ERROR);
I don't think we want to an unknown error here. If driver is not present, it is
a serious error. You should throw some assert here.

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:140: GetWeakPtr(),
Why do we need to introduce a helper function for it? Calling
weak_factory_.GetWeakPtr() is simple enough.

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:160:
gcm_profile_service_->driver()->AddAppHandler(kAppIdPrefix, this);
I don't think we will hit this because calling GCMDriver::Register will fail
with UNKNOWN_ERROR when no app handler is added.

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.h (right):

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:64: Profile* profile_;
// It owns our owner.
nit: double space before //.

[Michael van Ouwerkerk, 2014-06-17 19:22:06]

Thanks for the review. Please take another look!

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

https://codereview.chromium.org/338363003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:37: void
PushMessagingServiceImpl::InitializeForProfile(Profile* profile) {
Good question. I checked this, and we don't run in Incognito, and this code path
just runs once from ProfileImpl constructor. I think this is correct.

On 2014/06/17 17:24:00, fgorski wrote:
> Is there any chance that this function gets called multiple times with number
of
> registrations > 0 triggering the DCHECK in AddAppHandler?
> 
> Switching in and out of incognito comes to mind, or similar cases.
> 
> It's just a thought, perhaps worth exploring.

https://codereview.chromium.org/338363003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:41:
switches::kEnableExperimentalWebPlatformFeatures)) {
On 2014/06/17 17:24:00, fgorski wrote:
> nit: fix alignment

What is wrong with it? I figured a wrapped line should be indented by 4 spaces.

https://codereview.chromium.org/338363003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:51: // Create the
GCMProfileService, and hence intiantiate this class.
On 2014/06/17 17:24:00, fgorski wrote:
> typo in instantiate

Done.

https://codereview.chromium.org/338363003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:132:
DidRegister(app_id, callback, "", GCMClient::UNKNOWN_ERROR);
On 2014/06/17 17:24:00, fgorski wrote:
> Perhaps GCMClient::GCM_DISABLED is more suitable, depending how you want to
> handle that to the user. It will definitely be more useful for figuring out
what
> might have gone wrong,  when you consider monitoring the API usage.

Done.

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

https://codereview.chromium.org/338363003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_service_impl.h:64: Profile* profile_;
// It owns our owner.
On 2014/06/17 17:24:00, fgorski wrote:
> nit: 2 spaces before //

Done.

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

https://codereview.chromium.org/338363003/diff/1/components/gcm_driver/gcm_dr...
components/gcm_driver/gcm_driver.cc:167: iter = app_handlers_.begin();
On 2014/06/17 17:24:00, fgorski wrote:
> nit: any reason not to put it in the for header?

Done.

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

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:24: const std::string
kAppIdPrefix = "push:";
On 2014/06/17 17:59:30, jianli wrote:
> nit: use char array.

Done.

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:67: //
TODO(mvanouwerkerk): Automatically unregister Service Workers from GCM when
On 2014/06/17 17:59:30, jianli wrote:
> Not sure I understand the comment here. Do we want to unregister from the
> constructor?

Ah, sorry I just took that from John's sketch without cleaning it up. We'd want
to decrement the registration counter whenever a SW in uninstalled. We'd also
unregister its Push API GCM registration. And if the counter is 0 we would  call
RemoveAppHandler. This is all for a later patch.

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:132:
DidRegister(app_id, callback, "", GCMClient::UNKNOWN_ERROR);
On 2014/06/17 17:59:30, jianli wrote:
> I don't think we want to an unknown error here. If driver is not present, it
is
> a serious error. You should throw some assert here.

I changed it to GCM_DISABLED wdyt?

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:140: GetWeakPtr(),
On 2014/06/17 17:59:30, jianli wrote:
> Why do we need to introduce a helper function for it? Calling
> weak_factory_.GetWeakPtr() is simple enough.

Agreed.

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:160:
gcm_profile_service_->driver()->AddAppHandler(kAppIdPrefix, this);
On 2014/06/17 17:59:30, jianli wrote:
> I don't think we will hit this because calling GCMDriver::Register will fail
> with UNKNOWN_ERROR when no app handler is added.

Good catch. We still need to add this as an AppHandler on-demand, but before
calling Register. I've made GetAppHandler public so we can check whether or not
we've added ourselves already.

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.h (right):

https://codereview.chromium.org/338363003/diff/10001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:64: Profile* profile_;
// It owns our owner.
On 2014/06/17 17:59:30, jianli wrote:
> nit: double space before //.

Done.

[Michael van Ouwerkerk, 2014-06-17 19:30:42]

Jochen, could you review this as general owner? Filip and Jian already looked at
this from a GCM perspective.

[jianli, 2014-06-17 22:05:18]

lgtm

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

https://codereview.chromium.org/338363003/diff/30001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:130:
DidRegister(app_id, callback, "", GCMClient::GCM_DISABLED);
It is not about a different error code we return. Since this is not going to be
hit, could you please simply use NOTREACHED().

[jochen (gone - plz use gerrit), 2014-06-18 03:58:40]

lgtm

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

https://codereview.chromium.org/338363003/diff/30001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:24: static const char
kAppIdPrefix[] = "push:";
no static

https://codereview.chromium.org/338363003/diff/30001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:50: return;
nit { }

https://codereview.chromium.org/338363003/diff/30001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:52:
GCMProfileService* gcm_service = gcm::GCMProfileServiceFactory::GetForProfile(
is this clang-formatted?

https://codereview.chromium.org/338363003/diff/30001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:162:
registration_count + 1);
what do you do on overflow?

https://codereview.chromium.org/338363003/diff/30001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/338363003/diff/30001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver.cc:151: DCHECK(app_handlers_.count(app_id) ==
0);
DCHECK_EQ

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

Thanks for the quick reviews!

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

https://codereview.chromium.org/338363003/diff/30001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:24: static const char
kAppIdPrefix[] = "push:";
On 2014/06/18 03:58:40, jochen traveling until Jun 23 wrote:
> no static

Done.

https://codereview.chromium.org/338363003/diff/30001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:50: return;
On 2014/06/18 03:58:40, jochen traveling until Jun 23 wrote:
> nit { }

Done.

https://codereview.chromium.org/338363003/diff/30001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:52:
GCMProfileService* gcm_service = gcm::GCMProfileServiceFactory::GetForProfile(
On 2014/06/18 03:58:40, jochen traveling until Jun 23 wrote:
> is this clang-formatted?

Done.

https://codereview.chromium.org/338363003/diff/30001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:130:
DidRegister(app_id, callback, "", GCMClient::GCM_DISABLED);
On 2014/06/17 22:05:17, jianli wrote:
> It is not about a different error code we return. Since this is not going to
be
> hit, could you please simply use NOTREACHED().

Done.

https://codereview.chromium.org/338363003/diff/30001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:162:
registration_count + 1);
On 2014/06/18 03:58:40, jochen traveling until Jun 23 wrote:
> what do you do on overflow?

While I don't expect anyone will get that far, let's not leave it to chance. I
added a check in Register to fail at a reasonably high limit.

https://codereview.chromium.org/338363003/diff/30001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/338363003/diff/30001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver.cc:151: DCHECK(app_handlers_.count(app_id) ==
0);
On 2014/06/18 03:58:40, jochen traveling until Jun 23 wrote:
> DCHECK_EQ

Done.

[Michael van Ouwerkerk, 2014-06-18 12:09:10]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-06-18 12:10:34]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/338363003/5...

[commit-bot: I haz the power, 2014-06-18 15:26:32]

Change committed as 278087