Ensure web app icons are updated after a manifest update

When web apps were using the Extension system it got icon updating for
free. Now that BMO is enabled and there's no underlying Extension app
manifest updates must trigger the shortcut icon updating explicitly.

This change affects Linux, Windows and Mac. Chrome OS icon updating
requires app service integration to be followed up with in a later CL:
https://chromium-review.googlesource.com/c/chromium/src/+/2222180/1

Bug: 1084405
Change-Id: Id22327069d2b9ed5f2381d2c159846417d79afab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2216143
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Auto-Submit: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773589}

chrome/browser/apps/platform_apps/shortcut_manager.cc

@@ -108,7 +108,10 @@ void AppShortcutManager::OnExtensionWillBeInstalled(
     const Extension* extension,
     bool is_update,
     const std::string& old_name) {
-  if (!extension->is_app())
+  // Bookmark apps are handled in
+  // web_app::AppShortcutManager::OnWebAppInstalled() and
+  // web_app::AppShortcutManager::OnWebAppManifestUpdated().
+  if (!extension->is_app() || extension->from_bookmark())
     return;
 
   // If the app is being updated, update any existing shortcuts but do not

chrome/browser/web_applications/components/app_registrar.cc

@@ -47,6 +47,12 @@ void AppRegistrar::NotifyWebAppInstalled(const AppId& app_id) {
   // the WebappInstallSource in this event.
 }
 
+void AppRegistrar::NotifyWebAppManifestUpdated(const AppId& app_id,
+                                               base::StringPiece old_name) {
+  for (AppRegistrarObserver& observer : observers_)
+    observer.OnWebAppManifestUpdated(app_id, old_name);
+}
+
 void AppRegistrar::NotifyWebAppUninstalled(const AppId& app_id) {
   for (AppRegistrarObserver& observer : observers_)
     observer.OnWebAppUninstalled(app_id);

chrome/browser/web_applications/components/app_registrar.h

@@ -153,6 +153,8 @@ class AppRegistrar {
   void RemoveObserver(AppRegistrarObserver* observer);
 
   void NotifyWebAppInstalled(const AppId& app_id);
+  void NotifyWebAppManifestUpdated(const AppId& app_id,
+                                   base::StringPiece old_name);
   void NotifyWebAppUninstalled(const AppId& app_id);
   void NotifyWebAppLocallyInstalledStateChanged(const AppId& app_id,
                                                 bool is_locally_installed);

chrome/browser/web_applications/components/app_registrar_observer.h

@@ -14,6 +14,12 @@ class AppRegistrarObserver : public base::CheckedObserver {
  public:
   virtual void OnWebAppInstalled(const AppId& app_id) {}
 
+  // Called when any field of a web app's local manifest is updated.
+  // Note that |old_name| will always be the same as the current name as we
+  // don't support name updating yet. See TODO(crbug.com/1088338).
+  virtual void OnWebAppManifestUpdated(const AppId& app_id,
+                                       base::StringPiece old_name) {}
+
   // |app_id| still registered in the AppRegistrar. For bookmark apps, use
   // BookmarkAppRegistrar::FindExtension to convert this |app_id| to Extension
   // pointer.

chrome/browser/web_applications/components/app_shortcut_manager.cc

@@ -8,6 +8,8 @@
 
 #include "base/callback.h"
 #include "base/feature_list.h"
+#include "base/no_destructor.h"
+#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_features.h"
@@ -35,6 +37,11 @@ void OnShortcutsInfoRetrievedRegisterShortcutsMenuWithOs(
       std::move(shortcut_info->profile_path), shortcuts);
 }
 
+AppShortcutManager::ShortcutCallback& GetShortcutUpdateCallbackForTesting() {
+  static base::NoDestructor<AppShortcutManager::ShortcutCallback> callback;
+  return *callback;
+}
+
 }  // namespace
 
 AppShortcutManager::AppShortcutManager(Profile* profile) : profile_(profile) {}
@@ -74,6 +81,17 @@ void AppShortcutManager::OnWebAppInstalled(const AppId& app_id) {
 #endif
 }
 
+void AppShortcutManager::OnWebAppManifestUpdated(const AppId& app_id,
+                                                 base::StringPiece old_name) {
+  if (!CanCreateShortcuts())
+    return;
+
+  GetShortcutInfoForApp(
+      app_id, base::BindOnce(
+                  &AppShortcutManager::OnShortcutInfoRetrievedUpdateShortcuts,
+                  weak_ptr_factory_.GetWeakPtr(), base::UTF8ToUTF16(old_name)));
+}
+
 void AppShortcutManager::OnWebAppUninstalled(const AppId& app_id) {
   std::unique_ptr<ShortcutInfo> shortcut_info = BuildShortcutInfo(app_id);
   base::FilePath shortcut_data_dir =
@@ -97,6 +115,11 @@ void AppShortcutManager::OnWebAppProfileWillBeDeleted(const AppId& app_id) {
   DeleteSharedAppShims(app_id);
 }
 
+void AppShortcutManager::SetShortcutUpdateCallbackForTesting(
+    base::OnceCallback<void(const ShortcutInfo*)> callback) {
+  GetShortcutUpdateCallbackForTesting() = std::move(callback);
+}
+
 void AppShortcutManager::DeleteSharedAppShims(const AppId& app_id) {
 #if defined(OS_MACOSX)
   bool delete_multi_profile_shortcuts =
@@ -208,4 +231,21 @@ void AppShortcutManager::OnShortcutInfoRetrievedRegisterRunOnOsLogin(
   internals::ScheduleRegisterRunOnOsLogin(std::move(info), std::move(callback));
 }
 
+void AppShortcutManager::OnShortcutInfoRetrievedUpdateShortcuts(
+    base::string16 old_name,
+    std::unique_ptr<ShortcutInfo> shortcut_info) {
+  if (GetShortcutUpdateCallbackForTesting())
+    std::move(GetShortcutUpdateCallbackForTesting()).Run(shortcut_info.get());
+
+  if (suppress_shortcuts_for_testing() || !shortcut_info)
+    return;
+
+  base::FilePath shortcut_data_dir =
+      internals::GetShortcutDataDir(*shortcut_info);
+  internals::PostShortcutIOTask(
+      base::BindOnce(&internals::UpdatePlatformShortcuts,
+                     std::move(shortcut_data_dir), std::move(old_name)),
+      std::move(shortcut_info));
+}
+
 }  // namespace web_app

chrome/browser/web_applications/components/app_shortcut_manager.h

@@ -45,6 +45,8 @@ class AppShortcutManager : public AppRegistrarObserver {
 
   // AppRegistrarObserver:
   void OnWebAppInstalled(const AppId& app_id) override;
+  void OnWebAppManifestUpdated(const web_app::AppId& app_id,
+                               base::StringPiece old_name) override;
   void OnWebAppUninstalled(const AppId& app_id) override;
   void OnWebAppProfileWillBeDeleted(const AppId& app_id) override;
 
@@ -82,6 +84,9 @@ class AppShortcutManager : public AppRegistrarObserver {
   virtual void GetShortcutInfoForApp(const AppId& app_id,
                                      GetShortcutInfoCallback callback) = 0;
 
+  using ShortcutCallback = base::OnceCallback<void(const ShortcutInfo*)>;
+  static void SetShortcutUpdateCallbackForTesting(ShortcutCallback callback);
+
  protected:
   void DeleteSharedAppShims(const AppId& app_id);
   void OnShortcutsCreated(const AppId& app_id,
@@ -90,6 +95,9 @@ class AppShortcutManager : public AppRegistrarObserver {
 
   AppRegistrar* registrar() { return registrar_; }
   Profile* profile() { return profile_; }
+  bool suppress_shortcuts_for_testing() const {
+    return suppress_shortcuts_for_testing_;
+  }
 
  private:
   void OnShortcutInfoRetrievedCreateShortcuts(
@@ -101,6 +109,10 @@ class AppShortcutManager : public AppRegistrarObserver {
       RegisterRunOnOsLoginCallback callback,
       std::unique_ptr<ShortcutInfo> info);
 
+  void OnShortcutInfoRetrievedUpdateShortcuts(
+      base::string16 old_name,
+      std::unique_ptr<ShortcutInfo> info);
+
   ScopedObserver<AppRegistrar, AppRegistrarObserver> app_registrar_observer_{
       this};
 

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.cc

@@ -149,10 +149,10 @@ void BookmarkAppInstallFinalizer::FinalizeUpdate(
 
   scoped_refptr<CrxInstaller> crx_installer =
       crx_installer_factory_.Run(profile_);
-  crx_installer->set_installer_callback(
-      base::BindOnce(&BookmarkAppInstallFinalizer::OnExtensionUpdated,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(expected_app_id),
-                     std::move(callback), crx_installer));
+  crx_installer->set_installer_callback(base::BindOnce(
+      &BookmarkAppInstallFinalizer::OnExtensionUpdated,
+      weak_ptr_factory_.GetWeakPtr(), std::move(expected_app_id),
+      existing_extension->short_name(), std::move(callback), crx_installer));
   crx_installer->InitializeCreationFlagsForUpdate(existing_extension,
                                                   Extension::NO_FLAGS);
   crx_installer->set_install_source(existing_extension->location());
@@ -302,6 +302,7 @@ void BookmarkAppInstallFinalizer::OnExtensionInstalled(
 
 void BookmarkAppInstallFinalizer::OnExtensionUpdated(
     const web_app::AppId& expected_app_id,
+    const std::string& old_name,
     InstallFinalizedCallback callback,
     scoped_refptr<CrxInstaller> crx_installer,
     const base::Optional<CrxInstallError>& error) {
@@ -322,6 +323,8 @@ void BookmarkAppInstallFinalizer::OnExtensionUpdated(
     return;
   }
 
+  registrar().NotifyWebAppManifestUpdated(extension->id(), old_name);
+
   std::move(callback).Run(extension->id(),
                           web_app::InstallResultCode::kSuccessAlreadyInstalled);
 }

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.h

@@ -77,6 +77,7 @@ class BookmarkAppInstallFinalizer : public web_app::InstallFinalizer {
                             const base::Optional<CrxInstallError>& error);
 
   void OnExtensionUpdated(const web_app::AppId& expected_app_id,
+                          const std::string& old_name,
                           InstallFinalizedCallback callback,
                           scoped_refptr<CrxInstaller> crx_installer,
                           const base::Optional<CrxInstallError>& error);

chrome/browser/web_applications/manifest_update_manager_browsertest.cc

@@ -54,6 +54,8 @@ constexpr char kInstallableIconList[] = R"(
     }
   ]
 )";
+constexpr SkColor kInstallableIconTopLeftColor =
+    SkColorSetRGB(0x15, 0x96, 0xE0);
 
 constexpr char kAnotherInstallableIconList[] = R"(
   [
@@ -64,6 +66,8 @@ constexpr char kAnotherInstallableIconList[] = R"(
     }
   ]
 )";
+constexpr SkColor kAnotherInstallableIconTopLeftColor =
+    SkColorSetRGB(0x5C, 0x5C, 0x5C);
 
 ManifestUpdateManager& GetManifestUpdateManager(Browser* browser) {
   return WebAppProviderBase::GetProviderBase(browser->profile())
@@ -137,6 +141,26 @@ class ManifestUpdateManagerBrowserTest
 
   void SetUpOnMainThread() override {
     GetProvider().shortcut_manager().SuppressShortcutsForTesting();
+    // Cannot construct RunLoop in constructor due to threading restrictions.
+    shortcut_run_loop_.emplace();
+    AppShortcutManager::SetShortcutUpdateCallbackForTesting(
+        base::BindOnce(&ManifestUpdateManagerBrowserTest::OnShortcutUpdated,
+                       base::Unretained(this)));
+  }
+
+  void OnShortcutUpdated(const ShortcutInfo* shortcut_info) {
+    if (shortcut_info) {
+      updated_shortcut_top_left_color_ =
+          shortcut_info->favicon.begin()->AsBitmap().getColor(0, 0);
+    }
+    shortcut_run_loop_->Quit();
+  }
+
+  void AwaitShortcutsUpdated(SkColor top_left_color) {
+    if (!GetProvider().shortcut_manager().CanCreateShortcuts())
+      return;
+    shortcut_run_loop_->Run();
+    EXPECT_EQ(updated_shortcut_top_left_color_, top_left_color);
   }
 
   std::unique_ptr<net::test_server::HttpResponse> RequestHandlerOverride(
@@ -240,6 +264,9 @@ class ManifestUpdateManagerBrowserTest
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
 
+  base::Optional<base::RunLoop> shortcut_run_loop_;
+  base::Optional<SkColor> updated_shortcut_top_left_color_;
+
   DISALLOW_COPY_AND_ASSIGN(ManifestUpdateManagerBrowserTest);
 };
 
@@ -546,6 +573,7 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest,
             ManifestUpdateResult::kAppUpdated);
   histogram_tester_.ExpectBucketCount(kUpdateHistogramName,
                                       ManifestUpdateResult::kAppUpdated, 1);
+  AwaitShortcutsUpdated(kInstallableIconTopLeftColor);
   EXPECT_EQ(GetProvider().registrar().GetAppThemeColor(app_id), SK_ColorRED);
 }
 
@@ -572,6 +600,7 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest, CheckKeepsSameName) {
             ManifestUpdateResult::kAppUpdated);
   histogram_tester_.ExpectBucketCount(kUpdateHistogramName,
                                       ManifestUpdateResult::kAppUpdated, 1);
+  AwaitShortcutsUpdated(kInstallableIconTopLeftColor);
   EXPECT_EQ(GetProvider().registrar().GetAppThemeColor(app_id), SK_ColorRED);
   // The app name must not change without user confirmation.
   EXPECT_EQ(GetProvider().registrar().GetAppShortName(app_id), "App name 1");
@@ -596,6 +625,7 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest,
             ManifestUpdateResult::kAppUpdated);
   histogram_tester_.ExpectBucketCount(kUpdateHistogramName,
                                       ManifestUpdateResult::kAppUpdated, 1);
+  AwaitShortcutsUpdated(kAnotherInstallableIconTopLeftColor);
 }
 
 IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest,
@@ -620,6 +650,7 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest,
             ManifestUpdateResult::kAppUpdated);
   histogram_tester_.ExpectBucketCount(kUpdateHistogramName,
                                       ManifestUpdateResult::kAppUpdated, 1);
