From 81b307ad3b8c4c3365b13c5b7dca4878736d69a0 Mon Sep 17 00:00:00 2001 From: Evan Stade Date: Thu, 7 Jan 2021 19:10:07 +0000 Subject: [PATCH] [Code health] Fix gn check issues in //extensions/browser/value_store This required adding deps, which in turn created circular includes. To address this, the patch * moves settings_namespace.{cc,} into the //extensions/browser target * passes the task runner from GetExtensionFileTaskRunner() as a parameter into value store code Bug: 1158986 Change-Id: I91ae5c90d82c5cbe5eb1d2a62cd1dd65a003e74c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2601640 Reviewed-by: Erik Staab Reviewed-by: Devlin Commit-Queue: Evan Stade Cr-Commit-Position: refs/heads/master@{#841134} --- .gn | 1 - .../api/storage/policy_value_store.cc | 2 +- .../api/storage/settings_apitest.cc | 2 +- .../api/storage/settings_sync_processor.cc | 2 +- .../api/storage/syncable_settings_storage.cc | 2 +- .../browser/api/extensions_api_client.h | 2 +- extensions/browser/api/storage/BUILD.gn | 2 - .../browser/api/storage/settings_observer.h | 2 +- .../browser/api/storage/settings_test_util.h | 2 +- extensions/browser/api/storage/storage_api.h | 2 +- .../browser/api/storage/storage_frontend.h | 2 +- .../api/storage/storage_frontend_unittest.cc | 2 +- extensions/browser/state_store.cc | 8 +++- extensions/browser/value_store/BUILD.gn | 3 ++ .../settings_namespace.cc | 2 +- .../settings_namespace.h | 6 +-- .../browser/value_store/value_store_factory.h | 2 +- .../value_store/value_store_frontend.cc | 44 ++++++++++++------- .../value_store/value_store_frontend.h | 12 ++++- .../value_store_frontend_unittest.cc | 10 ++++- 20 files changed, 69 insertions(+), 41 deletions(-) rename extensions/browser/{api/storage => value_store}/settings_namespace.cc (94%) rename extensions/browser/{api/storage => value_store}/settings_namespace.h (81%) diff --git a/.gn b/.gn index f63ba606b6fea4..4b92e310b19a8f 100644 --- a/.gn +++ b/.gn @@ -138,7 +138,6 @@ no_check_targets = [ "//extensions/browser/api/webcam_private:*", # 8 errors "//extensions/browser/api:*", # 7 errors "//extensions/browser/updater:*", # 31 errors - "//extensions/browser/value_store:*", # 5 errors "//extensions/browser:*", # 20 errors "//extensions:*", # 75 errors "//headless:*", # 167 errors diff --git a/chrome/browser/extensions/api/storage/policy_value_store.cc b/chrome/browser/extensions/api/storage/policy_value_store.cc index 264828c2308756..f5c20f6439c184 100644 --- a/chrome/browser/extensions/api/storage/policy_value_store.cc +++ b/chrome/browser/extensions/api/storage/policy_value_store.cc @@ -11,7 +11,7 @@ #include "components/policy/core/common/policy_map.h" #include "components/policy/core/common/policy_types.h" #include "extensions/browser/api/storage/backend_task_runner.h" -#include "extensions/browser/api/storage/settings_namespace.h" +#include "extensions/browser/value_store/settings_namespace.h" #include "extensions/browser/value_store/value_store_change.h" namespace extensions { diff --git a/chrome/browser/extensions/api/storage/settings_apitest.cc b/chrome/browser/extensions/api/storage/settings_apitest.cc index 2bceabdf49e754..e5db87af45cf24 100644 --- a/chrome/browser/extensions/api/storage/settings_apitest.cc +++ b/chrome/browser/extensions/api/storage/settings_apitest.cc @@ -35,9 +35,9 @@ #include "components/sync/test/model/sync_error_factory_mock.h" #include "content/public/test/browser_test.h" #include "extensions/browser/api/storage/backend_task_runner.h" -#include "extensions/browser/api/storage/settings_namespace.h" #include "extensions/browser/api/storage/storage_frontend.h" #include "extensions/browser/extension_system.h" +#include "extensions/browser/value_store/settings_namespace.h" #include "extensions/common/value_builder.h" #include "extensions/test/extension_test_message_listener.h" #include "extensions/test/result_catcher.h" diff --git a/chrome/browser/extensions/api/storage/settings_sync_processor.cc b/chrome/browser/extensions/api/storage/settings_sync_processor.cc index 583bdbfc415144..8674d0c41afaf4 100644 --- a/chrome/browser/extensions/api/storage/settings_sync_processor.cc +++ b/chrome/browser/extensions/api/storage/settings_sync_processor.cc @@ -11,7 +11,7 @@ #include "components/sync/model/sync_data.h" #include "components/sync/protocol/extension_setting_specifics.pb.h" #include "extensions/browser/api/storage/backend_task_runner.h" -#include "extensions/browser/api/storage/settings_namespace.h" +#include "extensions/browser/value_store/settings_namespace.h" namespace extensions { diff --git a/chrome/browser/extensions/api/storage/syncable_settings_storage.cc b/chrome/browser/extensions/api/storage/syncable_settings_storage.cc index 3457a1f2854ce6..c0fdf60e0a17f1 100644 --- a/chrome/browser/extensions/api/storage/syncable_settings_storage.cc +++ b/chrome/browser/extensions/api/storage/syncable_settings_storage.cc @@ -15,7 +15,7 @@ #include "components/sync/model/sync_error.h" #include "components/sync/protocol/extension_setting_specifics.pb.h" #include "extensions/browser/api/storage/backend_task_runner.h" -#include "extensions/browser/api/storage/settings_namespace.h" +#include "extensions/browser/value_store/settings_namespace.h" namespace extensions { diff --git a/extensions/browser/api/extensions_api_client.h b/extensions/browser/api/extensions_api_client.h index d85c543baddd42..2768f0e8dc2b89 100644 --- a/extensions/browser/api/extensions_api_client.h +++ b/extensions/browser/api/extensions_api_client.h @@ -14,7 +14,7 @@ #include "build/chromeos_buildflags.h" #include "extensions/browser/api/clipboard/clipboard_api.h" #include "extensions/browser/api/declarative_content/content_rules_registry.h" -#include "extensions/browser/api/storage/settings_namespace.h" +#include "extensions/browser/value_store/settings_namespace.h" #include "extensions/common/api/clipboard.h" #include "extensions/common/extension.h" #include "extensions/common/extension_id.h" diff --git a/extensions/browser/api/storage/BUILD.gn b/extensions/browser/api/storage/BUILD.gn index 90f8831665bb8b..db0070b7e07f04 100644 --- a/extensions/browser/api/storage/BUILD.gn +++ b/extensions/browser/api/storage/BUILD.gn @@ -13,8 +13,6 @@ source_set("storage") { "backend_task_runner.h", "local_value_store_cache.cc", "local_value_store_cache.h", - "settings_namespace.cc", - "settings_namespace.h", "settings_observer.h", "settings_storage_quota_enforcer.cc", "settings_storage_quota_enforcer.h", diff --git a/extensions/browser/api/storage/settings_observer.h b/extensions/browser/api/storage/settings_observer.h index daa7e6932c0ef1..12ee6e7f4882b1 100644 --- a/extensions/browser/api/storage/settings_observer.h +++ b/extensions/browser/api/storage/settings_observer.h @@ -6,7 +6,7 @@ #define EXTENSIONS_BROWSER_API_STORAGE_SETTINGS_OBSERVER_H_ #include "base/observer_list_threadsafe.h" -#include "extensions/browser/api/storage/settings_namespace.h" +#include "extensions/browser/value_store/settings_namespace.h" namespace extensions { diff --git a/extensions/browser/api/storage/settings_test_util.h b/extensions/browser/api/storage/settings_test_util.h index 6001455c235cbd..7d6efbe4cf1500 100644 --- a/extensions/browser/api/storage/settings_test_util.h +++ b/extensions/browser/api/storage/settings_test_util.h @@ -12,9 +12,9 @@ #include "base/compiler_specific.h" #include "base/memory/ref_counted.h" #include "chrome/test/base/testing_profile.h" -#include "extensions/browser/api/storage/settings_namespace.h" #include "extensions/browser/event_router.h" #include "extensions/browser/mock_extension_system.h" +#include "extensions/browser/value_store/settings_namespace.h" #include "extensions/browser/value_store/value_store_factory.h" #include "extensions/common/extension.h" diff --git a/extensions/browser/api/storage/storage_api.h b/extensions/browser/api/storage/storage_api.h index b30de8f9ab808c..fce167d009f658 100644 --- a/extensions/browser/api/storage/storage_api.h +++ b/extensions/browser/api/storage/storage_api.h @@ -9,9 +9,9 @@ #include "base/compiler_specific.h" #include "base/memory/ref_counted.h" -#include "extensions/browser/api/storage/settings_namespace.h" #include "extensions/browser/api/storage/settings_observer.h" #include "extensions/browser/extension_function.h" +#include "extensions/browser/value_store/settings_namespace.h" #include "extensions/browser/value_store/value_store.h" namespace extensions { diff --git a/extensions/browser/api/storage/storage_frontend.h b/extensions/browser/api/storage/storage_frontend.h index c4642e7927b8f5..4f1bc3fd0915ce 100644 --- a/extensions/browser/api/storage/storage_frontend.h +++ b/extensions/browser/api/storage/storage_frontend.h @@ -11,10 +11,10 @@ #include "base/macros.h" #include "base/memory/ref_counted.h" -#include "extensions/browser/api/storage/settings_namespace.h" #include "extensions/browser/api/storage/settings_observer.h" #include "extensions/browser/api/storage/value_store_cache.h" #include "extensions/browser/browser_context_keyed_api_factory.h" +#include "extensions/browser/value_store/settings_namespace.h" namespace content { class BrowserContext; diff --git a/extensions/browser/api/storage/storage_frontend_unittest.cc b/extensions/browser/api/storage/storage_frontend_unittest.cc index 3da66c8bbe1af3..84998eb955264a 100644 --- a/extensions/browser/api/storage/storage_frontend_unittest.cc +++ b/extensions/browser/api/storage/storage_frontend_unittest.cc @@ -14,9 +14,9 @@ #include "content/public/test/test_browser_context.h" #include "content/public/test/test_utils.h" #include "extensions/browser/api/extensions_api_client.h" -#include "extensions/browser/api/storage/settings_namespace.h" #include "extensions/browser/api/storage/settings_test_util.h" #include "extensions/browser/extensions_test.h" +#include "extensions/browser/value_store/settings_namespace.h" #include "extensions/browser/value_store/value_store.h" #include "extensions/browser/value_store/value_store_factory_impl.h" #include "testing/gtest/include/gtest/gtest.h" diff --git a/extensions/browser/state_store.cc b/extensions/browser/state_store.cc index d6e844bca34f65..fc31167a9b0059 100644 --- a/extensions/browser/state_store.cc +++ b/extensions/browser/state_store.cc @@ -15,6 +15,7 @@ #include "content/public/browser/browser_context.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/notification_types.h" +#include "extensions/browser/extension_file_task_runner.h" #include "extensions/browser/value_store/value_store_factory.h" #include "extensions/common/extension.h" @@ -74,8 +75,11 @@ StateStore::StateStore(content::BrowserContext* context, const scoped_refptr& store_factory, ValueStoreFrontend::BackendType backend_type, bool deferred_load) - : store_(new ValueStoreFrontend(store_factory, backend_type)), - task_queue_(new DelayedTaskQueue()) { + : store_( + std::make_unique(store_factory, + backend_type, + GetExtensionFileTaskRunner())), + task_queue_(std::make_unique()) { extension_registry_observer_.Add(ExtensionRegistry::Get(context)); if (deferred_load) { diff --git a/extensions/browser/value_store/BUILD.gn b/extensions/browser/value_store/BUILD.gn index c8972567b107a4..8e444da12051ec 100644 --- a/extensions/browser/value_store/BUILD.gn +++ b/extensions/browser/value_store/BUILD.gn @@ -17,6 +17,8 @@ source_set("value_store") { "leveldb_scoped_database.h", "leveldb_value_store.cc", "leveldb_value_store.h", + "settings_namespace.cc", + "settings_namespace.h", "value_store.cc", "value_store.h", "value_store_change.cc", @@ -30,6 +32,7 @@ source_set("value_store") { deps = [ "//base", + "//content/public/browser", "//extensions/common", "//third_party/leveldatabase", ] diff --git a/extensions/browser/api/storage/settings_namespace.cc b/extensions/browser/value_store/settings_namespace.cc similarity index 94% rename from extensions/browser/api/storage/settings_namespace.cc rename to extensions/browser/value_store/settings_namespace.cc index 1189073a5e47b7..b6d9be11dd971a 100644 --- a/extensions/browser/api/storage/settings_namespace.cc +++ b/extensions/browser/value_store/settings_namespace.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "extensions/browser/api/storage/settings_namespace.h" +#include "extensions/browser/value_store/settings_namespace.h" #include "base/notreached.h" diff --git a/extensions/browser/api/storage/settings_namespace.h b/extensions/browser/value_store/settings_namespace.h similarity index 81% rename from extensions/browser/api/storage/settings_namespace.h rename to extensions/browser/value_store/settings_namespace.h index e32612b96c02d2..3101bde80e9edd 100644 --- a/extensions/browser/api/storage/settings_namespace.h +++ b/extensions/browser/value_store/settings_namespace.h @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef EXTENSIONS_BROWSER_API_STORAGE_SETTINGS_NAMESPACE_H_ -#define EXTENSIONS_BROWSER_API_STORAGE_SETTINGS_NAMESPACE_H_ +#ifndef EXTENSIONS_BROWSER_VALUE_STORE_SETTINGS_NAMESPACE_H_ +#define EXTENSIONS_BROWSER_VALUE_STORE_SETTINGS_NAMESPACE_H_ #include @@ -31,4 +31,4 @@ Namespace FromString(const std::string& ns_string); } // namespace extensions -#endif // EXTENSIONS_BROWSER_API_STORAGE_SETTINGS_NAMESPACE_H_ +#endif // EXTENSIONS_BROWSER_VALUE_STORE_SETTINGS_NAMESPACE_H_ diff --git a/extensions/browser/value_store/value_store_factory.h b/extensions/browser/value_store/value_store_factory.h index be0000bf591960..25580d176a0451 100644 --- a/extensions/browser/value_store/value_store_factory.h +++ b/extensions/browser/value_store/value_store_factory.h @@ -9,7 +9,7 @@ #include #include "base/memory/ref_counted.h" -#include "extensions/browser/api/storage/settings_namespace.h" +#include "extensions/browser/value_store/settings_namespace.h" #include "extensions/common/extension_id.h" class ValueStore; diff --git a/extensions/browser/value_store/value_store_frontend.cc b/extensions/browser/value_store/value_store_frontend.cc index 3391c0da5cf404..c06ee7dcba7e1f 100644 --- a/extensions/browser/value_store/value_store_frontend.cc +++ b/extensions/browser/value_store/value_store_frontend.cc @@ -11,26 +11,28 @@ #include "base/logging.h" #include "base/macros.h" #include "base/memory/scoped_refptr.h" +#include "base/sequenced_task_runner.h" #include "base/trace_event/trace_event.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" -#include "extensions/browser/api/storage/backend_task_runner.h" #include "extensions/browser/value_store/leveldb_value_store.h" #include "extensions/browser/value_store/value_store_factory.h" using content::BrowserThread; -using extensions::ValueStoreFactory; -using extensions::IsOnBackendSequence; -using extensions::GetBackendTaskRunner; + +namespace extensions { class ValueStoreFrontend::Backend : public base::RefCountedThreadSafe { public: Backend(const scoped_refptr& store_factory, - BackendType backend_type) - : store_factory_(store_factory), backend_type_(backend_type) {} + BackendType backend_type, + const scoped_refptr& task_runner) + : store_factory_(store_factory), + backend_type_(backend_type), + task_runner_(task_runner) {} void Get(const std::string& key, ValueStoreFrontend::ReadCallback callback) { - DCHECK(IsOnBackendSequence()); + DCHECK(task_runner_->RunsTasksInCurrentSequence()); LazyInit(); ValueStore::ReadResult result = storage_->Get(key); @@ -50,7 +52,7 @@ class ValueStoreFrontend::Backend : public base::RefCountedThreadSafe { } void Set(const std::string& key, std::unique_ptr value) { - DCHECK(IsOnBackendSequence()); + DCHECK(task_runner_->RunsTasksInCurrentSequence()); LazyInit(); // We don't need the old value, so skip generating changes. ValueStore::WriteResult result = storage_->Set( @@ -61,7 +63,7 @@ class ValueStoreFrontend::Backend : public base::RefCountedThreadSafe { } void Remove(const std::string& key) { - DCHECK(IsOnBackendSequence()); + DCHECK(task_runner_->RunsTasksInCurrentSequence()); LazyInit(); storage_->Remove(key); } @@ -70,12 +72,12 @@ class ValueStoreFrontend::Backend : public base::RefCountedThreadSafe { friend class base::RefCountedThreadSafe; virtual ~Backend() { - if (storage_ && !IsOnBackendSequence()) - GetBackendTaskRunner()->DeleteSoon(FROM_HERE, storage_.release()); + if (storage_ && !task_runner_->RunsTasksInCurrentSequence()) + task_runner_->DeleteSoon(FROM_HERE, storage_.release()); } void LazyInit() { - DCHECK(IsOnBackendSequence()); + DCHECK(task_runner_->RunsTasksInCurrentSequence()); if (storage_) return; TRACE_EVENT0("ValueStoreFrontend::Backend", "LazyInit"); @@ -100,6 +102,8 @@ class ValueStoreFrontend::Backend : public base::RefCountedThreadSafe { scoped_refptr store_factory_; BackendType backend_type_; + scoped_refptr task_runner_; + // The actual ValueStore that handles persisting the data to disk. Used // exclusively on the backend sequence. std::unique_ptr storage_; @@ -111,8 +115,12 @@ class ValueStoreFrontend::Backend : public base::RefCountedThreadSafe { ValueStoreFrontend::ValueStoreFrontend( const scoped_refptr& store_factory, - BackendType backend_type) - : backend_(base::MakeRefCounted(store_factory, backend_type)) { + BackendType backend_type, + const scoped_refptr& task_runner) + : backend_(base::MakeRefCounted(store_factory, + backend_type, + task_runner)), + task_runner_(task_runner) { DCHECK_CURRENTLY_ON(BrowserThread::UI); } @@ -123,7 +131,7 @@ ValueStoreFrontend::~ValueStoreFrontend() { void ValueStoreFrontend::Get(const std::string& key, ReadCallback callback) { DCHECK_CURRENTLY_ON(BrowserThread::UI); - GetBackendTaskRunner()->PostTask( + task_runner_->PostTask( FROM_HERE, base::BindOnce(&ValueStoreFrontend::Backend::Get, backend_, key, std::move(callback))); } @@ -132,7 +140,7 @@ void ValueStoreFrontend::Set(const std::string& key, std::unique_ptr value) { DCHECK_CURRENTLY_ON(BrowserThread::UI); - GetBackendTaskRunner()->PostTask( + task_runner_->PostTask( FROM_HERE, base::BindOnce(&ValueStoreFrontend::Backend::Set, backend_, key, std::move(value))); } @@ -140,7 +148,9 @@ void ValueStoreFrontend::Set(const std::string& key, void ValueStoreFrontend::Remove(const std::string& key) { DCHECK_CURRENTLY_ON(BrowserThread::UI); - GetBackendTaskRunner()->PostTask( + task_runner_->PostTask( FROM_HERE, base::BindOnce(&ValueStoreFrontend::Backend::Remove, backend_, key)); } + +} // namespace extensions diff --git a/extensions/browser/value_store/value_store_frontend.h b/extensions/browser/value_store/value_store_frontend.h index 786db045db240b..118f3c41d13e4f 100644 --- a/extensions/browser/value_store/value_store_frontend.h +++ b/extensions/browser/value_store/value_store_frontend.h @@ -14,9 +14,12 @@ #include "base/values.h" #include "extensions/browser/value_store/value_store.h" +namespace base { +class SequencedTaskRunner; +} + namespace extensions { class ValueStoreFactory; -} // namespace extensions // A frontend for a LeveldbValueStore, for use on the UI thread. class ValueStoreFrontend { @@ -28,7 +31,8 @@ class ValueStoreFrontend { ValueStoreFrontend( const scoped_refptr& store_factory, - BackendType backend_type); + BackendType backend_type, + const scoped_refptr& task_runner); ~ValueStoreFrontend(); // Retrieves a value from the database asynchronously, passing a copy to @@ -48,7 +52,11 @@ class ValueStoreFrontend { // on the FILE thread. scoped_refptr backend_; + scoped_refptr task_runner_; + DISALLOW_COPY_AND_ASSIGN(ValueStoreFrontend); }; +} // namespace extensions + #endif // EXTENSIONS_BROWSER_VALUE_STORE_VALUE_STORE_FRONTEND_H_ diff --git a/extensions/browser/value_store/value_store_frontend_unittest.cc b/extensions/browser/value_store/value_store_frontend_unittest.cc index c5437c6bf7b50d..6098373cee3c50 100644 --- a/extensions/browser/value_store/value_store_frontend_unittest.cc +++ b/extensions/browser/value_store/value_store_frontend_unittest.cc @@ -13,10 +13,13 @@ #include "base/path_service.h" #include "content/public/test/browser_task_environment.h" #include "content/public/test/test_utils.h" +#include "extensions/browser/extension_file_task_runner.h" #include "extensions/browser/value_store/test_value_store_factory.h" #include "extensions/common/extension_paths.h" #include "testing/gtest/include/gtest/gtest.h" +namespace extensions { + class ValueStoreFrontendTest : public testing::Test { public: ValueStoreFrontendTest() {} @@ -43,8 +46,9 @@ class ValueStoreFrontendTest : public testing::Test { // Reset the value store, reloading the DB from disk. void ResetStorage() { - storage_.reset(new ValueStoreFrontend( - factory_, ValueStoreFrontend::BackendType::RULES)); + storage_ = std::make_unique( + factory_, ValueStoreFrontend::BackendType::RULES, + GetExtensionFileTaskRunner()); } bool Get(const std::string& key, std::unique_ptr* output) { @@ -112,3 +116,5 @@ TEST_F(ValueStoreFrontendTest, ChangesPersistAfterReload) { ASSERT_FALSE(Get("key2", &value)); } + +} // namespace extensions