New issue
Advanced search Search tips

Issue 906060 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 4
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 906056

Blocking:
issue 889902



Sign in to add a comment

Convert from IdentityManager::ClearPrimaryAccount to PrimaryAccountMutator::ClearPrimaryAccount

Project Member Reported by sdefresne@chromium.org, Nov 16

Issue description

IdentityManager should not allow modification of the primary account. Instead the mutation should be performed via the PrimaryAccountMutator. The IdentityManager::ClearPrimaryAccount method out dates the introduction of PrimaryAccountMutator but is now deprecated.

The conversion should be straight-forward from:

  IdentityManagerFactory::GetForProfile(profile)
      ->ClearPrimaryAccount(
          IdentityManager::ClearAccountTokensAction::...,
          signin_metrics::ProfileSignout::...,
          signin_metrics::SignoutDelete::...);

to:

  IdentityManagerFactory::GetForProfile(profile)
      ->GetPrimaryAccountMutator()
      ->ClearPrimaryAccount(
          PrimaryAccountMutator::ClearAccountsAction::...,
          signin_metrics::ProfileSignout::...,
          signin_metrics::SignoutDelete::...);

Once all the client have been migrated, the method IdentityManager::ClearPrimaryAccount should be removed.
 
Owner: ma...@igalia.com
Status: Started (was: Available)
Taking this one
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 4

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

commit a93f5da5337b58a02d049feb990dc4d19712eac8
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Tue Dec 04 12:20:01 2018

Migrate ClearPrimaryAccount() from IdentityManager to PrimaryAccountMutator

Do not IdentityManager::ClearPrimaryAccount and its associated tests yet,
even if no other code is using this now, it will be done in a follow-up CL.

Bug:  906060 
Change-Id: I369d2adbaea5b1e131fb5f17d3c8671a2d5eb9af
Reviewed-on: https://chromium-review.googlesource.com/c/1356548
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613518}
[modify] https://crrev.com/a93f5da5337b58a02d049feb990dc4d19712eac8/chrome/browser/android/signin/signin_manager_android.cc
[modify] https://crrev.com/a93f5da5337b58a02d049feb990dc4d19712eac8/chrome/browser/profiles/profile_manager.cc
[modify] https://crrev.com/a93f5da5337b58a02d049feb990dc4d19712eac8/chrome/browser/ui/views/profiles/profile_chooser_view.cc
[modify] https://crrev.com/a93f5da5337b58a02d049feb990dc4d19712eac8/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/a93f5da5337b58a02d049feb990dc4d19712eac8/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/a93f5da5337b58a02d049feb990dc4d19712eac8/ios/chrome/browser/signin/authentication_service.mm
[modify] https://crrev.com/a93f5da5337b58a02d049feb990dc4d19712eac8/ios/chrome/browser/ui/recent_tabs/recent_tabs_coordinator_unittest.mm

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 4

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

commit e0e56325ab61e31a37655a7a87310f58ee2ff546
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Tue Dec 04 13:15:16 2018

Allow specifying an account consistency method for IdentityTestEnvironment

We need to have a way to specify the signin::AccountConsistencyMethod used
for the internal SigninManager used by IdentityTestEnvironment when one is
not passed using the more complex (and discouraged) constructors, so add
an extra optional parameter, assuming "disabled" as the default value to
match the default behavior of FakeSigninManager.

Bug:  906060 
Change-Id: I3a15272a415e305d39023f4df9635ba8b93c4846
Reviewed-on: https://chromium-review.googlesource.com/c/1358505
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613525}
[modify] https://crrev.com/e0e56325ab61e31a37655a7a87310f58ee2ff546/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/e0e56325ab61e31a37655a7a87310f58ee2ff546/services/identity/public/cpp/identity_test_environment.h

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 4

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

commit d99b2fd706de0d4a2eef2a59171805ef28f7a5bf
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Tue Dec 04 15:23:16 2018

Migrate IdentityManager::ClearPrimaryAccount tests to PrimaryAccountMutator

This tests would otherwise get lost, move them to PrimaryAccountMutatorTest
so that we can keep testing the same use cases and code paths from there.

Bug:  906060 
Change-Id: Ie72bfe4e7420e4f880f7c0b905e64d64850e7de7
Reviewed-on: https://chromium-review.googlesource.com/c/1358506
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613556}
[modify] https://crrev.com/d99b2fd706de0d4a2eef2a59171805ef28f7a5bf/services/identity/public/cpp/primary_account_mutator_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 4

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

commit f66492a025fbc726bdf7c99a654e1eab6b06e3e4
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Tue Dec 04 16:35:55 2018

Refactor ClearPrimaryAccount_KeepAll and ClearPrimaryAccount_RemoveAll

We can simplify these two tests now a lot, based on the helper test
function used in other ClearPrimaryAccount_* tests.

Bug:  906060 
Change-Id: I4a3386e1eff390bf87d2b932472d08cec26ff2aa
Reviewed-on: https://chromium-review.googlesource.com/c/1360731
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613572}
[modify] https://crrev.com/f66492a025fbc726bdf7c99a654e1eab6b06e3e4/services/identity/public/cpp/primary_account_mutator_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 4

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

commit eaed4b1e370c8b51c127f0e9cd4731b7c29aa81d
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Tue Dec 04 21:34:44 2018

Remove deprecated & unused IdentityManager::ClearPrimaryAccount() API

This got migrated to PrimaryAccountMutator::ClearPrimaryAccount() in
CL 1356548, so we can remove it now, along with the related tests.

Bug:  906060 
Change-Id: I8159f0c5e2ebf08ebd047c960c2966164e2281c2
Reviewed-on: https://chromium-review.googlesource.com/c/1358462
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613703}
[modify] https://crrev.com/eaed4b1e370c8b51c127f0e9cd4731b7c29aa81d/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/eaed4b1e370c8b51c127f0e9cd4731b7c29aa81d/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/eaed4b1e370c8b51c127f0e9cd4731b7c29aa81d/services/identity/public/cpp/identity_manager_unittest.cc

Status: Fixed (was: Started)
Fixed!

Sign in to add a comment