[local-search-service] Allow LSS client to GetIndex multiple times

This CL allows LSS clients to use GetIndex multiple times.
It also allows SearchMetricsReporter being shared between multiple
clients.

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

Bug: 1137560
Change-Id: I23b2433424d1ccae45ed8cbce1d490d5fa07759d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580667Reviewed-by: Jia Meng <jiameng@chromium.org>
Commit-Queue: Thanh Nguyen <thanhdng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835100}

chromeos/components/local_search_service/local_search_service.cc

@@ -36,20 +36,20 @@ void LocalSearchService::BindIndex(
                                         index_id, /*local_state*/ nullptr))
                  .first;
     }
+    if (!it->second) {
+      std::move(callback).Run("Returned Index is null.");
+      return;
+    }
+    if (reporter_remote)
+      it->second->SetReporterRemote(std::move(reporter_remote));
   }
 
   if (it == indices_.end()) {
     std::move(callback).Run("Error creating an Index.");
     return;
   }
-  if (!it->second) {
-    std::move(callback).Run("Returned Index is null.");
-    return;
-  }
 
   it->second->BindReceiver(std::move(index_receiver));
-  if (reporter_remote)
-    it->second->SetReporterRemote(std::move(reporter_remote));
   std::move(callback).Run(base::nullopt);
 }
 

chromeos/components/local_search_service/local_search_service_unittest.cc

@@ -136,7 +136,7 @@ TEST_F(LocalSearchServiceTest, BindMultipleTimes) {
   mojo::Remote<mojom::Index> first_index_remote;
   BindIndexAndCheck(IndexId::kCrosSettings, Backend::kInvertedIndex,
                     first_index_remote.BindNewPipeAndPassReceiver(),
-                    mojo::NullRemote());
+                    reporter_->BindNewPipeAndPassRemote());
   IndexGetSizeAndCheckResults(&first_index_remote, 0u);
   const std::map<std::string, std::vector<ContentWithId>> data_to_register = {
       {"id1",
@@ -151,14 +151,14 @@ TEST_F(LocalSearchServiceTest, BindMultipleTimes) {
   mojo::Remote<mojom::Index> second_index_remote;
   BindIndexAndCheck(IndexId::kCrosSettings, Backend::kInvertedIndex,
                     second_index_remote.BindNewPipeAndPassReceiver(),
-                    mojo::NullRemote());
+                    reporter_->BindNewPipeAndPassRemote());
   IndexGetSizeAndCheckResults(&second_index_remote, 2u);
 
   // Bind the third time with different id, should get a new index.
   mojo::Remote<mojom::Index> third_index_remote;
   BindIndexAndCheck(IndexId::kHelpApp, Backend::kInvertedIndex,
                     third_index_remote.BindNewPipeAndPassReceiver(),
-                    mojo::NullRemote());
+                    reporter_->BindNewPipeAndPassRemote());
   IndexGetSizeAndCheckResults(&third_index_remote, 0u);
 }
 

chromeos/components/local_search_service/search_metrics_reporter.cc

@@ -96,36 +96,37 @@ SearchMetricsReporter::~SearchMetricsReporter() = default;
 void SearchMetricsReporter::OnSearchPerformed(
     IndexId index_id,
     OnSearchPerformedCallback callback) {
-  if (!index_id_)
-    index_id_ = index_id;
-  const size_t index = static_cast<size_t>(*index_id_);
+  const size_t index = static_cast<size_t>(index_id);
   const char* daily_count_pref = kDailyCountPrefs[index];
   ++daily_counts_[index];
   pref_service_->SetInteger(daily_count_pref, daily_counts_[index]);
   std::move(callback).Run();
 }
 
-void SearchMetricsReporter::SetIndexIdForTesting(IndexId index_id) {
-  DCHECK(!index_id_);
-  index_id_ = index_id;
-  DCHECK_LT(static_cast<size_t>(index_id), kDailyCountPrefs.size());
-}
-
 void SearchMetricsReporter::ReportDailyMetricsForTesting(
     metrics::DailyEvent::IntervalType type) {
   ReportDailyMetrics(type);
 }
 
