New issue
Advanced search Search tips

Issue 809923 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 729590
issue 796544
issue 809927
issue 840703



Sign in to add a comment

Convert GCM to talk to Identity Service client library

Project Member Reported by blundell@chromium.org, Feb 7 2018

Issue description

Currently //components/gcm_driver interacts with the user's Google identity via ProfileIdentityProvider and AccountTracker (the latter being backed by the former). GCMAccountTracker uses these classes to make access token requests for all accounts known to AccountTracker.

This should change to interacting with the Identity Service client library. 
 
As an addendum, note that when looking at GCMCAccountTracker, it is ambiguous whether this class can take in an AccountTracker instance that is backed by DeviceIdentityProvider. However, I verified that the only instance of GCMAccountTracker creation is here: https://cs.chromium.org/chromium/src/components/gcm_driver/gcm_profile_service.cc?type=cs&q=%22GCMAccountTracker(%22&sq=package:chromium&l=113

and that the |gaia_account_tracker| being supplied there is backed by a ProfileIdentityProvider instance:
https://cs.chromium.org/chromium/src/components/gcm_driver/gcm_profile_service.cc?type=cs&q=%22GCMAccountTracker(%22&sq=package:chromium&l=158
Blocking: 809927
Blocking: 796544
Components: Internals>Services>Identity
Status: Available (was: Untriaged)
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/021e3e4d708a695a7714daf1eae89cbfa4a07708

commit 021e3e4d708a695a7714daf1eae89cbfa4a07708
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Apr 19 01:03:24 2018

Move gaia::AccountTracker into //components/gcm_driver

This class is deprecated and slated for removal. It now has only one
remaining consumer. This CL moves it into the directory of that consumer,
with followup merging it into the code of that consumer.

This CL also excludes AccountTracker and GCMAccountTracker from the
build on Android, as they're not used on that platform.

Bug: 729590,  809923 
Change-Id: I225edf86a3af293779a8cc231ffffac711f22246
Reviewed-on: https://chromium-review.googlesource.com/1002845
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551904}
[modify] https://crrev.com/021e3e4d708a695a7714daf1eae89cbfa4a07708/components/gcm_driver/BUILD.gn
[rename] https://crrev.com/021e3e4d708a695a7714daf1eae89cbfa4a07708/components/gcm_driver/account_tracker.cc
[rename] https://crrev.com/021e3e4d708a695a7714daf1eae89cbfa4a07708/components/gcm_driver/account_tracker.h
[rename] https://crrev.com/021e3e4d708a695a7714daf1eae89cbfa4a07708/components/gcm_driver/account_tracker_unittest.cc
[modify] https://crrev.com/021e3e4d708a695a7714daf1eae89cbfa4a07708/components/gcm_driver/gcm_account_tracker.cc
[modify] https://crrev.com/021e3e4d708a695a7714daf1eae89cbfa4a07708/components/gcm_driver/gcm_account_tracker.h
[modify] https://crrev.com/021e3e4d708a695a7714daf1eae89cbfa4a07708/components/gcm_driver/gcm_account_tracker_unittest.cc
[modify] https://crrev.com/021e3e4d708a695a7714daf1eae89cbfa4a07708/components/gcm_driver/gcm_profile_service.cc
[modify] https://crrev.com/021e3e4d708a695a7714daf1eae89cbfa4a07708/google_apis/BUILD.gn

Project Member

Comment 5 by bugdroid1@chromium.org, May 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dee2d72ad9cc43c9e8362f32186490c8c9283e83

commit dee2d72ad9cc43c9e8362f32186490c8c9283e83
Author: Colin Blundell <blundell@chromium.org>
Date: Mon May 07 10:46:52 2018

[GCM] Use SigninManager in GCMProfileService::IdentityObserver

As a first step toward completely eliminating the usage of
ProfileIdentityProvider in //components/gcm_driver, this CL threads in
SigninManager and replaces direct usage of ProfileIdentityProvider in
GCMProfileService::IdentityObserver with equivalent usage of
SigninManager.

Note that it is easy to verify that the usage is equivalent:
The ProfileIdentityProvider method calls are replaced with their
implementations in profile_identity_provider.cc.

Full design doc here:
https://docs.google.com/document/d/1OmNrIiMDkF7eOYVB4RIiDvY7pQz3zUrOf-D4TVwdRQA/edit?ts=5aa6b936#

