Reland "Migrate pinned icons in ImplicitApp subdirs."

This reverts commit 250c173e.

Reason for revert: <reverting did not fix crbug.com/999467>

Original change's description:
> Revert "Migrate pinned icons in ImplicitApp subdirs."
> 
> This reverts commit 4105186f.
> 
> Reason for revert: <crbug.com/999467>
> 
> Original change's description:
> > Migrate pinned icons in ImplicitApp subdirs.
> >
> > Chrome migrates pinned icons in UserPinned/Taskbar dir, but not the
> > subdirectories of UserPinned/ImplicitAppShortcuts. It's unclear to me why
> > Windows puts some pinned icons in the ImplicitAppShortcuts subdirectories,
> > but we should handle them.
> >
> > Bug: 980239
> > Change-Id: Ia41c3853b2a4bb78781a39733f388915f58c02eb
> > TBR:
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1710722
> > Commit-Queue: David Bienvenu <davidbienvenu@chromium.org>
> > Reviewed-by: Greg Thompson <grt@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#681035}
> 
> TBR=msarda@chromium.org,grt@chromium.org,davidbienvenu@chromium.org
> 
> Bug: 980239
> Change-Id: I2b19e825ccd0f1bd2975678d9acb1ad7ff90cc6a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1787560
> Reviewed-by: David Bienvenu <davidbienvenu@chromium.org>
> Commit-Queue: David Bienvenu <davidbienvenu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#693898}

TBR=davidbienvenu@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 980239
Change-Id: Ibe0af2947a119bdccea80067fe86f0d1d5bb7af6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1797692Reviewed-by: David Bienvenu <davidbienvenu@chromium.org>
Commit-Queue: David Bienvenu <davidbienvenu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695758}

chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc

