Add a feature param for what app packages we are allowed to fetch for

Bug: 1014210
Change-Id: Ica50e27d5cae28ffdf5a2ea6c38fb16c0052e7d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2140208Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757164}

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -256,6 +256,9 @@ OptimizationGuideHintsManager::OptimizationGuideHintsManager(
               optimization_guide::features::
                   GetOptimizationGuideServiceGetHintsURL(),
               pref_service)),
+      external_app_packages_approved_for_fetch_(
+          optimization_guide::features::
+              ExternalAppPackageNamesApprovedForFetch()),
       top_host_provider_(top_host_provider),
       clock_(base::DefaultClock::GetInstance()) {
   DCHECK(optimization_guide_service_);
@@ -723,30 +726,52 @@ bool OptimizationGuideHintsManager::IsGoogleURL(const GURL& url) const {
                                        google_util::DISALLOW_SUBDOMAIN);
 }
 
-void OptimizationGuideHintsManager::OnPredictionUpdated(
+bool OptimizationGuideHintsManager::IsAllowedToFetchForNavigationPrediction(
     const base::Optional<NavigationPredictorKeyedService::Prediction>
-        prediction) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  if (registered_optimization_types_.empty())
-    return;
-
-  if (!prediction.has_value())
-    return;
+        prediction) const {
+  if (!prediction)
+    return false;
 
-  if (prediction->prediction_source() !=
+  if (prediction->prediction_source() ==
       NavigationPredictorKeyedService::PredictionSource::
           kAnchorElementsParsedFromWebPage) {
-    return;
+    const base::Optional<GURL> source_document_url =
+        prediction->source_document_url();
+    if (!source_document_url || source_document_url->is_empty())
+      return false;
+
+    // We only extract next predicted navigations from Google URLs.
+    return IsGoogleURL(*source_document_url);
   }
 
-  const base::Optional<GURL>& source_document_url =
-      prediction->source_document_url();
-  if (!source_document_url || source_document_url->is_empty())
-    return;
+  if (prediction->prediction_source() ==
+      NavigationPredictorKeyedService::PredictionSource::kExternalAndroidApp) {
+    if (external_app_packages_approved_for_fetch_.empty())
+      return false;
+
+    const base::Optional<std::vector<std::string>> external_app_packages_name =
+        prediction->external_app_packages_name();
+    if (!external_app_packages_name || external_app_packages_name->empty())
+      return false;
 
-  // We only extract next predicted navigations from Google URLs.
-  if (!IsGoogleURL(source_document_url.value()))
+    for (const auto& package_name : *external_app_packages_name) {
+      if (external_app_packages_approved_for_fetch_.find(package_name) ==
+          external_app_packages_approved_for_fetch_.end())
+        return false;
+    }
+    // If we get here, all apps have been approved for fetching.
+    return true;
+  }
+
+  return false;
+}
+
+void OptimizationGuideHintsManager::OnPredictionUpdated(
+    const base::Optional<NavigationPredictorKeyedService::Prediction>
+        prediction) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (!IsAllowedToFetchForNavigationPrediction(prediction))
     return;
 
   // Extract the target hosts and URLs. Use a flat set to remove duplicates.
@@ -1086,6 +1111,9 @@ bool OptimizationGuideHintsManager::IsAllowedToFetchNavigationHints(
     const GURL& url) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  if (registered_optimization_types_.empty())
+    return false;
+
   if (!IsUserPermittedToFetchFromRemoteOptimizationGuide(profile_))
     return false;
 

chrome/browser/optimization_guide/optimization_guide_hints_manager.h

@@ -179,7 +179,13 @@ class OptimizationGuideHintsManager
       HintsFetched_AtSRP_ECT_SLOW_2G_NonHTTPOrHTTPSHostsRemoved);
   FRIEND_TEST_ALL_PREFIXES(
       OptimizationGuideHintsManagerFetchingTest,
-      HintsFetched_ExternalAndroidApp_ECT_SLOW_2G_NonHTTPOrHTTPSHostsRemoved);
+      HintsFetched_ExternalAndroidApp_ECT_SLOW_2G_NonHTTPOrHTTPSHostsRemovedAppWhitelisted);
+  FRIEND_TEST_ALL_PREFIXES(
+      OptimizationGuideHintsManagerFetchingTest,
+      HintsFetched_ExternalAndroidApp_ECT_SLOW_2G_NonHTTPOrHTTPSHostsRemovedNotAllAppsWhitelisted);
+  FRIEND_TEST_ALL_PREFIXES(
+      OptimizationGuideHintsManagerFetchingTest,
+      HintsFetched_ExternalAndroidApp_ECT_SLOW_2G_NonHTTPOrHTTPSHostsRemovedAppNotWhitelisted);
 
   // Processes the hints component.
   //
@@ -304,6 +310,11 @@ class OptimizationGuideHintsManager
   // search results page (www.google.*).
   bool IsGoogleURL(const GURL& url) const;
 