TBR=zea@chromium.org, rockot@chromium.org

Bug:  809923 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I20515d7431f0e8f5370a47cd04c8b21eb0874d6f
Reviewed-on: https://chromium-review.googlesource.com/1004999
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556408}
[modify] https://crrev.com/dee2d72ad9cc43c9e8362f32186490c8c9283e83/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
[modify] https://crrev.com/dee2d72ad9cc43c9e8362f32186490c8c9283e83/chrome/browser/gcm/gcm_profile_service_factory.cc
[modify] https://crrev.com/dee2d72ad9cc43c9e8362f32186490c8c9283e83/chrome/browser/gcm/gcm_profile_service_unittest.cc
[modify] https://crrev.com/dee2d72ad9cc43c9e8362f32186490c8c9283e83/components/gcm_driver/gcm_profile_service.cc
[modify] https://crrev.com/dee2d72ad9cc43c9e8362f32186490c8c9283e83/components/gcm_driver/gcm_profile_service.h
[modify] https://crrev.com/dee2d72ad9cc43c9e8362f32186490c8c9283e83/ios/chrome/browser/services/gcm/ios_chrome_gcm_profile_service_factory.cc

Comment 6 by treib@chromium.org, May 8 2018

Cc: treib@chromium.org

Comment 7 by treib@chromium.org, May 8 2018

Blocking: 840703
Project Member

Comment 8 by bugdroid1@chromium.org, May 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/04720563e44ad0aaf1388917bf1bb79d74fa563e

commit 04720563e44ad0aaf1388917bf1bb79d74fa563e
Author: Colin Blundell <blundell@chromium.org>
Date: Mon May 14 09:19:40 2018

Rationalize (GCM)AccountTracker unittests for ChromeOS

The GCMAccountTracker and AccountTracker classes listen for the
GoogleSigninSucceeded/GoogleSignedOut callbacks indirectly via
ProfileIdentityProvider. Their unittests cause these callbacks to fire
via calls to FakeIdentityProvider::{LogIn, LogOut}.

These classes are also used on ChromeOS, where the callbacks will never
fire (ChromeOS does not use SigninManager, which fires the callbacks).
The structure of these unittests presents a challenge to refactoring the
production code to depend on SigninManager directly: the
SigninManagerBase test infrastructure, which is all that can be used
on ChromeOS, rightfully has no mechanism for firing the callbacks.

This CL rationalizes these tests for ChromeOS via:
- Commenting out the tests that actually exercise the callbacks firing
- Changing the rest of the tests to simply set the primary account info
  *without* causing the callbacks to fire
- Renaming utility functions as relevant to make the structure clear

Making this change paves the way for a followup change that ports these
classes (and their tests) to depend directly on SigninManager.

Bug:  809923 
Change-Id: Ia6c067d845f7bbb7bd78928ac163bda2fdd77a20
Reviewed-on: https://chromium-review.googlesource.com/1042391
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558235}
[modify] https://crrev.com/04720563e44ad0aaf1388917bf1bb79d74fa563e/components/gcm_driver/account_tracker_unittest.cc
[modify] https://crrev.com/04720563e44ad0aaf1388917bf1bb79d74fa563e/components/gcm_driver/gcm_account_tracker_unittest.cc
[modify] https://crrev.com/04720563e44ad0aaf1388917bf1bb79d74fa563e/google_apis/gaia/fake_identity_provider.cc
[modify] https://crrev.com/04720563e44ad0aaf1388917bf1bb79d74fa563e/google_apis/gaia/fake_identity_provider.h

Project Member

Comment 9 by bugdroid1@chromium.org, May 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf6d4c5f3a083a5f5ea7264b8e5f47083887b6cf

commit bf6d4c5f3a083a5f5ea7264b8e5f47083887b6cf
Author: Colin Blundell <blundell@chromium.org>
Date: Wed May 16 11:12:29 2018

[GCM] Use SigninManager in AccountTracker

As a step toward completely eliminating the usage of
ProfileIdentityProvider in //components/gcm_driver, this CL replaces
direct usage of ProfileIdentityProvider in AccountTracker with
equivalent usage of SigninManager.

Note that it is easy to verify that the usage is equivalent:
- The IdentityProvider instance passed in to AccountTracker is always
  a ProfileIdentityProvider instance.
