Guard rename Sharing devices behind feature flag

Bug: 1024845
Change-Id: Ia08a00398eb5cbc6809a055423b46b434e0dbe49
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1917161
Commit-Queue: Alex Chau <alexchau@chromium.org>
Reviewed-by: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715708}

chrome/browser/about_flags.cc

@@ -3845,6 +3845,10 @@ const FeatureEntry kFeatureEntries[] = {
      flag_descriptions::kSharingPeerConnectionSenderDescription, kOsAll,
      FEATURE_VALUE_TYPE(kSharingPeerConnectionSender)},
 
+    {"sharing-rename-devices", flag_descriptions::kSharingRenameDevicesName,
+     flag_descriptions::kSharingRenameDevicesDescription, kOsAll,
+     FEATURE_VALUE_TYPE(kSharingRenameDevices)},
+
 #if defined(OS_CHROMEOS)
     {"discover-app", flag_descriptions::kEnableDiscoverAppName,
      flag_descriptions::kEnableDiscoverAppDescription, kOsCrOS,

chrome/browser/flag-metadata.json

@@ -3195,6 +3195,11 @@
     "owners": [ "//chrome/browser/sharing/OWNERS" ],
     "expiry_milestone": 83
   },
+  {
+    "name": "sharing-rename-devices",
+    "owners": [ "//chrome/browser/sharing/OWNERS" ],
+    "expiry_milestone": 83
+  },
   {
     "name": "sharing-use-device-info",
     "owners": [ "//chrome/browser/sharing/OWNERS" ],

chrome/browser/flag_descriptions.cc

@@ -1882,6 +1882,12 @@ const char kSharingPeerConnectionSenderDescription[] =
     "Enables the sender devices to connect with chosen device using a peer to "
     "peer connection for transferring data.";
 
+const char kSharingRenameDevicesName[] =
+    "Enable device renaming in Sharing features.";
+const char kSharingRenameDevicesDescription[] =
+    "Enables renaming devices using HardwareInfo when populating device list "
+    "and sender device info.";
+
 const char kSharingUseDeviceInfoName[] =
     "Enable Sharing device registration in DeviceInfo";
 const char kSharingUseDeviceInfoDescription[] =

chrome/browser/flag_descriptions.h

@@ -1101,6 +1101,9 @@ extern const char kSharingPeerConnectionReceiverDescription[];
 extern const char kSharingPeerConnectionSenderName[];
 extern const char kSharingPeerConnectionSenderDescription[];
 
+extern const char kSharingRenameDevicesName[];
+extern const char kSharingRenameDevicesDescription[];
+
 extern const char kSharingUseDeviceInfoName[];
 extern const char kSharingUseDeviceInfoDescription[];
 

chrome/browser/sharing/features.cc

@@ -15,3 +15,6 @@ const base::Feature kSharingQRCodeGenerator{"SharingQRCodeGenerator",
 
 const base::Feature kSharingDeriveVapidKey{"SharingDeriveVapidKey",
                                            base::FEATURE_DISABLED_BY_DEFAULT};
+
+const base::Feature kSharingRenameDevices{"SharingRenameDevices",
+                                          base::FEATURE_DISABLED_BY_DEFAULT};

chrome/browser/sharing/features.h

@@ -20,4 +20,7 @@ extern const base::Feature kSharingQRCodeGenerator;
 // Feature flag to enable deriving VAPID key from Sync.
 extern const base::Feature kSharingDeriveVapidKey;
 
+// Feature flag to enable device renaming.
+extern const base::Feature kSharingRenameDevices;
+
 #endif  // CHROME_BROWSER_SHARING_FEATURES_H_

chrome/browser/sharing/sharing_device_source_sync_unittest.cc

@@ -10,7 +10,9 @@
 #include "base/guid.h"
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
+#include "chrome/browser/sharing/features.h"
 #include "chrome/browser/sharing/sharing_utils.h"
 #include "components/sync/driver/test_sync_service.h"
 #include "components/sync_device_info/device_info.h"
@@ -36,6 +38,10 @@ std::unique_ptr<syncer::DeviceInfo> CreateDeviceInfo(
 
 class SharingDeviceSourceSyncTest : public testing::Test {
  public:
+  SharingDeviceSourceSyncTest() {
+    scoped_feature_list_.InitAndEnableFeature(kSharingRenameDevices);
+  }
+
   std::unique_ptr<SharingDeviceSourceSync> CreateDeviceSource(
       bool wait_until_ready) {
     auto device_source = std::make_unique<SharingDeviceSourceSync>(
@@ -59,6 +65,7 @@ class SharingDeviceSourceSyncTest : public testing::Test {
  protected:
   content::BrowserTaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  base::test::ScopedFeatureList scoped_feature_list_;
 
   syncer::TestSyncService test_sync_service_;
   syncer::FakeLocalDeviceInfoProvider fake_local_device_info_provider_;

chrome/browser/sharing/sharing_utils.cc

@@ -73,15 +73,16 @@ SharingDeviceNames GetSharingDeviceNames(const syncer::DeviceInfo* device) {
 
   base::SysInfo::HardwareInfo hardware_info = device->hardware_info();
   sync_pb::SyncEnums::DeviceType type = device->device_type();
-  // We only want to apply renaming for sign-in only devices. sign-in only
-  // devices has client_name == model. Additionally, Android and Chrome OS also
-  // uses model as client_name, so we should avoid renaming them as well.
-  // Lastly, avoid renaming if HardwareInfo is not available for M78- devices,
+  // 1. Skip renaming for M78- devices where HardwareInfo is not available.
+  // 2. Skip renaming if client_name is high quality i.e. not equals to model.
+  // 3. Skip renaming for Android and Chrome OS devices if feature is not
+  //    enabled, which always have low quality client_name.
   if (hardware_info.model.empty() ||
       hardware_info.model != device->client_name() ||
-      type == sync_pb::SyncEnums::TYPE_CROS ||
-      type == sync_pb::SyncEnums::TYPE_PHONE ||
-      type == sync_pb::SyncEnums::TYPE_TABLET) {
+      (!base::FeatureList::IsEnabled(kSharingRenameDevices) &&
+       (type == sync_pb::SyncEnums::TYPE_CROS ||
+        type == sync_pb::SyncEnums::TYPE_PHONE ||
+        type == sync_pb::SyncEnums::TYPE_TABLET))) {
     device_names.full_name = device_names.short_name = device->client_name();
     return device_names;
   }

chrome/browser/sharing/sharing_utils_unittest.cc

@@ -60,7 +60,20 @@ TEST_F(SharingUtilsTest, GetSharingDeviceNames_AppleDevices_FullySynced) {
   EXPECT_EQ("Bobs-iMac", names.short_name);
 }
 
-TEST_F(SharingUtilsTest, GetSharingDeviceNames_ChromeOSDevices) {
+TEST_F(SharingUtilsTest, GetSharingDeviceNames_ChromeOSDevices_FeatureEnabled) {
+  scoped_feature_list_.InitAndEnableFeature(kSharingRenameDevices);
+  std::unique_ptr<syncer::DeviceInfo> device = CreateFakeDeviceInfo(
+      "guid", "Chromebook", sync_pb::SyncEnums_DeviceType_TYPE_CROS,
+      {"Google", "Chromebook", ""});
+  SharingDeviceNames names = GetSharingDeviceNames(device.get());
+
+  EXPECT_EQ("Google Chromebook", names.full_name);
+  EXPECT_EQ("Google Chromebook", names.short_name);
+}
+
+TEST_F(SharingUtilsTest,
+       GetSharingDeviceNames_ChromeOSDevices_FeatureDisabled) {
+  scoped_feature_list_.InitAndDisableFeature(kSharingRenameDevices);
   std::unique_ptr<syncer::DeviceInfo> device = CreateFakeDeviceInfo(
       "guid", "Chromebook", sync_pb::SyncEnums_DeviceType_TYPE_CROS,
       {"Google", "Chromebook", ""});
@@ -70,7 +83,19 @@ TEST_F(SharingUtilsTest, GetSharingDeviceNames_ChromeOSDevices) {
   EXPECT_EQ("Chromebook", names.short_name);
 }
 
-TEST_F(SharingUtilsTest, GetSharingDeviceNames_AndroidPhones) {
+TEST_F(SharingUtilsTest, GetSharingDeviceNames_AndroidPhones_FeatureEnabled) {
+  scoped_feature_list_.InitAndEnableFeature(kSharingRenameDevices);
+  std::unique_ptr<syncer::DeviceInfo> device = CreateFakeDeviceInfo(
+      "guid", "Pixel 2", sync_pb::SyncEnums_DeviceType_TYPE_PHONE,
+      {"Google", "Pixel 2", ""});
+  SharingDeviceNames names = GetSharingDeviceNames(device.get());
+
+  EXPECT_EQ("Google Phone Pixel 2", names.full_name);
+  EXPECT_EQ("Google Phone", names.short_name);
+}
+
+TEST_F(SharingUtilsTest, GetSharingDeviceNames_AndroidPhones_FeatureDisabled) {
+  scoped_feature_list_.InitAndDisableFeature(kSharingRenameDevices);
   std::unique_ptr<syncer::DeviceInfo> device = CreateFakeDeviceInfo(
       "guid", "Pixel 2", sync_pb::SyncEnums_DeviceType_TYPE_PHONE,
       {"Google", "Pixel 2", ""});
@@ -80,7 +105,19 @@ TEST_F(SharingUtilsTest, GetSharingDeviceNames_AndroidPhones) {
   EXPECT_EQ("Pixel 2", names.short_name);
 }
 
-TEST_F(SharingUtilsTest, GetSharingDeviceNames_AndroidTablets) {
+TEST_F(SharingUtilsTest, GetSharingDeviceNames_AndroidTablets_FeatureEnabled) {
+  scoped_feature_list_.InitAndEnableFeature(kSharingRenameDevices);
+  std::unique_ptr<syncer::DeviceInfo> device = CreateFakeDeviceInfo(
+      "guid", "Pixel C", sync_pb::SyncEnums_DeviceType_TYPE_TABLET,
+      {"Google", "Pixel C", ""});
+  SharingDeviceNames names = GetSharingDeviceNames(device.get());
+
+  EXPECT_EQ("Google Tablet Pixel C", names.full_name);
+  EXPECT_EQ("Google Tablet", names.short_name);
+}
+
+TEST_F(SharingUtilsTest, GetSharingDeviceNames_AndroidTablets_FeatureDisabled) {
+  scoped_feature_list_.InitAndDisableFeature(kSharingRenameDevices);
   std::unique_ptr<syncer::DeviceInfo> device = CreateFakeDeviceInfo(
       "guid", "Pixel C", sync_pb::SyncEnums_DeviceType_TYPE_TABLET,
       {"Google", "Pixel C", ""});

tools/metrics/histograms/enums.xml

@@ -36358,6 +36358,7 @@ from previous Chrome versions.
   <int value="-1252976780" label="ShillSandboxing:disabled"/>
   <int value="-1252706530" label="AutoFetchOnNetErrorPage:disabled"/>
   <int value="-1251411236" label="disable-new-md-input-view"/>
+  <int value="-1251359563" label="SharingRenameDevices:disabled"/>
   <int value="-1250611337" label="ChromeVoxArcSupport:disabled"/>
   <int value="-1248478422" label="enable-zip-archiver-packer"/>
   <int value="-1247631459" label="TerminalSystemAppSplits:disabled"/>
@@ -36574,6 +36575,7 @@ from previous Chrome versions.
   <int value="-984052166" label="DoodlesOnLocalNtp:enabled"/>
   <int value="-983342281" label="TabSwitcherLongpressMenu:disabled"/>
   <int value="-981237342" label="SyncUSSAutofillWalletMetadata:disabled"/>
+  <int value="-980889204" label="SharingRenameDevices:enabled"/>
   <int value="-980317085" label="EnablePasswordsAccountStorage:disabled"/>
   <int value="-980260493" label="NTPSnippets:disabled"/>
   <int value="-979313250" label="enable-google-branded-context-menu"/>