Integrate AppNotifyChannelSetup with TokenService.

Integrate AppNotifyChannelSetup with TokenService.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110410

[Munjal (Google), 2011-11-11 00:43:47]

Rick, it will be nice if you can also take a look apart from Antony.

[asargent_no_longer_on_chrome, 2011-11-11 01:23:03]

LGTM

http://codereview.chromium.org/8526015/diff/1004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8526015/diff/1004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notify_channel_setup.cc:170: void
AppNotifyChannelSetup::UnregisterForTokenServiceNotifications() {
You could probably skip having this entire method, and instead just call
registrar_.RemoveAll() in ::EndFetchTokens.

[Munjal (Google), 2011-11-11 01:49:27]

http://codereview.chromium.org/8526015/diff/1004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8526015/diff/1004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notify_channel_setup.cc:170: void
AppNotifyChannelSetup::UnregisterForTokenServiceNotifications() {
On 2011/11/11 01:23:04, Antony Sargent wrote:
> You could probably skip having this entire method, and instead just call
> registrar_.RemoveAll() in ::EndFetchTokens.

Done. I kept the Unregister* method just for a slight readability benefit so
match with Register*.

[Rick Campbell, 2011-11-11 21:18:58]

http://codereview.chromium.org/8526015/diff/5002/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8526015/diff/5002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notify_channel_setup.cc:103: if
(fetch_token_success_count_ + fetch_token_fail_count_ == 2) {
I'll defer to others' judgement here, but I'm uncomfortable with the literal
constants here and just a few lines down.

http://codereview.chromium.org/8526015/diff/5002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notify_channel_setup.cc:114: return
!token_service->HasTokenForService(GaiaConstants::kLSOService) ||
Similarly here -- clearly related to the literal 2 on which I commented above. 
And then again below with IsGaiaServiceRelevant.
Perhaps it makes sense to factor out the service list, and it's size.

http://codereview.chromium.org/8526015/diff/5002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notify_channel_setup.cc:155: // static
Similarly here -- clearly related to the literal 2 on which I commented above. 
Perhaps it makes sense to factor out the service list, and it's size.

[Munjal (Google), 2011-11-14 23:03:15]

http://codereview.chromium.org/8526015/diff/5002/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8526015/diff/5002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notify_channel_setup.cc:103: if
(fetch_token_success_count_ + fetch_token_fail_count_ == 2) {
On 2011/11/11 21:18:58, Rick Campbell wrote:
> I'll defer to others' judgement here, but I'm uncomfortable with the literal
> constants here and just a few lines down.

Done.

http://codereview.chromium.org/8526015/diff/5002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notify_channel_setup.cc:114: return
!token_service->HasTokenForService(GaiaConstants::kLSOService) ||
On 2011/11/11 21:18:58, Rick Campbell wrote:
> Similarly here -- clearly related to the literal 2 on which I commented above.

> And then again below with IsGaiaServiceRelevant.
> Perhaps it makes sense to factor out the service list, and it's size.

Done.

http://codereview.chromium.org/8526015/diff/5002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notify_channel_setup.cc:155: // static
On 2011/11/11 21:18:58, Rick Campbell wrote:
> Similarly here -- clearly related to the literal 2 on which I commented above.

> Perhaps it makes sense to factor out the service list, and it's size.

Done.

[Rick Campbell, 2011-11-16 18:13:35]

lgtm with one nit

http://codereview.chromium.org/8526015/diff/9001/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8526015/diff/9001/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notify_channel_setup.cc:53: static const char*
kRelevantGaiaServices[kRelevantGaiaServicesCount] = {
Probably better to let the dimension be implicit and then initialize
kRelevantGaiaServicesCount using arraysize from base/basictypes.h

[Munjal (Google), 2011-11-16 19:54:46]

http://codereview.chromium.org/8526015/diff/9001/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8526015/diff/9001/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notify_channel_setup.cc:53: static const char*
kRelevantGaiaServices[kRelevantGaiaServicesCount] = {
On 2011/11/16 18:13:35, Rick Campbell wrote:
> Probably better to let the dimension be implicit and then initialize
> kRelevantGaiaServicesCount using arraysize from base/basictypes.h

Done.

[Rick Campbell, 2011-11-16 19:59:29]

LGTM