- The IdentityProvider method calls are replaced with their
  implementations in profile_identity_provider.cc.

The production changes are straightforward, but substantial changes are
required to the GCMAccountTracker and AccountTracker unittests to have
them use FakeSigninManager(Base) where they were previously using
FakeIdentityProvider. In particular, the GCMAccountTracker unittest
now requires an explicit separation of when an account being added is
the primary account vs. a secondary account (whereas
FakeIdentityProvider was happy to simply override its value for the
active account, SigninManager does not allow for signing in when there
is already a signed-in account).

The goop required to set up this fake infrastructure in the unittests
will be substantially simplified when we complete the process of
converting the production code to talk entirely to IdentityManager.

Note that for the moment IdentityProvider is still used by
AccountTracker to access the token service. A followup CL will
pass in the token service directly and completely eliminate
AccountTracker's usage of IdentityProvider.

Full design doc here:
https://docs.google.com/document/d/1OmNrIiMDkF7eOYVB4RIiDvY7pQz3zUrOf-D4TVwdRQA/edit?ts=5aa6b936#

Bug:  809923 
Change-Id: Ifef69551a215571362610167c3a4cb38c932ff70
Reviewed-on: https://chromium-review.googlesource.com/1046587
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559036}
[modify] https://crrev.com/bf6d4c5f3a083a5f5ea7264b8e5f47083887b6cf/components/gcm_driver/BUILD.gn
[modify] https://crrev.com/bf6d4c5f3a083a5f5ea7264b8e5f47083887b6cf/components/gcm_driver/DEPS
[modify] https://crrev.com/bf6d4c5f3a083a5f5ea7264b8e5f47083887b6cf/components/gcm_driver/account_tracker.cc
[modify] https://crrev.com/bf6d4c5f3a083a5f5ea7264b8e5f47083887b6cf/components/gcm_driver/account_tracker.h
[modify] https://crrev.com/bf6d4c5f3a083a5f5ea7264b8e5f47083887b6cf/components/gcm_driver/account_tracker_unittest.cc
[modify] https://crrev.com/bf6d4c5f3a083a5f5ea7264b8e5f47083887b6cf/components/gcm_driver/gcm_account_tracker_unittest.cc
[modify] https://crrev.com/bf6d4c5f3a083a5f5ea7264b8e5f47083887b6cf/components/gcm_driver/gcm_profile_service.cc

Project Member

Comment 10 by bugdroid1@chromium.org, May 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f878993246cf9636795648f4e0183328dd835e6d

commit f878993246cf9636795648f4e0183328dd835e6d
Author: Colin Blundell <blundell@chromium.org>
Date: Tue May 22 11:35:56 2018

[GCM] Use PO2TS directly rather than via ProfileIdentityProvider

To complete the elimination of usage of
ProfileIdentityProvider in //components/gcm_driver, this CL replaces
usage of ProfileIdentityProvider in (GCM)AccountTracker to obtain the
Oauth2TokenService with direct usage of the ProfileOAuth2TokenService
instance that is backing the ProfileIdentityProvider instance. This CL
is a step on the road to eventually using IdentityManager in
//components/gcm_driver.

The CL is completely straightforward.

Full design doc here:
https://docs.google.com/document/d/1OmNrIiMDkF7eOYVB4RIiDvY7pQz3zUrOf-D4TVwdRQA/edit?ts=5aa6b936#

TBR=rockot@chromium.org, zea@chromium.org

Bug:  809923 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I4a8c443851b0d4bc660656f528d0cd5a32a62bc6
Reviewed-on: https://chromium-review.googlesource.com/1046969
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560535}
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/chrome/browser/gcm/gcm_profile_service_factory.cc
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/chrome/browser/gcm/gcm_profile_service_unittest.cc
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/components/gcm_driver/account_tracker.cc
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/components/gcm_driver/account_tracker.h
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/components/gcm_driver/account_tracker_unittest.cc
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/components/gcm_driver/gcm_account_tracker.cc
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/components/gcm_driver/gcm_account_tracker.h
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/components/gcm_driver/gcm_account_tracker_unittest.cc
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/components/gcm_driver/gcm_profile_service.cc
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/components/gcm_driver/gcm_profile_service.h
[modify] https://crrev.com/f878993246cf9636795648f4e0183328dd835e6d/ios/chrome/browser/services/gcm/ios_chrome_gcm_profile_service_factory.cc

