[local-search-service] Implement LocalSearchServiceProxy + Factory

This CL implements LocalSearchServiceProxy and
LocalSearchServiceProxyFactory. It also fixes typos and pref names.

Design doc: go/lss-sandboxing
Implementation plan: go/lss-sandboxing-impl

Bug: 1137560
Change-Id: I115a801a52fd0ed398485dd9ea9caf089fddd789
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2559477Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Jia Meng <jiameng@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Thanh Nguyen <thanhdng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832606}

chrome/browser/prefs/browser_prefs.cc

@@ -341,6 +341,7 @@
 #include "chrome/browser/upgrade_detector/upgrade_detector_chromeos.h"
 #include "chromeos/audio/audio_devices_pref_handler_impl.h"
 #include "chromeos/components/account_manager/account_manager.h"
+#include "chromeos/components/local_search_service/search_metrics_reporter.h"
 #include "chromeos/components/local_search_service/search_metrics_reporter_sync.h"
 #include "chromeos/components/quick_answers/public/cpp/quick_answers_prefs.h"
 #include "chromeos/constants/chromeos_switches.h"
@@ -691,6 +692,9 @@ void RegisterLocalState(PrefRegistrySimple* registry) {
   chromeos::KioskAppManager::RegisterPrefs(registry);
   chromeos::KioskCryptohomeRemover::RegisterPrefs(registry);
   chromeos::language_prefs::RegisterPrefs(registry);
+  chromeos::local_search_service::SearchMetricsReporter::
+      RegisterLocalStatePrefs(registry);
+  // TODO(crbug/1137560): Remove the Sync version later after LSS is sandboxed.
   chromeos::local_search_service::SearchMetricsReporterSync::
       RegisterLocalStatePrefs(registry);
   chromeos::login::SecurityTokenSessionController::RegisterLocalStatePrefs(

chromeos/components/local_search_service/BUILD.gn

@@ -63,7 +63,7 @@ source_set("local_search_service_provider") {
 
   deps = [
     "//chromeos/components/local_search_service:local_search_service",
-    "//chromeos/components/local_search_service/public/cpp:cpp",
+    "//chromeos/components/local_search_service/public/cpp:local_search_service_provider",
     "//content/public/browser:browser",
   ]
 }
@@ -115,6 +115,7 @@ source_set("unit_tests") {
     "inverted_index_unittest.cc",
     "linear_map_search_unittest.cc",
     "local_search_service_provider_unittest.cc",
+    "local_search_service_proxy_unittest.cc",
     "local_search_service_sync_proxy_unittest.cc",
     "local_search_service_sync_unittest.cc",
     "local_search_service_unittest.cc",
@@ -129,7 +130,10 @@ source_set("unit_tests") {
     ":local_search_service_proxy",
     ":test_support",
     "//base/test:test_support",
+    "//chromeos/components/local_search_service/public/cpp:cpp",
+    "//chromeos/components/local_search_service/public/cpp:local_search_service_provider",
     "//components/prefs:test_support",
+    "//content/test:test_support",
     "//mojo/public/cpp/bindings:bindings",
     "//testing/gtest",
   ]

chromeos/components/local_search_service/DEPS

@@ -4,4 +4,5 @@ include_rules = [
   "+components/metrics",
   "+components/prefs",
   "+content/public/browser",
+  "+content/public/test",
 ]

chromeos/components/local_search_service/local_search_service_provider_for_testing.cc

@@ -9,16 +9,15 @@
 namespace chromeos {
 namespace local_search_service {
 
-LocalSearchServiceProviderForTestting::LocalSearchServiceProviderForTestting() {
+LocalSearchServiceProviderForTesting::LocalSearchServiceProviderForTesting() {
   LocalSearchServiceProvider::Set(this);
 }
 
-LocalSearchServiceProviderForTestting::
-    ~LocalSearchServiceProviderForTestting() {
+LocalSearchServiceProviderForTesting::~LocalSearchServiceProviderForTesting() {
   LocalSearchServiceProvider::Set(nullptr);
 }
 
-void LocalSearchServiceProviderForTestting::BindLocalSearchService(
+void LocalSearchServiceProviderForTesting::BindLocalSearchService(
     mojo::PendingReceiver<mojom::LocalSearchService> receiver) {
   service_ = std::make_unique<LocalSearchService>(std::move(receiver));
 }

chromeos/components/local_search_service/local_search_service_provider_for_testing.h

@@ -13,11 +13,10 @@ namespace local_search_service {
 
 // An implementation that runs LocalSearchService in-process for testing
 // purpose.
-class LocalSearchServiceProviderForTestting
-    : public LocalSearchServiceProvider {
+class LocalSearchServiceProviderForTesting : public LocalSearchServiceProvider {
  public:
-  LocalSearchServiceProviderForTestting();
-  ~LocalSearchServiceProviderForTestting() override;
+  LocalSearchServiceProviderForTesting();
+  ~LocalSearchServiceProviderForTesting() override;
 
   // LocalSearchServiceProvider:
   void BindLocalSearchService(

chromeos/components/local_search_service/local_search_service_provider_unittest.cc

@@ -15,7 +15,7 @@ namespace local_search_service {
 class LocalSearchServiceProviderTest : public testing::Test {
  public:
   void SetUp() override {
-    provider_ = std::make_unique<LocalSearchServiceProviderForTestting>();
+    provider_ = std::make_unique<LocalSearchServiceProviderForTesting>();
   }
 
  protected:

chromeos/components/local_search_service/local_search_service_proxy_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/components/local_search_service/public/cpp/local_search_service_proxy.h"
+
+#include <memory>
+
+#include "base/test/task_environment.h"
+#include "chromeos/components/local_search_service/local_search_service_provider_for_testing.h"
+#include "components/prefs/testing_pref_service.h"
+#include "content/public/test/browser_task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace local_search_service {
+
+class LocalSearchServiceProxyTest : public testing::Test {
+ public:
+  void SetUp() override {
+    service_proxy_ = std::make_unique<LocalSearchServiceProxy>();
+
+    // Swap providers for testing purpose.
+    provider_ = std::make_unique<LocalSearchServiceProviderForTesting>();
+    provider_.swap(service_proxy_->local_search_service_provider_);
+  }
+
+ protected:
+  void CheckReporter(bool is_null_expected) {
+    bool is_null = false;
+    if (!service_proxy_->reporter_)
+      is_null = true;
+    ASSERT_EQ(is_null, is_null_expected);
+  }
+
+  void IndexGetSizeAndCheckResults(mojo::Remote<mojom::Index>* index_remote,
+                                   uint32_t expected_num_items) {
+    bool callback_done = false;
+    uint32_t num_items = 0;
+    (*index_remote)
+        ->GetSize(base::BindOnce(
+            [](bool* callback_done, uint32_t* num_items, uint64_t size) {
+              *callback_done = true;
+              *num_items = size;
+            },
+            &callback_done, &num_items));
+    task_environment_.RunUntilIdle();
+    ASSERT_TRUE(callback_done);
+    EXPECT_EQ(num_items, expected_num_items);
+  }
+
+  TestingPrefServiceSimple pref_service_;
+  content::BrowserTaskEnvironment task_environment_{
+      base::test::TaskEnvironment::MainThreadType::DEFAULT,
+      base::test::TaskEnvironment::ThreadPoolExecutionMode::QUEUED};
+
+  std::unique_ptr<LocalSearchServiceProxy> service_proxy_;
+  std::unique_ptr<LocalSearchServiceProvider> provider_;
+};
+
+TEST_F(LocalSearchServiceProxyTest, TestWithLocalState) {
+  mojo::Remote<mojom::Index> index_remote;
+  SearchMetricsReporter::RegisterLocalStatePrefs(pref_service_.registry());
+  service_proxy_->SetLocalState(&pref_service_);
+  service_proxy_->GetIndex(IndexId::kCrosSettings, Backend::kLinearMap,
+                           index_remote.BindNewPipeAndPassReceiver());
+  task_environment_.RunUntilIdle();
+
+  // Check that index_remote is bound.
+  IndexGetSizeAndCheckResults(&index_remote, 0u);
+
+  // Check reporter
+  CheckReporter(/*is_null_expected*/ false);
+}
+
+TEST_F(LocalSearchServiceProxyTest, TestWithoutLocalState) {
+  mojo::Remote<mojom::Index> index_remote;
+  service_proxy_->GetIndex(IndexId::kCrosSettings, Backend::kLinearMap,
+                           index_remote.BindNewPipeAndPassReceiver());
+  task_environment_.RunUntilIdle();
+
+  // Check that index_remote is bound.
+  IndexGetSizeAndCheckResults(&index_remote, 0u);
+
+  // Check reporter
+  CheckReporter(/*is_null_expected*/ true);
+}
+
+}  // namespace local_search_service
+}  // namespace chromeos

chromeos/components/local_search_service/pref_names.cc

@@ -8,6 +8,14 @@ namespace chromeos {
 namespace local_search_service {
 namespace prefs {
 
+// TODO(thanhdng): clean this up after LSS is sandboxed.
+const char kLocalSearchServiceSyncMetricsDailySample[] =
+    "local_search_service_sync.metrics.daily_sample";
+const char kLocalSearchServiceSyncMetricsCrosSettingsCount[] =
+    "local_search_service_sync.metrics.cros_settings_count";
+const char kLocalSearchServiceSyncMetricsHelpAppCount[] =
+    "local_search_service_sync.metrics.help_app_count";
+
 const char kLocalSearchServiceMetricsDailySample[] =
     "local_search_service.metrics.daily_sample";
 const char kLocalSearchServiceMetricsCrosSettingsCount[] =

chromeos/components/local_search_service/pref_names.h

@@ -10,13 +10,18 @@ namespace local_search_service {
 namespace prefs {
 
 // Integer pref used by the metrics::DailyEvent owned by
-// local_search_service::MetricsReporter.
+// local_search_service::SearchMetricsReporter.
 extern const char kLocalSearchServiceMetricsDailySample[];
+// TODO(thanhdng): clean this up after LSS is sandboxed.
+extern const char kLocalSearchServiceSyncMetricsDailySample[];
 
 // Integer prefs used to back event counts reported by
-// local_search_service::MetricsReporter.
+// local_search_service::SearchMetricsReporter.
 extern const char kLocalSearchServiceMetricsCrosSettingsCount[];
 extern const char kLocalSearchServiceMetricsHelpAppCount[];
+// TODO(thanhdng): clean this up after LSS is sandboxed.
+extern const char kLocalSearchServiceSyncMetricsCrosSettingsCount[];
+extern const char kLocalSearchServiceSyncMetricsHelpAppCount[];
 
 }  // namespace prefs
 }  // namespace local_search_service

chromeos/components/local_search_service/public/cpp/BUILD.gn

@@ -4,7 +4,7 @@
 
 assert(is_chromeos, "Non-ChromeOS builds cannot depend on //chromeos")
 
-source_set("cpp") {
+source_set("local_search_service_provider") {
   sources = [
     "local_search_service_provider.cc",
     "local_search_service_provider.h",
@@ -15,3 +15,20 @@ source_set("cpp") {
     "//chromeos/components/local_search_service/public/mojom",
   ]
 }
+
+source_set("cpp") {
+  sources = [
+    "local_search_service_proxy.cc",
+    "local_search_service_proxy.h",
+    "local_search_service_proxy_factory.cc",
+    "local_search_service_proxy_factory.h",
+  ]
+
+  deps = [
+    ":local_search_service_provider",
+    "//chromeos/components/local_search_service:local_search_service",
+    "//chromeos/components/local_search_service:local_search_service_provider",
+    "//chromeos/components/local_search_service/public/mojom",
+    "//components/prefs:prefs",
+  ]
+}

chromeos/components/local_search_service/public/cpp/local_search_service_proxy.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/components/local_search_service/public/cpp/local_search_service_proxy.h"
+
+#include <memory>
+
+#include "chromeos/components/local_search_service/oop_local_search_service_provider.h"
+#include "components/prefs/pref_service.h"
+
+namespace chromeos {
+namespace local_search_service {
+
+namespace {
+
+void OnBindIndexDone(const base::Optional<std::string>& error) {
+  // TODO(thanhdng): add a histogram to log this.
+  if (error)
+    LOG(ERROR) << "BindIndex error: " << error.value();
+}
+
+}  // namespace
+
+LocalSearchServiceProxy::LocalSearchServiceProxy() {
+  // Create an instance of OopLocalSearchServiceProvider.
+  // This will set |g_provider|.
+  local_search_service_provider_ =
+      std::make_unique<OopLocalSearchServiceProvider>();
+}
+
+LocalSearchServiceProxy::~LocalSearchServiceProxy() = default;
+
+void LocalSearchServiceProxy::SetLocalState(PrefService* local_state) {
+  DCHECK(local_state);
+  if (!reporter_) {
+    reporter_ = std::make_unique<SearchMetricsReporter>(local_state);
+  }
+}
+
+void LocalSearchServiceProxy::GetIndex(
+    IndexId index_id,
+    Backend backend,
+    mojo::PendingReceiver<mojom::Index> index_receiver) {
+  auto* service = GetService();
+  if (!service) {
+    // In this case, client's mojom receiver will not be bound. Hence the
+    // the client should always check it before using it.
+    return;
+  }
+
+  service->BindIndex(
+      index_id, backend, std::move(index_receiver),
+      reporter_ ? reporter_->BindNewPipeAndPassRemote() : mojo::NullRemote(),
+      base::BindOnce(&OnBindIndexDone));
+}
+
+mojom::LocalSearchService* LocalSearchServiceProxy::GetService() {
+  if (!service_) {
+    auto* provider = LocalSearchServiceProvider::Get();
+    if (provider) {
+      provider->BindLocalSearchService(service_.BindNewPipeAndPassReceiver());
+    } else {
+      LOG(FATAL) << "LocalSearchServiceProvider::Set() must be called "
+                 << "before any instances of LocalSearchService can be used.";
+      return nullptr;
+    }
+    service_.reset_on_disconnect();
+  }
+
+  return service_.get();
+}
+
+}  // namespace local_search_service
+}  // namespace chromeos

chromeos/components/local_search_service/public/cpp/local_search_service_proxy.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_COMPONENTS_LOCAL_SEARCH_SERVICE_PUBLIC_CPP_LOCAL_SEARCH_SERVICE_PROXY_H_
+#define CHROMEOS_COMPONENTS_LOCAL_SEARCH_SERVICE_PUBLIC_CPP_LOCAL_SEARCH_SERVICE_PROXY_H_
+
+#include <memory>
+
+#include "chromeos/components/local_search_service/public/cpp/local_search_service_provider.h"
+#include "chromeos/components/local_search_service/public/mojom/index.mojom.h"
+#include "chromeos/components/local_search_service/public/mojom/local_search_service.mojom.h"
+#include "chromeos/components/local_search_service/search_metrics_reporter.h"
+#include "components/keyed_service/core/keyed_service.h"
+
+class PrefService;
+
+namespace chromeos {
+namespace local_search_service {
+
+class LocalSearchServiceProxy : public KeyedService {
+ public:
+  LocalSearchServiceProxy();
+  ~LocalSearchServiceProxy() override;
+
+  LocalSearchServiceProxy(const LocalSearchServiceProxy&) = delete;
+  LocalSearchServiceProxy& operator=(const LocalSearchServiceProxy&) = delete;
+
+  // This function will be called by LocalSearchServiceClient or PreProfileInit.
+  // Note: |local_state| will be shared by all LSS clients and it's used to
+  // create daily metrics reporter.
+  void SetLocalState(PrefService* local_state);
+
+  // A client will call this function to request its index and have the
+  // remote bound to it.
+  // Client should always check if the receiver end is bound before using the
+  // index.
+  void GetIndex(IndexId index_id,
+                Backend backend,
+                mojo::PendingReceiver<mojom::Index> index_receiver);
+
+ private:
+  friend class LocalSearchServiceProxyTest;
+
+  // GetService will lazily create a LocalSearchService.
+  mojom::LocalSearchService* GetService();
+
+  std::unique_ptr<LocalSearchServiceProvider> local_search_service_provider_;
+  mojo::Remote<mojom::LocalSearchService> service_;
+  std::unique_ptr<SearchMetricsReporter> reporter_;
+};
+
+}  // namespace local_search_service
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_LOCAL_SEARCH_SERVICE_PUBLIC_CPP_LOCAL_SEARCH_SERVICE_PROXY_H_

chromeos/components/local_search_service/public/cpp/local_search_service_proxy_factory.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/components/local_search_service/public/cpp/local_search_service_proxy_factory.h"
+
+#include "chromeos/components/local_search_service/public/cpp/local_search_service_proxy.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+
+namespace chromeos {
+namespace local_search_service {
+
+// static
+LocalSearchServiceProxy* LocalSearchServiceProxyFactory::GetForBrowserContext(
+    content::BrowserContext* context) {
+  return static_cast<LocalSearchServiceProxy*>(
+      LocalSearchServiceProxyFactory::GetInstance()
+          ->GetServiceForBrowserContext(context, /*create=*/true));
+}
+
+// static
+LocalSearchServiceProxyFactory* LocalSearchServiceProxyFactory::GetInstance() {
+  static base::NoDestructor<LocalSearchServiceProxyFactory> instance;
+  return instance.get();
+}
+
+LocalSearchServiceProxyFactory::LocalSearchServiceProxyFactory()
+    : BrowserContextKeyedServiceFactory(
+          "LocalSearchServiceProxy",
+          BrowserContextDependencyManager::GetInstance()) {}
+
+LocalSearchServiceProxyFactory::~LocalSearchServiceProxyFactory() = default;
+
+content::BrowserContext* LocalSearchServiceProxyFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  // The service should exist in incognito mode.
+  return context;
+}
+
+KeyedService* LocalSearchServiceProxyFactory::BuildServiceInstanceFor(
+    content::BrowserContext* /*context*/) const {
+  return new LocalSearchServiceProxy();
+}
+
+}  // namespace local_search_service
+}  // namespace chromeos

chromeos/components/local_search_service/public/cpp/local_search_service_proxy_factory.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_COMPONENTS_LOCAL_SEARCH_SERVICE_PUBLIC_CPP_LOCAL_SEARCH_SERVICE_PROXY_FACTORY_H_
+#define CHROMEOS_COMPONENTS_LOCAL_SEARCH_SERVICE_PUBLIC_CPP_LOCAL_SEARCH_SERVICE_PROXY_FACTORY_H_
+
+#include "base/no_destructor.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+
+namespace chromeos {
+namespace local_search_service {
+class LocalSearchServiceProxy;
+
+class LocalSearchServiceProxyFactory
+    : public BrowserContextKeyedServiceFactory {
+ public:
+  static LocalSearchServiceProxy* GetForBrowserContext(
+      content::BrowserContext* context);
+  static LocalSearchServiceProxyFactory* GetInstance();
+
+  LocalSearchServiceProxyFactory(const LocalSearchServiceProxyFactory&) =
+      delete;
+  LocalSearchServiceProxyFactory& operator=(
+      const LocalSearchServiceProxyFactory&) = delete;
+
+ private:
+  friend class base::NoDestructor<LocalSearchServiceProxyFactory>;
+
+  LocalSearchServiceProxyFactory();
+  ~LocalSearchServiceProxyFactory() override;
+
+  // BrowserContextKeyedServiceFactory:
+  content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const override;
+  KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* context) const override;
+};
+
+}  // namespace local_search_service
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_LOCAL_SEARCH_SERVICE_PUBLIC_CPP_LOCAL_SEARCH_SERVICE_PROXY_FACTORY_H_

chromeos/components/local_search_service/search_metrics_reporter_sync.cc

@@ -22,8 +22,8 @@ constexpr base::TimeDelta kCheckDailyEventInternal =
 // Prefs corresponding to IndexId values.
 constexpr std::array<const char*, SearchMetricsReporterSync::kNumberIndexIds>
     kDailyCountPrefs = {
-        prefs::kLocalSearchServiceMetricsCrosSettingsCount,
-        prefs::kLocalSearchServiceMetricsHelpAppCount,
+        prefs::kLocalSearchServiceSyncMetricsCrosSettingsCount,
+        prefs::kLocalSearchServiceSyncMetricsHelpAppCount,
 };
 
 // Histograms corresponding to IndexId values.
@@ -68,7 +68,7 @@ class SearchMetricsReporterSync::DailyEventObserver
 void SearchMetricsReporterSync::RegisterLocalStatePrefs(
     PrefRegistrySimple* registry) {
   metrics::DailyEvent::RegisterPref(
-      registry, prefs::kLocalSearchServiceMetricsDailySample);
+      registry, prefs::kLocalSearchServiceSyncMetricsDailySample);
   for (const char* daily_count_pref : kDailyCountPrefs) {
     registry->RegisterIntegerPref(daily_count_pref, 0);
   }
@@ -79,7 +79,7 @@ SearchMetricsReporterSync::SearchMetricsReporterSync(
     : pref_service_(local_state_pref_service),
       daily_event_(std::make_unique<metrics::DailyEvent>(
           pref_service_,
-          prefs::kLocalSearchServiceMetricsDailySample,
+          prefs::kLocalSearchServiceSyncMetricsDailySample,
           kDailyEventIntervalName)) {
   for (size_t i = 0; i < kDailyCountPrefs.size(); ++i) {
     daily_counts_[i] = pref_service_->GetInteger(kDailyCountPrefs[i]);

chromeos/components/local_search_service/search_metrics_reporter_sync_unittest.cc

@@ -77,8 +77,8 @@ TEST_F(SearchMetricsReporterSyncTest, CountAndReportEvents) {
 TEST_F(SearchMetricsReporterSyncTest, LoadInitialCountsFromPrefs) {
   // Create a new reporter and check that it loads its initial event counts from
   // prefs.
-  pref_service_.SetInteger(prefs::kLocalSearchServiceMetricsCrosSettingsCount,
-                           2);
+  pref_service_.SetInteger(
+      prefs::kLocalSearchServiceSyncMetricsCrosSettingsCount, 2);
   SetReporter(IndexId::kCrosSettings);
 
   TriggerDailyEventAndVerifyHistograms(