New issue
Advanced search Search tips

Issue 797899 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 19
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 883318



Sign in to add a comment

Port signin_error_notifier to use IdentityManager

Project Member Reported by blundell@chromium.org, Dec 28 2017

Issue description

signin_error_notifier_factory_ash’s include looks unused.
signin_error_notifer_ash_unittest builds a fake signin manager. Is this necessary, given that the production code doesn't seem to have a dependence on SigninManager? If it is, we need to investigate what the actual production dependence is for conversion of that dependence (and then conversion of the test dependence).

 
Blocking: 796544
Components: Internals>Services>Identity
Status: Available (was: Untriaged)
Owner: jdmuys@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 7

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

commit 8b44b279a8b14d3c7e6fdaac4b3caa19f2339bab
Author: Jean-denis Muys <jdmuys@chromium.org>
Date: Tue Aug 07 10:31:29 2018

Remove signin_error_notifier_ash unittest dependencies on SigninManager

This CL is part of the long-term goal of eliminating SigninManager as a
public API for Chromium's identity code, replaced by IdentityManager.

signin_error_notifer_ash_unittest builds a fake signin manager.
This is not necessary: tests succeed without that.
So removed creation of unused fake signin manager from tests.

Bug:  797899 
Change-Id: I7bcfca18f2c94192e629385a7c481957387e7fec
Reviewed-on: https://chromium-review.googlesource.com/1158685
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Jean-Denis Muys <jdmuys@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581190}
[modify] https://crrev.com/8b44b279a8b14d3c7e6fdaac4b3caa19f2339bab/chrome/browser/signin/signin_error_notifier_ash_unittest.cc

Blocking: -796544 883318
Labels: -Pri-3 Proj-Servicification Proj-Servicification-VendorBug Pri-1
Owner: ----
Status: Available (was: Assigned)
Summary: Port signin_error_notifier to use IdentityManager (was: Can signin_error_notifier's dependences on signin internal code be removed?)
signin_error_notifier_ash_unittest.cc uses ProfileOAuth2TokenService; this looks like it can easily be ported to IdentityTestEnvironment. signin_error_notifier_factory_ash's include still looks unused.
Owner: ma...@igalia.com
Status: Started (was: Available)
Taking this one. CL here:
https://chromium-review.googlesource.com/c/chromium/src/+/1380091
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 19

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

commit d6811dd8700bae1302711674c7047d3fee1fbe22
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Wed Dec 19 17:35:58 2018

Port signin_error_notifier to use IdentityManager

Use IdentityTestEnvironment and remove usage and dependencies on
ProfileOAuth2TokenService and AccountTrackerService.

Bug:  797899 
Change-Id: Ide3000776d460c95638f9a78de2778b4b43f5dba
Reviewed-on: https://chromium-review.googlesource.com/c/1380016
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617866}
[modify] https://crrev.com/d6811dd8700bae1302711674c7047d3fee1fbe22/chrome/browser/signin/signin_error_notifier_ash_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 21

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

commit 3c8b0e2cff6b57e5e4decafc31836b2d0332043c
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Fri Dec 21 08:47:09 2018

Make IdentityTestEnvironment::GetIdentityTestEnvironmentFactories() public

Also, adapt callers of AppendIdentityTestEnvironmentFactories() to use
GetIdentityTestEnvironmentFactories() instead when the end result would
end up being cleaner this way.

TBR=poromov@chromium.org

Bug:  797899 
Change-Id: I2d1cc2e6d7ee4da6f62a0257c4e603c4f7dbbb08
Reviewed-on: https://chromium-review.googlesource.com/c/1384408
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618483}
[modify] https://crrev.com/3c8b0e2cff6b57e5e4decafc31836b2d0332043c/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
[modify] https://crrev.com/3c8b0e2cff6b57e5e4decafc31836b2d0332043c/chrome/browser/profiles/gaia_info_update_service_unittest.cc
[modify] https://crrev.com/3c8b0e2cff6b57e5e4decafc31836b2d0332043c/chrome/browser/signin/identity_test_environment_profile_adaptor.cc
[modify] https://crrev.com/3c8b0e2cff6b57e5e4decafc31836b2d0332043c/chrome/browser/signin/identity_test_environment_profile_adaptor.h
[modify] https://crrev.com/3c8b0e2cff6b57e5e4decafc31836b2d0332043c/chrome/browser/signin/signin_error_notifier_ash_unittest.cc
[modify] https://crrev.com/3c8b0e2cff6b57e5e4decafc31836b2d0332043c/chrome/browser/signin/signin_global_error_unittest.cc

Sign in to add a comment