New issue
Advanced search Search tips

Issue 890790 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Task

Blocked on:
issue 902296

Blocking:
issue 883318
issue 883330



Sign in to add a comment

Convert chrome/browser/signin/dice_response_handler_unittest.cc to IdentityManager

Project Member Reported by sdefresne@chromium.org, Oct 1

Issue description

API used:
- SigninManager::SignOut()
- SigninManager::IsAuthenticated()

 
Owner: toniki...@chromium.org
Status: Started (was: Available)
Blockedon: 902296
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 8

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

commit f4a5aa2a645ba65b07940d9d77552b384b9b600a
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Thu Nov 08 13:12:50 2018

[s13n] Convert DiceResponseHandler to IdentityManager

This CL only migrates DiceResponseHandler production code away from
SigninManager, leaves migration away from ProfileOAuth2TokenService
and unittests for a follow up step.

Note that the unittest c/b/signin/dice_response_handler_unittest.cc
was minimally updated to keep passing.

BUG=890790, 902296 

Change-Id: Iecdf43b80afe6514b7f19075c2bb62aadfbdbe4c
Reviewed-on: https://chromium-review.googlesource.com/c/1318550
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606439}
[modify] https://crrev.com/f4a5aa2a645ba65b07940d9d77552b384b9b600a/chrome/browser/signin/dice_response_handler.cc
[modify] https://crrev.com/f4a5aa2a645ba65b07940d9d77552b384b9b600a/chrome/browser/signin/dice_response_handler.h
[modify] https://crrev.com/f4a5aa2a645ba65b07940d9d77552b384b9b600a/chrome/browser/signin/dice_response_handler_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 16

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

commit c5dd68a2ff56130df365ff606b9087647cd012f8
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Fri Nov 16 06:50:11 2018

[s13n] Convert c/b/signin/dice_response_handler_unittest.cc to IdentityManager

This CL is a follow up of [1], where production code was migrated away
from using SigninManager APIs directly, in favor of IdentityManager
(//services/identity). This is done as part of the servicification effort.

After this CL, the last remaining step will be converting DiceResponseHandler
(production code) away from using ProfileOAuth2TokenService directly,
blocked of  crbug.com/887870 .

[1] https://crrev.com/c/1318550

BUG=890790

Change-Id: Ifc54410bd7112c1e2fae01d6770ab6afa002481d
Reviewed-on: https://chromium-review.googlesource.com/c/1316927
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608684}
[modify] https://crrev.com/c5dd68a2ff56130df365ff606b9087647cd012f8/chrome/browser/signin/dice_response_handler_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 4

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

commit c6123f52549d35a5097f582442d2589e1aa8807c
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Fri Jan 04 13:05:34 2019

Use IdentityTestEnvironment APIs when applicable in //c/b/signin/dice_response_handler_unittest.cc

CL is a (driven-by) clean up pass on dice_response_handler_unittests.cc,
where pair calls to AccountTrackerService::SeedAccountInfo and
IdentityTestEnvironment::SetRefreshTokenForAccount are replaced by calls
to the wrapper IdentityTestEnvironment::MakeAccountAvailable.

CL also replaces from the tests the knowledge about the specific
refresh token values (eg "refresh_token_for_bleh"), by checks that
verify whether the IdentityManager holds a persistent error state or not.

BUG=890790

Change-Id: I74215310276023a387854d840a96763545699dfe
Reviewed-on: https://chromium-review.googlesource.com/c/1393303
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#619930}
[modify] https://crrev.com/c6123f52549d35a5097f582442d2589e1aa8807c/chrome/browser/signin/dice_response_handler_unittest.cc

Blocking: 883318
Status: Started (was: Fixed)
I think that it's clearest to keep this bug open given that we still need to move the unittest away from direct knowledge of IdentityManager's internals (i.e., use the no-arg IdentityTestEnvironment constructor and eliminate all the legacy includes/usage). My understanding is that that isn't possible until the production code is converted, so leaving the blocking relationship in place.

Sign in to add a comment