+  AwaitShortcutsUpdated(kInstallableIconTopLeftColor);
 
   // Policy installed apps should continue to be not uninstallable by the user
   // after updating.
@@ -646,6 +677,7 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest,
             ManifestUpdateResult::kAppUpdated);
   histogram_tester_.ExpectBucketCount(kUpdateHistogramName,
                                       ManifestUpdateResult::kAppUpdated, 1);
+  AwaitShortcutsUpdated(kInstallableIconTopLeftColor);
   EXPECT_EQ(GetProvider().registrar().GetAppScope(app_id),
             http_server_.GetURL("/"));
 }
@@ -669,6 +701,7 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest,
             ManifestUpdateResult::kAppUpdated);
   histogram_tester_.ExpectBucketCount(kUpdateHistogramName,
                                       ManifestUpdateResult::kAppUpdated, 1);
+  AwaitShortcutsUpdated(kInstallableIconTopLeftColor);
   EXPECT_EQ(GetProvider().registrar().GetAppDisplayMode(app_id),
             DisplayMode::kStandalone);
 }
@@ -734,6 +767,7 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest,
             ManifestUpdateResult::kAppUpdated);
   histogram_tester_.ExpectBucketCount(kUpdateHistogramName,
                                       ManifestUpdateResult::kAppUpdated, 1);
