WebApp: Extract InstallFinalizer::CanSkipAppUpdateForSync method.

Plumb InstallFinalizer to BookmarkAppInstallManager.

CanSkipAppUpdateForSync will be reused in WebAppInstallManager.

Bug: 915043
Change-Id: Ied37333203e3d972accecde5bd8ee3d157862959
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1585744
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654792}

chrome/browser/web_applications/bookmark_apps/bookmark_app_install_manager.cc

@@ -10,13 +10,13 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/scoped_observer.h"
-#include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/extensions/bookmark_app_helper.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/installable/installable_metrics.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/web_applications/components/install_finalizer.h"
 #include "chrome/browser/web_applications/components/install_manager_observer.h"
 #include "chrome/browser/web_applications/components/install_options.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
@@ -26,7 +26,6 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "extensions/browser/extension_system.h"
-#include "extensions/common/extension.h"
 
 namespace extensions {
 
@@ -252,8 +251,10 @@ void OnGetWebApplicationInfo(const BookmarkAppInstallManager* install_manager,
 
 }  // namespace
 
-BookmarkAppInstallManager::BookmarkAppInstallManager(Profile* profile)
-    : InstallManager(profile) {
+BookmarkAppInstallManager::BookmarkAppInstallManager(
+    Profile* profile,
+    web_app::InstallFinalizer* finalizer)
+    : InstallManager(profile), finalizer_(finalizer) {
   bookmark_app_helper_factory_ = base::BindRepeating(
       [](Profile* profile, const WebApplicationInfo& web_app_info,
          content::WebContents* web_contents,
@@ -380,23 +381,14 @@ void BookmarkAppInstallManager::InstallOrUpdateWebAppFromSync(
       ExtensionSystem::Get(profile())->extension_service();
   DCHECK(extension_service);
 
-  const Extension* extension = extension_service->GetInstalledExtension(app_id);
-
-  // Return if there are no bookmark app details that need updating.
-  const std::string extension_sync_data_name =
-      base::UTF16ToUTF8(web_application_info->title);
-  const std::string bookmark_app_description =
-      base::UTF16ToUTF8(web_application_info->description);
-  if (extension &&
-      extension->non_localized_name() == extension_sync_data_name &&
-      extension->description() == bookmark_app_description) {
+  if (finalizer_->CanSkipAppUpdateForSync(app_id, *web_application_info))
     return;
-  }
 
 #if defined(OS_CHROMEOS)
-  const bool is_locally_installed = true;
+  bool is_locally_installed = true;
 #else
-  const bool is_locally_installed = extension != nullptr;
+  bool is_locally_installed =
+      extension_service->GetInstalledExtension(app_id) != nullptr;
 #endif
 
   CreateOrUpdateBookmarkApp(extension_service, web_application_info.get(),

chrome/browser/web_applications/bookmark_apps/bookmark_app_install_manager.h

@@ -20,6 +20,7 @@ class WebContents;
 }
 
 namespace web_app {
+class InstallFinalizer;
 class WebAppDataRetriever;
 }
 
@@ -31,7 +32,8 @@ class BookmarkAppHelper;
 // crbug.com/915043.
 class BookmarkAppInstallManager final : public web_app::InstallManager {
  public:
-  explicit BookmarkAppInstallManager(Profile* profile);
+  BookmarkAppInstallManager(Profile* profile,
+                            web_app::InstallFinalizer* finalizer);
   ~BookmarkAppInstallManager() override;
 
   // InstallManager:
@@ -83,6 +85,7 @@ class BookmarkAppInstallManager final : public web_app::InstallManager {
  private:
   BookmarkAppHelperFactory bookmark_app_helper_factory_;
   DataRetrieverFactory data_retriever_factory_;
+  web_app::InstallFinalizer* finalizer_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkAppInstallManager);
 };

chrome/browser/web_applications/bookmark_apps/bookmark_app_install_manager_unittest.cc

@@ -19,6 +19,7 @@
 #include "chrome/browser/web_applications/components/install_options.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/test/test_data_retriever.h"
+#include "chrome/browser/web_applications/test/test_install_finalizer.h"
 #include "chrome/common/web_application_info.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
@@ -33,7 +34,9 @@ class BookmarkAppInstallManagerTest : public ChromeRenderViewHostTestHarness {
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
 
-    install_manager_ = std::make_unique<BookmarkAppInstallManager>(profile());
+    install_finalizer_ = std::make_unique<web_app::TestInstallFinalizer>();
+    install_manager_ = std::make_unique<BookmarkAppInstallManager>(
+        profile(), install_finalizer_.get());
 
     extensions::TestExtensionSystem* test_extension_system =
         static_cast<extensions::TestExtensionSystem*>(
@@ -69,6 +72,7 @@ class BookmarkAppInstallManagerTest : public ChromeRenderViewHostTestHarness {
   }
 
  protected:
+  std::unique_ptr<web_app::TestInstallFinalizer> install_finalizer_;
   std::unique_ptr<BookmarkAppInstallManager> install_manager_;
 };
 

chrome/browser/web_applications/components/install_finalizer.h

@@ -60,6 +60,10 @@ 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 ~InstallFinalizer() = default;
 };
 

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.cc

@@ -9,8 +9,10 @@
 
 #include "base/bind.h"
 #include "base/optional.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/bookmark_app_extension_util.h"
 #include "chrome/browser/extensions/crx_installer.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
@@ -19,6 +21,7 @@
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/web_application_info.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
 #include "extensions/browser/install/crx_install_error.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_id.h"
@@ -165,6 +168,31 @@ void BookmarkAppInstallFinalizer::RevealAppShim(const web_app::AppId& app_id) {
   BookmarkAppRevealAppShim(profile_, app);
 }
 
+bool BookmarkAppInstallFinalizer::CanSkipAppUpdateForSync(
+    const web_app::AppId& app_id,
+    const WebApplicationInfo& web_app_info) const {
+  ExtensionService* extension_service =
+      ExtensionSystem::Get(profile_)->extension_service();
+  DCHECK(extension_service);
+
+  const Extension* extension = extension_service->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;
+}
+
 void BookmarkAppInstallFinalizer::SetCrxInstallerFactoryForTesting(
     CrxInstallerFactory crx_installer_factory) {
   crx_installer_factory_ = crx_installer_factory;

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.h

@@ -41,6 +41,9 @@ class BookmarkAppInstallFinalizer : public web_app::InstallFinalizer {
                    content::WebContents* web_contents) 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;
 
   using CrxInstallerFactory =
       base::RepeatingCallback<scoped_refptr<CrxInstaller>(Profile*)>;

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer_unittest.cc

@@ -297,4 +297,41 @@ TEST_F(BookmarkAppInstallFinalizerTest, ForceLaunchContainer) {
   run_loop.Run();
 }
 
+TEST_F(BookmarkAppInstallFinalizerTest, CanSkipAppUpdateForSync) {
+  BookmarkAppInstallFinalizer installer(profile());
+
+  auto info = std::make_unique<WebApplicationInfo>();
+  info->app_url = GURL(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(installer.CanSkipAppUpdateForSync(app_id, *info));
+
+  base::RunLoop run_loop;
+  web_app::InstallFinalizer::FinalizeOptions options;
+
+  installer.FinalizeInstall(
+      *info, options,
+      base::BindLambdaForTesting([&](const web_app::AppId& installed_app_id,
+                                     web_app::InstallResultCode code) {
+        EXPECT_EQ(web_app::InstallResultCode::kSuccess, code);
+        EXPECT_EQ(app_id, installed_app_id);
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+
+  EXPECT_TRUE(installer.CanSkipAppUpdateForSync(app_id, *info));
+
+  WebApplicationInfo info_with_diff_title = *info;
+  info_with_diff_title.title = base::ASCIIToUTF16("Title2");
+  EXPECT_FALSE(installer.CanSkipAppUpdateForSync(app_id, info_with_diff_title));
+
+  WebApplicationInfo info_with_diff_description = *info;
+  info_with_diff_description.title = base::ASCIIToUTF16("Description2");
+  EXPECT_FALSE(
+      installer.CanSkipAppUpdateForSync(app_id, info_with_diff_description));
+}
+
 }  // namespace extensions

chrome/browser/web_applications/test/test_install_finalizer.cc

@@ -82,6 +82,12 @@ 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 false;
+}
+
 void TestInstallFinalizer::SetNextFinalizeInstallResult(
     const AppId& app_id,
     InstallResultCode code) {

chrome/browser/web_applications/test/test_install_finalizer.h

@@ -36,6 +36,9 @@ 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;
 
   void SetNextFinalizeInstallResult(const AppId& app_id,
                                     InstallResultCode code);

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -149,4 +149,11 @@ void WebAppInstallFinalizer::RevealAppShim(const AppId& app_id) {
   NOTIMPLEMENTED();
 }
 
+bool WebAppInstallFinalizer::CanSkipAppUpdateForSync(
+    const AppId& app_id,
+    const WebApplicationInfo& web_app_info) const {
+  NOTIMPLEMENTED();
+  return true;
+}
+
 }  // namespace web_app

chrome/browser/web_applications/web_app_install_finalizer.h

@@ -41,6 +41,9 @@ class WebAppInstallFinalizer 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;
 
  private:
   void OnDataWritten(InstallFinalizedCallback callback,

chrome/browser/web_applications/web_app_provider.cc

@@ -130,8 +130,8 @@ void WebAppProvider::CreateBookmarkAppsSubsystems(Profile* profile) {
     install_manager_ = std::make_unique<WebAppInstallManager>(
         profile, install_finalizer_.get());
   } else {
-    install_manager_ =
-        std::make_unique<extensions::BookmarkAppInstallManager>(profile);
+    install_manager_ = std::make_unique<extensions::BookmarkAppInstallManager>(
+        profile, install_finalizer_.get());
   }
 
   pending_app_manager_ =