+mojo::PendingRemote<mojom::SearchMetricsReporter>
+SearchMetricsReporter::BindNewPipeAndPassRemote() {
+  receivers_.push_back(
+      std::make_unique<mojo::Receiver<mojom::SearchMetricsReporter>>(this));
+  return receivers_.back()->BindNewPipeAndPassRemote();
+}
+
 void SearchMetricsReporter::ReportDailyMetrics(
     metrics::DailyEvent::IntervalType type) {
-  if (!index_id_)
+  // Do nothing on the first run.
+  if (type == metrics::DailyEvent::IntervalType::FIRST_RUN)
     return;
 
-  // Don't send metrics on first run or if the clock is changed.
+  // Only send metrics for DAY_ELAPSED event.
   if (type == metrics::DailyEvent::IntervalType::DAY_ELAPSED) {
-    const size_t index = static_cast<size_t>(*index_id_);
-    base::UmaHistogramCounts1000(kDailyCountHistograms[index],
-                                 daily_counts_[index]);
+    for (size_t index = 0; index < kDailyCountPrefs.size(); ++index) {
+      base::UmaHistogramCounts1000(kDailyCountHistograms[index],
+                                   daily_counts_[index]);
+    }
   }
 
   for (size_t i = 0; i < kDailyCountPrefs.size(); ++i) {

chromeos/components/local_search_service/search_metrics_reporter.h

@@ -50,15 +50,10 @@ class SearchMetricsReporter : public mojom::SearchMetricsReporter {
   void OnSearchPerformed(IndexId index_id,
                          OnSearchPerformedCallback callback) override;
 
-  // Sets |index_id_|.
-  void SetIndexIdForTesting(IndexId index_id);
-
   // Calls ReportDailyMetrics directly.
   void ReportDailyMetricsForTesting(metrics::DailyEvent::IntervalType type);
 
-  mojo::PendingRemote<mojom::SearchMetricsReporter> BindNewPipeAndPassRemote() {
-    return receiver_.BindNewPipeAndPassRemote();
-  }
+  mojo::PendingRemote<mojom::SearchMetricsReporter> BindNewPipeAndPassRemote();
 
  private:
   class DailyEventObserver;
@@ -67,9 +62,6 @@ class SearchMetricsReporter : public mojom::SearchMetricsReporter {
   // |daily_event_|.
   void ReportDailyMetrics(metrics::DailyEvent::IntervalType type);
 
-  // Used as an index into |daily_counts_| for counting searches.
-  base::Optional<IndexId> index_id_;
-
   PrefService* pref_service_;  // Not owned.
 
   std::unique_ptr<metrics::DailyEvent> daily_event_;
@@ -81,7 +73,8 @@ class SearchMetricsReporter : public mojom::SearchMetricsReporter {
   // Initial values will be loaded from prefs service.
   std::array<int, kNumberIndexIds> daily_counts_;
 
-  mojo::Receiver<mojom::SearchMetricsReporter> receiver_{this};
+  std::vector<std::unique_ptr<mojo::Receiver<mojom::SearchMetricsReporter>>>
+      receivers_;
 };
 
 }  // namespace local_search_service

chromeos/components/local_search_service/search_metrics_reporter_unittest.cc

@@ -30,9 +30,8 @@ class SearchMetricsReporterTest : public testing::Test {
   void TearDown() override { reporter_.reset(); }
 
  protected:
-  void SetReporter(IndexId index_id) {
+  void SetReporter() {
     reporter_ = std::make_unique<SearchMetricsReporter>(&pref_service_);
-    reporter_->SetIndexIdForTesting(index_id);
   }
 
   // Notifies |reporter_| that a search is performed.
@@ -67,18 +66,33 @@ class SearchMetricsReporterTest : public testing::Test {
 };
 
 TEST_F(SearchMetricsReporterTest, CountAndReportEvents) {
-  SetReporter(IndexId::kCrosSettings);
+  SetReporter();
+  base::HistogramTester tester;
+
   SendOnSearchPerformedAndCheck(IndexId::kCrosSettings);
   SendOnSearchPerformedAndCheck(IndexId::kCrosSettings);
   SendOnSearchPerformedAndCheck(IndexId::kCrosSettings);
-  TriggerDailyEventAndVerifyHistograms(SearchMetricsReporter::kCrosSettingsName,
-                                       3);
+  TriggerDailyEvent(metrics::DailyEvent::IntervalType::DAY_ELAPSED);
+  tester.ExpectUniqueSample(SearchMetricsReporter::kHelpAppName, 0, 1);
+  tester.ExpectUniqueSample(SearchMetricsReporter::kCrosSettingsName, 3, 1);
 
   // The next day, another two searches.
   SendOnSearchPerformedAndCheck(IndexId::kCrosSettings);
   SendOnSearchPerformedAndCheck(IndexId::kCrosSettings);
-  TriggerDailyEventAndVerifyHistograms(SearchMetricsReporter::kCrosSettingsName,
-                                       2);
+  SendOnSearchPerformedAndCheck(IndexId::kHelpApp);
+  TriggerDailyEvent(metrics::DailyEvent::IntervalType::DAY_ELAPSED);
+  tester.ExpectBucketCount(SearchMetricsReporter::kHelpAppName, 1, 1);
+  tester.ExpectBucketCount(SearchMetricsReporter::kCrosSettingsName, 2, 1);
+
+  // Next day, CLOCK_CHANGED event happens, nothing more is logged.
+  SendOnSearchPerformedAndCheck(IndexId::kHelpApp);
+  SendOnSearchPerformedAndCheck(IndexId::kCrosSettings);
+  SendOnSearchPerformedAndCheck(IndexId::kHelpApp);
+  SendOnSearchPerformedAndCheck(IndexId::kCrosSettings);
+  SendOnSearchPerformedAndCheck(IndexId::kHelpApp);
+  TriggerDailyEvent(metrics::DailyEvent::IntervalType::CLOCK_CHANGED);
+  tester.ExpectTotalCount(SearchMetricsReporter::kHelpAppName, 2);
+  tester.ExpectTotalCount(SearchMetricsReporter::kCrosSettingsName, 2);
 }
 
 TEST_F(SearchMetricsReporterTest, LoadInitialCountsFromPrefs) {
@@ -86,8 +100,7 @@ TEST_F(SearchMetricsReporterTest, LoadInitialCountsFromPrefs) {
   // prefs.
   pref_service_.SetInteger(prefs::kLocalSearchServiceMetricsCrosSettingsCount,
                            2);
-  SetReporter(IndexId::kCrosSettings);
-
+  SetReporter();
   TriggerDailyEventAndVerifyHistograms(SearchMetricsReporter::kCrosSettingsName,
                                        2);
 
@@ -98,7 +111,7 @@ TEST_F(SearchMetricsReporterTest, LoadInitialCountsFromPrefs) {
 }
 
 TEST_F(SearchMetricsReporterTest, IgnoreDailyEventFirstRun) {
-  SetReporter(IndexId::kCrosSettings);
+  SetReporter();
   // metrics::DailyEvent notifies observers immediately on first run. Histograms
   // shouldn't be sent in this case.
   base::HistogramTester tester;
@@ -107,7 +120,7 @@ TEST_F(SearchMetricsReporterTest, IgnoreDailyEventFirstRun) {
 }
 
 TEST_F(SearchMetricsReporterTest, IgnoreDailyEventClockChanged) {
-  SetReporter(IndexId::kCrosSettings);
+  SetReporter();
   SendOnSearchPerformedAndCheck(IndexId::kCrosSettings);
 
   // metrics::DailyEvent notifies observers if it sees that the system clock has