Note: The CL here: https://codereview.chromium.org/1523743003 seems to indicate that the intent was for PO2TS::OnRefreshTokensLoaded() to always be fired on ChromeOS, and thus that the above circumstances are a bug to be fixed. achuith@, does that seem right to you?
I am planning to have SigninManagerBase::Initialize() invoke PO2TS::LoadCredentials(), as SigninManager::Initialize() currently does. Complicating factor is that SigninManager::Initialize() invokes SigninManagerBase::Initialize() at the *beginning* of its flow and PO2TS::LoadCredentials() at the *end* of its flow, with state that can impact the latter invocation happening in between. Here is my complete plan:
- Have SigninManagerBase take in PO2TS
- Add SigninManagerBase::FinalizeInit(), which is meant to be called at the end of initialization of SigninManagerBase *as well as any subclass of it.*
- On CrOS, have SigninManagerBase::Initialize() call SigninManagerBase::FinalizeInit() directly as the last thing it does.
- On other platforms, have SigninManager::Initialize() call SigninManagerBase::FinalizeInit() as the last thing it does.
- Have SigninManagerBase::FinalizeInit() call PO2TS::LoadCredentials().
achuith@/xiyuan@, could you point me to the right reviewer for this change from the CrOS POV? knn@, who wrote the CL referenced in c#1, appears to no longer be working on Chrome. Thanks!
Hi Kushagra! I would like to make this fix, but it's not a short-term priority as it's not blocking the conversion of the codebase to IdentityManager, which is our current focus. The easiest short-term fix would likely be to ensure that LoadCredentials() is called in the flow that you're going through in crbug.com/900590 .
Hi Jochen,
We had talked about you potentially taking on ChromeOS-related corner cases that are tricky but not on the near-term critical path for IdentityManager. This is the biggest one. c#3 indicates the approach that I was planning on, with one step elided: When we move the call into PO2TS::LoadCredentials() into SigninManagerBase::FinalizeInit(), we should also remove all its explicit ChromeOS-specific callsites.
Is this something that could interest you? I would guess that getting it to stick might be bumpy, i.e., we might well uncover some really nice corner case after it lands and starts to roll out. All the more reason to start developing it well before it gets on any critical path :).
Summary: Have SigninManagerBase internally guarantee that PO2TS::LoadCredentials() is always called (was: Determine what to do wrt ProfileOAuth2TokenService::OnRefreshTokensLoaded() not being fired in all circumstances on ChromeOS)
Comment 1 by blundell@chromium.org
, Jul 27 2017