Owner: blundell@chromium.org
Status: Started (was: Available)
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e197b29404a4db3a68fc0ff08da484feeb0700ca

commit e197b29404a4db3a68fc0ff08da484feeb0700ca
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jun 22 14:06:39 2018

Refactor how GCMAccountTracker stores pending token requests

In preparation for porting GCM to use IdentityManager to fetch access
tokens rather than ProfileOAuth2TokenService, this CL refactors how
GCMAccountTracker internally stores pending requests. Currently it
stores them as a vector of raw OAuth2TokenService::Request pointers;
when such a pointer gets passed in OnGetToken{Success, Failure}, it
looks it up in the vector to delete it. However, the IdentityManager
facilities do not pass pointers to the analogous "request" objects
(called AccessTokenFetcher), while the client is still responsible
for deleting those objects when desired.

This CL changes GCMAccountTracker to maintain a map of account IDs
to OAuth2TokenService::Request unique_ptrs. When a request is
complete, it simply erases the entry in the map by key. One hiccup
is that GCMAccountTracker currently can make two concurrent requests
for the same account:

- It makes token requests for accounts in AccountTracker's list
  of accounts that are in GCMAccountTracker's TOKEN_NEEDED state.
- When it makes a request, it transitions the state of the account to
  GETTING_TOKEN.
- When it receives a notification that an account has signed out,
  it transitions that account to ACCOUNT_REMOVED state.
- When it receives a notification that an account has signed in,
  if it has the account in ACCOUNT_REMOVED state it transitions that
  account to GETTING_TOKEN state and refetches access tokens.

However, GCMAccountTracker already short-circuits out of taking any
action on completion of access token requests if the account is in
ACCOUNT_REMOVED state (beyond deleting the request).  This
CL simply changes GCMAccountTracker to never make two concurrent
token requests for the same account by deleting a token request when
transitioning an account to ACCOUNT_REMOVED state.

Bug:  809923 
Change-Id: I2bae540133ae4db3f2cfea47332b2907dd88dd6d
Reviewed-on: https://chromium-review.googlesource.com/1104689
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569599}
[modify] https://crrev.com/e197b29404a4db3a68fc0ff08da484feeb0700ca/components/gcm_driver/gcm_account_tracker.cc
[modify] https://crrev.com/e197b29404a4db3a68fc0ff08da484feeb0700ca/components/gcm_driver/gcm_account_tracker.h

Project Member

Comment 13 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aa0f4f090b990d9498bbafd3b8387d20a0fea9bc

commit aa0f4f090b990d9498bbafd3b8387d20a0fea9bc
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Jun 28 14:41:45 2018

Fix GCMAccountTracker unittest

Noticed while preparing
https://chromium-review.googlesource.com/c/chromium/src/+/1113548: this
test currently does not test what it is intended to. It is intended to
test that when an ongoing access token request is fulfilled with an
expired token, GCMAccountTracker notices that the token is no longer
valid and marks the account as needing a token request. However, the
current structure of the test prevents any access token request ever
being made for the newly-added account: the account is added when the
driver isn't connected, in which case GCMAccountTracker doesn't make
access token requests. Hence, the test passes even if the call to
IssueExpiredAccessToken() is replaced by a call to IssueAccessToken().

This CL changes the test to match its intention: we add the account
with the driver connected and check that the request was started, then
disconnect the driver before fulfilling the request (so that another
token request isn't made immediately), and *then* check that a new
token request is now marked as being needed. In particular, the test
now fails if the replacement mentioned in the previous paragraph is
made.

Bug:  809923 
Change-Id: Idc2458d4a0d8e9e702ac8a61cdff9ad138e16fa3
Reviewed-on: https://chromium-review.googlesource.com/1113677
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571118}
[modify] https://crrev.com/aa0f4f090b990d9498bbafd3b8387d20a0fea9bc/components/gcm_driver/gcm_account_tracker_unittest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Jun 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1b7c909f68e09954e8716e12170755256108818

commit a1b7c909f68e09954e8716e12170755256108818
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jun 29 06:28:06 2018

Make account components explicit in GCMAccountTracker unittest

