New issue
Advanced search Search tips

Issue 909715 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 8
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 903718
issue 908810

Blocking:
issue 883318
issue 883330



Sign in to add a comment

Eliminate account_investigator_unittest's knowledge of PO2TS, SigninManager, et al

Project Member Reported by blundell@chromium.org, Nov 28

Issue description

See blocking bug.
 
Cc: toniki...@chromium.org
Cc: -toniki...@chromium.org
Owner: toniki...@chromium.org
Status: Assigned (was: Available)
I think we can do 3 steps here:

1) get rid of signinmanager and po2ts direct use.

2) make it possible not to pass the |gaia_cookie_manager_service_| to AccountInvestigator. For that we would need to fix  bug 903718  (I think I am volunteering, but I might need guidance with the unittest for the API)

3) make use of the simple IdentityTestEnvironment ctor, once we need not to refer to signin manager, po2ts and gaia cookie manager service directly. This depends ib  bug 908810 .
Blockedon: 903718
This makes total sense, Antonio! Thanks for the investigation here!
Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 4

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

commit e950ee8af59711652eec212757d30a69d2c07685
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Fri Jan 04 16:25:28 2019

Remove unused signin_manager getter from account_investigator_unittest.cc

This is an oneliner removal follow up of crrev.com/c/1351152.

TBR=msarda@chromium.org
BUG= 909715 

Change-Id: I8bc16bfb73348c23425351761695cca2258342c6
Reviewed-on: https://chromium-review.googlesource.com/c/1396237
Reviewed-by: Antonio Gomes <tonikitoo@igalia.com>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#619966}
[modify] https://crrev.com/e950ee8af59711652eec212757d30a69d2c07685/components/signin/core/browser/account_investigator_unittest.cc

This is being implemented in 3 steps, as per comment #2:

CL 1/3: https://crrev.com/c/1396238 (Make signout accounts available through identity::AccountsInCookieJarInfo).
CL 2/3: https://crrev.com/c/1396240 (Remove GaiaCookieManagerService dependency from AccountsInvestigator).
CL 3/3: https://crrev.com/c/1396242 (Eliminate account_investigator_unittest's knowledge of PO2TS, SigninManager, et al).
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 7

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

commit 2619c4f277cfa4d759fcbbe0b5ed29b5da722064
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Mon Jan 07 16:49:12 2019

Make signed out accounts available through identity::AccountsInCookieJarInfo

This is the first out 3 CLs that will make AccountInvestigator and its
unittests not depend/use GaiaCookieManagerService anymore.

For this, it was needed to extend identity::AccountsInCookieJarInfo to
hold "signed out accounts" information as well.
This will allow AccountInvestigator to inherit from
IdentityManager::Observer instead of GaiaCookieManagerService::Observer
without losing any functionality. The actual inheritance change is done
in a follow up CL [2].

[2] https://crrev.com/c/1396240.

BUG= 909715 

Change-Id: I252ba9c24e2aa63bdc53e29a1f4cf994608abd6c
Reviewed-on: https://chromium-review.googlesource.com/c/1396238
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620355}
[modify] https://crrev.com/2619c4f277cfa4d759fcbbe0b5ed29b5da722064/components/sync/driver/sync_auth_util.cc
[modify] https://crrev.com/2619c4f277cfa4d759fcbbe0b5ed29b5da722064/components/sync/driver/sync_session_durations_metrics_recorder.cc
[modify] https://crrev.com/2619c4f277cfa4d759fcbbe0b5ed29b5da722064/services/identity/public/cpp/accounts_in_cookie_jar_info.cc
[modify] https://crrev.com/2619c4f277cfa4d759fcbbe0b5ed29b5da722064/services/identity/public/cpp/accounts_in_cookie_jar_info.h
[modify] https://crrev.com/2619c4f277cfa4d759fcbbe0b5ed29b5da722064/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/2619c4f277cfa4d759fcbbe0b5ed29b5da722064/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/2619c4f277cfa4d759fcbbe0b5ed29b5da722064/services/identity/public/cpp/identity_manager_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 7

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

commit b92aad536edcac81e0daa90deb484e668995d157
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Mon Jan 07 17:19:12 2019

Remove GaiaCookieManagerService dependency from AccountsInvestigator

This a 2nd out 3 CLs that will make AccountInvestigator and its unittests
not depend/use GaiaCookieManagerService anymore.

Here AccountInvestigator replaces inheritance of
GaiaCookieManagerService::Observer by IdentityManager::Observer.

Note that because of the existing AccountInfo <-> ListedAccount
conversions, a helper function was introduced with a TODO to convert
the whole AccountInvestigator and its unit tests to use the former.

BUG=859882, 909715 

Change-Id: Iaf23e19622258ce63957c6254df3aac652558a08
Reviewed-on: https://chromium-review.googlesource.com/c/1396240
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620363}
[modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/chrome/browser/signin/account_investigator_factory.cc
[modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/components/signin/core/browser/account_investigator.cc
[modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/components/signin/core/browser/account_investigator.h
[modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/components/signin/core/browser/account_investigator_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 8

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

commit e67e554ecdb3bffd16141379863b6a811da42f2b
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Tue Jan 08 14:49:18 2019

Eliminate account_investigator_unittest's knowledge of PO2TS, SigninManager, et al

This cl is the last one of a series of 3 CLs that aim at removing the
knowledge about SigninManager, PO2TS, SigninClient and AccountTrackerService
and GaiaCookieManagerService from AccountInvestigatorTest class.

For this, the CL starts to make use of the simpler IdentityTestEnvironment ctor
from AccountInvestigatorTest and introduces a new method (namely
SetListOfAccountsInGaiaCookieStale) to IdentityTestEnvironment.

BUG= 909715 

Change-Id: I82449032d06fdea795450fabce28caf3928bea88
Reviewed-on: https://chromium-review.googlesource.com/c/1396242
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620712}
[modify] https://crrev.com/e67e554ecdb3bffd16141379863b6a811da42f2b/components/signin/core/browser/account_investigator_unittest.cc
[modify] https://crrev.com/e67e554ecdb3bffd16141379863b6a811da42f2b/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/e67e554ecdb3bffd16141379863b6a811da42f2b/services/identity/public/cpp/identity_test_environment.h

Status: Fixed (was: Started)

Sign in to add a comment