Commit 638ab304 authored by Victor Costan's avatar Victor Costan Committed by Commit Bot

AppCache: clean up WeakPtrFactory members in classes with children.

This CL marks AppCache clases with WeakPtrFactory members as final, and
removes WeakPtrFactory members from classes that have subclasses.

The superclasses of AppCache classes with WeakPtrFactory members were
checked for WeakPtrFactory members, resulting in minor cleanups.

This CL does not tackle AppCacheUrlRequestJob, because net:URLRequestJob
(which has a WeakPtrFactory) has 35 subclasses. This case will be
addressed separately.

TBR=kinuko

Change-Id: I04bb0757c34fa03620517d04571cba5a0e4a77a5
Reviewed-on: https://chromium-review.googlesource.com/c/1267337
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: default avatarMarijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601431}
parent 557c93c3
......@@ -26,7 +26,7 @@ class ChromeAppCacheService;
// its child processes. There is a distinct host for each child process.
// Messages are handled on the IO thread. The RenderProcessHostImpl creates
// an instance and delegates calls to it.
class AppCacheDispatcherHost : public mojom::AppCacheBackend {
class AppCacheDispatcherHost final : public mojom::AppCacheBackend {
public:
AppCacheDispatcherHost(ChromeAppCacheService* appcache_service,
int process_id);
......
......@@ -49,7 +49,7 @@ class CONTENT_EXPORT AppCacheGroup
public:
// Called just after an appcache update has completed.
virtual void OnUpdateComplete(AppCacheGroup* group) = 0;
virtual ~UpdateObserver() {}
virtual ~UpdateObserver() = default;
};
enum UpdateAppCacheStatus {
......
......@@ -76,7 +76,7 @@ class CONTENT_EXPORT AppCacheHost
// Called just prior to the instance being deleted.
virtual void OnDestructionImminent(AppCacheHost* host) = 0;
virtual ~Observer() {}
virtual ~Observer() = default;
};
AppCacheHost(int host_id, AppCacheFrontend* frontend,
......
......@@ -141,18 +141,19 @@ std::unique_ptr<base::ListValue> GetListValueForAppCacheResourceInfoVector(
AppCacheInternalsUI::Proxy::Proxy(
base::WeakPtr<AppCacheInternalsUI> appcache_internals_ui,
const base::FilePath& partition_path)
: appcache_internals_ui_(appcache_internals_ui),
: appcache_internals_ui_(std::move(appcache_internals_ui)),
partition_path_(partition_path) {}
void AppCacheInternalsUI::Proxy::Initialize(
const scoped_refptr<ChromeAppCacheService>& chrome_appcache_service) {
scoped_refptr<ChromeAppCacheService> chrome_appcache_service) {
if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
base::PostTaskWithTraits(
FROM_HERE, {BrowserThread::IO},
base::BindOnce(&Proxy::Initialize, this, chrome_appcache_service));
base::BindOnce(&Proxy::Initialize, this,
std::move(chrome_appcache_service)));
return;
}
appcache_service_ = chrome_appcache_service->AsWeakPtr();
appcache_service_ = chrome_appcache_service->GetWeakPtr();
shutdown_called_ = false;
preparing_response_ = false;
}
......
......@@ -32,7 +32,7 @@ namespace content {
// This implementation is based on the WebUI API and consists of a controller on
// The UI thread which communicates (through a Proxy) with the AppCacheService
// and AppCache storage which live on the IO thread.
class AppCacheInternalsUI : public WebUIController {
class AppCacheInternalsUI final : public WebUIController {
public:
explicit AppCacheInternalsUI(WebUI* web_ui);
~AppCacheInternalsUI() override;
......@@ -76,7 +76,7 @@ class AppCacheInternalsUI : public WebUIController {
scoped_refptr<net::IOBuffer> response_data,
int net_result_code);
void Initialize(
const scoped_refptr<ChromeAppCacheService>& chrome_appcache_service);
scoped_refptr<ChromeAppCacheService> chrome_appcache_service);
void Shutdown();
base::WeakPtr<AppCacheInternalsUI> appcache_internals_ui_;
......
......@@ -44,7 +44,7 @@ class AppCacheHost;
// given the opportunity to hijack the request along the way. Callers
// should use AppCacheHost::CreateRequestHandler to manufacture instances
// that can retrieve resources for a particular host.
class CONTENT_EXPORT AppCacheRequestHandler
class CONTENT_EXPORT AppCacheRequestHandler final
: public base::SupportsUserData::Data,
public AppCacheHost::Observer,
public AppCacheServiceImpl::Observer,
......
......@@ -392,8 +392,7 @@ AppCacheServiceImpl::AppCacheServiceImpl(
quota_client_(nullptr),
quota_manager_proxy_(quota_manager_proxy),
request_context_(nullptr),
force_keep_session_state_(false),
weak_factory_(this) {
force_keep_session_state_(false) {
if (quota_manager_proxy_.get()) {
quota_client_ = new AppCacheQuotaClient(this);
quota_manager_proxy_->RegisterClient(quota_client_);
......
......@@ -79,7 +79,7 @@ class CONTENT_EXPORT AppCacheServiceImpl : public AppCacheService {
// ref provided.
virtual void OnServiceReinitialized(
AppCacheStorageReference* old_storage_ref) {}
virtual ~Observer() {}
virtual ~Observer() = default;
};
// If not using quota management, the proxy may be NULL.
......@@ -159,9 +159,7 @@ class CONTENT_EXPORT AppCacheServiceImpl : public AppCacheService {
AppCacheStorage* storage() const { return storage_.get(); }
base::WeakPtr<AppCacheServiceImpl> AsWeakPtr() {
return weak_factory_.GetWeakPtr();
}
virtual base::WeakPtr<AppCacheServiceImpl> GetWeakPtr() = 0;
// Disables the exit-time deletion of session-only data.
void set_force_keep_session_state() { force_keep_session_state_ = true; }
......@@ -218,8 +216,6 @@ class CONTENT_EXPORT AppCacheServiceImpl : public AppCacheService {
scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter_;
private:
base::WeakPtrFactory<AppCacheServiceImpl> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(AppCacheServiceImpl);
};
......
......@@ -89,6 +89,22 @@ class MockResponseReader : public AppCacheResponseReader {
int data_size_;
};
class TestAppCacheServiceImpl : public AppCacheServiceImpl {
public:
explicit TestAppCacheServiceImpl(
storage::QuotaManagerProxy* quota_manager_proxy)
: AppCacheServiceImpl(quota_manager_proxy), weak_factory_(this) {}
~TestAppCacheServiceImpl() override = default;
base::WeakPtr<AppCacheServiceImpl> GetWeakPtr() override {
return weak_factory_.GetWeakPtr();
}
private:
base::WeakPtrFactory<TestAppCacheServiceImpl> weak_factory_;
};
} // namespace
......@@ -98,7 +114,7 @@ class AppCacheServiceImplTest : public testing::Test {
: kOriginURL("http://hello/"),
kOrigin(url::Origin::Create(kOriginURL)),
kManifestUrl(kOriginURL.Resolve("manifest")),
service_(new AppCacheServiceImpl(nullptr)),
service_(std::make_unique<TestAppCacheServiceImpl>(nullptr)),
delete_result_(net::OK),
delete_completion_count_(0) {
// Setup to use mock storage.
......@@ -339,8 +355,8 @@ TEST_F(AppCacheServiceImplTest, ScheduleReinitialize) {
const base::TimeDelta kOneHour(base::TimeDelta::FromHours(1));
// Do things get initialized as expected?
std::unique_ptr<AppCacheServiceImpl> service(
new AppCacheServiceImpl(nullptr));
std::unique_ptr<AppCacheServiceImpl> service =
std::make_unique<TestAppCacheServiceImpl>(nullptr);
EXPECT_TRUE(service->last_reinit_time_.is_null());
EXPECT_FALSE(service->reinit_timer_.IsRunning());
EXPECT_EQ(kNoDelay, service->next_reinit_delay_);
......
......@@ -24,8 +24,7 @@ AppCacheStorage::AppCacheStorage(AppCacheServiceImpl* service)
: last_cache_id_(kUnitializedId),
last_group_id_(kUnitializedId),
last_response_id_(kUnitializedId),
service_(service),
weak_factory_(this) {}
service_(service) {}
AppCacheStorage::~AppCacheStorage() {
DCHECK(delegate_references_.empty());
......@@ -98,10 +97,6 @@ void AppCacheStorage::LoadResponseInfo(const GURL& manifest_url,
info_load->StartIfNeeded();
}
base::WeakPtr<AppCacheStorage> AppCacheStorage::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
void AppCacheStorage::UpdateUsageMapAndNotify(const url::Origin& origin,
int64_t new_usage) {
DCHECK_GE(new_usage, 0);
......
......@@ -87,7 +87,7 @@ class CONTENT_EXPORT AppCacheStorage {
const GURL& mainfest_url) {}
protected:
virtual ~Delegate() {}
virtual ~Delegate() = default;
};
explicit AppCacheStorage(AppCacheServiceImpl* service);
......@@ -203,6 +203,9 @@ class CONTENT_EXPORT AppCacheStorage {
// Returns true if the AppCacheStorage instance is initialized.
virtual bool IsInitialized() = 0;
// Returns a weak pointer reference to the AppCacheStorage instance.
virtual base::WeakPtr<AppCacheStorage> GetWeakPtr() = 0;
// Generates unique storage ids for different object types.
int64_t NewCacheId() { return ++last_cache_id_; }
int64_t NewGroupId() { return ++last_group_id_; }
......@@ -216,9 +219,6 @@ class CONTENT_EXPORT AppCacheStorage {
// Simple ptr back to the service object that owns us.
AppCacheServiceImpl* service() { return service_; }
// Returns a weak pointer reference to the AppCacheStorage instance.
base::WeakPtr<AppCacheStorage> GetWeakPtr();
protected:
friend class content::AppCacheQuotaClientTest;
friend class content::AppCacheResponseTest;
......@@ -340,10 +340,6 @@ class CONTENT_EXPORT AppCacheStorage {
content::appcache_storage_unittest::AppCacheStorageTest,
UsageMap);
// The WeakPtrFactory below must occur last in the class definition so they
// get destroyed last.
base::WeakPtrFactory<AppCacheStorage> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(AppCacheStorage);
};
......
......@@ -1740,6 +1740,10 @@ bool AppCacheStorageImpl::IsInitialized() {
return IsInitTaskComplete();
}
base::WeakPtr<AppCacheStorage> AppCacheStorageImpl::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
void AppCacheStorageImpl::DelayedStartDeletingUnusedResponses() {
// Only if we haven't already begun.
if (!did_start_deleting_responses_) {
......
......@@ -33,7 +33,7 @@ namespace content {
class AppCacheStorageImplTest;
class ChromeAppCacheServiceTest;
class AppCacheStorageImpl : public AppCacheStorage {
class CONTENT_EXPORT AppCacheStorageImpl final : public AppCacheStorage {
public:
explicit AppCacheStorageImpl(AppCacheServiceImpl* service);
~AppCacheStorageImpl() override;
......@@ -76,6 +76,7 @@ class AppCacheStorageImpl : public AppCacheStorage {
void DeleteResponses(const GURL& manifest_url,
const std::vector<int64_t>& response_ids) override;
bool IsInitialized() override;
base::WeakPtr<AppCacheStorage> GetWeakPtr() override;
private:
// The AppCacheStorageImpl class methods and datamembers may only be
......@@ -154,7 +155,7 @@ class AppCacheStorageImpl : public AppCacheStorage {
const GURL& manifest_url);
// Don't call this when |is_disabled_| is true.
CONTENT_EXPORT AppCacheDiskCache* disk_cache();
AppCacheDiskCache* disk_cache();
// The directory in which we place files in the file system.
base::FilePath cache_directory_;
......
......@@ -158,6 +158,22 @@ class IOThread : public base::Thread {
std::unique_ptr<net::URLRequestContext> request_context_;
};
class TestAppCacheServiceImpl : public AppCacheServiceImpl {
public:
explicit TestAppCacheServiceImpl(
storage::QuotaManagerProxy* quota_manager_proxy)
: AppCacheServiceImpl(quota_manager_proxy), weak_factory_(this) {}
~TestAppCacheServiceImpl() override = default;
base::WeakPtr<AppCacheServiceImpl> GetWeakPtr() override {
return weak_factory_.GetWeakPtr();
}
private:
base::WeakPtrFactory<TestAppCacheServiceImpl> weak_factory_;
};
std::unique_ptr<base::test::ScopedTaskEnvironment> scoped_task_environment;
std::unique_ptr<IOThread> io_thread;
std::unique_ptr<base::Thread> background_thread;
......@@ -393,7 +409,7 @@ class AppCacheStorageImplTest : public testing::Test {
void SetUpTest() {
DCHECK(io_thread->task_runner()->BelongsToCurrentThread());
service_.reset(new AppCacheServiceImpl(nullptr));
service_ = std::make_unique<TestAppCacheServiceImpl>(nullptr);
service_->Initialize(base::FilePath());
mock_quota_manager_proxy_ = new MockQuotaManagerProxy();
service_->quota_manager_proxy_ = mock_quota_manager_proxy_;
......@@ -1710,7 +1726,7 @@ class AppCacheStorageImplTest : public testing::Test {
}
// Recreate the service to point at the db and corruption on disk.
service_.reset(new AppCacheServiceImpl(nullptr));
service_ = std::make_unique<TestAppCacheServiceImpl>(nullptr);
service_->set_request_context(io_thread->request_context());
service_->Initialize(temp_directory_.GetPath());
mock_quota_manager_proxy_ = new MockQuotaManagerProxy();
......
......@@ -34,8 +34,8 @@ namespace {
//
// This class owns and scopes the lifetime of the AppCacheRequestHandler
// for the duration of a subresource load.
class SubresourceLoader : public network::mojom::URLLoader,
public network::mojom::URLLoaderClient {
class SubresourceLoader final : public network::mojom::URLLoader,
public network::mojom::URLLoaderClient {
public:
SubresourceLoader(
network::mojom::URLLoaderRequest url_loader_request,
......@@ -67,7 +67,7 @@ class SubresourceLoader : public network::mojom::URLLoader,
}
private:
~SubresourceLoader() override {}
~SubresourceLoader() override = default;
void OnConnectionError() { delete this; }
......
......@@ -25,7 +25,7 @@ class AppCacheRequestHandler;
class AppCacheServiceImpl;
// Implements the URLLoaderFactory mojom for AppCache subresource requests.
class CONTENT_EXPORT AppCacheSubresourceURLFactory
class CONTENT_EXPORT AppCacheSubresourceURLFactory final
: public network::mojom::URLLoaderFactory {
public:
~AppCacheSubresourceURLFactory() override;
......
......@@ -42,7 +42,7 @@ class AppCacheUpdateJobTest;
class HostNotifier;
// Application cache Update algorithm and state.
class CONTENT_EXPORT AppCacheUpdateJob
class CONTENT_EXPORT AppCacheUpdateJob final
: public AppCacheStorage::Delegate,
public AppCacheHost::Observer,
public AppCacheServiceImpl::Observer {
......
......@@ -18,7 +18,7 @@ namespace content {
// URLRequest subclass for the UpdateRequestBase class. Provides functionality
// to update the AppCache using functionality provided by the URLRequest class.
class AppCacheUpdateJob::UpdateURLRequest
class AppCacheUpdateJob::UpdateURLRequest final
: public AppCacheUpdateJob::UpdateRequestBase,
public net::URLRequest::Delegate {
public:
......
......@@ -32,9 +32,10 @@ class AppCacheURLLoaderRequest;
// AppCacheJob wrapper for a network::mojom::URLLoader implementation which
// returns responses stored in the AppCache.
class CONTENT_EXPORT AppCacheURLLoaderJob : public AppCacheJob,
public AppCacheStorage::Delegate,
public network::mojom::URLLoader {
class CONTENT_EXPORT AppCacheURLLoaderJob final
: public AppCacheJob,
public AppCacheStorage::Delegate,
public network::mojom::URLLoader {
public:
~AppCacheURLLoaderJob() override;
......
......@@ -15,7 +15,7 @@ namespace content {
// AppCacheRequest wrapper for the ResourceRequest class for users of
// URLLoader.
class CONTENT_EXPORT AppCacheURLLoaderRequest : public AppCacheRequest {
class CONTENT_EXPORT AppCacheURLLoaderRequest final : public AppCacheRequest {
public:
// Factory function to create an instance of the AppCacheResourceRequest
// class.
......
......@@ -29,9 +29,10 @@ class AppCacheURLRequestJobTest;
// A net::URLRequestJob derivative that knows how to return a response stored
// in the appcache.
class CONTENT_EXPORT AppCacheURLRequestJob : public AppCacheJob,
public AppCacheStorage::Delegate,
public net::URLRequestJob {
class CONTENT_EXPORT AppCacheURLRequestJob final
: public AppCacheJob,
public AppCacheStorage::Delegate,
public net::URLRequestJob {
public:
~AppCacheURLRequestJob() override;
......
......@@ -17,7 +17,9 @@ namespace content {
ChromeAppCacheService::ChromeAppCacheService(
storage::QuotaManagerProxy* quota_manager_proxy)
: AppCacheServiceImpl(quota_manager_proxy), resource_context_(nullptr) {}
: AppCacheServiceImpl(quota_manager_proxy),
resource_context_(nullptr),
weak_factory_(this) {}
void ChromeAppCacheService::InitializeOnIOThread(
const base::FilePath& cache_path,
......@@ -67,6 +69,10 @@ void ChromeAppCacheService::UnregisterBackend(
AppCacheServiceImpl::UnregisterBackend(backend_impl);
}
base::WeakPtr<AppCacheServiceImpl> ChromeAppCacheService::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
void ChromeAppCacheService::Shutdown() {
bindings_.CloseAllBindings();
}
......
......@@ -8,6 +8,7 @@
#include "base/compiler_specific.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/sequenced_task_runner_helpers.h"
#include "content/browser/appcache/appcache_backend_impl.h"
#include "content/browser/appcache/appcache_policy.h"
......@@ -41,7 +42,7 @@ struct ChromeAppCacheServiceDeleter;
//
// TODO(dpranke): Fix dependencies on AppCacheServiceImpl so that we don't have
// to worry about clients calling AppCacheServiceImpl methods.
class CONTENT_EXPORT ChromeAppCacheService
class CONTENT_EXPORT ChromeAppCacheService final
: public base::RefCountedThreadSafe<ChromeAppCacheService,
ChromeAppCacheServiceDeleter>,
public AppCacheServiceImpl,
......@@ -76,6 +77,7 @@ class CONTENT_EXPORT ChromeAppCacheService
// AppCacheServiceImpl override
void UnregisterBackend(AppCacheBackendImpl* backend_impl) override;
base::WeakPtr<AppCacheServiceImpl> GetWeakPtr() override;
protected:
~ChromeAppCacheService() override;
......@@ -95,6 +97,8 @@ class CONTENT_EXPORT ChromeAppCacheService
// A map from a process_id to a binding_id.
std::map<int, mojo::BindingId> process_bindings_;
base::WeakPtrFactory<ChromeAppCacheService> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(ChromeAppCacheService);
};
......
......@@ -13,17 +13,27 @@
namespace content {
static void DeferredCallCallback(net::CompletionOnceCallback callback, int rv) {
std::move(callback).Run(rv);
MockAppCacheService::MockAppCacheService()
: AppCacheServiceImpl(nullptr),
mock_delete_appcaches_for_origin_result_(net::OK),
delete_called_count_(0),
weak_factory_(this) {
storage_.reset(new MockAppCacheStorage(this));
}
MockAppCacheService::~MockAppCacheService() = default;
void MockAppCacheService::DeleteAppCachesForOrigin(
const url::Origin& origin,
net::CompletionOnceCallback callback) {
++delete_called_count_;
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(&DeferredCallCallback, std::move(callback),
FROM_HERE, base::BindOnce(std::move(callback),
mock_delete_appcaches_for_origin_result_));
}
base::WeakPtr<AppCacheServiceImpl> MockAppCacheService::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
} // namespace content
......@@ -15,18 +15,16 @@ namespace content {
// For use by unit tests.
class MockAppCacheService : public AppCacheServiceImpl {
public:
MockAppCacheService()
: AppCacheServiceImpl(NULL),
mock_delete_appcaches_for_origin_result_(net::OK),
delete_called_count_(0) {
storage_.reset(new MockAppCacheStorage(this));
}
MockAppCacheService();
~MockAppCacheService() override;
// Just returns a canned completion code without actually
// removing groups and caches in our mock storage instance.
void DeleteAppCachesForOrigin(const url::Origin& origin,
net::CompletionOnceCallback callback) override;
base::WeakPtr<AppCacheServiceImpl> GetWeakPtr() override;
void set_quota_manager_proxy(storage::QuotaManagerProxy* proxy) {
quota_manager_proxy_ = proxy;
}
......@@ -40,6 +38,7 @@ class MockAppCacheService : public AppCacheServiceImpl {
private:
int mock_delete_appcaches_for_origin_result_;
int delete_called_count_;
base::WeakPtrFactory<MockAppCacheService> weak_factory_;
};
} // namespace content
......
......@@ -203,6 +203,10 @@ bool MockAppCacheStorage::IsInitialized() {
return false;
}
base::WeakPtr<AppCacheStorage> MockAppCacheStorage::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
void MockAppCacheStorage::ProcessGetAllInfo(
scoped_refptr<DelegateReference> delegate_ref) {
if (delegate_ref->delegate)
......
......@@ -51,7 +51,7 @@ class AppCacheUpdateJobTest;
// Note: This class is also being used to bootstrap our development efforts.
// We can get layout tests up and running, and back fill with real storage
// somewhat in parallel.
class MockAppCacheStorage : public AppCacheStorage {
class MockAppCacheStorage final : public AppCacheStorage {
public:
explicit MockAppCacheStorage(AppCacheServiceImpl* service);
~MockAppCacheStorage() override;
......@@ -87,6 +87,7 @@ class MockAppCacheStorage : public AppCacheStorage {
void DeleteResponses(const GURL& manifest_url,
const std::vector<int64_t>& response_ids) override;
bool IsInitialized() override;
base::WeakPtr<AppCacheStorage> GetWeakPtr() override;
private:
friend class AppCacheRequestHandlerTest;
......
......@@ -49,7 +49,7 @@ class CONTENT_EXPORT AppCacheService {
net::CompletionOnceCallback callback) = 0;
protected:
virtual ~AppCacheService() {}
virtual ~AppCacheService() = default;
};
} // namespace content
......
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