The GCMAccountTracker unittest currently plays fast-and-loose with the
notion of an "account ID", which can be either the Gaia ID or the
email address for a given account depending on the platform. This is a
blocker to porting GCMAccountTracker to use IdentityManager, as the
IdentityManager test infrastructure deliberately makes a much more
explicit and enforced separation between email address, Gaia ID, and
account ID. This CL paves the way for that porting by changing the
GCMAccountTracker unittest to be explicit about its usage of the
email address, Gaia ID, and account ID of a given account.

Interestingly, this change exposed an actual production bug
(https://crbug.com/856170).

Bug:  809923 
Change-Id: I46e2f632bacd89ba55132255a95bcb089d4fb3c8
Reviewed-on: https://chromium-review.googlesource.com/1113548
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571408}
[modify] https://crrev.com/a1b7c909f68e09954e8716e12170755256108818/components/gcm_driver/gcm_account_tracker_unittest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Jul 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/354f2210efffdbe578d4342f625b00e76332d360

commit 354f2210efffdbe578d4342f625b00e76332d360
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jul 16 14:45:08 2018

[GCM] Port GCMAccountTracker to use IdentityManager

The change is straightforward: Rather than using
ProfileOAuth2TokenService to make access token requests,
GCMAccountTracker now constructs AccessTokenFetchers via
IdentityManager. The substantive changes in the CL are the ones in
gcm_account_tracker.*. Everything else is just various types of
plumbing.

Note that in the near-term I will be removing the need for
GCMProfileService to know about SigninManager/PO2TS at all, leaving
it taking in only IdentityManager. However, that change is blocked
on converting the rest of the code in //components/gcm_driver to
talk to IdentityManager.

TBR=zea@chromium.org, rockot@chromium.org

Bug:  809923 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I12d2f1998d398a9081355c55fb56761cf660a753
Reviewed-on: https://chromium-review.googlesource.com/1113740
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575256}
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/chrome/browser/gcm/gcm_profile_service_factory.cc
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/chrome/browser/gcm/gcm_profile_service_unittest.cc
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/components/gcm_driver/BUILD.gn
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/components/gcm_driver/DEPS
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/components/gcm_driver/gcm_account_tracker.cc
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/components/gcm_driver/gcm_account_tracker.h
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/components/gcm_driver/gcm_account_tracker_unittest.cc
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/components/gcm_driver/gcm_profile_service.cc
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/components/gcm_driver/gcm_profile_service.h
[modify] https://crrev.com/354f2210efffdbe578d4342f625b00e76332d360/ios/chrome/browser/gcm/ios_chrome_gcm_profile_service_factory.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Jul 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1e3e4808476c11a75a9341c8fae3726fec86d30

commit f1e3e4808476c11a75a9341c8fae3726fec86d30
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Jul 17 07:29:47 2018

[GCM] Port GCMProfileService to use IdentityManager

Part of the porting of //components/gcm_driver to use IdentityManager
rather than SigninManager and ProfileOAuth2TokenService. This particular
change is completely straightforward, with GCMProfileService's helper
class IdentityObserver observing IdentityManager rather than
SigninManager.

Bug:  809923 
Change-Id: I821893895b2b122cec852a5bbe0c62f1a0b52670
Reviewed-on: https://chromium-review.googlesource.com/1113926
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575571}
[modify] https://crrev.com/f1e3e4808476c11a75a9341c8fae3726fec86d30/components/gcm_driver/gcm_profile_service.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Jul 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39ba6f46c46947a80d2c36026bedbbec8d5e50fd

commit 39ba6f46c46947a80d2c36026bedbbec8d5e50fd
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Jul 24 07:11:31 2018

Make account components explicit in AccountTracker unittest

The AccountTracker unittest currently plays fast-and-loose with the
notion of an "account ID", which can be either the Gaia ID or the
email address for a given account depending on the platform. This is a
blocker to porting AccountTracker to use IdentityManager, as the
IdentityManager test infrastructure deliberately makes a much more
explicit and enforced separation between email address, Gaia ID, and
account ID, reflecting how these elements are used in production.
In particular:
- Adding an account takes in the gaia ID and email and returns an
  account ID.
- Calls that interact with the token service take in the account ID.

This CL paves the way for that porting by changing the
AccountTracker unittest to be explicit about its usage of the
email address, Gaia ID, and account ID of a given account. The test now
adds accounts by passing the gaia ID/email, obtains the account IDs of
the added accounts, and interacts with the token service via those
account IDs.

