diff options
author | cpu@chromium.org <cpu@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-08 12:33:46 +0000 |
---|---|---|
committer | cpu@chromium.org <cpu@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-08 12:33:46 +0000 |
commit | e4d0ed2731bd622c36ded53ae2c9957f6bfc4241 (patch) | |
tree | 8eb674b89678c38605c52f5e4ff6125100db8ab9 /components | |
parent | 99dbb184ea5182e07a738a9c901494ccb970d01f (diff) | |
download | chromium_src-e4d0ed2731bd622c36ded53ae2c9957f6bfc4241.zip chromium_src-e4d0ed2731bd622c36ded53ae2c9957f6bfc4241.tar.gz chromium_src-e4d0ed2731bd622c36ded53ae2c9957f6bfc4241.tar.bz2 |
Revert 205038 "Un-refcount AutofillWebData and TokenWebData"
Not sure if this is the culprit but lots of bots are red
unittest runs in linux mac and chromeos are not completing (timeout)
no output. Only for CL common to all, this is one.
I'll unrevert if it does not help.
> Un-refcount AutofillWebData and TokenWebData
>
> depends on https://codereview.chromium.org/15927029/
>
> BUG=230920
>
> Review URL: https://chromiumcodereview.appspot.com/16154031
TBR=caitkp@chromium.org
Review URL: https://codereview.chromium.org/15937020
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@205055 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'components')
10 files changed, 117 insertions, 91 deletions
diff --git a/components/autofill/browser/autocomplete_history_manager.cc b/components/autofill/browser/autocomplete_history_manager.cc index 7ad0cc3..f3f6f3f 100644 --- a/components/autofill/browser/autocomplete_history_manager.cc +++ b/components/autofill/browser/autocomplete_history_manager.cc @@ -117,7 +117,7 @@ void AutocompleteHistoryManager::OnGetAutocompleteSuggestions( return; } - if (autofill_data_) { + if (autofill_data_.get()) { pending_query_handle_ = autofill_data_->GetFormValuesForElementName( name, prefix, kMaxAutocompleteMenuItems, this); } @@ -153,13 +153,13 @@ void AutocompleteHistoryManager::OnFormSubmitted(const FormData& form) { } } - if (!values.empty() && autofill_data_) + if (!values.empty() && autofill_data_.get()) autofill_data_->AddFormFields(values); } void AutocompleteHistoryManager::OnRemoveAutocompleteEntry( const base::string16& name, const base::string16& value) { - if (autofill_data_) + if (autofill_data_.get()) autofill_data_->RemoveFormValueForElementName(name, value); } @@ -170,7 +170,7 @@ void AutocompleteHistoryManager::SetExternalDelegate( void AutocompleteHistoryManager::CancelPendingQuery() { if (pending_query_handle_) { - if (autofill_data_) + if (autofill_data_.get()) autofill_data_->CancelRequest(pending_query_handle_); pending_query_handle_ = 0; } diff --git a/components/autofill/browser/autocomplete_history_manager.h b/components/autofill/browser/autocomplete_history_manager.h index 4bba83a..9a55fa4 100644 --- a/components/autofill/browser/autocomplete_history_manager.h +++ b/components/autofill/browser/autocomplete_history_manager.h @@ -69,7 +69,7 @@ class AutocompleteHistoryManager : public content::WebContentsObserver, void CancelPendingQuery(); content::BrowserContext* browser_context_; - AutofillWebDataService* autofill_data_; + scoped_refptr<AutofillWebDataService> autofill_data_; BooleanPrefMember autofill_enabled_; diff --git a/components/autofill/browser/autocomplete_history_manager_unittest.cc b/components/autofill/browser/autocomplete_history_manager_unittest.cc index cc6707f..57da55a 100644 --- a/components/autofill/browser/autocomplete_history_manager_unittest.cc +++ b/components/autofill/browser/autocomplete_history_manager_unittest.cc @@ -38,11 +38,43 @@ class MockWebDataService : public AutofillWebDataService { public: MockWebDataService() : AutofillWebDataService() { + current_mock_web_data_service_ = this; } MOCK_METHOD1(AddFormFields, void(const std::vector<FormFieldData>&)); + static scoped_refptr<MockWebDataService> GetCurrent() { + if (!current_mock_web_data_service_) { + return new MockWebDataService(); + } + return current_mock_web_data_service_; + } + + protected: virtual ~MockWebDataService() {} + + private: + // Keep track of the most recently created instance, so that it can be + // associated with the current profile when Build() is called. + static MockWebDataService* current_mock_web_data_service_; +}; + +MockWebDataService* MockWebDataService::current_mock_web_data_service_ = NULL; + +class MockWebDataServiceWrapperCurrent : public MockWebDataServiceWrapperBase { + public: + static BrowserContextKeyedService* Build(content::BrowserContext* profile) { + return new MockWebDataServiceWrapperCurrent(); + } + + MockWebDataServiceWrapperCurrent() {} + + virtual scoped_refptr<AutofillWebDataService> GetAutofillWebData() OVERRIDE { + return MockWebDataService::GetCurrent(); + } + + private: + DISALLOW_COPY_AND_ASSIGN(MockWebDataServiceWrapperCurrent); }; class MockAutofillManagerDelegate @@ -58,37 +90,25 @@ class MockAutofillManagerDelegate DISALLOW_COPY_AND_ASSIGN(MockAutofillManagerDelegate); }; -BrowserContextKeyedService* BuildMockWebDataServiceWrapper( - content::BrowserContext* profile) { - return new MockWebDataServiceWrapper( - NULL, - new MockWebDataService(), - NULL); -} - } // namespace class AutocompleteHistoryManagerTest : public ChromeRenderViewHostTestHarness { protected: virtual void SetUp() OVERRIDE { ChromeRenderViewHostTestHarness::SetUp(); - MockWebDataServiceWrapper* wrapper = - static_cast<MockWebDataServiceWrapper*>( - WebDataServiceFactory::GetInstance()->SetTestingFactoryAndUse( - profile(), BuildMockWebDataServiceWrapper)); - web_data_service_ = - static_cast<MockWebDataService*>(wrapper->GetAutofillWebData()); + web_data_service_ = new MockWebDataService(); + WebDataServiceFactory::GetInstance()->SetTestingFactory( + profile(), MockWebDataServiceWrapperCurrent::Build); autocomplete_manager_.reset(new AutocompleteHistoryManager(web_contents())); } virtual void TearDown() OVERRIDE { autocomplete_manager_.reset(); - web_data_service_->ShutdownOnUIThread(); web_data_service_ = NULL; ChromeRenderViewHostTestHarness::TearDown(); } - MockWebDataService* web_data_service_; + scoped_refptr<MockWebDataService> web_data_service_; scoped_ptr<AutocompleteHistoryManager> autocomplete_manager_; MockAutofillManagerDelegate manager_delegate; }; @@ -110,7 +130,7 @@ TEST_F(AutocompleteHistoryManagerTest, CreditCardNumberValue) { valid_cc.form_control_type = "text"; form.fields.push_back(valid_cc); - EXPECT_CALL(*web_data_service_, AddFormFields(_)).Times(0); + EXPECT_CALL(*web_data_service_.get(), AddFormFields(_)).Times(0); autocomplete_manager_->OnFormSubmitted(form); } @@ -133,7 +153,7 @@ TEST_F(AutocompleteHistoryManagerTest, NonCreditCardNumberValue) { invalid_cc.form_control_type = "text"; form.fields.push_back(invalid_cc); - EXPECT_CALL(*(web_data_service_), AddFormFields(_)).Times(1); + EXPECT_CALL(*(web_data_service_.get()), AddFormFields(_)).Times(1); autocomplete_manager_->OnFormSubmitted(form); } @@ -153,7 +173,7 @@ TEST_F(AutocompleteHistoryManagerTest, SSNValue) { ssn.form_control_type = "text"; form.fields.push_back(ssn); - EXPECT_CALL(*web_data_service_, AddFormFields(_)).Times(0); + EXPECT_CALL(*web_data_service_.get(), AddFormFields(_)).Times(0); autocomplete_manager_->OnFormSubmitted(form); } @@ -174,7 +194,7 @@ TEST_F(AutocompleteHistoryManagerTest, SearchField) { search_field.form_control_type = "search"; form.fields.push_back(search_field); - EXPECT_CALL(*(web_data_service_), AddFormFields(_)).Times(1); + EXPECT_CALL(*(web_data_service_.get()), AddFormFields(_)).Times(1); autocomplete_manager_->OnFormSubmitted(form); } diff --git a/components/autofill/browser/personal_data_manager.cc b/components/autofill/browser/personal_data_manager.cc index e2afca4..a399548 100644 --- a/components/autofill/browser/personal_data_manager.cc +++ b/components/autofill/browser/personal_data_manager.cc @@ -146,11 +146,11 @@ void PersonalDataManager::Init(BrowserContext* browser_context) { if (!browser_context_->IsOffTheRecord()) metric_logger_->LogIsAutofillEnabledAtStartup(IsAutofillEnabled()); - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); // WebDataService may not be available in tests. - if (!autofill_data) + if (!autofill_data.get()) return; LoadProfiles(); @@ -166,9 +166,9 @@ PersonalDataManager::~PersonalDataManager() { if (!browser_context_) return; - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (autofill_data) + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (autofill_data.get()) autofill_data->RemoveObserver(this); } @@ -375,9 +375,9 @@ void PersonalDataManager::AddProfile(const AutofillProfile& profile) { if (FindByGUID<AutofillProfile>(web_profiles_, profile.guid())) return; - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (!autofill_data) + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (!autofill_data.get()) return; // Don't add a duplicate. @@ -408,9 +408,9 @@ void PersonalDataManager::UpdateProfile(const AutofillProfile& profile) { return; } - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (!autofill_data) + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (!autofill_data.get()) return; // Make the update. @@ -441,9 +441,9 @@ void PersonalDataManager::AddCreditCard(const CreditCard& credit_card) { if (FindByGUID<CreditCard>(credit_cards_, credit_card.guid())) return; - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (!autofill_data) + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (!autofill_data.get()) return; // Don't add a duplicate. @@ -474,9 +474,9 @@ void PersonalDataManager::UpdateCreditCard(const CreditCard& credit_card) { return; } - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (!autofill_data) + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (!autofill_data.get()) return; // Make the update. @@ -496,9 +496,9 @@ void PersonalDataManager::RemoveByGUID(const std::string& guid) { if (!is_credit_card && !is_profile) return; - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (!autofill_data) + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (!autofill_data.get()) return; if (is_credit_card) @@ -784,9 +784,9 @@ void PersonalDataManager::SetProfiles(std::vector<AutofillProfile>* profiles) { address_of<AutofillProfile>); AutofillProfile::AdjustInferredLabels(&profile_pointers); - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (!autofill_data) + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (!autofill_data.get()) return; // Any profiles that are not in the new profile list should be removed from @@ -838,9 +838,9 @@ void PersonalDataManager::SetCreditCards( it++; } - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (!autofill_data) + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (!autofill_data.get()) return; // Any credit cards that are not in the new credit card list should be @@ -878,9 +878,9 @@ void PersonalDataManager::SetCreditCards( } void PersonalDataManager::LoadProfiles() { - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (!autofill_data) { + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (!autofill_data.get()) { NOTREACHED(); return; } @@ -898,9 +898,9 @@ void PersonalDataManager::LoadAuxiliaryProfiles() { #endif void PersonalDataManager::LoadCreditCards() { - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (!autofill_data) { + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (!autofill_data.get()) { NOTREACHED(); return; } @@ -949,9 +949,9 @@ void PersonalDataManager::ReceiveLoadedCreditCards( void PersonalDataManager::CancelPendingQuery( WebDataServiceBase::Handle* handle) { if (*handle) { - AutofillWebDataService* autofill_data = - AutofillWebDataService::FromBrowserContext(browser_context_); - if (!autofill_data) { + scoped_refptr<AutofillWebDataService> autofill_data( + AutofillWebDataService::FromBrowserContext(browser_context_)); + if (!autofill_data.get()) { NOTREACHED(); return; } diff --git a/components/autofill/browser/personal_data_manager_unittest.cc b/components/autofill/browser/personal_data_manager_unittest.cc index 1a4ffd5..6c20883 100644 --- a/components/autofill/browser/personal_data_manager_unittest.cc +++ b/components/autofill/browser/personal_data_manager_unittest.cc @@ -539,9 +539,9 @@ TEST_F(PersonalDataManagerTest, Refresh) { profile_pointers.push_back(&profile2); AutofillProfile::AdjustInferredLabels(&profile_pointers); - AutofillWebDataService* wds = + scoped_refptr<AutofillWebDataService> wds = AutofillWebDataService::FromBrowserContext(profile_.get()); - ASSERT_TRUE(wds); + ASSERT_TRUE(wds.get()); wds->AddAutofillProfile(profile2); personal_data_->Refresh(); diff --git a/components/autofill/browser/webdata/autofill_webdata_service.h b/components/autofill/browser/webdata/autofill_webdata_service.h index a85896d..b861c64 100644 --- a/components/autofill/browser/webdata/autofill_webdata_service.h +++ b/components/autofill/browser/webdata/autofill_webdata_service.h @@ -43,11 +43,9 @@ class AutofillWebDataService : public AutofillWebData, AutofillWebDataService(scoped_refptr<WebDatabaseService> wdbs, const ProfileErrorCallback& callback); - virtual ~AutofillWebDataService(); - // Retrieve an AutofillWebDataService for the given context. // Can return NULL in some contexts. - static AutofillWebDataService* FromBrowserContext( + static scoped_refptr<AutofillWebDataService> FromBrowserContext( content::BrowserContext* context); // WebDataServiceBase implementation. @@ -103,6 +101,8 @@ class AutofillWebDataService : public AutofillWebData, const base::Callback<void(AutofillWebDataBackend*)>& callback); protected: + virtual ~AutofillWebDataService(); + virtual void NotifyAutofillMultipleChangedOnUIThread(); base::WeakPtr<AutofillWebDataService> AsWeakPtr() { diff --git a/components/autofill/browser/webdata/web_data_service_unittest.cc b/components/autofill/browser/webdata/web_data_service_unittest.cc index b12326b..128ac1c 100644 --- a/components/autofill/browser/webdata/web_data_service_unittest.cc +++ b/components/autofill/browser/webdata/web_data_service_unittest.cc @@ -76,15 +76,15 @@ class WebDataServiceTest : public testing::Test { wdbs_->AddTable(scoped_ptr<WebDatabaseTable>(new AutofillTable("en-US"))); wdbs_->LoadDatabase(); - wds_.reset(new AutofillWebDataService( - wdbs_, WebDataServiceBase::ProfileErrorCallback())); + wds_ = new AutofillWebDataService( + wdbs_, WebDataServiceBase::ProfileErrorCallback()); wds_->Init(); } virtual void TearDown() { wds_->ShutdownOnUIThread(); wdbs_->ShutdownDatabase(); - wds_.reset(); + wds_ = NULL; wdbs_ = NULL; WaitForDatabaseThread(); @@ -107,7 +107,7 @@ class WebDataServiceTest : public testing::Test { content::TestBrowserThread ui_thread_; content::TestBrowserThread db_thread_; base::FilePath profile_dir_; - scoped_ptr<AutofillWebDataService> wds_; + scoped_refptr<AutofillWebDataService> wds_; scoped_refptr<WebDatabaseService> wdbs_; base::ScopedTempDir temp_dir_; }; @@ -135,8 +135,7 @@ class WebDataServiceAutofillTest : public WebDataServiceTest { BrowserThread::PostTask( BrowserThread::DB, FROM_HERE, - base::Bind(add_observer_func, - base::Unretained(wds_.get()), &observer_)); + base::Bind(add_observer_func, wds_, &observer_)); WaitForDatabaseThread(); } @@ -147,8 +146,7 @@ class WebDataServiceAutofillTest : public WebDataServiceTest { BrowserThread::PostTask( BrowserThread::DB, FROM_HERE, - base::Bind(remove_observer_func, - base::Unretained(wds_.get()), &observer_)); + base::Bind(remove_observer_func, wds_, &observer_)); WaitForDatabaseThread(); WebDataServiceTest::TearDown(); diff --git a/components/webdata/common/web_data_service_base.h b/components/webdata/common/web_data_service_base.h index 0510184..11d5d3e 100644 --- a/components/webdata/common/web_data_service_base.h +++ b/components/webdata/common/web_data_service_base.h @@ -23,7 +23,9 @@ class Thread; } // Base for WebDataService class hierarchy. -class WEBDATA_EXPORT WebDataServiceBase { +class WEBDATA_EXPORT WebDataServiceBase + : public base::RefCountedThreadSafe<WebDataServiceBase, + content::BrowserThread::DeleteOnUIThread> { public: // All requests return an opaque handle of the following type. typedef int Handle; @@ -46,8 +48,6 @@ class WEBDATA_EXPORT WebDataServiceBase { WebDataServiceBase(scoped_refptr<WebDatabaseService> wdbs, const ProfileErrorCallback& callback); - virtual ~WebDataServiceBase(); - // Cancel any pending request. You need to call this method if your // WebDataServiceConsumer is about to be deleted. virtual void CancelRequest(Handle h); @@ -88,13 +88,20 @@ class WEBDATA_EXPORT WebDataServiceBase { virtual WebDatabase* GetDatabase(); protected: + virtual ~WebDataServiceBase(); + // Our database service. scoped_refptr<WebDatabaseService> wdbs_; private: - ProfileErrorCallback profile_error_callback_; + friend struct content::BrowserThread::DeleteOnThread< + content::BrowserThread::UI>; + friend class base::DeleteHelper<WebDataServiceBase>; + // We have to friend RCTS<> so WIN shared-lib build is happy (crbug/112250). + friend class base::RefCountedThreadSafe<WebDataServiceBase, + content::BrowserThread::DeleteOnUIThread>; - DISALLOW_COPY_AND_ASSIGN(WebDataServiceBase); + ProfileErrorCallback profile_error_callback_; }; #endif // COMPONENTS_WEBDATA_COMMON_WEB_DATA_SERVICE_BASE_H_ diff --git a/components/webdata/common/web_data_service_test_util.cc b/components/webdata/common/web_data_service_test_util.cc index 0ece6a4..2720721 100644 --- a/components/webdata/common/web_data_service_test_util.cc +++ b/components/webdata/common/web_data_service_test_util.cc @@ -22,8 +22,8 @@ void MockWebDataServiceWrapperBase::Shutdown() { // all the webdatas in. MockWebDataServiceWrapper::MockWebDataServiceWrapper( scoped_refptr<WebDataService> fake_service, - AutofillWebDataService* fake_autofill, - TokenWebData* fake_token) + scoped_refptr<AutofillWebDataService> fake_autofill, + scoped_refptr<TokenWebData> fake_token) : fake_autofill_web_data_(fake_autofill), fake_token_web_data_(fake_token), fake_web_data_(fake_service) { @@ -32,13 +32,13 @@ MockWebDataServiceWrapper::MockWebDataServiceWrapper( MockWebDataServiceWrapper::~MockWebDataServiceWrapper() { } -AutofillWebDataService* +scoped_refptr<AutofillWebDataService> MockWebDataServiceWrapper::GetAutofillWebData() { - return fake_autofill_web_data_.get(); + return fake_autofill_web_data_; } -TokenWebData* MockWebDataServiceWrapper::GetTokenWebData() { - return fake_token_web_data_.get(); +scoped_refptr<TokenWebData> MockWebDataServiceWrapper::GetTokenWebData() { + return fake_token_web_data_; } scoped_refptr<WebDataService> MockWebDataServiceWrapper::GetWebData() { diff --git a/components/webdata/common/web_data_service_test_util.h b/components/webdata/common/web_data_service_test_util.h index 6fb3b63..4f0f6e5 100644 --- a/components/webdata/common/web_data_service_test_util.h +++ b/components/webdata/common/web_data_service_test_util.h @@ -58,20 +58,21 @@ class MockWebDataServiceWrapper : public MockWebDataServiceWrapperBase { public: MockWebDataServiceWrapper( scoped_refptr<WebDataService> fake_service, - autofill::AutofillWebDataService* fake_autofill, - TokenWebData* fake_token); + scoped_refptr<autofill::AutofillWebDataService> fake_autofill, + scoped_refptr<TokenWebData> fake_token); virtual ~MockWebDataServiceWrapper(); - virtual autofill::AutofillWebDataService* GetAutofillWebData() OVERRIDE; + virtual scoped_refptr<autofill::AutofillWebDataService> + GetAutofillWebData() OVERRIDE; - virtual TokenWebData* GetTokenWebData() OVERRIDE; + virtual scoped_refptr<TokenWebData> GetTokenWebData() OVERRIDE; virtual scoped_refptr<WebDataService> GetWebData() OVERRIDE; protected: - scoped_ptr<autofill::AutofillWebDataService> fake_autofill_web_data_; - scoped_ptr<TokenWebData> fake_token_web_data_; + scoped_refptr<autofill::AutofillWebDataService> fake_autofill_web_data_; + scoped_refptr<TokenWebData> fake_token_web_data_; scoped_refptr<WebDataService> fake_web_data_; private: |