[s13n] Convert AccountReconcilor away from SigninManager and PO2TS |
|||||||||||
Issue descriptionAPIs: - SigninManager::GetPrimaryAccountId - PO2TS::RefreshTokenHasError - PO2TS::GetAuthError - PO2TS::UpdateCredentials ( bug 907901 ) - PO2TS::GetAccounts - PO2TS::AreAllCredentialsLoaded
,
Nov 14
,
Dec 11
,
Dec 12
,
Dec 18
,
Dec 18
,
Jan 14
,
Jan 14
Recovering the work here.
bool AccountReconcilor::IsTokenServiceReady() {
#if defined(OS_CHROMEOS)
// TODO(droger): ChromeOS should use the same logic as other platforms. See
// https://crbug.com/749535
// On ChromeOS, there are cases where the token service is never fully
// initialized and AreAllCredentialsLoaded() always return false.
return token_service_->AreAllCredentialsLoaded() ||
(token_service_->GetAccounts().size() > 0);
#else
return token_service_->AreAllCredentialsLoaded();
#endif
Colin, David. Please see the snippet above.
Even with bug 749535 fixed (Have SigninManagerBase internally guarantee that PO2TS::LoadCredentials() is always called) do still need to call ::IsTokenServiceReady? What is more, does this still need to have OS_CHROMEOS ifdef?
,
Jan 14
I think you are correct and we can now remove the #ifdef. ChromeOS can just use the same code as other platforms now.
,
Jan 14
Thanks David.
To make sure I will be doing the right thing:
1) can ::IsTokenServiceReady be eliminated altogether (assuming it'd actually always return true)
2) just the ifdef OS_CHROMEOS could be removed? (so we would still be calling PO2TS::AreAllCredentialsLoaded
3) can we remove the all to PO2TS::AreAllCredentialsLoaded, and leave the IsTokenServiceReady like
bool AccountReconcilor::IsTokenServiceReady() {
#if defined(OS_CHROMEOS)
// TODO(droger): ChromeOS should use the same logic as other platforms. See
// https://crbug.com/749535
// On ChromeOS, there are cases where the token service is never fully
// initialized and AreAllCredentialsLoaded() always return false.
return token_service_->GetAccounts().size() > 0;
#else
return true;
#endif
?
,
Jan 14
1) See answer below
2) Yes we should keep the call to AreAllCredentialsLoaded. The credential loading is asynchronous on some platforms. Calling LoadCredentials does not guarantee that the accounts are loaded, because they need to be read from disk, which takes time.
3)
bool AccountReconcilor::IsTokenServiceReady() {
return token_service_->AreAllCredentialsLoaded();
}
,
Jan 15
,
Jan 16
(6 days ago)
,
Jan 18
(4 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6e62190579ce1577d9fae75760117fc32563f9c9 commit 6e62190579ce1577d9fae75760117fc32563f9c9 Author: Antonio Gomes <tonikitoo@igalia.com> Date: Fri Jan 18 16:40:43 2019 [s13n] Convert AccountReconcilor away from PO2TS This is the second part of Bug 903907 , where AccountReconcilor is being migrated away from both SigninManager and PO2TS APIs, in favor of IdentityManager. The former was handled by [1]. [1] https://crrev.com/c/1330223 This CL migrates AccountReconcilor away from PO2TS. No functionality change is expected. BUG= 903907 Change-Id: If75dadf19069fba1033d56270ae9e3ea6092da06 Reviewed-on: https://chromium-review.googlesource.com/c/1416170 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@{#624159} [modify] https://crrev.com/6e62190579ce1577d9fae75760117fc32563f9c9/chrome/browser/signin/account_reconcilor_factory.cc [modify] https://crrev.com/6e62190579ce1577d9fae75760117fc32563f9c9/chrome/browser/signin/dice_response_handler_unittest.cc [modify] https://crrev.com/6e62190579ce1577d9fae75760117fc32563f9c9/components/signin/core/browser/account_reconcilor.cc [modify] https://crrev.com/6e62190579ce1577d9fae75760117fc32563f9c9/components/signin/core/browser/account_reconcilor.h [modify] https://crrev.com/6e62190579ce1577d9fae75760117fc32563f9c9/components/signin/core/browser/account_reconcilor_unittest.cc [modify] https://crrev.com/6e62190579ce1577d9fae75760117fc32563f9c9/components/signin/ios/browser/account_consistency_service_unittest.mm [modify] https://crrev.com/6e62190579ce1577d9fae75760117fc32563f9c9/ios/chrome/browser/signin/account_reconcilor_factory.cc
,
Jan 18
(4 days ago)
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by bugdroid1@chromium.org
, Nov 14