desktop-pwas: Change InstallCallback to return a GURL in addition to the app id

ProcessAppOperations will need to include the URL of the app that was
installed. To avoid having two distinct callbacks, this CL changes
InstallCallback to also return the url.

Bug: 864904
Change-Id: Ie296a90b4588c0d301c9a03d487433b9d56832b1
Reviewed-on: https://chromium-review.googlesource.com/1174589
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583181}

chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager_unittest.cc

@@ -39,9 +39,8 @@ class WebAppPolicyManagerTest::TestPendingAppManager
   TestPendingAppManager() = default;
   ~TestPendingAppManager() override = default;
 
-  void Install(
-      AppInfo app_to_install,
-      PendingAppManager::InstallCallback callback) override {}
+  void Install(AppInfo app_to_install,
+               PendingAppManager::OnceInstallCallback callback) override {}
 
   void ProcessAppOperations(std::vector<AppInfo> apps_to_install) override {
     last_apps_to_install_ = std::move(apps_to_install);

chrome/browser/web_applications/components/pending_app_manager.h

@@ -22,7 +22,8 @@ namespace web_app {
 // should wait for the update request to finish before uninstalling the app.
 class PendingAppManager {
  public:
-  using InstallCallback = base::OnceCallback<void(const std::string&)>;
+  using OnceInstallCallback =
+      base::OnceCallback<void(const GURL& app_url, const std::string&)>;
 
   // How the app will be launched after installation.
   enum class LaunchContainer {
@@ -49,13 +50,13 @@ class PendingAppManager {
   // Queues an installation operation with the highest priority. Essentially
   // installing the app immediately if there are no ongoing operations or
   // installing the app right after the current operation finishes. Runs its
-  // callback with the id of the installed app or an empty string if the
-  // installation fails.
+  // callback with the URL in |app_to_install| and with the id of the installed
+  // app or an empty string if the installation fails.
   //
   // Fails if the same operation has been queued before. Should only be used in
   // response to a user action e.g. the user clicked an install button.
   virtual void Install(AppInfo app_to_install,
-                       InstallCallback callback) = 0;
+                       OnceInstallCallback callback) = 0;
 
   // Adds |apps_to_install| to the queue of operations.
   virtual void ProcessAppOperations(std::vector<AppInfo> apps_to_install) = 0;

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc

@@ -35,12 +35,12 @@ std::unique_ptr<BookmarkAppInstallationTask> InstallationTaskCreateWrapper(
 }  // namespace
 
 struct PendingBookmarkAppManager::Installation {
-  Installation(AppInfo info, InstallCallback callback)
+  Installation(AppInfo info, OnceInstallCallback callback)
       : info(std::move(info)), callback(std::move(callback)) {}
   ~Installation() = default;
 
   AppInfo info;
-  InstallCallback callback;
+  OnceInstallCallback callback;
 };
 
 PendingBookmarkAppManager::PendingBookmarkAppManager(Profile* profile)
@@ -51,15 +51,15 @@ PendingBookmarkAppManager::PendingBookmarkAppManager(Profile* profile)
 PendingBookmarkAppManager::~PendingBookmarkAppManager() = default;
 
 void PendingBookmarkAppManager::Install(AppInfo app_to_install,
-                                        InstallCallback callback) {
+                                        OnceInstallCallback callback) {
   // Check that we are not already installing the same app.
   if (current_installation_ && current_installation_->info == app_to_install) {
-    std::move(callback).Run(std::string());
+    std::move(callback).Run(app_to_install.url, std::string());
     return;
   }
   for (const auto& installation : installation_queue_) {
     if (installation->info == app_to_install) {
-      std::move(callback).Run(std::string());
+      std::move(callback).Run(app_to_install.url, std::string());
       return;
     }
   }
@@ -127,7 +127,7 @@ void PendingBookmarkAppManager::CurrentInstallationFinished(
 
   std::unique_ptr<Installation> installation;
   installation.swap(current_installation_);
-  std::move(installation->callback).Run(app_id);
+  std::move(installation->callback).Run(installation->info.url, app_id);
 }
 
 void PendingBookmarkAppManager::DidFinishLoad(

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h

@@ -45,8 +45,7 @@ class PendingBookmarkAppManager final : public web_app::PendingAppManager,
   ~PendingBookmarkAppManager() override;
 
   // web_app::PendingAppManager
-  void Install(AppInfo app_to_install,
-               InstallCallback callback) override;
+  void Install(AppInfo app_to_install, OnceInstallCallback callback) override;
   void ProcessAppOperations(std::vector<AppInfo> apps_to_install) override;
 
   void SetFactoriesForTesting(WebContentsFactory web_contents_factory,

chrome/browser/web_applications/extensions/pending_bookmark_app_manager_unittest.cc

@@ -114,12 +114,16 @@ class PendingBookmarkAppManagerTest : public ChromeRenderViewHostTestHarness {
                                                                      false);
   }
 
-  void InstallCallback(const std::string& app_id) {
+  void InstallCallback(const GURL& url, const std::string& app_id) {
+    install_callback_url_ = url;
     install_succeeded_ = !app_id.empty();
   }
 
  protected:
-  void ResetResults() { install_succeeded_ = base::nullopt; }
+  void ResetResults() {
+    install_succeeded_.reset();
+    install_callback_url_.reset();
+  }
 
   const PendingBookmarkAppManager::WebContentsFactory&
   test_web_contents_creator() {
@@ -142,9 +146,12 @@ class PendingBookmarkAppManagerTest : public ChromeRenderViewHostTestHarness {
 
   bool install_succeeded() { return install_succeeded_.value(); }
 
+  const GURL& install_callback_url() { return install_callback_url_.value(); }
+
  private:
   content::WebContentsTester* web_contents_tester_ = nullptr;
   base::Optional<bool> install_succeeded_;
+  base::Optional<GURL> install_callback_url_;
 
   PendingBookmarkAppManager::WebContentsFactory test_web_contents_creator_;
   PendingBookmarkAppManager::TaskFactory successful_installation_task_creator_;
@@ -167,6 +174,7 @@ TEST_F(PendingBookmarkAppManagerTest, Install_Succeeds) {
   web_contents_tester()->NavigateAndCommit(GURL(kFooWebAppUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kFooWebAppUrl));
   EXPECT_TRUE(install_succeeded());
+  EXPECT_EQ(GURL(kFooWebAppUrl), install_callback_url());
 }
 
 TEST_F(PendingBookmarkAppManagerTest, Install_SucceedsTwice) {
@@ -183,6 +191,7 @@ TEST_F(PendingBookmarkAppManagerTest, Install_SucceedsTwice) {
   web_contents_tester()->NavigateAndCommit(GURL(kFooWebAppUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kFooWebAppUrl));
   EXPECT_TRUE(install_succeeded());
+  EXPECT_EQ(GURL(kFooWebAppUrl), install_callback_url());
   ResetResults();
 
   pending_app_manager.Install(
@@ -194,6 +203,7 @@ TEST_F(PendingBookmarkAppManagerTest, Install_SucceedsTwice) {
   web_contents_tester()->NavigateAndCommit(GURL(kBarWebAppUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kBarWebAppUrl));
   EXPECT_TRUE(install_succeeded());
+  EXPECT_EQ(GURL(kBarWebAppUrl), install_callback_url());
 }
 
 TEST_F(PendingBookmarkAppManagerTest, Install_PendingSuccessfulTask) {
@@ -215,6 +225,7 @@ TEST_F(PendingBookmarkAppManagerTest, Install_PendingSuccessfulTask) {
   web_contents_tester()->NavigateAndCommit(GURL(kFooWebAppUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kFooWebAppUrl));
   EXPECT_TRUE(install_succeeded());
+  EXPECT_EQ(GURL(kFooWebAppUrl), install_callback_url());
   ResetResults();
 
   // Finish the second install.
@@ -222,6 +233,7 @@ TEST_F(PendingBookmarkAppManagerTest, Install_PendingSuccessfulTask) {
   web_contents_tester()->NavigateAndCommit(GURL(kBarWebAppUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kBarWebAppUrl));
   EXPECT_TRUE(install_succeeded());
+  EXPECT_EQ(GURL(kBarWebAppUrl), install_callback_url());
 }
 
 TEST_F(PendingBookmarkAppManagerTest, Install_PendingFailingTask) {
@@ -243,6 +255,7 @@ TEST_F(PendingBookmarkAppManagerTest, Install_PendingFailingTask) {
   web_contents_tester()->NavigateAndCommit(GURL(kBarWebAppUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kBarWebAppUrl));
   EXPECT_FALSE(install_succeeded());
+  EXPECT_EQ(GURL(kFooWebAppUrl), install_callback_url());
   ResetResults();
 
   // Finish the second install.
@@ -250,6 +263,7 @@ TEST_F(PendingBookmarkAppManagerTest, Install_PendingFailingTask) {
   web_contents_tester()->NavigateAndCommit(GURL(kBarWebAppUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kBarWebAppUrl));
   EXPECT_TRUE(install_succeeded());
+  EXPECT_EQ(GURL(kBarWebAppUrl), install_callback_url());
 }
 
 TEST_F(PendingBookmarkAppManagerTest, Install_ReentrantCallback) {
@@ -260,8 +274,9 @@ TEST_F(PendingBookmarkAppManagerTest, Install_ReentrantCallback) {
   // Call install with a callback that tries to install another app.
   pending_app_manager.Install(
       GetFooAppInfo(),
-      base::BindLambdaForTesting([&](const std::string& app_id) {
-        InstallCallback(app_id);
+      base::BindLambdaForTesting([&](const GURL& provided_url,
+                                     const std::string& app_id) {
+        InstallCallback(provided_url, app_id);
         pending_app_manager.Install(
             GetBarAppInfo(),
             base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
@@ -272,12 +287,14 @@ TEST_F(PendingBookmarkAppManagerTest, Install_ReentrantCallback) {
   web_contents_tester()->NavigateAndCommit(GURL(kFooWebAppUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kFooWebAppUrl));
   EXPECT_TRUE(install_succeeded());
+  EXPECT_EQ(GURL(kFooWebAppUrl), install_callback_url());
   ResetResults();
 
   base::RunLoop().RunUntilIdle();
   web_contents_tester()->NavigateAndCommit(GURL(kBarWebAppUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kBarWebAppUrl));
   EXPECT_TRUE(install_succeeded());
+  EXPECT_EQ(GURL(kBarWebAppUrl), install_callback_url());
 }
 
 TEST_F(PendingBookmarkAppManagerTest, Install_FailsSameInstallPending) {
@@ -297,6 +314,7 @@ TEST_F(PendingBookmarkAppManagerTest, Install_FailsSameInstallPending) {
 
   // The second install should fail; the app is already getting installed.
   EXPECT_FALSE(install_succeeded());
+  EXPECT_EQ(GURL(kFooWebAppUrl), install_callback_url());
   ResetResults();
 
   // The original install should still be able to succeed.
@@ -304,6 +322,7 @@ TEST_F(PendingBookmarkAppManagerTest, Install_FailsSameInstallPending) {
   web_contents_tester()->NavigateAndCommit(GURL(kFooWebAppUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kFooWebAppUrl));
   EXPECT_TRUE(install_succeeded());
+  EXPECT_EQ(GURL(kFooWebAppUrl), install_callback_url());
 }
 
 TEST_F(PendingBookmarkAppManagerTest, Install_FailsLoadIncorrectURL) {
@@ -320,6 +339,7 @@ TEST_F(PendingBookmarkAppManagerTest, Install_FailsLoadIncorrectURL) {
   web_contents_tester()->NavigateAndCommit(GURL(kWrongUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kWrongUrl));
   EXPECT_FALSE(install_succeeded());
+  EXPECT_EQ(GURL(kFooWebAppUrl), install_callback_url());
 }
 
 }  // namespace extensions