WebApp: Extract RevealAppShim util and pure virtual function.

Refactor ReparentTab util.

ReparentTab and RevealAppShim are unrelated steps.

Bug: 915043
Change-Id: I37a1f463bf06e41e91c4bb0229a125af11903bd9
Reviewed-on: https://chromium-review.googlesource.com/c/1482432Reviewed-by: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635419}

chrome/browser/extensions/bookmark_app_extension_util.cc

@@ -50,25 +50,34 @@ void BookmarkAppCreateOsShortcuts(Profile* profile,
 #endif  // !defined(OS_CHROMEOS)
 }
 
-void BookmarkAppReparentTab(Profile* profile,
-                            content::WebContents* contents,
-                            const Extension* extension,
-                            LaunchType launch_type) {
-#if defined(OS_MACOSX)
+void BookmarkAppReparentTab(content::WebContents* contents,
+                            const Extension* extension) {
+#if !defined(OS_MACOSX)
+  // Reparent the tab into an app window immediately when opening as a window.
   // TODO(https://crbug.com/915571): Reparent the tab on Mac just like the
   // other platforms.
+  if (base::FeatureList::IsEnabled(::features::kDesktopPWAWindowing))
+    ReparentWebContentsIntoAppBrowser(contents, extension);
+#endif  // !defined(OS_MACOSX)
+}
+
+bool CanBookmarkAppRevealAppShim() {
+#if defined(OS_MACOSX)
+  return true;
+#else   // defined(OS_MACOSX)
+  return false;
+#endif  // !defined(OS_MACOSX)
+}
+
+void BookmarkAppRevealAppShim(Profile* profile, const Extension* extension) {
+  DCHECK(CanBookmarkAppRevealAppShim());
+#if defined(OS_MACOSX)
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
           ::switches::kDisableHostedAppShimCreation)) {
     Profile* current_profile = profile->GetOriginalProfile();
     web_app::RevealAppShimInFinderForApp(current_profile, extension);
   }
-#else
-  // Reparent the tab into an app window immediately when opening as a window.
-  if (base::FeatureList::IsEnabled(::features::kDesktopPWAWindowing) &&
-      launch_type == LAUNCH_TYPE_WINDOW && !profile->IsOffTheRecord()) {
-    ReparentWebContentsIntoAppBrowser(contents, extension);
-  }
-#endif  // !defined(OS_MACOSX)
+#endif  // defined(OS_MACOSX)
 }
 
 }  // namespace extensions

chrome/browser/extensions/bookmark_app_extension_util.h

@@ -5,8 +5,6 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_BOOKMARK_APP_EXTENSION_UTIL_H_
 #define CHROME_BROWSER_EXTENSIONS_BOOKMARK_APP_EXTENSION_UTIL_H_
 
-#include "extensions/common/constants.h"
-
 class Profile;
 
 namespace content {
@@ -19,10 +17,13 @@ class Extension;
 
 void BookmarkAppCreateOsShortcuts(Profile* profile, const Extension* extension);
 
-void BookmarkAppReparentTab(Profile* profile,
-                            content::WebContents* contents,
-                            const Extension* extension,
-                            LaunchType launch_type);
+void BookmarkAppReparentTab(content::WebContents* contents,
+                            const Extension* extension);
+
+// Returns true if OS supports AppShim revealing,
+bool CanBookmarkAppRevealAppShim();
+// Reveals AppShim in Finder for a given app,
+void BookmarkAppRevealAppShim(Profile* profile, const Extension* extension);
 
 }  // namespace extensions
 

chrome/browser/extensions/bookmark_app_helper.cc

@@ -479,8 +479,12 @@ void BookmarkAppHelper::FinishInstallation(const Extension* extension) {
       (chrome::FindBrowserWithWebContents(contents_) != nullptr);
   // TODO(loyso): Reparenting must be implemented in
   // chrome/browser/ui/web_applications/ UI layer as a post-install step.
-  if (reparent_tab)
-    BookmarkAppReparentTab(profile_, contents_, extension, launch_type);
+  if (reparent_tab) {
+    DCHECK(!profile_->IsOffTheRecord());
+    BookmarkAppReparentTab(contents_, extension);
+    if (CanBookmarkAppRevealAppShim())
+      BookmarkAppRevealAppShim(profile_, extension);
+  }
 
   callback_.Run(extension, web_app_info_);
 }

chrome/browser/web_applications/components/install_finalizer.h

@@ -33,9 +33,10 @@ class InstallFinalizer {
                                InstallFinalizedCallback callback) = 0;
 
   virtual void CreateOsShortcuts(const AppId& app_id) = 0;
-  virtual void ReparentTab(const WebApplicationInfo& web_app_info,
-                           const AppId& app_id,
+  virtual void ReparentTab(const AppId& app_id,
                            content::WebContents* web_contents) = 0;
+  virtual bool CanRevealAppShim() const = 0;
+  virtual void RevealAppShim(const AppId& app_id) = 0;
 
   virtual ~InstallFinalizer() = default;
 };

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.cc