+  // Returns true if we can make a request for hints for |prediction|.
+  bool IsAllowedToFetchForNavigationPrediction(
+      const base::Optional<NavigationPredictorKeyedService::Prediction>
+          prediction) const;
+
   // NavigationPredictorKeyedService::Observer:
   void OnPredictionUpdated(
       const base::Optional<NavigationPredictorKeyedService::Prediction>
@@ -404,6 +415,10 @@ class OptimizationGuideHintsManager
   std::unique_ptr<optimization_guide::HintsFetcherFactory>
       hints_fetcher_factory_;
 
+  // The external app packages that have been approved for fetching from the
+  // remote Optimization Guide Service.
+  base::flat_set<std::string> external_app_packages_approved_for_fetch_;
+
   // The top host provider that can be queried. Not owned.
   optimization_guide::TopHostProvider* top_host_provider_ = nullptr;
 

chrome/browser/optimization_guide/optimization_guide_hints_manager_unittest.cc

@@ -1952,7 +1952,9 @@ class OptimizationGuideHintsManagerFetchingTest
   OptimizationGuideHintsManagerFetchingTest() {
     scoped_list_.InitAndEnableFeatureWithParameters(
         optimization_guide::features::kRemoteOptimizationGuideFetching,
-        {{"max_concurrent_page_navigation_fetches", "2"}});
+        {{"max_concurrent_page_navigation_fetches", "2"},
+         {"approved_external_app_packages",
+          "org.example.whatever,com.foo.bar"}});
   }
  private:
   base::test::ScopedFeatureList scoped_list_;
@@ -2263,9 +2265,45 @@ TEST_F(OptimizationGuideHintsManagerFetchingTest,
 }
 
 // Verify that optimization hints are not fetched if the prediction for the next