+  AwaitShortcutsUpdated(SK_ColorBLUE);
 
   // Check that the installed icon is now blue.
   base::RunLoop run_loop;

chrome/browser/web_applications/manifest_update_task.cc

@@ -228,6 +228,7 @@ void ManifestUpdateTask::OnAllAppWindowsClosed() {
   DCHECK(web_application_info_.has_value());
 
   // The app's name must not change due to an automatic update.
+  // TODO(crbug.com/1088338): Provide a safe way for apps to update their name.
   web_application_info_->title =
       base::UTF8ToUTF16(registrar_.GetAppShortName(app_id_));
 

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -354,7 +354,8 @@ void WebAppInstallFinalizer::FinalizeUpdate(
 
   CommitCallback commit_callback = base::BindOnce(
       &WebAppInstallFinalizer::OnDatabaseCommitCompletedForUpdate,
-      weak_ptr_factory_.GetWeakPtr(), std::move(callback), app_id);
+      weak_ptr_factory_.GetWeakPtr(), std::move(callback), app_id,
+      existing_web_app->name());
 
   SetWebAppManifestFieldsAndWriteData(web_app_info, std::move(web_app),
                                       std::move(commit_callback));
@@ -537,6 +538,7 @@ void WebAppInstallFinalizer::OnDatabaseCommitCompletedForInstall(
 void WebAppInstallFinalizer::OnDatabaseCommitCompletedForUpdate(
     InstallFinalizedCallback callback,
     AppId app_id,
+    std::string old_name,
     bool success) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!success) {
@@ -544,6 +546,7 @@ void WebAppInstallFinalizer::OnDatabaseCommitCompletedForUpdate(
     return;
   }
 
+  registrar().NotifyWebAppManifestUpdated(app_id, old_name);
   std::move(callback).Run(app_id, InstallResultCode::kSuccessAlreadyInstalled);
 }
 

chrome/browser/web_applications/web_app_install_finalizer.h

@@ -85,6 +85,7 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
                                            bool success);
   void OnDatabaseCommitCompletedForUpdate(InstallFinalizedCallback callback,
                                           AppId app_id,
+                                          std::string old_name,
                                           bool success);
   void OnFallbackInstallFinalized(const AppId& app_in_sync_install_id,
                                   InstallFinalizedCallback callback,