Enable model fetching for non-MSBB

This ensures we don't send hosts and field trials for non-MSBB users

Bug: 1146151
Change-Id: I146977f62943aaa77a0b85cd7196a13ad5ca660a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2579838Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835259}

chrome/browser/optimization_guide/prediction/prediction_manager.cc

@@ -321,6 +321,10 @@ void PredictionManager::RegisterOptimizationTargets(
   if (new_optimization_targets.size() == 0)
     return;
 
+  // If no fetch is scheduled, maybe schedule one.
+  if (!fetch_timer_.IsRunning())
+    MaybeScheduleModelAndHostModelFeaturesFetch();
+
   // Start loading the host model features if they are not already.
   if (!host_model_features_loaded_) {
     LoadHostModelFeatures();
@@ -605,7 +609,8 @@ void PredictionManager::SetPredictionModelDownloadManagerForTesting(
 
 void PredictionManager::FetchModelsAndHostModelFeatures() {
   SEQUENCE_CHECKER(sequence_checker_);
-  if (!IsUserPermittedToFetchFromRemoteOptimizationGuide(profile_))
+
+  if (!features::IsRemoteFetchingEnabled())
     return;
 
   ScheduleModelsAndHostModelFeaturesFetch();
@@ -633,24 +638,31 @@ void PredictionManager::FetchModelsAndHostModelFeatures() {
     prediction_model_download_manager_->CancelAllPendingDownloads();
 
   std::vector<std::string> top_hosts;
-  // If the top host provider is not available, the user has likely not seen the
-  // Lite mode infobar, so top hosts cannot be provided. However, prediction
-  // models are allowed to be fetched.
-  if (top_host_provider_) {
-    top_hosts = top_host_provider_->GetTopHosts();
-
-    // Remove hosts that are already available in the host model features cache.
-    // The request should still be made in case there is a new model or a model
-    // that does not rely on host model features to be fetched.
-    auto it = top_hosts.begin();
-    while (it != top_hosts.end()) {
-      if (host_model_features_cache_.Peek(*it) !=
-          host_model_features_cache_.end()) {
-        it = top_hosts.erase(it);
-        continue;
+  std::vector<proto::FieldTrial> active_field_trials;
+  // Top hosts and active field trials convey some sort of user information, so
+  // ensure that the user has opted into the right permissions before adding
+  // these fields to the request.
+  if (IsUserPermittedToFetchFromRemoteOptimizationGuide(profile_)) {
+    if (top_host_provider_) {
+      top_hosts = top_host_provider_->GetTopHosts();
+
+      // Remove hosts that are already available in the host model features
+      // cache. The request should still be made in case there is a new model or
+      // a model that does not rely on host model features to be fetched.
+      auto it = top_hosts.begin();
+      while (it != top_hosts.end()) {
+        if (host_model_features_cache_.Peek(*it) !=
+            host_model_features_cache_.end()) {
+          it = top_hosts.erase(it);
+          continue;
+        }
+        ++it;
       }
-      ++it;
     }
+    google::protobuf::RepeatedPtrField<proto::FieldTrial> current_field_trials =
+        GetActiveFieldTrialsAllowedForFetch();
+    active_field_trials = std::vector<proto::FieldTrial>(
+        {current_field_trials.begin(), current_field_trials.end()});
   }
 
   if (!prediction_model_fetcher_) {
@@ -688,7 +700,8 @@ void PredictionManager::FetchModelsAndHostModelFeatures() {
   }
 
   prediction_model_fetcher_->FetchOptimizationGuideServiceModels(
-      models_info, top_hosts, optimization_guide::proto::CONTEXT_BATCH_UPDATE,
+      models_info, top_hosts, active_field_trials,
+      optimization_guide::proto::CONTEXT_BATCH_UPDATE,
       base::BindOnce(&PredictionManager::OnModelsAndHostFeaturesFetched,
                      ui_weak_ptr_factory_.GetWeakPtr()));
 }
@@ -1097,7 +1110,7 @@ bool PredictionManager::ProcessAndStoreHostModelFeatures(
 }
 
 void PredictionManager::MaybeScheduleModelAndHostModelFeaturesFetch() {
-  if (!IsUserPermittedToFetchFromRemoteOptimizationGuide(profile_))
+  if (!features::IsRemoteFetchingEnabled())
     return;
 
   if (optimization_guide::switches::

chrome/browser/optimization_guide/prediction/prediction_manager_browsertest.cc

@@ -350,6 +350,10 @@ class PredictionManagerBrowserTestBase : public InProcessBrowserTest {
     expected_field_trial_name_hashes_ = expected_field_trial_name_hashes;
   }
 
+  void SetExpectedHostsSentInRequest(bool expected_hosts_sent_in_request) {
+    expected_hosts_sent_in_request_ = expected_hosts_sent_in_request;
+  }
+
   GURL https_url_with_content() { return https_url_with_content_; }
   GURL https_url_without_content() { return https_url_without_content_; }
 
@@ -392,11 +396,15 @@ class PredictionManagerBrowserTestBase : public InProcessBrowserTest {
     EXPECT_EQ(seen_field_trial_name_hashes.size(),
               expected_field_trial_name_hashes_.size());
 
+    EXPECT_EQ(expected_hosts_sent_in_request_, !models_request.hosts().empty());
+    std::vector<std::string> hosts;
+    if (expected_hosts_sent_in_request_) {
+      hosts = {"example1.com", https_server_->GetURL("/").host()};
+    }
     response->set_code(net::HTTP_OK);
     std::unique_ptr<optimization_guide::proto::GetModelsResponse>
-        get_models_response = BuildGetModelsResponse(
-            {"example1.com", https_server_->GetURL("/").host()},
-            {client_model_feature_});
+        get_models_response =
+            BuildGetModelsResponse(hosts, {client_model_feature_});
     if (response_type_ == PredictionModelsFetcherRemoteResponseType::
                               kSuccessfulWithFeaturesAndNoModels) {
       get_models_response->clear_models();
@@ -431,6 +439,7 @@ class PredictionManagerBrowserTestBase : public InProcessBrowserTest {
           kSuccessfulWithModelsAndFeatures;
   std::unique_ptr<OptimizationGuideConsumerWebContentsObserver> consumer_;
   base::flat_set<uint32_t> expected_field_trial_name_hashes_;
+  bool expected_hosts_sent_in_request_ = true;
 };
 
 class PredictionManagerBrowserTest : public PredictionManagerBrowserTestBase {
@@ -753,6 +762,72 @@ IN_PROC_BROWSER_TEST_F(
   run_loop->Run();
 }
 
+class PredictionManagerNoUserPermissionsTest
+    : public PredictionManagerBrowserTest {
+ public:
+  PredictionManagerNoUserPermissionsTest() {
+    // Hosts and field trials should not be sent.
+    SetExpectedHostsSentInRequest(false);
+    SetExpectedFieldTrialNames({});
+  }
+
+  ~PredictionManagerNoUserPermissionsTest() override = default;
+
+  void SetUpCommandLine(base::CommandLine* cmd) override {
+    PredictionManagerBrowserTest::SetUpCommandLine(cmd);
+
+    // Remove switches that enable user permissions.
+    cmd->RemoveSwitch("enable-spdy-proxy-auth");
+    cmd->RemoveSwitch(switches::kDisableCheckingUserPermissionsForTesting);
+  }
+
+ private:
+  void InitializeFeatureList() override {
+    scoped_feature_list_.InitWithFeaturesAndParameters(
+        {
+            {features::kOptimizationHints, {}},
+            {features::kRemoteOptimizationGuideFetching, {}},
+            {features::kOptimizationTargetPrediction, {}},
+            {features::kOptimizationHintsFieldTrials,
+             {{"allowed_field_trial_names",
+               "scoped_feature_list_trial_for_OptimizationHints,scoped_feature_"
+               "list_trial_for_OptimizationHintsFetching"}}},
+        },
+        {});
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(PredictionManagerNoUserPermissionsTest,
+                       DISABLE_ON_WIN_MAC_CHROMEOS(
+                           HostsAndFieldTrialsNotPassedWhenNoUserPermissions)) {
+  base::HistogramTester histogram_tester;
+
+  SetResponseType(PredictionModelsFetcherRemoteResponseType::
+                      kSuccessfulWithModelsAndFeatures);
+  RegisterWithKeyedService();
+
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionManager.HostModelFeaturesStored", 1);
+
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionManager.PredictionModelsStored", 1);
+
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionModelLoadedVersion.PainfulPageLoad", 1);
+
+  SetCallbackOnConsumer(base::DoNothing());
+  ui_test_utils::NavigateToURL(browser(), https_url_with_content());
+
+  // Expect that we did not fetch for host and that we did not get any host
+  // model features.
+  histogram_tester.ExpectBucketCount(
+      "OptimizationGuide.PredictionManager.HasHostModelFeaturesForHost", false,
+      1);
+}
+
 // Implementation of a download system logger that provides the ability to wait
 // for certain events to happen, notably added and progressing downloads.
 class DownloadServiceObserver : public download::Logger::Observer {
@@ -860,8 +935,17 @@ class PredictionManagerModelDownloadingBrowserTest
             {features::kOptimizationTargetPrediction, {}},
             {features::kOptimizationGuideModelDownloading,
              {{"unrestricted_model_downloading", "true"}}},
+            {features::kOptimizationHintsFieldTrials,
+             {{"allowed_field_trial_names",
+               "scoped_feature_list_trial_for_OptimizationHints,scoped_feature_"
+               "list_trial_for_OptimizationHintsFetching"}}},
         },
         {});
+    SetExpectedFieldTrialNames(base::flat_set<uint32_t>(
+        {variations::HashName(
+             "scoped_feature_list_trial_for_OptimizationHints"),
+         variations::HashName(
+             "scoped_feature_list_trial_for_OptimizationHintsFetching")}));
   }
 
   std::unique_ptr<DownloadServiceObserver> download_service_observer_;

chrome/browser/optimization_guide/prediction/prediction_model_fetcher.cc

@@ -44,10 +44,10 @@ PredictionModelFetcher::PredictionModelFetcher(
 PredictionModelFetcher::~PredictionModelFetcher() = default;
 
 bool PredictionModelFetcher::FetchOptimizationGuideServiceModels(
-    const std::vector<optimization_guide::proto::ModelInfo>&
-        models_request_info,
+    const std::vector<proto::ModelInfo>& models_request_info,
     const std::vector<std::string>& hosts,
-    optimization_guide::proto::RequestContext request_context,
+    const std::vector<proto::FieldTrial>& active_field_trials,
+    proto::RequestContext request_context,
     ModelsFetchedCallback models_fetched_callback) {
   SEQUENCE_CHECKER(sequence_checker_);
 
@@ -70,8 +70,8 @@ bool PredictionModelFetcher::FetchOptimizationGuideServiceModels(
 
   pending_models_request_->set_request_context(request_context);
 
-  *pending_models_request_->mutable_active_field_trials() =
-      optimization_guide::GetActiveFieldTrialsAllowedForFetch();
+  *pending_models_request_->mutable_active_field_trials() = {
+      active_field_trials.begin(), active_field_trials.end()};
 
   // Limit the number of hosts to fetch features for, the list of hosts
   // is assumed to be ordered from most to least important by the top

chrome/browser/optimization_guide/prediction/prediction_model_fetcher.h

@@ -48,10 +48,10 @@ class PredictionModelFetcher {
   // request is complete providing the GetModelsResponse object if successful or
   // nullopt if the fetch failed or no fetch is needed. Virtualized for testing.
   virtual bool FetchOptimizationGuideServiceModels(
-      const std::vector<optimization_guide::proto::ModelInfo>&
-          models_request_info,
+      const std::vector<proto::ModelInfo>& models_request_info,
       const std::vector<std::string>& hosts,
-      optimization_guide::proto::RequestContext request_context,
+      const std::vector<proto::FieldTrial>& active_field_trials,
+      proto::RequestContext request_context,
       ModelsFetchedCallback models_fetched_callback);
 
  private:

chrome/browser/optimization_guide/prediction/prediction_model_fetcher_unittest.cc

@@ -42,10 +42,8 @@ class PredictionModelFetcherTest : public testing::Test {
 
   ~PredictionModelFetcherTest() override {}
 
-  void OnModelsFetched(
-      base::Optional<
-          std::unique_ptr<optimization_guide::proto::GetModelsResponse>>
-          get_models_response) {
+  void OnModelsFetched(base::Optional<std::unique_ptr<proto::GetModelsResponse>>
+                           get_models_response) {
     if (get_models_response)
       models_fetched_ = true;
   }
@@ -65,14 +63,13 @@ class PredictionModelFetcherTest : public testing::Test {
   }
 
  protected:
-  bool FetchModels(
-      const std::vector<optimization_guide::proto::ModelInfo>
-          models_request_info,
-      const std::vector<std::string>& hosts,
-      const optimization_guide::proto::RequestContext& request_context) {
+  bool FetchModels(const std::vector<proto::ModelInfo> models_request_info,
+                   const std::vector<std::string>& hosts,
+                   const std::vector<proto::FieldTrial>& active_field_trials,
+                   proto::RequestContext request_context) {
     bool status =
         prediction_model_fetcher_->FetchOptimizationGuideServiceModels(
-            models_request_info, hosts, request_context,
+            models_request_info, hosts, active_field_trials, request_context,
             base::BindOnce(&PredictionModelFetcherTest::OnModelsFetched,
                            base::Unretained(this)));
     RunUntilIdle();
@@ -120,10 +117,10 @@ TEST_F(PredictionModelFetcherTest, FetchOptimizationGuideServiceModels) {
   base::HistogramTester histogram_tester;
   std::string response_content;
   std::vector<std::string> hosts = {"foo.com", "bar.com"};
-  std::vector<optimization_guide::proto::ModelInfo> models_request_info({});
-  EXPECT_TRUE(FetchModels(
-      models_request_info, hosts,
-      optimization_guide::proto::RequestContext::CONTEXT_BATCH_UPDATE));
+  std::vector<proto::ModelInfo> models_request_info({});
+  std::vector<proto::FieldTrial> active_field_trials({});
+  EXPECT_TRUE(FetchModels(models_request_info, hosts, active_field_trials,
+                          proto::RequestContext::CONTEXT_BATCH_UPDATE));
   VerifyHasPendingFetchRequests();
 
   histogram_tester.ExpectUniqueSample(
@@ -146,12 +143,14 @@ TEST_F(PredictionModelFetcherTest,
   std::string response_content;
   std::vector<std::string> hosts;
   for (size_t i = 0;
-       i <= features::MaxHostsForOptimizationGuideServiceModelsFetch() + 1; i++)
+       i <= features::MaxHostsForOptimizationGuideServiceModelsFetch() + 1;
+       i++) {
     hosts.push_back("host" + base::NumberToString(i) + ".com");
-  std::vector<optimization_guide::proto::ModelInfo> models_request_info({});
-  EXPECT_TRUE(FetchModels(
-      models_request_info, hosts,
-      optimization_guide::proto::RequestContext::CONTEXT_BATCH_UPDATE));
+  }
+  std::vector<proto::ModelInfo> models_request_info({});
+  std::vector<proto::FieldTrial> active_field_trials({});
+  EXPECT_TRUE(FetchModels(models_request_info, hosts, active_field_trials,
+                          proto::RequestContext::CONTEXT_BATCH_UPDATE));
   VerifyHasPendingFetchRequests();
 
   histogram_tester.ExpectUniqueSample(
@@ -173,10 +172,10 @@ TEST_F(PredictionModelFetcherTest, FetchFilterInvalidHosts) {
   std::string response_content;
   std::vector<std::string> hosts = {"192.168.1.1", "_abc", "localhost",
                                     "foo.com"};
-  std::vector<optimization_guide::proto::ModelInfo> models_request_info({});
-  EXPECT_TRUE(FetchModels(
-      models_request_info, hosts,
-      optimization_guide::proto::RequestContext::CONTEXT_BATCH_UPDATE));
+  std::vector<proto::ModelInfo> models_request_info({});
+  std::vector<proto::FieldTrial> active_field_trials({});
+  EXPECT_TRUE(FetchModels(models_request_info, hosts, active_field_trials,
+                          proto::RequestContext::CONTEXT_BATCH_UPDATE));
   VerifyHasPendingFetchRequests();
 
   histogram_tester.ExpectUniqueSample(
@@ -199,10 +198,10 @@ TEST_F(PredictionModelFetcherTest, FetchReturned404) {
   std::string response_content;
 
   std::vector<std::string> hosts = {"foo.com", "bar.com"};
-  std::vector<optimization_guide::proto::ModelInfo> models_request_info({});
-  EXPECT_TRUE(FetchModels(
-      models_request_info, hosts,
-      optimization_guide::proto::RequestContext::CONTEXT_BATCH_UPDATE));
+  std::vector<proto::ModelInfo> models_request_info({});
+  std::vector<proto::FieldTrial> active_field_trials({});
+  EXPECT_TRUE(FetchModels(models_request_info, hosts, active_field_trials,
+                          proto::RequestContext::CONTEXT_BATCH_UPDATE));
   // Send a 404 to HintsFetcher.
   SimulateResponse(response_content, net::HTTP_NOT_FOUND);
   EXPECT_FALSE(models_fetched());
@@ -220,10 +219,10 @@ TEST_F(PredictionModelFetcherTest, FetchReturnBadResponse) {
   std::string response_content = "not proto";
 
   std::vector<std::string> hosts = {"foo.com", "bar.com"};
-  std::vector<optimization_guide::proto::ModelInfo> models_request_info({});
-  EXPECT_TRUE(FetchModels(
-      models_request_info, hosts,
-      optimization_guide::proto::RequestContext::CONTEXT_BATCH_UPDATE));
+  std::vector<proto::ModelInfo> models_request_info({});
+  std::vector<proto::FieldTrial> active_field_trials({});
+  EXPECT_TRUE(FetchModels(models_request_info, hosts, active_field_trials,
+                          proto::RequestContext::CONTEXT_BATCH_UPDATE));
   VerifyHasPendingFetchRequests();
   EXPECT_TRUE(SimulateResponse(response_content, net::HTTP_OK));
   EXPECT_FALSE(models_fetched());
@@ -233,16 +232,15 @@ TEST_F(PredictionModelFetcherTest, FetchAttemptWhenNetworkOffline) {
   SetConnectionOffline();
   std::string response_content;
   std::vector<std::string> hosts = {"foo.com", "bar.com"};
-  std::vector<optimization_guide::proto::ModelInfo> models_request_info({});
-  EXPECT_FALSE(FetchModels(
-      models_request_info, hosts,
-      optimization_guide::proto::RequestContext::CONTEXT_BATCH_UPDATE));
+  std::vector<proto::ModelInfo> models_request_info({});
+  std::vector<proto::FieldTrial> active_field_trials({});
+  EXPECT_FALSE(FetchModels(models_request_info, hosts, active_field_trials,
+                           proto::RequestContext::CONTEXT_BATCH_UPDATE));
   EXPECT_FALSE(models_fetched());
 
   SetConnectionOnline();
-  EXPECT_TRUE(FetchModels(
-      models_request_info, hosts,
-      optimization_guide::proto::RequestContext::CONTEXT_BATCH_UPDATE));
+  EXPECT_TRUE(FetchModels(models_request_info, hosts, active_field_trials,
+                          proto::RequestContext::CONTEXT_BATCH_UPDATE));
   VerifyHasPendingFetchRequests();
   EXPECT_TRUE(SimulateResponse(response_content, net::HTTP_OK));
   EXPECT_TRUE(models_fetched());
@@ -252,10 +250,10 @@ TEST_F(PredictionModelFetcherTest, EmptyModelInfoAndHosts) {
   base::HistogramTester histogram_tester;
   std::string response_content;
   std::vector<std::string> hosts = {};
-  std::vector<optimization_guide::proto::ModelInfo> models_request_info({});
-  EXPECT_FALSE(FetchModels(
-      models_request_info, hosts,
-      optimization_guide::proto::RequestContext::CONTEXT_BATCH_UPDATE));
+  std::vector<proto::ModelInfo> models_request_info({});
+  std::vector<proto::FieldTrial> active_field_trials({});
+  EXPECT_FALSE(FetchModels(models_request_info, hosts, active_field_trials,
+                           proto::RequestContext::CONTEXT_BATCH_UPDATE));
 
   EXPECT_FALSE(models_fetched());
 }