-// likely navigations are provided by external Android app.
-TEST_F(OptimizationGuideHintsManagerFetchingTest,
-       HintsFetched_ExternalAndroidApp_ECT_SLOW_2G_NonHTTPOrHTTPSHostsRemoved) {
+// likely navigations are provided by external Android app that is whitelisted.
+TEST_F(
+    OptimizationGuideHintsManagerFetchingTest,
+    HintsFetched_ExternalAndroidApp_ECT_SLOW_2G_NonHTTPOrHTTPSHostsRemovedAppWhitelisted) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      optimization_guide::switches::kDisableCheckingUserPermissionsForTesting);
+  hints_manager()->RegisterOptimizationTypes(
+      {optimization_guide::proto::DEFER_ALL_SCRIPT});
+  InitializeWithDefaultConfig("1.0.0.0");
+
+  // Set ECT estimate so fetch is activated.
+  hints_manager()->OnEffectiveConnectionTypeChanged(
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_SLOW_2G);
+  base::HistogramTester histogram_tester;
+  std::vector<GURL> sorted_predicted_urls;
+  sorted_predicted_urls.push_back(GURL("https://foo.com/page1.html"));
+  sorted_predicted_urls.push_back(GURL("file://non-web-bar.com/"));
+  sorted_predicted_urls.push_back(GURL("http://httppage.com/"));
+
+  std::vector<std::string> external_app_packages_name;
+  external_app_packages_name.push_back("com.foo.bar");
+
+  NavigationPredictorKeyedService::Prediction prediction_external_android_app(
+      nullptr, base::nullopt, external_app_packages_name,
+      NavigationPredictorKeyedService::PredictionSource::kExternalAndroidApp,
+      sorted_predicted_urls);
+  hints_manager()->OnPredictionUpdated(prediction_external_android_app);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintsFetcher.GetHintsRequest.HostCount", 2, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintsFetcher.GetHintsRequest.UrlCount", 2, 1);
+}
+
+// Verify that optimization hints are not fetched if the prediction for the next
+// likely navigations are provided by external Android app that is not
+// whitelisted, even though one of the apps was whitelisted.
+TEST_F(
+    OptimizationGuideHintsManagerFetchingTest,
+    HintsFetched_ExternalAndroidApp_ECT_SLOW_2G_NonHTTPOrHTTPSHostsRemovedNotAllAppsWhitelisted) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       optimization_guide::switches::kDisableCheckingUserPermissionsForTesting);
   hints_manager()->RegisterOptimizationTypes(
@@ -2282,35 +2320,55 @@ TEST_F(OptimizationGuideHintsManagerFetchingTest,
   sorted_predicted_urls.push_back(GURL("http://httppage.com/"));
 
   std::vector<std::string> external_app_packages_name;
-  external_app_packages_name.push_back("com.example.foo");
+  external_app_packages_name.push_back("com.foo.bar");
+  external_app_packages_name.push_back("com.example.notwhitelisted");
 
   NavigationPredictorKeyedService::Prediction prediction_external_android_app(
       nullptr, base::nullopt, external_app_packages_name,
       NavigationPredictorKeyedService::PredictionSource::kExternalAndroidApp,
       sorted_predicted_urls);
   hints_manager()->OnPredictionUpdated(prediction_external_android_app);
+  // Nothing should be fetched.
   histogram_tester.ExpectTotalCount(
       "OptimizationGuide.HintsFetcher.GetHintsRequest.HostCount", 0);
-  // Ensure that we only include 2 URLs in the request.
   histogram_tester.ExpectTotalCount(
       "OptimizationGuide.HintsFetcher.GetHintsRequest.UrlCount", 0);
+}
 
-  // Now fetch again with a prediction from anchor elements. This time
-  // optimization hints should be requested.
-  NavigationPredictorKeyedService::Prediction prediction_anchor_elements(
-      nullptr, GURL("https://www.google.com/"),
-      /*external_app_packages_name=*/{},
-      NavigationPredictorKeyedService::PredictionSource::
-          kAnchorElementsParsedFromWebPage,
+// Verify that optimization hints are not fetched if the prediction for the next
+// likely navigations are provided by external Android app that is not
+// whitelisted.
+TEST_F(
+    OptimizationGuideHintsManagerFetchingTest,
+    HintsFetched_ExternalAndroidApp_ECT_SLOW_2G_NonHTTPOrHTTPSHostsRemovedAppNotWhitelisted) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      optimization_guide::switches::kDisableCheckingUserPermissionsForTesting);
+  hints_manager()->RegisterOptimizationTypes(
+      {optimization_guide::proto::DEFER_ALL_SCRIPT});
+  InitializeWithDefaultConfig("1.0.0.0");
+
+  // Set ECT estimate so fetch is activated.
+  hints_manager()->OnEffectiveConnectionTypeChanged(
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_SLOW_2G);
+  base::HistogramTester histogram_tester;
+  std::vector<GURL> sorted_predicted_urls;
+  sorted_predicted_urls.push_back(GURL("https://foo.com/page1.html"));
+  sorted_predicted_urls.push_back(GURL("file://non-web-bar.com/"));
+  sorted_predicted_urls.push_back(GURL("http://httppage.com/"));
+
+  std::vector<std::string> external_app_packages_name;
+  external_app_packages_name.push_back("com.example.notwhitelisted");
+
+  NavigationPredictorKeyedService::Prediction prediction_external_android_app(
+      nullptr, base::nullopt, external_app_packages_name,
+      NavigationPredictorKeyedService::PredictionSource::kExternalAndroidApp,
       sorted_predicted_urls);
-  hints_manager()->OnPredictionUpdated(prediction_anchor_elements);
-  // Ensure that we include both web hosts in the request. These would be
-  // foo.com and httppage.com.
-  histogram_tester.ExpectUniqueSample(
-      "OptimizationGuide.HintsFetcher.GetHintsRequest.HostCount", 2, 1);
-  // Ensure that we only include 2 URLs in the request.
-  histogram_tester.ExpectUniqueSample(
-      "OptimizationGuide.HintsFetcher.GetHintsRequest.UrlCount", 2, 1);
+  hints_manager()->OnPredictionUpdated(prediction_external_android_app);
+  // Nothing should be fetched.
+  histogram_tester.ExpectTotalCount(
+      "OptimizationGuide.HintsFetcher.GetHintsRequest.HostCount", 0);
+  histogram_tester.ExpectTotalCount(
+      "OptimizationGuide.HintsFetcher.GetHintsRequest.UrlCount", 0);
 }
 
 TEST_F(OptimizationGuideHintsManagerFetchingTest, HintsFetched_AtSRP_ECT_4G) {

components/optimization_guide/optimization_guide_features.cc

@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/field_trial_params.h"
+#include "base/strings/string_split.h"
 #include "build/build_config.h"
 #include "components/optimization_guide/optimization_guide_constants.h"
 #include "components/optimization_guide/optimization_guide_switches.h"
@@ -255,5 +256,17 @@ int PredictionModelFetchRandomMaxDelaySecs() {
                                           "fetch_random_max_delay_secs", 180);
 }
 
+base::flat_set<std::string> ExternalAppPackageNamesApprovedForFetch() {
+  std::string value = base::GetFieldTrialParamValueByFeature(
+      kRemoteOptimizationGuideFetching, "approved_external_app_packages");
+  if (value.empty())
+    return {};
+
+  std::vector<std::string> app_packages_list = base::SplitString(
+      value, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+  return base::flat_set<std::string>(app_packages_list.begin(),
+                                     app_packages_list.end());
+}
+
 }  // namespace features
 }  // namespace optimization_guide

components/optimization_guide/optimization_guide_features.h

@@ -8,6 +8,7 @@
 #include <string>
 #include <utility>
 
+#include "base/containers/flat_set.h"
 #include "base/feature_list.h"
 #include "base/optional.h"
 #include "base/time/time.h"
@@ -137,6 +138,10 @@ int PredictionModelFetchRandomMinDelaySecs();
 // fetch for prediction models and host model features.
 int PredictionModelFetchRandomMaxDelaySecs();
 
+// Returns a set of external Android app packages whose predictions have been
+// approved for fetching from the remote Optimization Guide Service.
+base::flat_set<std::string> ExternalAppPackageNamesApprovedForFetch();
+
 }  // namespace features
 }  // namespace optimization_guide