WebApps: Stop updating web app fields through sync

This CL updates the web app sync behaviour to ignore sync changes to
web app manifest fields.

This is in preparation for automatic local manifest updating which
may set device specific values so we must not sync these changes
to avoid creating sync wars. See:
https://chromium-review.googlesource.com/c/chromium/src/+/1811402

The existing implementation (check name & description) is from the
initial implementation of bookmark app sync:
https://chromium.googlesource.com/chromium/src/+/d93fcd51386a104ce54d9577ae0df8957d310c89
The fact that it doesn't sync for any of the other fields is likely
an oversight when those fields were added.

In the near future these synced fields will be used as fallback values
for the first sync install on a device, see:
https://chromium-review.googlesource.com/c/chromium/src/+/1858025
After the initial sync install these fields are no longer useful to us
as we will attempt to match the live site manifest rather than other
devices.

Bug: 926083
Change-Id: I146b3f4f1ddcd58b4766f69a12c3c0a61e6f495f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1737818Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Auto-Submit: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706697}

chrome/browser/extensions/extension_sync_service.cc

@@ -547,7 +547,7 @@ void ExtensionSyncService::ApplyBookmarkAppSyncData(
   auto* provider = web_app::WebAppProviderBase::GetProviderBase(profile_);
   DCHECK(provider);
 
-  provider->install_manager().InstallOrUpdateWebAppFromSync(
+  provider->install_manager().InstallWebAppFromSync(
       extension_sync_data.id(), std::move(web_app_info), base::DoNothing());
 }
 

chrome/browser/sync/test/integration/two_client_web_apps_sync_test.cc

@@ -22,6 +22,7 @@ class TwoClientWebAppsSyncTest : public SyncTest {
   ~TwoClientWebAppsSyncTest() override = default;
 
   AppId InstallApp(const WebApplicationInfo& info, Profile* profile) {
+    DCHECK(info.app_url.is_valid());
     base::RunLoop run_loop;
     AppId app_id;
 
@@ -148,4 +149,65 @@ IN_PROC_BROWSER_TEST_F(TwoClientWebAppsSyncTest, IsLocallyInstalled) {
   EXPECT_TRUE(AllProfilesHaveSameWebAppIds());
 }
 
+IN_PROC_BROWSER_TEST_F(TwoClientWebAppsSyncTest, AppFieldsChangeDoesNotSync) {
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(AllProfilesHaveSameWebAppIds());
+
+  const AppRegistrar& registrar0 = GetRegistrar(GetProfile(0));
+  const AppRegistrar& registrar1 = GetRegistrar(GetProfile(1));
+
+  WebApplicationInfo info_a;
+  info_a.title = base::UTF8ToUTF16("Test name A");
+  info_a.description = base::UTF8ToUTF16("Description A");
+  info_a.app_url = GURL("http://www.chromium.org/path/to/start_url");
+  info_a.scope = GURL("http://www.chromium.org/path/to/");
+  info_a.theme_color = SK_ColorBLUE;
+  AppId app_id_a = InstallApp(info_a, GetProfile(0));
+
+  EXPECT_EQ(WebAppInstallObserver(GetProfile(1)).AwaitNextInstall(), app_id_a);
+  EXPECT_EQ(base::UTF8ToUTF16(registrar1.GetAppShortName(app_id_a)),
+            info_a.title);
+  EXPECT_EQ(base::UTF8ToUTF16(registrar1.GetAppDescription(app_id_a)),
+            info_a.description);
+  EXPECT_EQ(registrar1.GetAppScope(app_id_a), info_a.scope);
+  EXPECT_EQ(registrar1.GetAppThemeColor(app_id_a), info_a.theme_color);
+  ASSERT_TRUE(AllProfilesHaveSameWebAppIds());
+
+  // Reinstall same app in Profile 0 with a different metadata aside from the
+  // name and start_url.
+  WebApplicationInfo info_b;
+  info_b.title = base::UTF8ToUTF16("Test name B");
+  info_b.description = base::UTF8ToUTF16("Description B");
+  info_b.app_url = GURL("http://www.chromium.org/path/to/start_url");
+  info_b.scope = GURL("http://www.chromium.org/path/to/");
+  info_b.theme_color = SK_ColorRED;
+  AppId app_id_b = InstallApp(info_b, GetProfile(0));
+  EXPECT_EQ(app_id_a, app_id_b);
+  EXPECT_EQ(base::UTF8ToUTF16(registrar0.GetAppShortName(app_id_a)),
+            info_b.title);
+  EXPECT_EQ(base::UTF8ToUTF16(registrar0.GetAppDescription(app_id_a)),
+            info_b.description);
+  EXPECT_EQ(registrar0.GetAppScope(app_id_a), info_b.scope);
+  EXPECT_EQ(registrar0.GetAppThemeColor(app_id_a), info_b.theme_color);
+
+  // Install a separate app just to have something to await on to ensure the
+  // sync has propagated to the other profile.
+  WebApplicationInfo infoC;
+  infoC.title = base::UTF8ToUTF16("Different test name");
+  infoC.app_url = GURL("http://www.notchromium.org/");
+  AppId app_id_c = InstallApp(infoC, GetProfile(0));
+  EXPECT_NE(app_id_a, app_id_c);
+  EXPECT_EQ(WebAppInstallObserver(GetProfile(1)).AwaitNextInstall(), app_id_c);
+
+  // After sync we should not see the metadata update in Profile 1.
+  EXPECT_EQ(base::UTF8ToUTF16(registrar1.GetAppShortName(app_id_a)),
+            info_a.title);
+  EXPECT_EQ(base::UTF8ToUTF16(registrar1.GetAppDescription(app_id_a)),
+            info_a.description);
+  EXPECT_EQ(registrar1.GetAppScope(app_id_a), info_a.scope);
+  EXPECT_EQ(registrar1.GetAppThemeColor(app_id_a), info_a.theme_color);
+
+  EXPECT_TRUE(AllProfilesHaveSameWebAppIds());
+}
+
 }  // namespace web_app

chrome/browser/web_applications/components/install_finalizer.h

@@ -71,10 +71,6 @@ class InstallFinalizer {
   virtual bool CanRevealAppShim() const = 0;
   virtual void RevealAppShim(const AppId& app_id) = 0;
 
-  virtual bool CanSkipAppUpdateForSync(
-      const AppId& app_id,
-      const WebApplicationInfo& web_app_info) const = 0;
-
   virtual bool CanUserUninstallFromSync(const AppId& app_id) const = 0;
 
   void SetSubsystems(AppRegistrar* registrar, WebAppUiManager* ui_manager);

chrome/browser/web_applications/components/install_manager.h

@@ -109,7 +109,7 @@ class InstallManager {
   // Starts background installation or an update of a web app from the sync
   // system. |web_application_info| contains received sync data. Icons will be
   // downloaded from the icon URLs provided in |web_application_info|.
-  virtual void InstallOrUpdateWebAppFromSync(
+  virtual void InstallWebAppFromSync(
       const AppId& app_id,
       std::unique_ptr<WebApplicationInfo> web_application_info,
       OnceInstallCallback callback) = 0;

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.cc

@@ -11,7 +11,6 @@
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/optional.h"
-#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/launch_util.h"
@@ -175,31 +174,6 @@ void BookmarkAppInstallFinalizer::RevealAppShim(const web_app::AppId& app_id) {
 #endif  // defined(OS_MACOSX)
 }
 
-bool BookmarkAppInstallFinalizer::CanSkipAppUpdateForSync(
-    const web_app::AppId& app_id,
-    const WebApplicationInfo& web_app_info) const {
-  ExtensionRegistry* extension_registry = ExtensionRegistry::Get(profile_);
-  DCHECK(extension_registry);
-
-  const Extension* extension =
-      extension_registry->GetInstalledExtension(app_id);
-  if (!extension)
-    return false;
-
-  // We can skip if there are no bookmark app details that need updating.
-  // TODO(loyso): We need to check more data fields. crbug.com/949427.
-  const std::string extension_sync_data_name =
-      base::UTF16ToUTF8(web_app_info.title);
-  const std::string bookmark_app_description =
-      base::UTF16ToUTF8(web_app_info.description);
-  if (extension->non_localized_name() == extension_sync_data_name &&
-      extension->description() == bookmark_app_description) {
-    return true;
-  }
-
-  return false;
-}
-
 bool BookmarkAppInstallFinalizer::CanUserUninstallFromSync(
     const web_app::AppId& app_id) const {
   const Extension* app = GetEnabledExtension(app_id);

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.h

@@ -45,9 +45,6 @@ class BookmarkAppInstallFinalizer : public web_app::InstallFinalizer {
                          CreateOsShortcutsCallback callback) override;
   bool CanRevealAppShim() const override;
   void RevealAppShim(const web_app::AppId& app_id) override;
-  bool CanSkipAppUpdateForSync(
-      const web_app::AppId& app_id,
-      const WebApplicationInfo& web_app_info) const override;
   bool CanUserUninstallFromSync(const web_app::AppId& app_id) const override;
 
   using CrxInstallerFactory =

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer_unittest.cc

@@ -368,43 +368,6 @@ TEST_F(BookmarkAppInstallFinalizerTest, NoNetworkInstallForArc) {
   run_loop.Run();
 }
 
-TEST_F(BookmarkAppInstallFinalizerTest, CanSkipAppUpdateForSync) {
-  auto info = std::make_unique<WebApplicationInfo>();
-  info->app_url = kWebAppUrl;
-  info->title = base::ASCIIToUTF16("Title1");
-  info->description = base::ASCIIToUTF16("Description1");
-
-  const web_app::AppId app_id = web_app::GenerateAppIdFromURL(info->app_url);
-
-  EXPECT_FALSE(finalizer().CanSkipAppUpdateForSync(app_id, *info));
-
-  base::RunLoop run_loop;
-  web_app::InstallFinalizer::FinalizeOptions options;
-  options.install_source = WebappInstallSource::SYNC;
-
-  finalizer().FinalizeInstall(
-      *info, options,
-      base::BindLambdaForTesting([&](const web_app::AppId& installed_app_id,
-                                     web_app::InstallResultCode code) {
-        EXPECT_EQ(web_app::InstallResultCode::kSuccessNewInstall, code);
-        EXPECT_EQ(app_id, installed_app_id);
-        run_loop.Quit();
-      }));
-  run_loop.Run();
-
-  EXPECT_TRUE(finalizer().CanSkipAppUpdateForSync(app_id, *info));
-
-  WebApplicationInfo info_with_diff_title = *info;
-  info_with_diff_title.title = base::ASCIIToUTF16("Title2");
-  EXPECT_FALSE(
-      finalizer().CanSkipAppUpdateForSync(app_id, info_with_diff_title));
-
-  WebApplicationInfo info_with_diff_description = *info;
-  info_with_diff_description.title = base::ASCIIToUTF16("Description2");
-  EXPECT_FALSE(
-      finalizer().CanSkipAppUpdateForSync(app_id, info_with_diff_description));
-}
-
 TEST_F(BookmarkAppInstallFinalizerTest, UninstallExternalWebApp_Successful) {
   InstallExternalApp(kWebAppUrl);
   ASSERT_EQ(1u, enabled_extensions().size());

chrome/browser/web_applications/extensions/install_manager_bookmark_app_unittest.cc

@@ -635,7 +635,7 @@ TEST_F(InstallManagerBookmarkAppTest, CreateWebAppFromInfo) {
                    .empty());
 }
 
-TEST_F(InstallManagerBookmarkAppTest, InstallOrUpdateWebAppFromSync) {
+TEST_F(InstallManagerBookmarkAppTest, InstallWebAppFromSync) {
   CreateEmptyDataRetriever();
 
   EXPECT_EQ(0u, registry()->enabled_extensions().size());
@@ -663,7 +663,7 @@ TEST_F(InstallManagerBookmarkAppTest, InstallOrUpdateWebAppFromSync) {
   {
     base::RunLoop run_loop;
 
-    provider->install_manager().InstallOrUpdateWebAppFromSync(
+    provider->install_manager().InstallWebAppFromSync(
         app_id, std::move(web_app_info),
         base::BindLambdaForTesting([&](const web_app::AppId& installed_app_id,
                                        web_app::InstallResultCode code) {
@@ -709,11 +709,11 @@ TEST_F(InstallManagerBookmarkAppTest, InstallOrUpdateWebAppFromSync) {
   {
     base::RunLoop run_loop;
 
-    provider->install_manager().InstallOrUpdateWebAppFromSync(
+    provider->install_manager().InstallWebAppFromSync(
         app_id, std::move(web_app_info2),
         base::BindLambdaForTesting([&](const web_app::AppId& installed_app_id,
                                        web_app::InstallResultCode code) {
-          EXPECT_EQ(web_app::InstallResultCode::kSuccessNewInstall, code);
+          EXPECT_EQ(web_app::InstallResultCode::kSuccessAlreadyInstalled, code);
           EXPECT_EQ(app_id, installed_app_id);
           run_loop.Quit();
         }));
@@ -722,15 +722,16 @@ TEST_F(InstallManagerBookmarkAppTest, InstallOrUpdateWebAppFromSync) {
   }
 
   {
+    // New fields from sync are not deployed as they are now managed by the
+    // ManifestUpdateManager.
     EXPECT_EQ(1u, registry()->enabled_extensions().size());
     const Extension* extension =
         registry()->enabled_extensions().begin()->get();
     EXPECT_TRUE(extension->from_bookmark());
-    EXPECT_EQ(kAlternativeAppTitle, extension->name());
+    EXPECT_EQ(kAppTitle, extension->name());
     EXPECT_EQ(kAppDescription, extension->description());
     EXPECT_EQ(kAppUrl, AppLaunchInfo::GetLaunchWebURL(extension));
-    EXPECT_EQ(GURL(kAppAlternativeScope),
-              GetScopeURLFromBookmarkApp(extension));
+    EXPECT_EQ(GURL(kAppScope), GetScopeURLFromBookmarkApp(extension));
     EXPECT_FALSE(extensions::IconsInfo::GetIconResource(
                      extension, kIconSizeSmall, ExtensionIconSet::MATCH_EXACTLY)
                      .empty());

chrome/browser/web_applications/extensions/pending_app_install_task_unittest.cc

@@ -239,13 +239,6 @@ class TestPendingAppInstallFinalizer : public InstallFinalizer {
     ++num_reveal_appshim_calls_;
   }
 
-  bool CanSkipAppUpdateForSync(
-      const AppId& app_id,
-      const WebApplicationInfo& web_app_info) const override {
-    NOTIMPLEMENTED();
-    return true;
-  }
-
   bool CanUserUninstallFromSync(const AppId& app_id) const override {
     NOTIMPLEMENTED();
     return false;

chrome/browser/web_applications/test/test_install_finalizer.cc

@@ -109,12 +109,6 @@ void TestInstallFinalizer::RevealAppShim(const AppId& app_id) {
   ++num_reveal_appshim_calls_;
 }
 
-bool TestInstallFinalizer::CanSkipAppUpdateForSync(
-    const AppId& app_id,
-    const WebApplicationInfo& web_app_info) const {
-  return next_can_skip_app_update_for_sync_;
-}
-
 bool TestInstallFinalizer::CanUserUninstallFromSync(const AppId& app_id) const {
   NOTIMPLEMENTED();
   return false;
@@ -134,9 +128,4 @@ void TestInstallFinalizer::SetNextUninstallExternalWebAppResult(
   next_uninstall_external_web_app_results_[app_url] = uninstalled;
 }
 
-void TestInstallFinalizer::SetNextCanSkipAppUpdateForSync(
-    bool can_skip_app_update_for_sync) {
-  next_can_skip_app_update_for_sync_ = can_skip_app_update_for_sync;
-}
-
 }  // namespace web_app

chrome/browser/web_applications/test/test_install_finalizer.h

@@ -45,16 +45,12 @@ class TestInstallFinalizer final : public InstallFinalizer {
                    content::WebContents* web_contents) override;
   bool CanRevealAppShim() const override;
   void RevealAppShim(const AppId& app_id) override;
-  bool CanSkipAppUpdateForSync(
-      const AppId& app_id,
-      const WebApplicationInfo& web_app_info) const override;
   bool CanUserUninstallFromSync(const AppId& app_id) const override;
 
   void SetNextFinalizeInstallResult(const AppId& app_id,
                                     InstallResultCode code);
   void SetNextUninstallExternalWebAppResult(const GURL& app_url,
                                             bool uninstalled);
-  void SetNextCanSkipAppUpdateForSync(bool can_skip_app_update_for_sync);
 
   std::unique_ptr<WebApplicationInfo> web_app_info() {
     return std::move(web_app_info_copy_);
@@ -83,7 +79,6 @@ class TestInstallFinalizer final : public InstallFinalizer {
   base::Optional<AppId> next_app_id_;
   base::Optional<InstallResultCode> next_result_code_;
   std::map<GURL, bool> next_uninstall_external_web_app_results_;
-  bool next_can_skip_app_update_for_sync_ = false;
 
   int num_create_os_shortcuts_calls_ = 0;
   int num_reparent_tab_calls_ = 0;

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -221,13 +221,6 @@ void WebAppInstallFinalizer::RevealAppShim(const AppId& app_id) {
   NOTIMPLEMENTED();
 }
 
-bool WebAppInstallFinalizer::CanSkipAppUpdateForSync(
-    const AppId& app_id,
-    const WebApplicationInfo& web_app_info) const {
-  NOTIMPLEMENTED();
-  return true;
-}
-
 bool WebAppInstallFinalizer::CanUserUninstallFromSync(
     const AppId& app_id) const {
   // TODO(crbug.com/901226): Implement it.

chrome/browser/web_applications/web_app_install_finalizer.h

@@ -38,9 +38,6 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
                          CreateOsShortcutsCallback callback) override;
   bool CanRevealAppShim() const override;
   void RevealAppShim(const AppId& app_id) override;
-  bool CanSkipAppUpdateForSync(
-      const AppId& app_id,
-      const WebApplicationInfo& web_app_info) const override;
   bool CanUserUninstallFromSync(const AppId& app_id) const override;
 
  private:

chrome/browser/web_applications/web_app_install_manager.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/install_finalizer.h"
@@ -100,11 +101,14 @@ void WebAppInstallManager::InstallWebAppWithParams(
   tasks_.insert(std::move(task));
 }
 
-void WebAppInstallManager::InstallOrUpdateWebAppFromSync(
+void WebAppInstallManager::InstallWebAppFromSync(
     const AppId& app_id,
     std::unique_ptr<WebApplicationInfo> web_application_info,
     OnceInstallCallback callback) {
-  if (finalizer()->CanSkipAppUpdateForSync(app_id, *web_application_info)) {
+  // Skip sync update if app exists.
+  // All manifest fields will be set locally via update (see crbug.com/926083)
+  // so we must not sync them in order to avoid a device-to-device sync war.
+  if (registrar()->IsInstalled(app_id)) {
     std::move(callback).Run(app_id,
                             InstallResultCode::kSuccessAlreadyInstalled);
     return;

chrome/browser/web_applications/web_app_install_manager.h

@@ -57,7 +57,7 @@ class WebAppInstallManager final : public InstallManager,
                                const InstallParams& install_params,
                                WebappInstallSource install_source,
                                OnceInstallCallback callback) override;
-  void InstallOrUpdateWebAppFromSync(
+  void InstallWebAppFromSync(
       const AppId& app_id,
       std::unique_ptr<WebApplicationInfo> web_application_info,
       OnceInstallCallback callback) override;

chrome/browser/web_applications/web_app_install_manager_unittest.cc

@@ -87,7 +87,7 @@ class WebAppInstallManagerTest : public WebAppTest {
 };
 
 TEST_F(WebAppInstallManagerTest,
-       InstallOrUpdateWebAppFromSync_TwoConcurrentInstallsAreRunInOrder) {
+       InstallWebAppFromSync_TwoConcurrentInstallsAreRunInOrder) {
   const GURL url1{"https://example.com/path"};
   const AppId app1_id = GenerateAppIdFromURL(url1);
 
@@ -154,7 +154,7 @@ TEST_F(WebAppInstallManagerTest,
   EXPECT_FALSE(install_manager().has_web_contents_for_testing());
 
   // Enqueue a request to install the 1st app.
-  install_manager().InstallOrUpdateWebAppFromSync(
+  install_manager().InstallWebAppFromSync(
       app1_id, CreateWebAppInfo(url1),
       base::BindLambdaForTesting(
           [&](const AppId& installed_app_id, InstallResultCode code) {
@@ -170,7 +170,7 @@ TEST_F(WebAppInstallManagerTest,
 
   // Immediately enqueue a request to install the 2nd app, WebContents is not
   // ready.
-  install_manager().InstallOrUpdateWebAppFromSync(
+  install_manager().InstallWebAppFromSync(
       app2_id, CreateWebAppInfo(url2),
       base::BindLambdaForTesting(
           [&](const AppId& installed_app_id, InstallResultCode code) {
@@ -206,7 +206,7 @@ TEST_F(WebAppInstallManagerTest,
 }
 
 TEST_F(WebAppInstallManagerTest,
-       InstallOrUpdateWebAppFromSync_InstallManagerDestroyed) {
+       InstallWebAppFromSync_InstallManagerDestroyed) {
   const GURL app_url("https://example.com/path");
   const AppId app_id = GenerateAppIdFromURL(app_url);
   NavigateAndCommit(app_url);
@@ -232,7 +232,7 @@ TEST_F(WebAppInstallManagerTest,
         return std::unique_ptr<WebAppDataRetriever>(std::move(data_retriever));
       }));
 
-  install_manager().InstallOrUpdateWebAppFromSync(
+  install_manager().InstallWebAppFromSync(
       app_id, CreateWebAppInfo(app_url),
       base::BindLambdaForTesting([](const web_app::AppId& installed_app_id,
                                     web_app::InstallResultCode code) {
@@ -248,24 +248,4 @@ TEST_F(WebAppInstallManagerTest,
   DestroyManagers();
 }
 
-TEST_F(WebAppInstallManagerTest,
-       InstallOrUpdateWebAppFromSync_CanSkipAppUpdateForSync) {
-  const GURL app_url("https://example.com/path");
-  const AppId app_id = GenerateAppIdFromURL(app_url);
-  NavigateAndCommit(app_url);
-
-  finalizer().SetNextCanSkipAppUpdateForSync(true);
-
-  base::RunLoop run_loop;
-  install_manager().InstallOrUpdateWebAppFromSync(
-      app_id, CreateWebAppInfo(app_url),
-      base::BindLambdaForTesting([&](const web_app::AppId& installed_app_id,
-                                     web_app::InstallResultCode code) {
-        EXPECT_EQ(InstallResultCode::kSuccessAlreadyInstalled, code);
-        EXPECT_EQ(app_id, installed_app_id);
-        run_loop.Quit();
-      }));
-  run_loop.Run();
-}
-
 }  // namespace web_app