Commit 3a0f6283 authored by Sergio Villar Senin's avatar Sergio Villar Senin Committed by Commit Bot

Stop listening for OnRefreshTokensLoaded() in FamilyInfoFetcher

In crrev.com/c/1454618 FamilyInfoFetcher was migrated to use
PrimaryAccountAccessTokenFetcher. However the code is still observing
IdentityManager for the OnRefreshTokensLoaded() notification. It turns
out that it is not very useful to listen to that as it's really
unreliable. The reason is that at the time StartFetching gets called,
OnRefreshTokensLoaded might or might not have happened already. If it
has *not* already happened, then we'll report an error to the
client. But if it *has* already happened, then we'll just end up
waiting forever anyway. And there's not even a way for the client to
know.

Bug: 920965
Change-Id: I49b8741bfe438855bdf9c74c1a52528bba3baa79
Reviewed-on: https://chromium-review.googlesource.com/c/1454959
Commit-Queue: Sergio Villar <svillar@igalia.com>
Reviewed-by: default avatarColin Blundell <blundell@chromium.org>
Reviewed-by: default avatarMarc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629928}
parent 49587f4a
...@@ -126,10 +126,6 @@ void FamilyInfoFetcher::StartGetFamilyMembers() { ...@@ -126,10 +126,6 @@ void FamilyInfoFetcher::StartGetFamilyMembers() {
} }
void FamilyInfoFetcher::StartFetching() { void FamilyInfoFetcher::StartFetching() {
// TODO(treib): we should stop listening to OnRefreshTokensLoaded which is
// unreliable for clients, as there is no way to know whether it has already
// happened or not, potentially leading to neverending waits. Note that this
// directly affects FamilyInfoFetcherTest.NoRefreshToken.
if (identity_manager_->HasAccountWithRefreshToken(primary_account_id_)) { if (identity_manager_->HasAccountWithRefreshToken(primary_account_id_)) {
StartFetchingAccessToken(); StartFetchingAccessToken();
} else { } else {
...@@ -159,16 +155,6 @@ void FamilyInfoFetcher::OnRefreshTokenUpdatedForAccount( ...@@ -159,16 +155,6 @@ void FamilyInfoFetcher::OnRefreshTokenUpdatedForAccount(
StartFetchingAccessToken(); StartFetchingAccessToken();
} }
void FamilyInfoFetcher::OnRefreshTokensLoaded() {
identity_manager_->RemoveObserver(this);
// The PO2TS has loaded all tokens, but we didn't get one for the account we
// want. We probably won't get one any time soon, so report an error.
DLOG(WARNING) << "Did not get a refresh token for account "
<< primary_account_id_;
consumer_->OnFailure(TOKEN_ERROR);
}
void FamilyInfoFetcher::OnAccessTokenFetchComplete( void FamilyInfoFetcher::OnAccessTokenFetchComplete(
GoogleServiceAuthError error, GoogleServiceAuthError error,
identity::AccessTokenInfo access_token_info) { identity::AccessTokenInfo access_token_info) {
......
...@@ -106,7 +106,6 @@ class FamilyInfoFetcher : public identity::IdentityManager::Observer { ...@@ -106,7 +106,6 @@ class FamilyInfoFetcher : public identity::IdentityManager::Observer {
// IdentityManager::Observer implementation: // IdentityManager::Observer implementation:
void OnRefreshTokenUpdatedForAccount( void OnRefreshTokenUpdatedForAccount(
const AccountInfo& account_info) override; const AccountInfo& account_info) override;
void OnRefreshTokensLoaded() override;
void OnAccessTokenFetchComplete(GoogleServiceAuthError error, void OnAccessTokenFetchComplete(GoogleServiceAuthError error,
identity::AccessTokenInfo access_token_info); identity::AccessTokenInfo access_token_info);
......
...@@ -288,11 +288,6 @@ TEST_F(FamilyInfoFetcherTest, NoRefreshToken) { ...@@ -288,11 +288,6 @@ TEST_F(FamilyInfoFetcherTest, NoRefreshToken) {
EXPECT_CALL(access_token_requested, Run()).Times(0); EXPECT_CALL(access_token_requested, Run()).Times(0);
identity_test_env_.SetCallbackForNextAccessTokenRequest( identity_test_env_.SetCallbackForNextAccessTokenRequest(
access_token_requested.Get()); access_token_requested.Get());
// After all refresh tokens have been loaded, there is still no token for our
// user, so we expect a token error.
EXPECT_CALL(*this, OnFailure(FamilyInfoFetcher::TOKEN_ERROR));
identity_test_env_.ReloadAccountsFromDisk();
} }
TEST_F(FamilyInfoFetcherTest, GetTokenFailure) { TEST_F(FamilyInfoFetcherTest, GetTokenFailure) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment