Skip to content

Commit

Permalink
Revert of non-new-profile-management creates a "no-op" style account_…
Browse files Browse the repository at this point in the history 
…reconcilor, (https://codereview.chromium.org/276463002/)

Reason for revert:
unit tests fail on mac

Original issue's description:
> non-new-profile-management creates a "no-op" style account_reconcilor,
> useful for tracking stats but won't have any real effects.
> 
> Modify the AccountReconcilor_unittest to execute with the new_profile_management flag on.
> 
> BUG=357693
> TEST=Account Reconciler should function normally when
> new_profile_management flag is on. Should not have effects when the
> flag is off, but UMA stats (histograms) and logging (for
> --vmodule=account_reconcilor=1) should still trace the execution path.
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272131
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272826

NOTRY=true
NOTREECHECKS=true
TBR=mlerman@chromium.org

Review URL: https://codereview.chromium.org/297303003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@272859 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
jochen@chromium.org committed May 26, 2014
1 parent 643aff6 commit 99e9153
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 87 deletions.
3 changes: 2 additions & 1 deletion chrome/browser/profiles/profile_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,8 @@ void ProfileManager::DoFinalInitForServices(Profile* profile,
StartupTaskRunnerServiceFactory::GetForProfile(profile)->
StartDeferredTaskRunners();

AccountReconcilorFactory::GetForProfile(profile);
if (switches::IsNewProfileManagement())
AccountReconcilorFactory::GetForProfile(profile);
}

void ProfileManager::DoFinalInitLogging(Profile* profile) {
Expand Down
50 changes: 14 additions & 36 deletions chrome/browser/signin/account_reconcilor_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,20 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "base/command_line.h"
#include "base/memory/scoped_ptr.h"
#include "base/run_loop.h"
#include "base/strings/utf_string_conversions.h"
#include "base/time/time.h"
#include "chrome/browser/prefs/pref_service_syncable.h"
#include "chrome/browser/signin/account_reconcilor_factory.h"
#include "chrome/browser/signin/chrome_signin_client_factory.h"
#include "chrome/browser/signin/fake_profile_oauth2_token_service.h"
#include "chrome/browser/signin/fake_profile_oauth2_token_service_builder.h"
#include "chrome/browser/signin/fake_signin_manager.h"
#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
#include "chrome/browser/signin/signin_manager_factory.h"
#include "chrome/test/base/testing_browser_process.h"
#include "chrome/test/base/testing_profile.h"
#include "chrome/test/base/testing_profile_manager.h"
#include "components/signin/core/browser/account_reconcilor.h"
#include "components/signin/core/browser/profile_oauth2_token_service.h"
#include "components/signin/core/browser/signin_manager.h"
#include "components/signin/core/common/signin_switches.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "google_apis/gaia/gaia_urls.h"
#include "net/url_request/test_url_fetcher_factory.h"
Expand All @@ -42,9 +36,9 @@ class MockAccountReconcilor : public testing::StrictMock<AccountReconcilor> {
virtual ~MockAccountReconcilor() {}

MOCK_METHOD1(PerformMergeAction, void(const std::string& account_id));
MOCK_METHOD1(PerformStartRemoveAction, void(const std::string& account_id));
MOCK_METHOD1(StartRemoveAction, void(const std::string& account_id));
MOCK_METHOD3(
PerformFinishRemoveAction,
FinishRemoveAction,
void(const std::string& account_id,
const GoogleServiceAuthError& error,
const std::vector<std::pair<std::string, bool> >& accounts));
Expand Down Expand Up @@ -80,7 +74,7 @@ class AccountReconcilorTest : public testing::Test {
virtual void SetUp() OVERRIDE;
virtual void TearDown() OVERRIDE;

TestingProfile* profile() { return profile_; }
TestingProfile* profile() { return profile_.get(); }
FakeSigninManagerForTesting* signin_manager() { return signin_manager_; }
FakeProfileOAuth2TokenService* token_service() { return token_service_; }

Expand All @@ -105,12 +99,11 @@ class AccountReconcilorTest : public testing::Test {

private:
content::TestBrowserThreadBundle bundle_;
TestingProfile* profile_;
scoped_ptr<TestingProfile> profile_;
FakeSigninManagerForTesting* signin_manager_;
FakeProfileOAuth2TokenService* token_service_;
MockAccountReconcilor* mock_reconcilor_;
net::FakeURLFetcherFactory url_fetcher_factory_;
TestingProfileManager* testing_profile_manager_;
};

AccountReconcilorTest::AccountReconcilorTest()
Expand All @@ -120,26 +113,14 @@ AccountReconcilorTest::AccountReconcilorTest()
url_fetcher_factory_(NULL) {}

void AccountReconcilorTest::SetUp() {
CommandLine::ForCurrentProcess()->AppendSwitch(
switches::kNewProfileManagement);

testing_profile_manager_ =
new TestingProfileManager(TestingBrowserProcess::GetGlobal());
ASSERT_TRUE(testing_profile_manager_->SetUp());

TestingProfile::TestingFactories factories;
factories.push_back(std::make_pair(
ProfileOAuth2TokenServiceFactory::GetInstance(),
BuildFakeProfileOAuth2TokenService));
factories.push_back(std::make_pair(SigninManagerFactory::GetInstance(),
FakeSigninManagerBase::Build));
factories.push_back(std::make_pair(AccountReconcilorFactory::GetInstance(),
MockAccountReconcilor::Build));

profile_ = testing_profile_manager_->CreateTestingProfile("name",
scoped_ptr<PrefServiceSyncable>(),
base::UTF8ToUTF16("name"), 0, std::string(),
factories);
TestingProfile::Builder builder;
builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(),
BuildFakeProfileOAuth2TokenService);
builder.AddTestingFactory(SigninManagerFactory::GetInstance(),
FakeSigninManagerBase::Build);
builder.AddTestingFactory(AccountReconcilorFactory::GetInstance(),
MockAccountReconcilor::Build);
profile_ = builder.Build();

signin_manager_ =
static_cast<FakeSigninManagerForTesting*>(
Expand All @@ -151,8 +132,8 @@ void AccountReconcilorTest::SetUp() {
}

void AccountReconcilorTest::TearDown() {
// The |testing_profile_manager_| will handle destroying the profile.
delete testing_profile_manager_;
// Destroy the profile before all threads are torn down.
profile_.reset();
}

MockAccountReconcilor* AccountReconcilorTest::GetMockReconcilor() {
Expand Down Expand Up @@ -194,7 +175,6 @@ TEST_F(AccountReconcilorTest, SigninManagerRegistration) {
ASSERT_TRUE(reconcilor);
ASSERT_FALSE(reconcilor->IsRegisteredWithTokenService());

signin_manager()->set_password("password");
signin_manager()->OnExternalSigninCompleted(kTestEmail);
ASSERT_TRUE(reconcilor->IsRegisteredWithTokenService());

Expand All @@ -206,7 +186,6 @@ TEST_F(AccountReconcilorTest, SigninManagerRegistration) {

TEST_F(AccountReconcilorTest, Reauth) {
signin_manager()->SetAuthenticatedUsername(kTestEmail);
signin_manager()->set_password("password");

AccountReconcilor* reconcilor =
AccountReconcilorFactory::GetForProfile(profile());
Expand Down Expand Up @@ -443,7 +422,6 @@ TEST_F(AccountReconcilorTest, StartReconcileNoopMultiple) {
ASSERT_FALSE(reconcilor->is_reconcile_started_);
}


TEST_F(AccountReconcilorTest, StartReconcileAddToCookie) {
signin_manager()->SetAuthenticatedUsername("user@gmail.com");
token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
Expand Down
40 changes: 10 additions & 30 deletions components/signin/core/browser/account_reconcilor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "components/signin/core/browser/signin_client.h"
#include "components/signin/core/browser/signin_metrics.h"
#include "components/signin/core/browser/signin_oauth_helper.h"
#include "components/signin/core/common/profile_management_switches.h"
#include "google_apis/gaia/gaia_auth_fetcher.h"
#include "google_apis/gaia/gaia_auth_util.h"
#include "google_apis/gaia/gaia_constants.h"
Expand Down Expand Up @@ -336,7 +335,7 @@ void AccountReconcilor::OnRefreshTokenAvailable(const std::string& account_id) {

void AccountReconcilor::OnRefreshTokenRevoked(const std::string& account_id) {
VLOG(1) << "AccountReconcilor::OnRefreshTokenRevoked: " << account_id;
PerformStartRemoveAction(account_id);
StartRemoveAction(account_id);
}

void AccountReconcilor::OnRefreshTokensLoaded() {}
Expand All @@ -359,28 +358,22 @@ void AccountReconcilor::GoogleSignedOut(const std::string& username) {
}

void AccountReconcilor::PerformMergeAction(const std::string& account_id) {
if (!switches::IsNewProfileManagement())
return;
VLOG(1) << "AccountReconcilor::PerformMergeAction: " << account_id;
merge_session_helper_.LogIn(account_id);
}

void AccountReconcilor::PerformStartRemoveAction(
const std::string& account_id) {
VLOG(1) << "AccountReconcilor::PerformStartRemoveAction: " << account_id;
GetAccountsFromCookie(base::Bind(
&AccountReconcilor::PerformFinishRemoveAction,
base::Unretained(this),
account_id));
void AccountReconcilor::StartRemoveAction(const std::string& account_id) {
VLOG(1) << "AccountReconcilor::StartRemoveAction: " << account_id;
GetAccountsFromCookie(base::Bind(&AccountReconcilor::FinishRemoveAction,
base::Unretained(this),
account_id));
}

void AccountReconcilor::PerformFinishRemoveAction(
void AccountReconcilor::FinishRemoveAction(
const std::string& account_id,
const GoogleServiceAuthError& error,
const std::vector<std::pair<std::string, bool> >& accounts) {
if (!switches::IsNewProfileManagement())
return;
VLOG(1) << "AccountReconcilor::PerformFinishRemoveAction:"
VLOG(1) << "AccountReconcilor::FinishRemoveAction:"
<< " account=" << account_id << " error=" << error.ToString();
if (error.state() == GoogleServiceAuthError::NONE) {
AbortReconcile();
Expand All @@ -398,8 +391,6 @@ void AccountReconcilor::PerformFinishRemoveAction(

void AccountReconcilor::PerformAddToChromeAction(const std::string& account_id,
int session_index) {
if (!switches::IsNewProfileManagement())
return;
VLOG(1) << "AccountReconcilor::PerformAddToChromeAction:"
<< " account=" << account_id << " session_index=" << session_index;

Expand All @@ -410,8 +401,6 @@ void AccountReconcilor::PerformAddToChromeAction(const std::string& account_id,
}

void AccountReconcilor::PerformLogoutAllAccountsAction() {
if (!switches::IsNewProfileManagement())
return;
VLOG(1) << "AccountReconcilor::PerformLogoutAllAccountsAction";
merge_session_helper_.LogOutAllAccounts();
}
Expand Down Expand Up @@ -725,22 +714,13 @@ void AccountReconcilor::HandleFailedAccountIdCheck(
FinishReconcile();
}

void AccountReconcilor::PerformAddAccountToTokenService(
const std::string& account_id,
const std::string& refresh_token) {
// The flow should never get to this method if new_profile_management is
// false, but better safe than sorry.
if (!switches::IsNewProfileManagement())
return;
token_service_->UpdateCredentials(account_id, refresh_token);
}

void AccountReconcilor::HandleRefreshTokenFetched(
const std::string& account_id,
const std::string& refresh_token) {
if (!refresh_token.empty()) {
PerformAddAccountToTokenService(account_id, refresh_token);
token_service_->UpdateCredentials(account_id, refresh_token);
}

// Remove the account from the list that is being updated.
for (std::vector<std::pair<std::string, int> >::iterator i =
add_to_chrome_.begin();
Expand Down
7 changes: 2 additions & 5 deletions components/signin/core/browser/account_reconcilor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,10 @@ class AccountReconcilor : public KeyedService,
virtual void PerformAddToChromeAction(const std::string& account_id,
int session_index);
virtual void PerformLogoutAllAccountsAction();
virtual void PerformAddAccountToTokenService(
const std::string& account_id,
const std::string& refresh_token);

// Used to remove an account from chrome and the cookie jar.
virtual void PerformStartRemoveAction(const std::string& account_id);
virtual void PerformFinishRemoveAction(
virtual void StartRemoveAction(const std::string& account_id);
virtual void FinishRemoveAction(
const std::string& account_id,
const GoogleServiceAuthError& error,
const std::vector<std::pair<std::string, bool> >& accounts);
Expand Down
13 changes: 0 additions & 13 deletions google_apis/gaia/fake_gaia.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ const char kAuthHeaderOAuth[] = "OAuth ";

const char kListAccountsResponseFormat[] =
"[\"gaia.l.a.r\",[[\"gaia.l.a\",1,\"\",\"%s\",\"\",1,1,0]]]";
const char kPeopleGetResponseFormat[] = "{\"id\":\"%s\"}";

typedef std::map<std::string, std::string> CookieMap;

Expand Down Expand Up @@ -177,10 +176,6 @@ void FakeGaia::Initialize() {
// Handles /ListAccounts GAIA call.
REGISTER_RESPONSE_HANDLER(
gaia_urls->list_accounts_url(), HandleListAccounts);

// Handles /plus/v1/people/me
REGISTER_RESPONSE_HANDLER(
gaia_urls->people_get_url(), HandlePeopleGet);
}

scoped_ptr<HttpResponse> FakeGaia::HandleRequest(const HttpRequest& request) {
Expand Down Expand Up @@ -534,11 +529,3 @@ void FakeGaia::HandleListAccounts(const HttpRequest& request,
kListAccountsResponseFormat, merge_session_params_.email.c_str()));
http_response->set_code(net::HTTP_OK);
}


void FakeGaia::HandlePeopleGet(const HttpRequest& request,
BasicHttpResponse* http_response) {
http_response->set_content(base::StringPrintf(
kPeopleGetResponseFormat, "name"));
http_response->set_code(net::HTTP_OK);
}
2 changes: 0 additions & 2 deletions google_apis/gaia/fake_gaia.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,6 @@ class FakeGaia {
net::test_server::BasicHttpResponse* http_response);
void HandleListAccounts(const net::test_server::HttpRequest& request,
net::test_server::BasicHttpResponse* http_response);
void HandlePeopleGet(const net::test_server::HttpRequest& request,
net::test_server::BasicHttpResponse* http_response);

// Returns the access token associated with |auth_token| that matches the
// given |client_id| and |scope_string|. If |scope_string| is empty, the first
Expand Down

0 comments on commit 99e9153

Please sign in to comment.