Bug:  809923 
Change-Id: Ia0bf7fc90c035976f8c578563e7d81c597d4d635
Reviewed-on: https://chromium-review.googlesource.com/1138082
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577444}
[modify] https://crrev.com/39ba6f46c46947a80d2c36026bedbbec8d5e50fd/components/gcm_driver/account_tracker_unittest.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Jul 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98dd473e0e0bcb8e8f4510aae824d6f1a876715d

commit 98dd473e0e0bcb8e8f4510aae824d6f1a876715d
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jul 27 10:41:45 2018

AccountTracker: Minor code cleanup

Currently, AccountTracker::GoogleSigninSucceeded() calls
AccountTracker::OnRefreshTokenAvailable() for each account that the
token service knows about. However, this is conceptually somewhat
confusing, as it's not the case that the refresh tokens for those
accounts have suddenly become available. Moreover, an upcoming
conversion of this class to use IdentityManager will make it even less
suitable, as the corresponding observer callback passes information
that AccountTracker won't have (and doesn't need) when invoking it
manually.

This CL changes AccountTracker::GoogleSigninSucceeded() to directly
call through to AccountTracker::UpdateSigninState(), better reflecting
the actual flow.

Bug:  809923 
Change-Id: Ic7673659cb9383d55eb8eda5cfb84a22f3a72214
Reviewed-on: https://chromium-review.googlesource.com/1150223
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578585}
[modify] https://crrev.com/98dd473e0e0bcb8e8f4510aae824d6f1a876715d/components/gcm_driver/account_tracker.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Jul 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b5e688dcc4bffc28d177f53d4c2a17cafac3bc28

commit b5e688dcc4bffc28d177f53d4c2a17cafac3bc28
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jul 27 12:09:07 2018

IdentityManager test APIs: Add flexibility when clearing primary account

When clearing the primary account, three behaviors are possible:
1. Explicitly remove all accounts.
2. Explicitly keep all accounts.
3. Let the signin code internally decide between 1 and 2 based on its
own default policy.

The IdentityManager test APIS (IdentityTestEnvironment and
identity_test_utils.h) currently support only 3. However, an upcoming
conversion will require 1 and 2. This CL adds that support in to the
APIS for clearing the primary account.

Bug: 798699,  809923 
Change-Id: Idb717836c843a14a5f6d33aab5d60d7e14ff4d1c
Reviewed-on: https://chromium-review.googlesource.com/1149861
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578605}
[modify] https://crrev.com/b5e688dcc4bffc28d177f53d4c2a17cafac3bc28/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/b5e688dcc4bffc28d177f53d4c2a17cafac3bc28/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/b5e688dcc4bffc28d177f53d4c2a17cafac3bc28/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/b5e688dcc4bffc28d177f53d4c2a17cafac3bc28/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 20 by bugdroid1@chromium.org, Jul 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66ef2a877f14c2fa7b15b070c529e570eca617e6

commit 66ef2a877f14c2fa7b15b070c529e570eca617e6
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jul 27 13:00:48 2018

IdentityManager: Relax some constraints on test APIs

Various test APIs around IdentityManager currently DCHECK preconditions
when invoked (e.g., clearing the primary account DCHECKs that there is
a primary account). However, unittests for GCM that will shortly be
converted intentionally stress-test the system in various exceptional
ways, e.g., invoking the flow that the primary account was cleared when
there is no primary account. To accomodate porting those unittests
without having to reduce their coverage, this CL relaxes the constraints
on the relevant IdentityManager test APIs.

Bug: 798699,  809923 
Change-Id: Ibe2db519d0e231ea707ad09b329d6abc6ad7a9d2
Reviewed-on: https://chromium-review.googlesource.com/1149864
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578615}
[modify] https://crrev.com/66ef2a877f14c2fa7b15b070c529e570eca617e6/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/66ef2a877f14c2fa7b15b070c529e570eca617e6/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/66ef2a877f14c2fa7b15b070c529e570eca617e6/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 21 by bugdroid1@chromium.org, Jul 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4bf47a559863ac429f4c61d31abe827670390d15

commit 4bf47a559863ac429f4c61d31abe827670390d15
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jul 27 15:25:11 2018

IdentityTestEnv: Add API to wait for token requests for given account

