[Cros SR] Reflect user history deletion actions in the model.

We have an on-device model ranking commonly used URLs in the CrOS
launcher, which stores these URLs in the model itself. This CL
observes the history service and deletes URLs from the model that
are removed by the user.

Bug: 931149
Change-Id: If9e9654a00ae804807319a3dc5dc91b599698c7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1725032
Commit-Queue: Tony Yeoman <tby@chromium.org>
Reviewed-by: Jia Meng <jiameng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683049}

chrome/browser/ui/app_list/search/search_controller.cc

@@ -17,6 +17,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
 #include "chrome/browser/ui/app_list/app_list_model_updater.h"
@@ -72,7 +73,10 @@ SearchController::SearchController(AppListModelUpdater* model_updater,
     : mixer_(std::make_unique<Mixer>(model_updater)),
       list_controller_(list_controller) {
   std::unique_ptr<SearchResultRanker> ranker =
-      std::make_unique<SearchResultRanker>(profile,
+      std::make_unique<SearchResultRanker>(
+          profile,
+          HistoryServiceFactory::GetForProfile(
+              profile, ServiceAccessType::EXPLICIT_ACCESS),
           content::GetSystemConnector());
   ranker->InitializeRankers();
   mixer_->SetNonAppSearchResultRanker(std::move(ranker));

chrome/browser/ui/app_list/search/search_result_ranker/search_result_ranker.cc

@@ -16,10 +16,13 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task/post_task.h"
 #include "chrome/browser/chromeos/file_manager/file_tasks_notifier.h"
 #include "chrome/browser/chromeos/file_manager/file_tasks_notifier_factory.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/app_list/search/chrome_search_result.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/app_search_result_ranker.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/ranking_item_util.h"
@@ -126,9 +129,15 @@ float ReRange(const float score, const float min, const float max) {
 }  // namespace
 
 SearchResultRanker::SearchResultRanker(Profile* profile,
+                                       history::HistoryService* history_service,
                                        service_manager::Connector* connector)
-    : config_converter_(connector), profile_(profile) {
+    : config_converter_(connector),
+      history_service_observer_(this),
+      profile_(profile),
+      weak_factory_(this) {
   DCHECK(profile);
+  DCHECK(history_service);
+  history_service_observer_.Add(history_service);
   if (auto* notifier =
           file_manager::file_tasks::FileTasksNotifier::GetForProfile(
               profile_)) {
@@ -328,4 +337,43 @@ void SearchResultRanker::OnFilesOpened(
                               GetTypeFromFileTaskNotifier(file_open.open_type));
 }
 
+void SearchResultRanker::OnURLsDeleted(
+    history::HistoryService* history_service,
+    const history::DeletionInfo& deletion_info) {
+  if (!query_based_mixed_types_ranker_)
+    return;
+
+  if (deletion_info.IsAllHistory()) {
+    // TODO(931149): We clear the whole model because we expect most targets to
+    // be URLs. In future, consider parsing the targets and only deleting URLs.
+    query_based_mixed_types_ranker_->GetTargetData()->clear();
+  } else {
+    for (const auto& row : deletion_info.deleted_rows()) {
+      // In order to perform URL normalization, NormalizeId requires any omnibox
+      // item type as argument. Pass kOmniboxGeneric here as we don't know the
+      // specific type.
+      query_based_mixed_types_ranker_->RemoveTarget(
+          NormalizeId(row.url().spec(), RankingItemType::kOmniboxGeneric));
+    }
+  }
+
+  // Force a save to disk. It is possible to get many calls to OnURLsDeleted in
+  // quick succession, eg. when all history is cleared from a different device.
+  // So delay the save slightly and only perform one save for all updates during
+  // that delay.
+  if (!query_mixed_ranker_save_queued_) {
+    query_mixed_ranker_save_queued_ = true;
+    base::PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&SearchResultRanker::SaveQueryMixedRankerAfterDelete,
+                       weak_factory_.GetWeakPtr()),
+        TimeDelta::FromSeconds(3));
+  }
+}
+
+void SearchResultRanker::SaveQueryMixedRankerAfterDelete() {
+  query_based_mixed_types_ranker_->SaveToDisk();
+  query_mixed_ranker_save_queued_ = false;
+}
+
 }  // namespace app_list

chrome/browser/ui/app_list/search/search_result_ranker/search_result_ranker.h

@@ -13,6 +13,7 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/scoped_observer.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/file_manager/file_tasks_notifier.h"
@@ -23,6 +24,8 @@
 #include "chrome/browser/ui/app_list/search/search_result_ranker/app_launch_event_logger.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/app_search_result_ranker.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/recurrence_ranker_util.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/history_service_observer.h"
 
 namespace app_list {
 
@@ -35,9 +38,12 @@ enum class RankingItemType;
 // FetchRankings queries each model for ranking results. Rank modifies the
 // scores of provided search results, which are intended to be the output of a
 // search provider.
-class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
+class SearchResultRanker : file_manager::file_tasks::FileTasksObserver,
+                           history::HistoryServiceObserver {
  public:
-  SearchResultRanker(Profile* profile, service_manager::Connector* connector);
+  SearchResultRanker(Profile* profile,
+                     history::HistoryService* history_service,
+                     service_manager::Connector* connector);
   ~SearchResultRanker() override;
 
   // Performs all setup of rankers. This is separated from the constructor for
@@ -64,6 +70,10 @@ class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
   // file_manager::file_tasks::FileTaskObserver:
   void OnFilesOpened(const std::vector<FileOpenEvent>& file_opens) override;
 
+  // history::HistoryServiceObserver:
+  void OnURLsDeleted(history::HistoryService* history_service,
+                     const history::DeletionInfo& deletion_info) override;
+
   RecurrenceRanker* get_zero_state_mixed_types_ranker() {
     return zero_state_mixed_types_ranker_.get();
   }
@@ -77,6 +87,12 @@ class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
  private:
   FRIEND_TEST_ALL_PREFIXES(SearchResultRankerTest,
                            QueryMixedModelConfigDeployment);
+  FRIEND_TEST_ALL_PREFIXES(SearchResultRankerTest,
+                           QueryMixedModelDeletesURLCorrectly);
+
+  // Saves |query_based_mixed_types_ranker_| to disk. Called after a delay when
+  // URLs get deleted.
+  void SaveQueryMixedRankerAfterDelete();
 
   // Records the time of the last call to FetchRankings() and is used to
   // limit the number of queries to the models within a short timespan.
@@ -100,6 +116,9 @@ class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
   // these are local files and omnibox results.
   std::unique_ptr<RecurrenceRanker> query_based_mixed_types_ranker_;
   std::map<std::string, float> query_mixed_ranks_;
+  // Flag set when a delayed task to save the model is created. This is used to
+  // prevent several delayed tasks from being created.
+  bool query_mixed_ranker_save_queued_ = false;
 
   // Ranks files and previous queries for launcher zero-state.
   std::unique_ptr<RecurrenceRanker> zero_state_mixed_types_ranker_;
@@ -113,6 +132,9 @@ class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
   // Logs launch events and stores feature data for aggregated model.
   app_list::AppLaunchEventLogger app_launch_event_logger_;
 
+  ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
+      history_service_observer_;
+
   // TODO(931149): Move the AppSearchResultRanker instance and associated logic
   // to here.
 
@@ -121,6 +143,8 @@ class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
   base::flat_map<std::string, float> app_ranks_;
 
   Profile* profile_;
+
+  base::WeakPtrFactory<SearchResultRanker> weak_factory_;
 };
 
 }  // namespace app_list

chrome/browser/ui/app_list/search/search_result_ranker/search_result_ranker_unittest.cc

@@ -14,6 +14,7 @@
 
 #include "ash/public/cpp/app_list/app_list_features.h"
 #include "ash/public/cpp/app_list/app_list_types.h"
+#include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
@@ -26,6 +27,11 @@
 #include "chrome/browser/ui/app_list/search/search_result_ranker/ranking_item_util.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/recurrence_ranker.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/history/core/browser/history_database_params.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/history_types.h"
+#include "components/history/core/test/history_service_test_util.h"
+#include "components/history/core/test/test_history_database.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "services/data_decoder/public/cpp/test_data_decoder_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -39,6 +45,7 @@ using ResultType = ash::SearchResultType;
 using base::ScopedTempDir;
 using base::test::ScopedFeatureList;
 using testing::ElementsAre;
+using testing::UnorderedElementsAre;
 using testing::WhenSorted;
 
 class TestSearchResult : public ChromeSearchResult {
@@ -71,6 +78,10 @@ MATCHER_P(HasId, id, "") {
   return base::UTF16ToUTF8(arg.result->title()) == id;
 }
 
+MATCHER_P2(HasIdScore, id, score, "") {
+  return base::UTF16ToUTF8(arg.result->title()) == id && arg.score == score;
+}
+
 }  // namespace
 
 class SearchResultRankerTest : public testing::Test {
@@ -87,6 +98,11 @@ class SearchResultRankerTest : public testing::Test {
     profile_builder.SetProfileName("testuser@gmail.com");
     profile_builder.SetPath(temp_dir_.GetPath().AppendASCII("TestProfile"));
     profile_ = profile_builder.Build();
+
+    history_service_ = std::make_unique<history::HistoryService>();
+    history_service_->Init(
+        history::TestHistoryDatabaseParamsForPath(temp_dir_.GetPath()));
+    Wait();
   }
 
   std::unique_ptr<SearchResultRanker> MakeRanker(
@@ -100,8 +116,8 @@ class SearchResultRankerTest : public testing::Test {
           app_list_features::kEnableQueryBasedMixedTypesRanker);
     }
 
-    auto ranker = std::make_unique<SearchResultRanker>(profile_.get(),
-                                                       dd_service_.connector());
+    auto ranker = std::make_unique<SearchResultRanker>(
+        profile_.get(), history_service_.get(), dd_service_.connector());
     return ranker;
   }
 
@@ -116,9 +132,10 @@ class SearchResultRankerTest : public testing::Test {
     return results;
   }
 
+  history::HistoryService* history_service() { return history_service_.get(); }
+
   void Wait() { thread_bundle_.RunUntilIdle(); }
 
- private:
   content::TestBrowserThreadBundle thread_bundle_;
 
   // This is used only to make the ownership clear for the TestSearchResult
@@ -130,9 +147,11 @@ class SearchResultRankerTest : public testing::Test {
 
   ScopedFeatureList scoped_feature_list_;
   ScopedTempDir temp_dir_;
+  std::unique_ptr<history::HistoryService> history_service_;
 
   std::unique_ptr<Profile> profile_;
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(SearchResultRankerTest);
 };
 
@@ -336,4 +355,108 @@ TEST_F(SearchResultRankerTest, QueryMixedModelConfigDeployment) {
             "ExponentialWeightsEnsemble");
 }
 
+// Tests that, when a URL is deleted from the history service, the query-based
+// mixed-types model deletes it in memory and from disk.
+TEST_F(SearchResultRankerTest, QueryMixedModelDeletesURLCorrectly) {
+  // Create ranker.
+  const std::string json = R"({
+      "min_seconds_between_saves": 1000,
+      "target_limit": 100,
+      "target_decay": 0.5,
+      "condition_limit": 10,
+      "condition_decay": 0.5,
+      "predictor": {
+        "predictor_type": "fake"
+      }
+    })";
+
+  base::RunLoop run_loop;
+  auto ranker =
+      MakeRanker(true, {{"boost_coefficient", "1.0"}, {"config", json}});
+  ranker->set_json_config_parsed_for_testing(run_loop.QuitClosure());
+  ranker->InitializeRankers();
+  run_loop.Run();
+  Wait();
+
+  const base::FilePath model_path =
+      profile_->GetPath().AppendASCII("query_based_mixed_types_ranker.pb");
+
+  // Train the model on two URLs.
+  const std::string url_1 = "http://www.google.com/testing";
+  AppLaunchData url_1_data;
+  url_1_data.id = url_1;
+  url_1_data.ranking_item_type = RankingItemType::kOmniboxHistory;
+  url_1_data.query = "query";
+  ranker->Train(url_1_data);
+  ranker->Train(url_1_data);
+
+  const std::string url_2 = "http://www.other.com";
+  AppLaunchData url_2_data;
+  url_2_data.id = url_2;
+  url_2_data.ranking_item_type = RankingItemType::kOmniboxHistory;
+  url_2_data.query = "query";
+  ranker->Train(url_2_data);
+
+  // Expect the scores of the urls to reflect their training.
+  {
+    ranker->FetchRankings(base::UTF8ToUTF16("query"));
+    auto results = MakeSearchResults(
+        {url_1, url_2, "untrained"},
+        {ResultType::kOmnibox, ResultType::kOmnibox, ResultType::kOmnibox},
+        {0.0f, 0.0f, 0.5f});
+    ranker->Rank(&results);
+    EXPECT_THAT(results, UnorderedElementsAre(HasIdScore(url_1, 2.0f),
+                                              HasIdScore(url_2, 1.0f),
+                                              HasIdScore("untrained", 0.5f)));
+  }
+
+  // Now delete |url_1| from the history service and ensure we save the model to
+  // disk.
+  EXPECT_FALSE(base::PathExists(model_path));
+  history_service()->AddPage(GURL(url_1), base::Time::Now(),
+                             history::VisitSource::SOURCE_BROWSED);
+  history_service()->DeleteURL(GURL(url_1));
+  history::BlockUntilHistoryProcessesPendingRequests(history_service());
+  Wait();
+  EXPECT_TRUE(base::PathExists(model_path));
+
+  // Force cache expiry.
+  ranker->time_of_last_fetch_ = base::Time();
+
+  // Expect the score of |url_1| to be 0.0, it should have been deleted from
+  // the model.
+  {
+    ranker->FetchRankings(base::UTF8ToUTF16("query"));
+    auto results = MakeSearchResults(
+        {url_1, url_2, "untrained"},
+        {ResultType::kOmnibox, ResultType::kOmnibox, ResultType::kOmnibox},
+        {0.0f, 0.0f, 0.5f});
+    ranker->Rank(&results);
+    EXPECT_THAT(results, UnorderedElementsAre(HasIdScore(url_1, 0.0f),
+                                              HasIdScore(url_2, 1.0f),
+                                              HasIdScore("untrained", 0.5f)));
+  }
+
+  // Load a new ranker from disk and ensure |url_1| hasn't been retained.
+  base::RunLoop new_run_loop;
+  auto new_ranker = std::make_unique<SearchResultRanker>(
+      profile_.get(), history_service(), dd_service_.connector());
+  new_ranker->set_json_config_parsed_for_testing(new_run_loop.QuitClosure());
+  new_ranker->InitializeRankers();
+  new_run_loop.Run();
+  Wait();
+
+  {
+    new_ranker->FetchRankings(base::UTF8ToUTF16("query"));
+    auto results = MakeSearchResults(
+        {url_1, url_2, "untrained"},
+        {ResultType::kOmnibox, ResultType::kOmnibox, ResultType::kOmnibox},
+        {0.0f, 0.0f, 0.5f});
+    new_ranker->Rank(&results);
+    EXPECT_THAT(results, UnorderedElementsAre(HasIdScore(url_1, 0.0f),
+                                              HasIdScore(url_2, 1.0f),
+                                              HasIdScore("untrained", 0.5f)));
+  }
+}
+
 }  // namespace app_list