variations: Refactor GetCurrentFormFactor to VariationsServiceClient

Move the various GetCurrentFormFactor functions to the
VariationsServiceClient class.

BUG=b:177442621
TEST=autoninja -C out/Default/ components:components_unittests &&
xvfb-run ./out/Default/components_unittests
--gtest_filter=FieldTrialUtilTest.*

Change-Id: Ib677ed6cfbba91a287cb5c9ef06cf96332e4be3f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2633426
Auto-Submit: Kyle Williams <kdgwill@chromium.org>
Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844656}

components/variations/field_trial_config/BUILD.gn

@@ -67,7 +67,7 @@ static_library("field_trial_config") {
     ":field_trial_testing_config_action",
     "//base",
     "//components/variations",
-    "//components/variations/proto:proto",
+    "//components/variations/proto",
     "//net",
     "//ui/base",
   ]
@@ -83,6 +83,8 @@ source_set("unit_tests") {
     "//base",
     "//base/test:test_support",
     "//components/variations",
+    "//components/variations/service",
+    "//services/network/public/cpp",
     "//testing/gtest",
     "//ui/base",
   ]

components/variations/field_trial_config/DEPS

@@ -2,3 +2,10 @@ include_rules = [
   "+net/base",
   "+ui/base",
 ]
+
+# TODO(crbug.com/1167566): Remove when fake VariationsServiceClient created.
+specific_include_rules = {
+  "field_trial_util_unittest\.cc" : [
+      "+services/network/public/cpp/shared_url_loader_factory.h",
+  ],
+}

components/variations/field_trial_config/field_trial_util.cc

@@ -48,24 +48,12 @@ bool HasDeviceLevelMismatch(const FieldTrialTestingExperiment& experiment) {
          base::SysInfo::IsLowEndDevice();
 }
 
-// Gets current form factor and converts it from enum DeviceFormFactor to enum
-// Study_FormFactor.
-Study::FormFactor _GetCurrentFormFactor() {
-  switch (ui::GetDeviceFormFactor()) {
-    case ui::DEVICE_FORM_FACTOR_PHONE:
-      return Study::PHONE;
-    case ui::DEVICE_FORM_FACTOR_TABLET:
-      return Study::TABLET;
-    case ui::DEVICE_FORM_FACTOR_DESKTOP:
-      return Study::DESKTOP;
-  }
-}
-
 // Returns true if the experiment config has a missing form_factors or it
 // contains the current system's form_factor. Otherwise, it is False.