The current IdentityTestEnvironment APIs to wait for access token
requests and issue responses operate in a shotgun manner: when the next
access token request is received, the test API implementation issues a
response for *all* pending access token requests. However, unittests
that is targeted in an upcoming conversion requires more flexibility:
as the production code that it is testing interacts with multiple
accounts, the test issues access token requests for multiple accounts,
and then sequentially issues responses and checks that a specific flow
for each account in question was initiated in response to the specific
response. Moreover, one of these unittests issues responses in an
order different to the order in which the requests were made.

To accommodate porting of these tests, this CL adds the ability to wait
for an access token request for a given account and then issue a
response only for that account. If while waiting for a request from a
specific account a request for a *different* account is seen, the
waiting code forwards on the handling of the request for that other
account to the next iteration of the runloop; this allows for the case
where a test wants to handle requests for multiple accounts in an
order different to the order in which those requests were made.

We leave in a variant that does not take in the account ID and operates
as before, as this is convenient for the common case of tests that don't
care about this level of detail (usually because they are testing a
production flow that operates on the primary account).

Bug: 798699,  809923 
Change-Id: I6b1d93efd47b1eba128ac0b62c833736700e5196
Reviewed-on: https://chromium-review.googlesource.com/1149876
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578647}
[modify] https://crrev.com/4bf47a559863ac429f4c61d31abe827670390d15/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/4bf47a559863ac429f4c61d31abe827670390d15/services/identity/public/cpp/identity_test_environment.h

Project Member

Comment 22 by bugdroid1@chromium.org, Aug 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3

commit 2b8d7478856e0f025ce3afd70ea874d7e07c9bb3
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Aug 02 07:27:52 2018

[GCM] Port AccountTracker to use IdentityManager

This CL completes the conversion of GCM's production code to talk
to IdentityManager rather than legacy signin classes by converting
AccountTracker. The GCMAccountTracker unittest remains to be fully
converted away from the legacy signin classes; this will be done in a
followup CL.

TBR=rohitrao@chromium.org, rockot@chromium.org

Bug:  809923 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I1513b33850f52b532d83ac389a78464b68ed1a45
Reviewed-on: https://chromium-review.googlesource.com/1148200
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580101}
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/chrome/browser/gcm/gcm_profile_service_factory.cc
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/chrome/browser/gcm/gcm_profile_service_unittest.cc
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/components/gcm_driver/BUILD.gn
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/components/gcm_driver/account_tracker.cc
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/components/gcm_driver/account_tracker.h
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/components/gcm_driver/account_tracker_unittest.cc
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/components/gcm_driver/gcm_account_tracker_unittest.cc
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/components/gcm_driver/gcm_profile_service.cc
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/components/gcm_driver/gcm_profile_service.h
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/ios/chrome/browser/gcm/ios_chrome_gcm_profile_service_factory.cc
[modify] https://crrev.com/2b8d7478856e0f025ce3afd70ea874d7e07c9bb3/ios/web_view/internal/sync/web_view_gcm_profile_service_factory.mm

Project Member

Comment 23 by bugdroid1@chromium.org, Aug 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/efba0776724d1b88d14a9856a7f78487a5a68e52

commit efba0776724d1b88d14a9856a7f78487a5a68e52
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Aug 02 13:40:44 2018

Port GCMAccountTracker unittest to IdentityTestEnvironment

This CL ports the GCMAccountTracker unittest to IdentityTestEnvironment;
this is possible now that AccountTracker is no longer using the
internal signin classes, and hence the GCMAccountTracker unittest does
not need to be aware of those internal signin classes to construct an
AccountTracker instance.

This CL completes the conversion of GCM away from the internal signin
classes and rips out the no-longer-needed dependencies.

TBR=msarda@chromium.org

Bug:  809923 
Change-Id: I78f999fc66543b8d64b85053fbcf72973ce8ac2e
Reviewed-on: https://chromium-review.googlesource.com/1152968
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580153}
[modify] https://crrev.com/efba0776724d1b88d14a9856a7f78487a5a68e52/components/gcm_driver/BUILD.gn
[modify] https://crrev.com/efba0776724d1b88d14a9856a7f78487a5a68e52/components/gcm_driver/DEPS
[modify] https://crrev.com/efba0776724d1b88d14a9856a7f78487a5a68e52/components/gcm_driver/gcm_account_tracker_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment