Consolidate Chrome Cleanup distribution params under a single feature.

Also retires legacy URL for downloading Canary cleaner version.

Bug: 786964
Change-Id: I0026545eee940501f38c6b65a76e03dc2427481a
Reviewed-on: https://chromium-review.googlesource.com/965943Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Veranika Liaukevich <veranika@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544876}

chrome/browser/component_updater/sw_reporter_installer_win.cc

@@ -53,7 +53,6 @@ namespace component_updater {
 
 namespace {
 
-using safe_browsing::OnReporterSequenceDone;
 using safe_browsing::SwReporterInvocation;
 using safe_browsing::SwReporterInvocationSequence;
 
@@ -77,9 +76,6 @@ const uint8_t kSha256Hash[] = {0x6a, 0xc6, 0x0e, 0xe8, 0xf3, 0x97, 0xc0, 0xd6,
 const base::FilePath::CharType kSwReporterExeName[] =
     FILE_PATH_LITERAL("software_reporter_tool.exe");
 
-constexpr base::Feature kComponentTagFeature{kComponentTagFeatureName,
-                                             base::FEATURE_DISABLED_BY_DEFAULT};
-
 void SRTHasCompleted(SRTCompleted value) {
   UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.Cleaner.HasCompleted", value,
                             SRT_COMPLETED_MAX);
@@ -406,18 +402,20 @@ std::string SwReporterInstallerPolicy::GetName() const {
 update_client::InstallerAttributes
 SwReporterInstallerPolicy::GetInstallerAttributes() const {
   update_client::InstallerAttributes attributes;
-  if (base::FeatureList::IsEnabled(kComponentTagFeature)) {
-    // Pass the "tag" parameter to the installer; it will be used to choose
-    // which binary is downloaded.
-    constexpr char kTagParam[] = "tag";
+  if (base::FeatureList::IsEnabled(
+          safe_browsing::kChromeCleanupDistributionFeature)) {
+    // Pass the tag parameter to the installer as the "tag" attribute; it will
+    // be used to choose which binary is downloaded.
+    constexpr char kTagParamName[] = "reporter_omaha_tag";
     const std::string tag = variations::GetVariationParamValueByFeature(
-        kComponentTagFeature, kTagParam);
+        safe_browsing::kChromeCleanupDistributionFeature, kTagParamName);
 
     // If the tag is not a valid attribute (see the regexp in
     // ComponentInstallerPolicy::InstallerAttributes), set it to a valid but
     // unrecognized value so that nothing will be downloaded.
     constexpr size_t kMaxAttributeLength = 256;
     constexpr char kExtraAttributeChars[] = "-.,;+_=";
+    constexpr char kTagParam[] = "tag";
     if (tag.empty() ||
         !ValidateString(tag, kExtraAttributeChars, kMaxAttributeLength)) {
       ReportExperimentError(SW_REPORTER_EXPERIMENT_ERROR_BAD_TAG);

chrome/browser/component_updater/sw_reporter_installer_win.h

@@ -32,12 +32,6 @@ namespace component_updater {
 
 class ComponentUpdateService;
 
-// Expose the feature name so it can be referenced in tests.
-// TODO(crbug.com/786964): This feature will continue to exist as part of a
-// permanent variations study to control which version of the reporter gets
-// downloaded. Rename it to something that makes sense long-term.
-constexpr char kComponentTagFeatureName[] = "ExperimentalSwReporterEngine";
-
 constexpr char kSwReporterComponentId[] = "gkmgaooipdjhmangpemjhigmamcehddo";
 
 // These MUST match the values for SoftwareReporterExperimentError in

chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc

@@ -23,6 +23,7 @@
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
 #include "components/chrome_cleaner/public/constants/constants.h"
 #include "components/component_updater/mock_component_updater_service.h"
 #include "components/variations/variations_params_manager.h"
@@ -72,7 +73,7 @@ class SwReporterInstallerTest : public ::testing::Test {
   }
 
   void CreateFeatureWithTag(const std::string& tag) {
-    std::map<std::string, std::string> params{{"tag", tag}};
+    std::map<std::string, std::string> params{{"reporter_omaha_tag", tag}};
     CreateFeatureWithParams(params);
   }
 
@@ -83,9 +84,10 @@ class SwReporterInstallerTest : public ::testing::Test {
     // create a FieldTrial for this group and associate the params with the
     // feature. For the test just re-use the feature name as the trial name.
     variations_ = std::make_unique<variations::testing::VariationParamsManager>(
-        /*trial_name=*/kComponentTagFeatureName, params,
+        "SwReporterInstallerTestTrialName", params,
         /*associated_features=*/
-        std::set<std::string>{kComponentTagFeatureName});
+        std::set<std::string>{
+            safe_browsing::kChromeCleanupDistributionFeature.name});
   }
 
   void ExpectAttributesWithTag(const SwReporterInstallerPolicy& policy,

chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.cc

@@ -15,23 +15,19 @@
 namespace {
 
 // Field trial strings.
-const char kSRTCanaryGroup[] = "SRTCanary";
 const char kSRTPromptOffGroup[] = "Off";
 const char kSRTPromptSeedParam[] = "Seed";
 
 const char kSRTElevationTrial[] = "SRTElevation";
 const char kSRTElevationAsNeededGroup[] = "AsNeeded";
 
+// The download links of the Software Removal Tool.
 const char kDownloadRootPath[] =
     "https://dl.google.com/dl/softwareremovaltool/win/";
 
-// The download links of the Software Removal Tool.
-const char kMainSRTDownloadURL[] =
+const char kLegacySRTDownloadURL[] =
     "https://dl.google.com/dl"
     "/softwareremovaltool/win/chrome_cleanup_tool.exe";
-const char kCanarySRTDownloadURL[] =
-    "https://dl.google.com/dl"
-    "/softwareremovaltool/win/c/chrome_cleanup_tool.exe";
 
 }  // namespace
 
@@ -45,10 +41,8 @@ const base::Feature kRebootPromptDialogFeature{
 const base::Feature kUserInitiatedChromeCleanupsFeature{
     "UserInitiatedChromeCleanups", base::FEATURE_ENABLED_BY_DEFAULT};
 
-// TODO(b/786964): Rename this to remove ByBitness from the feature name once
-// all test scripts have been updated.
-const base::Feature kCleanerDownloadFeature{"DownloadCleanupToolByBitness",
-                                            base::FEATURE_DISABLED_BY_DEFAULT};
+const base::Feature kChromeCleanupDistributionFeature{
+    "ChromeCleanupDistribution", base::FEATURE_DISABLED_BY_DEFAULT};
 
 bool IsInSRTPromptFieldTrialGroups() {
   return !base::StartsWith(base::FieldTrialList::FindFullName(kSRTPromptTrial),
@@ -66,17 +60,13 @@ bool UserInitiatedCleanupsEnabled() {
 }
 
 GURL GetLegacyDownloadURL() {
-  if (base::StartsWith(base::FieldTrialList::FindFullName(kSRTPromptTrial),
-                       kSRTCanaryGroup, base::CompareCase::SENSITIVE)) {
-    return GURL(kCanarySRTDownloadURL);
-  }
-  return GURL(kMainSRTDownloadURL);
+  return GURL(kLegacySRTDownloadURL);
 }
 
 GURL GetSRTDownloadURL() {
-  constexpr char kDownloadGroupParam[] = "download_group";
+  constexpr char kCleanerDownloadGroupParam[] = "cleaner_download_group";
   const std::string download_group = base::GetFieldTrialParamValueByFeature(
-      kCleanerDownloadFeature, kDownloadGroupParam);
+      kChromeCleanupDistributionFeature, kCleanerDownloadGroupParam);
   if (download_group.empty())
     return GetLegacyDownloadURL();
 

chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h

@@ -59,6 +59,10 @@ extern const base::Feature kRebootPromptDialogFeature;
 // When enabled, users can initiate cleanups from the Settings page.
 extern const base::Feature kUserInitiatedChromeCleanupsFeature;
 
+// Feature, parameters of which control which software reporter and cleanup tool
+// versions will be downloaded. When not enabled, default versions will be used.
+extern const base::Feature kChromeCleanupDistributionFeature;
+
 extern const char kSRTPromptTrial[];
 
 // Returns true if this Chrome is in a field trial group which shows the SRT

chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win_unittest.cc

@@ -22,9 +22,9 @@ class SRTDownloadURLTest : public ::testing::Test {
   }
 
   void CreateDownloadFeature(const std::string& download_group_name) {
-    constexpr char kFeatureName[] = "DownloadCleanupToolByBitness";
+    constexpr char kFeatureName[] = "ChromeCleanupDistribution";
     std::map<std::string, std::string> params;
-    params["download_group"] = download_group_name;
+    params["cleaner_download_group"] = download_group_name;
     variations_.SetVariationParamsWithFeatureAssociations(
         "A trial name", params, {kFeatureName});
   }
@@ -39,12 +39,6 @@ TEST_F(SRTDownloadURLTest, Stable) {
             GetSRTDownloadURL().path());
 }
 
-TEST_F(SRTDownloadURLTest, Canary) {
-  CreatePromptTrial("SRTCanary");
-  EXPECT_EQ("/dl/softwareremovaltool/win/c/chrome_cleanup_tool.exe",
-            GetSRTDownloadURL().path());
-}
-
 TEST_F(SRTDownloadURLTest, Experiment) {
   CreateDownloadFeature("experiment");
   std::string expected_path;