[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/+/2601640Reviewed-by: Erik Staab <estaab@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841134}

.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

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 {

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"

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 {
 

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 {
 

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"

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",

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 {
 

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"
 

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 {

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;

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"

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<ValueStoreFactory>& store_factory,
                        ValueStoreFrontend::BackendType backend_type,
                        bool deferred_load)
-    : store_(new ValueStoreFrontend(store_factory, backend_type)),
-      task_queue_(new DelayedTaskQueue()) {
+    : store_(
+          std::make_unique<ValueStoreFrontend>(store_factory,
+                                               backend_type,
+                                               GetExtensionFileTaskRunner())),
+      task_queue_(std::make_unique<DelayedTaskQueue>()) {
   extension_registry_observer_.Add(ExtensionRegistry::Get(context));
 
   if (deferred_load) {

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",
   ]

extensions/browser/api/storage/settings_namespace.cc → 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"
 

extensions/browser/api/storage/settings_namespace.h → 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 <string>
 
@@ -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_

extensions/browser/value_store/value_store_factory.h

@@ -9,7 +9,7 @@
 #include <set>
 
 #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;

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<Backend> {
  public:
   Backend(const scoped_refptr<ValueStoreFactory>& store_factory,
-          BackendType backend_type)
-      : store_factory_(store_factory), backend_type_(backend_type) {}
+          BackendType backend_type,
+          const scoped_refptr<base::SequencedTaskRunner>& 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<Backend> {
   }
 
   void Set(const std::string& key, std::unique_ptr<base::Value> 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<Backend> {
   }
 
   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<Backend> {
   friend class base::RefCountedThreadSafe<Backend>;
 
   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<Backend> {
   scoped_refptr<ValueStoreFactory> store_factory_;
   BackendType backend_type_;
 
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
   // The actual ValueStore that handles persisting the data to disk. Used
   // exclusively on the backend sequence.
   std::unique_ptr<ValueStore> storage_;
@@ -111,8 +115,12 @@ class ValueStoreFrontend::Backend : public base::RefCountedThreadSafe<Backend> {
 
 ValueStoreFrontend::ValueStoreFrontend(
     const scoped_refptr<ValueStoreFactory>& store_factory,
-    BackendType backend_type)
-    : backend_(base::MakeRefCounted<Backend>(store_factory, backend_type)) {
+    BackendType backend_type,
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner)
+    : backend_(base::MakeRefCounted<Backend>(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<base::Value> 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

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<extensions::ValueStoreFactory>& store_factory,
-      BackendType backend_type);
+      BackendType backend_type,
+      const scoped_refptr<base::SequencedTaskRunner>& 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> backend_;
 
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
   DISALLOW_COPY_AND_ASSIGN(ValueStoreFrontend);
 };
 
+}  // namespace extensions
+
 #endif  // EXTENSIONS_BROWSER_VALUE_STORE_VALUE_STORE_FRONTEND_H_

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<ValueStoreFrontend>(
+        factory_, ValueStoreFrontend::BackendType::RULES,
+        GetExtensionFileTaskRunner());
   }
 
   bool Get(const std::string& key, std::unique_ptr<base::Value>* output) {
@@ -112,3 +116,5 @@ TEST_F(ValueStoreFrontendTest, ChangesPersistAfterReload) {
 
   ASSERT_FALSE(Get("key2", &value));
 }
+
+}  // namespace extensions