[GCM] Handling connection events in GCMAccountTracker

This patch is:
* Adding handling of connection events to the tracker
* Adding a method sanitizing tokens (removing expired ones)
* Adding relevant account tracker tests
* Making account mapper aware of gcm driver being started
* Making sure ShutdownHandler implementation is safe to
  call RemoveAppHandler (without causing a stack trace due
  to invalid iterator)
* Making sure that account mapper does not count toward
  registered app handlers when deciding to stop GCM Driver
* Updating FakeOAuth2TokenService to allow token request to
  be repeated.

Based of patch: https://codereview.chromium.org/617003005/

BUG=374969
TBR=rogerta@chromium.org

Committed: https://crrev.com/83afd876646fc45deb677780d16b615915cd56eb
Cr-Commit-Position: refs/heads/master@{#299804}

[fgorski, 2014-10-03 18:00:28]

fgorski@chromium.org changed reviewers:
+ jianli@chromium.org, rogerta@chromium.org

[fgorski, 2014-10-03 18:03:32]

PTAL

Roger, I have a small update in FakeOAuth2TokenService.
Jian Li, the rest of the code.

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

https://codereview.chromium.org/618003002/diff/80001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver.cc:154: iter != app_handlers_.end();) {
Jian Li, what do you think about this update?

[Roger Tawa OOO till Jul 10th, 2014-10-06 13:40:58]

lgtm fake_oauth2_token_service.cc

[fgorski, 2014-10-07 16:36:43]

fgorski@chromium.org changed reviewers:
+ zea@chromium.org
- jianli@chromium.org

[fgorski, 2014-10-07 16:36:43]

Nicolas, given that Jian Li is out this week. Could you take a look?

[Nicolas Zea, 2014-10-08 00:36:35]

LGTM with some nits

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.cc:202: ++iter;
nit: Why not have ++iter in the for loop itself?

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:59: // Creates an instance of
GCMAccountTracker. |account_tracker| is used to
nit: "Creates an instance.." isn't really necessary. All constructors do that.

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:98: virtual void
OnConnected(const net::IPEndPoint& ip_endpoint) OVERRIDE;
nit: OVERRIDE -> override (chromium is using the C++11 versions now)

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:107: bool SanitizeTokens();
nit: I think it might be a bit clearer if returning true meant success, i.e.
everything is ready, and returning false meant failure, i.e. tokens are still
needed.

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker_unittest.cc (right):

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker_unittest.cc:66: class
CustomFakeGCMDriver : public FakeGCMDriver {
nit: comment what it does differently from the normal fake?

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker_unittest.cc:72: const
std::vector<GCMClient::AccountTokenInfo>& account_tokens) OVERRIDE;
nit: OVERRIDE -> override

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker_unittest.cc:78: void
set_connected(bool connected) {
uber nit: the hacker_style inline method is generally just for setters/getters.
This is also calling a virtual observer method, so should be named SetConnected.

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker_unittest.cc:237: //
TODO(fgorsk): This actually need fixing.
Is this comment still relevant?

https://codereview.chromium.org/618003002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_account_mapper.cc (left):

https://codereview.chromium.org/618003002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_account_mapper.cc:51: DCHECK(!initialized_);
can this be initialized more than once now?

https://codereview.chromium.org/618003002/diff/140001/google_apis/gaia/fake_o...
File google_apis/gaia/fake_oauth2_token_service.cc (right):

https://codereview.chromium.org/618003002/diff/140001/google_apis/gaia/fake_o...
google_apis/gaia/fake_oauth2_token_service.cc:85: // Using a copy to make sure
retyring a request does not cause a stack when
nit:
retyring -> retrying
cause a stack -> cause a stack overflow

Also have comment int he above code too?

[fgorski, 2014-10-08 18:04:28]

Nicolas, PTAL.

I rewrote that loop with the iterator incremented inside to 2 loops. Please let
me know what you think.

Also, it currently has a NOTREACHED, which is like a DCHECK_EQ(status,
TOKEN_PRESENT).

Given the logic and state transitions, this could be even a CHECK_EQ, but I want
to be careful about that. Let me know what you think.

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.cc:202: ++iter;
On 2014/10/08 00:36:35, Nicolas Zea wrote:
> nit: Why not have ++iter in the for loop itself?

Invalidates the iterator in 178. Since it does not affect complexity, I'll
extract the removing to a separate loop.

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:59: // Creates an instance of
GCMAccountTracker. |account_tracker| is used to
On 2014/10/08 00:36:35, Nicolas Zea wrote:
> nit: "Creates an instance.." isn't really necessary. All constructors do that.

Done.

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:98: virtual void
OnConnected(const net::IPEndPoint& ip_endpoint) OVERRIDE;
On 2014/10/08 00:36:35, Nicolas Zea wrote:
> nit: OVERRIDE -> override (chromium is using the C++11 versions now)

Done.

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:107: bool SanitizeTokens();
On 2014/10/08 00:36:35, Nicolas Zea wrote:
> nit: I think it might be a bit clearer if returning true meant success, i.e.
> everything is ready, and returning false meant failure, i.e. tokens are still
> needed.

I agree, but with the next patch I'm actually removing return type.
SanitizeTokens will clean up. IsTokenFetchingNeeded() will be returning the
proper information.

This will be resolved in 631343002

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker_unittest.cc (right):

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker_unittest.cc:66: class
CustomFakeGCMDriver : public FakeGCMDriver {
On 2014/10/08 00:36:35, Nicolas Zea wrote:
> nit: comment what it does differently from the normal fake?

Done.

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker_unittest.cc:72: const
std::vector<GCMClient::AccountTokenInfo>& account_tokens) OVERRIDE;
On 2014/10/08 00:36:35, Nicolas Zea wrote:
> nit: OVERRIDE -> override

Done.

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker_unittest.cc:78: void
set_connected(bool connected) {
On 2014/10/08 00:36:35, Nicolas Zea wrote:
> uber nit: the hacker_style inline method is generally just for
setters/getters.
> This is also calling a virtual observer method, so should be named
SetConnected.

Uber done.

https://codereview.chromium.org/618003002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker_unittest.cc:237: //
TODO(fgorsk): This actually need fixing.
On 2014/10/08 00:36:35, Nicolas Zea wrote:
> Is this comment still relevant?

Removing.

https://codereview.chromium.org/618003002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_account_mapper.cc (left):

https://codereview.chromium.org/618003002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_account_mapper.cc:51: DCHECK(!initialized_);
On 2014/10/08 00:36:35, Nicolas Zea wrote:
> can this be initialized more than once now?

Initially it was a problem, as we discovered and fixed the purge issue =>
signing in after sign out is calling initialization again...

I spent so much time fixing the code and then forgot to reintroduce the check.
Fixed.

https://codereview.chromium.org/618003002/diff/140001/google_apis/gaia/fake_o...
File google_apis/gaia/fake_oauth2_token_service.cc (right):

https://codereview.chromium.org/618003002/diff/140001/google_apis/gaia/fake_o...
google_apis/gaia/fake_oauth2_token_service.cc:85: // Using a copy to make sure
retyring a request does not cause a stack when
On 2014/10/08 00:36:35, Nicolas Zea wrote:
> nit:
> retyring -> retrying
> cause a stack -> cause a stack overflow
> 
> Also have comment int he above code too?

Done.

[Nicolas Zea, 2014-10-09 21:39:03]

lgtm

[fgorski, 2014-10-09 21:44:39]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-10-09 21:46:06]

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

[commit-bot: I haz the power, 2014-10-09 22:08:50]

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

[commit-bot: I haz the power, 2014-10-09 22:08:51]

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

[fgorski, 2014-10-10 18:45:51]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-10-10 18:47:02]

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

[commit-bot: I haz the power, 2014-10-10 20:13:24]

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

[commit-bot: I haz the power, 2014-10-10 20:13:24]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fgorski, 2014-10-10 22:41:06]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-10-10 22:41:39]

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

[commit-bot: I haz the power, 2014-10-10 23:51:20]

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

[commit-bot: I haz the power, 2014-10-10 23:51:21]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fgorski, 2014-10-15 21:13:36]

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

[fgorski, 2014-10-15 21:13:37]

asvitkine@chromium.org: Please review changes in 

profile_chooser_controller_unittest.mm

I am faking one of the services that requires some thread switching.

Thanks.
Filip

[Alexei Svitkine (slow), 2014-10-15 21:15:50]

lgtm

[fgorski, 2014-10-15 21:41:39]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-10-15 21:42:34]

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

[commit-bot: I haz the power, 2014-10-16 01:12:02]

Committed patchset #11 (id:670001)

[commit-bot: I haz the power, 2014-10-16 01:12:51]

Patchset 11 (id:??) landed as
https://crrev.com/83afd876646fc45deb677780d16b615915cd56eb
Cr-Commit-Position: refs/heads/master@{#299804}