@@ -263,14 +263,14 @@ class ProfileShortcutManagerTest : public testing::Test {
   // Posts a task to call ShellUtil::CreateOrUpdateShortcut on the COM thread.
   void PostCreateOrUpdateShortcut(
       const base::Location& location,
+      ShellUtil::ShortcutLocation shortcut_location,
       const ShellUtil::ShortcutProperties& properties) {
     base::PostTaskAndReplyWithResult(
         base::CreateCOMSTATaskRunner({base::ThreadPool(), base::MayBlock()})
             .get(),
         location,
-        base::Bind(&ShellUtil::CreateOrUpdateShortcut,
-                   ShellUtil::SHORTCUT_LOCATION_DESKTOP, properties,
-                   ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS),
+        base::Bind(&ShellUtil::CreateOrUpdateShortcut, shortcut_location,
+                   properties, ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS),
         base::Bind([](bool succeeded) { EXPECT_TRUE(succeeded); }));
     task_environment_.RunUntilIdle();
   }
@@ -287,7 +287,8 @@ class ProfileShortcutManagerTest : public testing::Test {
     ShellUtil::ShortcutProperties properties(ShellUtil::CURRENT_USER);
     ShellUtil::AddDefaultShortcutProperties(GetExePath(), &properties);
     properties.set_shortcut_name(shortcut_name);
-    PostCreateOrUpdateShortcut(location, properties);
+    PostCreateOrUpdateShortcut(location, ShellUtil::SHORTCUT_LOCATION_DESKTOP,
+                               properties);
     EXPECT_TRUE(base::PathExists(shortcut_path)) << location.ToString();
 
     return shortcut_path;
@@ -297,7 +298,8 @@ class ProfileShortcutManagerTest : public testing::Test {
       const base::Location& location) {
     ShellUtil::ShortcutProperties properties(ShellUtil::SYSTEM_LEVEL);
     ShellUtil::AddDefaultShortcutProperties(GetExePath(), &properties);
-    PostCreateOrUpdateShortcut(location, properties);
+    PostCreateOrUpdateShortcut(location, ShellUtil::SHORTCUT_LOCATION_DESKTOP,
+                               properties);
     const base::FilePath system_level_shortcut_path =
         GetSystemShortcutsDirectory().Append(InstallUtil::GetShortcutName() +
                                              installer::kLnkExt);

chrome/browser/shell_integration_win.cc

@@ -492,6 +492,16 @@ void IsPinnedToTaskbarHelper::OnIsPinnedToTaskbarResult(
   delete this;
 }
 
+void MigrateChromeAndChromeProxyShortcuts(
+    const base::FilePath& chrome_exe,
+    const base::FilePath& chrome_proxy_path,
+    const base::FilePath& shortcut_path) {
+  win::MigrateShortcutsInPathInternal(chrome_exe, shortcut_path);
+
+  // Migrate any pinned PWA shortcuts in taskbar directory.
+  win::MigrateShortcutsInPathInternal(chrome_proxy_path, shortcut_path);
+}
+
 }  // namespace
 
 bool SetAsDefaultBrowser() {
@@ -700,28 +710,44 @@ void MigrateTaskbarPins() {
   // This needs to happen (e.g. so that the appid is fixed and the
   // run-time Chrome icon is merged with the taskbar shortcut), but it is not an
   // urgent task.
-  base::FilePath pins_path;
-  if (!base::PathService::Get(base::DIR_TASKBAR_PINS, &pins_path)) {
+  base::FilePath taskbar_path;
+  if (!base::PathService::Get(base::DIR_TASKBAR_PINS, &taskbar_path)) {
+    NOTREACHED();
+    return;
+  }
+
+  // Migrate any pinned shortcuts in ImplicitApps sub-directories.
+  base::FilePath implicit_apps_path;
+  if (!base::PathService::Get(base::DIR_IMPLICIT_APP_SHORTCUTS,
+                              &implicit_apps_path)) {
     NOTREACHED();
     return;
   }
 
   base::CreateCOMSTATaskRunner(
       {base::ThreadPool(), base::MayBlock(), base::TaskPriority::BEST_EFFORT})
-      ->PostTask(FROM_HERE,
-                 base::BindOnce(&MigrateTaskbarPinsCallback, pins_path));
+      ->PostTask(FROM_HERE, base::BindOnce(&MigrateTaskbarPinsCallback,
+                                           taskbar_path, implicit_apps_path));
 }
 
-void MigrateTaskbarPinsCallback(const base::FilePath& pins_path) {
+void MigrateTaskbarPinsCallback(const base::FilePath& taskbar_path,
+                                const base::FilePath& implicit_apps_path) {
   // Get full path of chrome.
   base::FilePath chrome_exe;
   if (!base::PathService::Get(base::FILE_EXE, &chrome_exe))
     return;
-
-  win::MigrateShortcutsInPathInternal(chrome_exe, pins_path);
-
-  // Migrate any pinned PWA shortcuts.
-  win::MigrateShortcutsInPathInternal(web_app::GetChromeProxyPath(), pins_path);
+  base::FilePath chrome_proxy_path(web_app::GetChromeProxyPath());
+
+  MigrateChromeAndChromeProxyShortcuts(chrome_exe, chrome_proxy_path,
+                                       taskbar_path);
+  base::FileEnumerator directory_enum(implicit_apps_path, /*recursive=*/false,
+                                      base::FileEnumerator::DIRECTORIES);
+  for (base::FilePath implicit_app_sub_directory = directory_enum.Next();
+       !implicit_app_sub_directory.empty();
+       implicit_app_sub_directory = directory_enum.Next()) {
+    MigrateChromeAndChromeProxyShortcuts(chrome_exe, chrome_proxy_path,
+                                         implicit_app_sub_directory);
+  }
 }
 
 void GetIsPinnedToTaskbarState(

chrome/browser/shell_integration_win.h

@@ -74,7 +74,8 @@ void GetIsPinnedToTaskbarState(
 void MigrateTaskbarPins();
 
 // Callback for MigrateTaskbarPins(). Exposed for testing.
-void MigrateTaskbarPinsCallback(const base::FilePath& pins_path);
+void MigrateTaskbarPinsCallback(const base::FilePath& pins_path,
+                                const base::FilePath& implicit_apps_path);
 
 // Migrates all shortcuts in |path| which point to |chrome_exe| such that they
 // have the appropriate AppUserModelId. Also clears the legacy dual_mode

chrome/browser/shell_integration_win_unittest.cc

@@ -43,7 +43,8 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
 
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-
+    ASSERT_TRUE(
+        temp_dir_sub_dir_.CreateUniqueTempDirUnderPath(temp_dir_.GetPath()));
     // A path to a random target.
     base::CreateTemporaryFileInDir(temp_dir_.GetPath(), &other_target_);
 
@@ -83,9 +84,10 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
   // |shortcut_properties| after copying it to an internal structure for later
   // verification.
   void AddTestShortcutAndResetProperties(
+      const base::FilePath& shortcut_dir,
       base::win::ShortcutProperties* shortcut_properties) {
     ShortcutTestObject shortcut_test_object;
-    base::FilePath shortcut_path = temp_dir_.GetPath().Append(
+    base::FilePath shortcut_path = shortcut_dir.Append(
         L"Shortcut " + base::NumberToString16(shortcuts_.size()) +
         installer::kLnkExt);
     shortcut_test_object.path = shortcut_path;
@@ -105,22 +107,22 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     // Shortcut 0 doesn't point to chrome.exe and thus should never be migrated.
     temp_properties.set_target(other_target_);
     temp_properties.set_app_id(L"Dumbo");
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 1 points to chrome.exe and thus should be migrated.
     temp_properties.set_target(chrome_exe_);
     temp_properties.set_app_id(L"Dumbo");
     temp_properties.set_dual_mode(false);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 2 points to chrome.exe, but already has the right appid and thus
     // should only be migrated if dual_mode is desired.
     temp_properties.set_target(chrome_exe_);
     temp_properties.set_app_id(default_profile_chrome_app_id_);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 3 is like shortcut 1, but it's appid is a prefix of the expected
     // appid instead of being totally different.
@@ -128,8 +130,8 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     chrome_app_id_is_prefix.push_back(L'1');
     temp_properties.set_target(chrome_exe_);
     temp_properties.set_app_id(chrome_app_id_is_prefix);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 4 is like shortcut 1, but it's appid is of the same size as the
     // expected appid.
@@ -137,14 +139,14 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
         default_profile_chrome_app_id_.size(), L'1');
     temp_properties.set_target(chrome_exe_);
     temp_properties.set_app_id(same_size_as_chrome_app_id);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 5 doesn't have an app_id, nor is dual_mode even set; they should
     // be set as expected upon migration.
     temp_properties.set_target(chrome_exe_);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 6 has a non-default profile directory and so should get a non-
     // default app id.
@@ -152,8 +154,8 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     temp_properties.set_app_id(L"Dumbo");
     temp_properties.set_arguments(
         L"--profile-directory=" + non_default_profile_);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 7 has a non-default user data directory and so should get a non-
     // default app id.
@@ -161,8 +163,8 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     temp_properties.set_app_id(L"Dumbo");
     temp_properties.set_arguments(
         L"--user-data-dir=\"" + non_default_user_data_dir_.value() + L"\"");
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 8 has a non-default user data directory as well as a non-default
     // profile directory and so should get a non-default app id.
@@ -171,8 +173,8 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     temp_properties.set_arguments(
         L"--user-data-dir=\"" + non_default_user_data_dir_.value() + L"\" " +
         L"--profile-directory=" + non_default_profile_);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 9 is a shortcut to an app and should get an app id for that app
     // rather than the chrome app id.
@@ -180,8 +182,8 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     temp_properties.set_app_id(L"Dumbo");
     temp_properties.set_arguments(
         L"--app-id=" + extension_id_);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 10 is a shortcut to an app with a non-default profile and should
     // get an app id for that app with a non-default app id rather than the
@@ -191,8 +193,8 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     temp_properties.set_arguments(
         L"--app-id=" + extension_id_ +
         L" --profile-directory=" + non_default_profile_);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 11 points to chrome.exe, has the chrome appid, and has
     // dual_mode set and thus should be migrated to the
@@ -200,21 +202,24 @@ class ShellIntegrationWinMigrateShortcutTest : public testing::Test {
     temp_properties.set_target(chrome_exe_);
     temp_properties.set_app_id(chrome_app_id_);
     temp_properties.set_dual_mode(true);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
 
     // Shortcut 12 is similar to 11 but with dual_mode explicitly set to false.
     temp_properties.set_target(chrome_exe_);
     temp_properties.set_app_id(chrome_app_id_);
     temp_properties.set_dual_mode(false);
-    ASSERT_NO_FATAL_FAILURE(
-        AddTestShortcutAndResetProperties(&temp_properties));
+    ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+        temp_dir_.GetPath(), &temp_properties));
   }
 
   base::win::ScopedCOMInitializer com_initializer_;
 
   base::ScopedTempDir temp_dir_;
 
+  // Used to test migration of shortcuts in ImplicitApps sub-directories.
+  base::ScopedTempDir temp_dir_sub_dir_;
+
   // Test shortcuts.
   std::vector<ShortcutTestObject> shortcuts_;
 
@@ -300,8 +305,9 @@ TEST_F(ShellIntegrationWinMigrateShortcutTest, ClearDualModeAndAdjustAppIds) {
             MigrateShortcutsInPathInternal(chrome_exe_, temp_dir_.GetPath()));
 }
 
-// Test that a chrome_proxy.exe shortcut (PWA) has its app_id migrated
-// to include the default profile name.
+// Test that chrome_proxy.exe shortcuts (PWA) have their app_id migrated
+// to include the default profile name. This tests both shortcuts in the
+// DIR_TASKBAR_PINS and sub-directories of DIR_IMPLICIT_APP_SHORTCUTS.
 TEST_F(ShellIntegrationWinMigrateShortcutTest, MigrateChromeProxyTest) {
   // Create shortcut to chrome_proxy_exe in executable directory,
   // using the default profile, with the AppModelId not containing the
@@ -310,12 +316,22 @@ TEST_F(ShellIntegrationWinMigrateShortcutTest, MigrateChromeProxyTest) {
   base::win::ShortcutProperties temp_properties;
   temp_properties.set_target(web_app::GetChromeProxyPath());
   temp_properties.set_app_id(L"Dumbo");
-  ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(&temp_properties));
+  ASSERT_NO_FATAL_FAILURE(
+      AddTestShortcutAndResetProperties(temp_dir_.GetPath(), &temp_properties));
+  temp_properties.set_target(web_app::GetChromeProxyPath());
+  temp_properties.set_app_id(L"Dumbo2");
+  ASSERT_NO_FATAL_FAILURE(AddTestShortcutAndResetProperties(
+      temp_dir_sub_dir_.GetPath(), &temp_properties));
 
-  MigrateTaskbarPinsCallback(temp_dir_.GetPath());
-  // Verify that the migrated shortcut now contains the default profile name.
+  MigrateTaskbarPinsCallback(temp_dir_.GetPath(), temp_dir_.GetPath());
+  // Verify that the migrated shortcut in temp_dir_ now contains the default
+  // profile name.
   shortcuts_[0].properties.set_app_id(default_profile_chrome_app_id_);
   base::win::ValidateShortcut(shortcuts_[0].path, shortcuts_[0].properties);
+  // Verify that the migrated shortcut in temp_dir_sub now contains the default
+  // profile name.
+  shortcuts_[1].properties.set_app_id(default_profile_chrome_app_id_);
+  base::win::ValidateShortcut(shortcuts_[1].path, shortcuts_[1].properties);
 }
 
 TEST(ShellIntegrationWinTest, GetAppModelIdForProfileTest) {