-bool HasFormFactor(const FieldTrialTestingExperiment& experiment) {
+bool HasFormFactor(const FieldTrialTestingExperiment& experiment,
+                   Study::FormFactor current_form_factor) {
   for (size_t i = 0; i < experiment.form_factors_size; ++i) {
-    if (experiment.form_factors[i] == _GetCurrentFormFactor())
+    if (experiment.form_factors[i] == current_form_factor)
       return true;
   }
   return experiment.form_factors_size == 0;
@@ -141,6 +129,7 @@ void ChooseExperiment(
     const FieldTrialTestingStudy& study,
     const VariationsSeedProcessor::UIStringOverrideCallback& callback,
     Study::Platform platform,
+    Study::FormFactor current_form_factor,
     base::FeatureList* feature_list) {
   const auto& command_line = *base::CommandLine::ForCurrentProcess();
   const FieldTrialTestingExperiment* chosen_experiment = nullptr;
@@ -148,7 +137,8 @@ void ChooseExperiment(
     const FieldTrialTestingExperiment* experiment = study.experiments + i;
     if (HasPlatform(*experiment, platform)) {
       if (!chosen_experiment && !HasDeviceLevelMismatch(*experiment) &&
-          HasFormFactor(*experiment) && HasMinOSVersion(*experiment)) {
+          HasFormFactor(*experiment, current_form_factor) &&
+          HasMinOSVersion(*experiment)) {
         chosen_experiment = experiment;
       }
 
@@ -197,11 +187,13 @@ void AssociateParamsFromFieldTrialConfig(
     const FieldTrialTestingConfig& config,
     const VariationsSeedProcessor::UIStringOverrideCallback& callback,
     Study::Platform platform,
+    Study::FormFactor current_form_factor,
     base::FeatureList* feature_list) {
   for (size_t i = 0; i < config.studies_size; ++i) {
     const FieldTrialTestingStudy& study = config.studies[i];
     if (study.experiments_size > 0) {
-      ChooseExperiment(study, callback, platform, feature_list);
+      ChooseExperiment(study, callback, platform, current_form_factor,
+                       feature_list);
     } else {
       DLOG(ERROR) << "Unexpected empty study: " << study.name;
     }
@@ -211,9 +203,10 @@ void AssociateParamsFromFieldTrialConfig(
 void AssociateDefaultFieldTrialConfig(
     const VariationsSeedProcessor::UIStringOverrideCallback& callback,
     Study::Platform platform,
+    Study::FormFactor current_form_factor,
     base::FeatureList* feature_list) {
   AssociateParamsFromFieldTrialConfig(kFieldTrialConfig, callback, platform,
-                                      feature_list);
+                                      current_form_factor, feature_list);
 }
 
 }  // namespace variations

components/variations/field_trial_config/field_trial_util.h

@@ -39,6 +39,7 @@ void AssociateParamsFromFieldTrialConfig(
     const FieldTrialTestingConfig& config,
     const VariationsSeedProcessor::UIStringOverrideCallback& callback,
     Study::Platform platform,
+    Study::FormFactor current_form_factor,
     base::FeatureList* feature_list);
 
 // Associates params and features to FieldTrial groups and forces the selection
@@ -47,6 +48,7 @@ void AssociateParamsFromFieldTrialConfig(
 void AssociateDefaultFieldTrialConfig(
     const VariationsSeedProcessor::UIStringOverrideCallback& callback,
     Study::Platform platform,
+    Study::FormFactor current_form_factor,
     base::FeatureList* feature_list);
 
 }  // namespace variations

components/variations/field_trial_config/field_trial_util_unittest.cc

@@ -19,8 +19,10 @@
 #include "base/test/scoped_feature_list.h"
 #include "components/variations/client_filterable_state.h"
 #include "components/variations/field_trial_config/fieldtrial_testing_config.h"
+#include "components/variations/service/variations_service_client.h"
 #include "components/variations/variations_associated_data.h"
 #include "components/variations/variations_seed_processor.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/device_form_factor.h"
 
@@ -54,6 +56,38 @@ class TestOverrideStringCallback {
   DISALLOW_COPY_AND_ASSIGN(TestOverrideStringCallback);
 };
 
+// TODO(crbug.com/1167566): Remove when fake VariationsServiceClient created.
+class TestVariationsServiceClient : public VariationsServiceClient {
+ public:
+  TestVariationsServiceClient() = default;
+  TestVariationsServiceClient(const TestVariationsServiceClient&) = delete;
+  TestVariationsServiceClient& operator=(const TestVariationsServiceClient&) =
+      delete;
+  ~TestVariationsServiceClient() override = default;
+
+  // VariationsServiceClient:
+  VersionCallback GetVersionForSimulationCallback() override {
+    return base::BindOnce([] { return base::Version(); });
+  }
+  scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory()
+      override {
+    return nullptr;
+  }
+  network_time::NetworkTimeTracker* GetNetworkTimeTracker() override {
+    return nullptr;
+  }
+  bool OverridesRestrictParameter(std::string* parameter) override {
+    return false;
+  }
+  bool IsEnterprise() override { return false; }
+
+ private:
+  // VariationsServiceClient:
+  version_info::Channel GetChannel() override {
+    return version_info::Channel::UNKNOWN;
+  }
+};
+
 class FieldTrialUtilTest : public ::testing::Test {
  public:
   FieldTrialUtilTest() {}
@@ -65,21 +99,9 @@ class FieldTrialUtilTest : public ::testing::Test {
     testing::ClearAllVariationParams();
   }
 
-  // Gets current form factor and converts it from enum DeviceFormFactor to enum
-  // Study_FormFactor.
-  Study::FormFactor _GetCurrentFormFactor() {
-    switch (ui::GetDeviceFormFactor()) {
-      case ui::DEVICE_FORM_FACTOR_PHONE:
-        return Study::PHONE;
-      case ui::DEVICE_FORM_FACTOR_TABLET:
-        return Study::TABLET;
-      case ui::DEVICE_FORM_FACTOR_DESKTOP:
-        return Study::DESKTOP;
-    }
-  }
-
  protected:
   TestOverrideStringCallback override_callback_;
+  TestVariationsServiceClient variation_service_client_;
 
   DISALLOW_COPY_AND_ASSIGN(FieldTrialUtilTest);
 };
@@ -177,10 +199,9 @@ TEST_F(FieldTrialUtilTest, AssociateParamsFromFieldTrialConfig) {
   };
 
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig,
-                                      override_callback_.callback(),
-                                      platform,
-                                      &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), platform,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("1", GetVariationParamValue("TestTrial1", "x"));
   EXPECT_EQ("2", GetVariationParamValue("TestTrial1", "y"));
@@ -245,10 +266,9 @@ TEST_F(FieldTrialUtilTest,
     };
 
     base::FeatureList feature_list;
-    AssociateParamsFromFieldTrialConfig(kConfig,
-                                        override_callback_.callback(),
-                                        platform,
-                                        &feature_list);
+    AssociateParamsFromFieldTrialConfig(
+        kConfig, override_callback_.callback(), platform,
+        variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
     EXPECT_EQ("1", GetVariationParamValue("TestTrial", "x"));
     EXPECT_EQ("2", GetVariationParamValue("TestTrial", "y"));
@@ -293,9 +313,9 @@ TEST_F(FieldTrialUtilTest,
 
   // The platforms don't match, so trial shouldn't be added.
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig, override_callback_.callback(),
-                                      Study::PLATFORM_ANDROID_WEBVIEW,
-                                      &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), Study::PLATFORM_ANDROID_WEBVIEW,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("", GetVariationParamValue("TestTrial", "x"));
   EXPECT_EQ("", GetVariationParamValue("TestTrial", "y"));
@@ -337,9 +357,9 @@ TEST_F(FieldTrialUtilTest,
 
   // One of the platforms matches, so trial should be added.
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig, override_callback_.callback(),
-                                      Study::PLATFORM_ANDROID_WEBVIEW,
-                                      &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), Study::PLATFORM_ANDROID_WEBVIEW,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("1", GetVariationParamValue("TestTrial", "x"));
   EXPECT_EQ("2", GetVariationParamValue("TestTrial", "y"));
@@ -372,10 +392,9 @@ TEST_F(FieldTrialUtilTest,
 
   // One of the form_factors matches, so trial should be added.
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig,
-                                      override_callback_.callback(),
-                                      platform,
-                                      &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), platform,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("1", GetVariationParamValue("TestTrial", "x"));
   EXPECT_EQ("2", GetVariationParamValue("TestTrial", "y"));
@@ -392,7 +411,8 @@ TEST_F(FieldTrialUtilTest,
 TEST_F(FieldTrialUtilTest,
        AssociateParamsFromFieldTrialConfigWithSingleFormFactor) {
   const Study::Platform platform = Study::PLATFORM_WINDOWS;
-  const Study::FormFactor form_factor = _GetCurrentFormFactor();
+  const Study::FormFactor form_factor =
+      variation_service_client_.GetCurrentFormFactor();
   const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params[] =
         {{"x", "1"}, {"y", "2"}};
   const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = {
@@ -409,10 +429,9 @@ TEST_F(FieldTrialUtilTest,
 
   // One of the form_factors matches, so trial should be added.
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig,
-                                      override_callback_.callback(),
-                                      platform,
-                                      &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), platform,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("1", GetVariationParamValue("TestTrial", "x"));
   EXPECT_EQ("2", GetVariationParamValue("TestTrial", "y"));
@@ -429,7 +448,8 @@ TEST_F(FieldTrialUtilTest,
 TEST_F(FieldTrialUtilTest,
        AssociateParamsFromFieldTrialConfigWithDifferentFormFactor) {
   const Study::Platform platform = Study::PLATFORM_WINDOWS;
-  const Study::FormFactor current_form_factor = _GetCurrentFormFactor();
+  const Study::FormFactor current_form_factor =
+      variation_service_client_.GetCurrentFormFactor();
   const Study::FormFactor all_form_factors[] =
       {Study::DESKTOP, Study::PHONE, Study::TABLET};
   for (size_t i = 0; i < base::size(all_form_factors); ++i) {
@@ -450,9 +470,9 @@ TEST_F(FieldTrialUtilTest,
 
     // The form factor don't match, so trial shouldn't be added.
     base::FeatureList feature_list;
-    AssociateParamsFromFieldTrialConfig(kConfig, override_callback_.callback(),
-                                        Study::PLATFORM_ANDROID_WEBVIEW,
-                                        &feature_list);
+    AssociateParamsFromFieldTrialConfig(
+        kConfig, override_callback_.callback(), Study::PLATFORM_ANDROID_WEBVIEW,
+        variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
     EXPECT_EQ("", GetVariationParamValue("TestTrial", "x"));
     EXPECT_EQ("", GetVariationParamValue("TestTrial", "y"));
@@ -537,10 +557,9 @@ TEST_F(FieldTrialUtilTest, AssociateFeaturesFromFieldTrialConfig) {
   };
 
   std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
-  AssociateParamsFromFieldTrialConfig(kConfig,
-                                      override_callback_.callback(),
-                                      platform,
-                                      feature_list.get());
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), platform,
+      variation_service_client_.GetCurrentFormFactor(), feature_list.get());
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeatureList(std::move(feature_list));
 
@@ -673,10 +692,9 @@ TEST_F(FieldTrialUtilTest, AssociateForcingFlagsFromFieldTrialConfig) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch("flag-3");
 
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig,
-                                      override_callback_.callback(),
-                                      platform,
-                                      &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), platform,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("TestGroup1", base::FieldTrialList::FindFullName("TestTrial1"));
   EXPECT_EQ("ForcedGroup2", base::FieldTrialList::FindFullName("TestTrial2"));
@@ -704,10 +722,9 @@ TEST_F(FieldTrialUtilTest,
 
   // One of the form_factors matches, so trial should be added.
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig,
-                                      override_callback_.callback(),
-                                      platform,
-                                      &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), platform,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("1", GetVariationParamValue("TestTrial", "x"));
   EXPECT_EQ("2", GetVariationParamValue("TestTrial", "y"));
@@ -755,8 +772,9 @@ TEST_F(FieldTrialUtilTest,
 
   // The is_low_end_device filter matches, so trial should be added.
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig, override_callback_.callback(),
-                                      platform, &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), platform,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("1", GetVariationParamValue("TestTrial", "x"));
   EXPECT_EQ("2", GetVariationParamValue("TestTrial", "y"));
@@ -797,8 +815,9 @@ TEST_F(FieldTrialUtilTest,
 
   // The is_low_end_device don't match, so trial shouldn't be added.
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig, override_callback_.callback(),
-                                      platform, &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), platform,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("", GetVariationParamValue("TestTrial", "x"));
   EXPECT_EQ("", GetVariationParamValue("TestTrial", "y"));
@@ -838,8 +857,9 @@ TEST_F(FieldTrialUtilTest,
 
   // The min_os_version filter matches, so trial should be added.
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig, override_callback_.callback(),
-                                      platform, &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), platform,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("1", GetVariationParamValue("TestTrial", "x"));
   EXPECT_EQ("2", GetVariationParamValue("TestTrial", "y"));
@@ -884,8 +904,9 @@ TEST_F(FieldTrialUtilTest,
 
   // The min_os_version doesn't match, so trial shouldn't be added.
   base::FeatureList feature_list;
-  AssociateParamsFromFieldTrialConfig(kConfig, override_callback_.callback(),
-                                      platform, &feature_list);
+  AssociateParamsFromFieldTrialConfig(
+      kConfig, override_callback_.callback(), platform,
+      variation_service_client_.GetCurrentFormFactor(), &feature_list);
 
   EXPECT_EQ("", GetVariationParamValue("TestTrial", "x"));
   EXPECT_EQ("", GetVariationParamValue("TestTrial", "y"));

components/variations/service/variations_field_trial_creator.cc

@@ -57,21 +57,6 @@ enum VariationsSeedExpiry {
   VARIATIONS_SEED_EXPIRY_ENUM_SIZE,
 };
 
-// Gets current form factor and converts it from enum DeviceFormFactor to enum
-// Study_FormFactor.
-Study::FormFactor GetCurrentFormFactor() {
-  switch (ui::GetDeviceFormFactor()) {
-    case ui::DEVICE_FORM_FACTOR_PHONE:
-      return Study::PHONE;
-    case ui::DEVICE_FORM_FACTOR_TABLET:
-      return Study::TABLET;
-    case ui::DEVICE_FORM_FACTOR_DESKTOP:
-      return Study::DESKTOP;
-  }
-  NOTREACHED();
-  return Study::DESKTOP;
-}
-
 // Returns the date that should be used by the VariationsSeedProcessor to do
 // expiry and start date checks.
 base::Time GetReferenceDateForExpiryChecks(PrefService* local_state) {
@@ -238,7 +223,7 @@ VariationsFieldTrialCreator::GetClientFilterableStateForVersion(
   state->os_version = ClientFilterableState::GetOSVersion();
   state->channel =
       ConvertProductChannelToStudyChannel(client_->GetChannelForVariations());
-  state->form_factor = GetCurrentFormFactor();
+  state->form_factor = client_->GetCurrentFormFactor();
   state->platform = GetPlatform();
   // TODO(crbug/1111131): Expand to other platforms.
 #if BUILDFLAG(IS_CHROMEOS_ASH) || defined(OS_ANDROID)
@@ -518,7 +503,7 @@ bool VariationsFieldTrialCreator::SetupFieldTrials(
     AssociateDefaultFieldTrialConfig(
         base::BindRepeating(&VariationsFieldTrialCreator::OverrideUIString,
                             base::Unretained(this)),
-        GetPlatform(), feature_list.get());
+        GetPlatform(), client_->GetCurrentFormFactor(), feature_list.get());
     used_testing_config = true;
   }
 #endif  // BUILDFLAG(FIELDTRIAL_TESTING_ENABLED)

components/variations/service/variations_field_trial_creator_unittest.cc

@@ -152,6 +152,7 @@ class MockSafeSeedManager : public SafeSeedManager {
   DISALLOW_COPY_AND_ASSIGN(MockSafeSeedManager);
 };
 
+// TODO(crbug.com/1167566): Remove when fake VariationsServiceClient created.
 class TestVariationsServiceClient : public VariationsServiceClient {
  public:
   TestVariationsServiceClient() = default;

components/variations/service/variations_service_client.cc

@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "components/variations/variations_switches.h"
+#include "ui/base/device_form_factor.h"
 
 namespace variations {
 
@@ -30,4 +31,17 @@ version_info::Channel VariationsServiceClient::GetChannelForVariations() {
   return GetChannel();
 }
 
+Study::FormFactor VariationsServiceClient::GetCurrentFormFactor() {
+  switch (ui::GetDeviceFormFactor()) {
+    case ui::DEVICE_FORM_FACTOR_PHONE:
+      return Study::PHONE;
+    case ui::DEVICE_FORM_FACTOR_TABLET:
+      return Study::TABLET;
+    case ui::DEVICE_FORM_FACTOR_DESKTOP:
+      return Study::DESKTOP;
+  }
+  NOTREACHED();
+  return Study::DESKTOP;
+}
+
 }  // namespace variations

components/variations/service/variations_service_client.h

@@ -11,6 +11,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/strings/string16.h"
 #include "base/version.h"
+#include "components/variations/proto/study.pb.h"
 #include "components/version_info/version_info.h"
 
 namespace network {
@@ -50,6 +51,9 @@ class VariationsServiceClient {
   // (which could be UNKNOWN).
   version_info::Channel GetChannelForVariations();
 
+  // Returns the current form factor of the device.
+  virtual Study::FormFactor GetCurrentFormFactor();
+
   // Returns whether the client is enterprise.
   // TODO(manukh): crbug.com/1003025. This is inconsistent with UMA which
   // analyzes brand_code to determine if the client is an enterprise user:

components/variations/service/variations_service_unittest.cc

@@ -69,6 +69,7 @@ std::unique_ptr<metrics::ClientInfo> StubLoadClientInfo() {
   return std::unique_ptr<metrics::ClientInfo>();
 }
 
+// TODO(crbug.com/1167566): Remove when fake VariationsServiceClient created.
 class TestVariationsServiceClient : public VariationsServiceClient {
  public:
   TestVariationsServiceClient() {