@@ -102,13 +102,21 @@ void BookmarkAppInstallFinalizer::CreateOsShortcuts(
 }
 
 void BookmarkAppInstallFinalizer::ReparentTab(
-    const WebApplicationInfo& web_app_info,
     const web_app::AppId& app_id,
     content::WebContents* web_contents) {
   DCHECK(web_contents);
+  DCHECK(!profile_->IsOffTheRecord());
+  const Extension* app = GetExtensionById(profile_, app_id);
+  BookmarkAppReparentTab(web_contents, app);
+}
+
+bool BookmarkAppInstallFinalizer::CanRevealAppShim() const {
+  return CanBookmarkAppRevealAppShim();
+}
+
+void BookmarkAppInstallFinalizer::RevealAppShim(const web_app::AppId& app_id) {
   const Extension* app = GetExtensionById(profile_, app_id);
-  const LaunchType launch_type = GetLaunchType(web_app_info);
-  BookmarkAppReparentTab(profile_, web_contents, app, launch_type);
+  BookmarkAppRevealAppShim(profile_, app);
 }
 
 }  // namespace extensions

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.h

@@ -36,9 +36,10 @@ class BookmarkAppInstallFinalizer : public web_app::InstallFinalizer {
   void FinalizeInstall(const WebApplicationInfo& web_app_info,
                        InstallFinalizedCallback callback) override;
   void CreateOsShortcuts(const web_app::AppId& app_id) override;
-  void ReparentTab(const WebApplicationInfo& web_app_info,
-                   const web_app::AppId& app_id,
+  void ReparentTab(const web_app::AppId& app_id,
                    content::WebContents* web_contents) override;
+  bool CanRevealAppShim() const override;
+  void RevealAppShim(const web_app::AppId& app_id) override;
 
   void SetCrxInstallerForTesting(scoped_refptr<CrxInstaller> crx_installer);
 

chrome/browser/web_applications/test/test_install_finalizer.cc

@@ -44,10 +44,17 @@ void TestInstallFinalizer::CreateOsShortcuts(const AppId& app_id) {
   ++num_create_os_shortcuts_calls_;
 }
 
-void TestInstallFinalizer::ReparentTab(const WebApplicationInfo& web_app_info,
-                                       const AppId& app_id,
+void TestInstallFinalizer::ReparentTab(const AppId& app_id,
                                        content::WebContents* web_contents) {
-  ++num_reparent_tab_num_calls_;
+  ++num_reparent_tab_calls_;
+}
+
+bool TestInstallFinalizer::CanRevealAppShim() const {
+  return true;
+}
+
+void TestInstallFinalizer::RevealAppShim(const AppId& app_id) {
+  ++num_reveal_appshim_calls_;
 }
 
 void TestInstallFinalizer::SetNextFinalizeInstallResult(

chrome/browser/web_applications/test/test_install_finalizer.h

@@ -24,9 +24,10 @@ class TestInstallFinalizer final : public InstallFinalizer {
   void FinalizeInstall(const WebApplicationInfo& web_app_info,
                        InstallFinalizedCallback callback) override;
   void CreateOsShortcuts(const AppId& app_id) override;
-  void ReparentTab(const WebApplicationInfo& web_app_info,
-                   const AppId& app_id,
+  void ReparentTab(const AppId& app_id,
                    content::WebContents* web_contents) override;
+  bool CanRevealAppShim() const override;
+  void RevealAppShim(const AppId& app_id) override;
 
   void SetNextFinalizeInstallResult(const AppId& app_id,
                                     InstallResultCode code);
@@ -36,7 +37,8 @@ class TestInstallFinalizer final : public InstallFinalizer {
   }
 
   int num_create_os_shortcuts_calls() { return num_create_os_shortcuts_calls_; }
-  int num_reparent_tab_num_calls() { return num_reparent_tab_num_calls_; }
+  int num_reparent_tab_calls() { return num_reparent_tab_calls_; }
+  int num_reveal_appshim_calls() { return num_reveal_appshim_calls_; }
 
  private:
   std::unique_ptr<WebApplicationInfo> web_app_info_copy_;
@@ -45,7 +47,8 @@ class TestInstallFinalizer final : public InstallFinalizer {
   base::Optional<InstallResultCode> next_result_code_;
 
   int num_create_os_shortcuts_calls_ = 0;
-  int num_reparent_tab_num_calls_ = 0;
+  int num_reparent_tab_calls_ = 0;
+  int num_reveal_appshim_calls_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(TestInstallFinalizer);
 };

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -101,11 +101,21 @@ void WebAppInstallFinalizer::CreateOsShortcuts(const AppId& app_id) {
   NOTIMPLEMENTED();
 }
 
-void WebAppInstallFinalizer::ReparentTab(const WebApplicationInfo& web_app_info,
-                                         const AppId& app_id,
+void WebAppInstallFinalizer::ReparentTab(const AppId& app_id,
                                          content::WebContents* web_contents) {
   // TODO(loyso): Implement it.
   NOTIMPLEMENTED();
 }
 
+bool WebAppInstallFinalizer::CanRevealAppShim() const {
+  // TODO(loyso): Implement it.
+  NOTIMPLEMENTED();
+  return false;
+}
+
+void WebAppInstallFinalizer::RevealAppShim(const AppId& app_id) {
+  // TODO(loyso): Implement it.
+  NOTIMPLEMENTED();
+}
+
 }  // namespace web_app

chrome/browser/web_applications/web_app_install_finalizer.h

@@ -29,9 +29,10 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
   void FinalizeInstall(const WebApplicationInfo& web_app_info,
                        InstallFinalizedCallback callback) override;
   void CreateOsShortcuts(const AppId& app_id) override;
-  void ReparentTab(const WebApplicationInfo& web_app_info,
-                   const AppId& app_id,
+  void ReparentTab(const AppId& app_id,
                    content::WebContents* web_contents) override;
+  bool CanRevealAppShim() const override;
+  void RevealAppShim(const AppId& app_id) override;
 
  private:
   void OnDataWritten(InstallFinalizedCallback callback,

chrome/browser/web_applications/web_app_install_manager.cc

@@ -201,8 +201,7 @@ void WebAppInstallManager::OnDialogCompleted(
   install_finalizer_->FinalizeInstall(
       web_app_info_copy,
       base::BindOnce(&WebAppInstallManager::OnInstallFinalized,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(web_app_info),
-                     for_installable_site));
+                     weak_ptr_factory_.GetWeakPtr(), std::move(web_app_info)));
 
   // Check that the finalizer hasn't called OnInstallFinalized synchronously:
   DCHECK(install_callback_);
@@ -210,7 +209,6 @@ void WebAppInstallManager::OnDialogCompleted(
 
 void WebAppInstallManager::OnInstallFinalized(
     std::unique_ptr<WebApplicationInfo> web_app_info,
-    ForInstallableSite for_installable_site,
     const AppId& app_id,
     InstallResultCode code) {
   if (InstallInterrupted())
@@ -222,7 +220,11 @@ void WebAppInstallManager::OnInstallFinalized(
     // TODO(loyso): Implement |create_shortcuts| to skip OS shortcuts creation.
     install_finalizer_->CreateOsShortcuts(app_id);
     // TODO(loyso): Implement |reparent_tab| to skip tab reparenting logic.
-    install_finalizer_->ReparentTab(*web_app_info, app_id, web_contents());
+    if (web_app_info->open_as_window)
+      install_finalizer_->ReparentTab(app_id, web_contents());
+
+    if (install_finalizer_->CanRevealAppShim())
+      install_finalizer_->RevealAppShim(app_id);
   }
 
   CallInstallCallback(app_id, code);

chrome/browser/web_applications/web_app_install_manager.h

@@ -77,7 +77,6 @@ class WebAppInstallManager final : public InstallManager,
                          bool user_accepted,
                          std::unique_ptr<WebApplicationInfo> web_app_info);
   void OnInstallFinalized(std::unique_ptr<WebApplicationInfo> web_app_info,
-                          ForInstallableSite for_installable_site,
                           const AppId& app_id,
                           InstallResultCode code);
 

chrome/browser/web_applications/web_app_install_manager_unittest.cc

@@ -131,6 +131,7 @@ class WebAppInstallManagerTest : public WebAppTest {
     web_app_info->description = base::UTF8ToUTF16(description);
     web_app_info->scope = scope;
     web_app_info->theme_color = theme_color;
+    web_app_info->open_as_window = true;
 
     auto data_retriever =
         std::make_unique<TestDataRetriever>(std::move(web_app_info));
@@ -613,7 +614,8 @@ TEST_F(WebAppInstallManagerTest, FinalizerMethodsCalled) {
   InstallWebApp();
 
   EXPECT_EQ(1, install_finalizer_->num_create_os_shortcuts_calls());
-  EXPECT_EQ(1, install_finalizer_->num_reparent_tab_num_calls());
+  EXPECT_EQ(1, install_finalizer_->num_reparent_tab_calls());
+  EXPECT_EQ(1, install_finalizer_->num_reveal_appshim_calls());
 }
 
 TEST_F(WebAppInstallManagerTest, FinalizerMethodsNotCalled) {
@@ -627,7 +629,8 @@ TEST_F(WebAppInstallManagerTest, FinalizerMethodsNotCalled) {
   EXPECT_EQ(InstallResultCode::kFailedUnknownReason, result.code);
 
   EXPECT_EQ(0, install_finalizer_->num_create_os_shortcuts_calls());
-  EXPECT_EQ(0, install_finalizer_->num_reparent_tab_num_calls());
+  EXPECT_EQ(0, install_finalizer_->num_reparent_tab_calls());
+  EXPECT_EQ(0, install_finalizer_->num_reveal_appshim_calls());
 }
 
 // TODO(loyso): Convert more tests from bookmark_app_helper_unittest.cc