[dPWA Testing] Implemented AwaitManifestUpdate and generated tests

Change-Id: I1aa0ecc670bd8b2fa9c3f4cf62ec294c4d9fe0b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3828038
Commit-Queue: Phillis Tang <phillis@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Phillis Tang <phillis@chromium.org>
Auto-Submit: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1046045}

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

@@ -21,6 +21,11 @@ Browser* TwoClientWebAppsIntegrationTestBase::CreateBrowser(Profile* profile) {
   return InProcessBrowserTest::CreateBrowser(profile);
 }
 
+void TwoClientWebAppsIntegrationTestBase::CloseBrowserSynchronously(
+    Browser* browser) {
+  InProcessBrowserTest::CloseBrowserSynchronously(browser);
+}
+
 void TwoClientWebAppsIntegrationTestBase::AddBlankTabAndShow(Browser* browser) {
   InProcessBrowserTest::AddBlankTabAndShow(browser);
 }

chrome/browser/sync/test/integration/two_client_web_apps_integration_test_base.h

@@ -22,6 +22,7 @@ class TwoClientWebAppsIntegrationTestBase
 
   // WebAppIntegrationTestDriver::TestDelegate:
   Browser* CreateBrowser(Profile* profile) override;
+  void CloseBrowserSynchronously(Browser* browser) override;
   void AddBlankTabAndShow(Browser* browser) override;
   const net::EmbeddedTestServer* EmbeddedTestServer() const override;
   std::vector<Profile*> GetAllProfiles() override;

chrome/browser/ui/views/web_apps/web_app_integration_browsertest_cros.cc

@@ -14,41 +14,5 @@ using WebAppIntegration = WebAppIntegrationTest;
 
 // Generated tests:
 
-IN_PROC_BROWSER_TEST_F(
-    WebAppIntegration,
-    WAI_29StandaloneNestedAWindowed_8StandaloneNestedAStandalone_28) {
-  // Test contents are generated by script. Please do not modify!
-  // See `docs/webapps/why-is-this-test-failing.md` or
-  // `docs/webapps/integration-testing-framework` for more info.
-  // Sheriffs: Disabling this test is supported.
-  helper_.CreateShortcut(Site::kStandaloneNestedA, WindowOptions::kWindowed);
-  helper_.ManifestUpdateScopeTo(Site::kStandaloneNestedA, Site::kStandalone);
-  helper_.ClosePwa();
-}
-
-IN_PROC_BROWSER_TEST_F(
-    WebAppIntegration,
-    WAI_31StandaloneNestedA_8StandaloneNestedAStandalone_28) {
-  // Test contents are generated by script. Please do not modify!
-  // See `docs/webapps/why-is-this-test-failing.md` or
-  // `docs/webapps/integration-testing-framework` for more info.
-  // Sheriffs: Disabling this test is supported.
-  helper_.InstallOmniboxIcon(InstallableSite::kStandaloneNestedA);
-  helper_.ManifestUpdateScopeTo(Site::kStandaloneNestedA, Site::kStandalone);
-  helper_.ClosePwa();
-}
-
-IN_PROC_BROWSER_TEST_F(
-    WebAppIntegration,
-    WAI_47StandaloneNestedA_8StandaloneNestedAStandalone_28) {
-  // Test contents are generated by script. Please do not modify!
-  // See `docs/webapps/why-is-this-test-failing.md` or
-  // `docs/webapps/integration-testing-framework` for more info.
-  // Sheriffs: Disabling this test is supported.
-  helper_.InstallMenuOption(InstallableSite::kStandaloneNestedA);
-  helper_.ManifestUpdateScopeTo(Site::kStandaloneNestedA, Site::kStandalone);
-  helper_.ClosePwa();
-}
-
 }  // namespace
 }  // namespace web_app::integration_tests

chrome/browser/ui/views/web_apps/web_app_integration_test_driver.cc

@@ -770,6 +770,25 @@ void WebAppIntegrationTestDriver::AcceptAppIdUpdateDialog() {
   AfterStateChangeAction();
 }
 
+void WebAppIntegrationTestDriver::AwaitManifestUpdate(Site site) {
+  if (!BeforeStateChangeAction(__FUNCTION__))
+    return;
+  AppId app_id = GetAppIdBySiteMode(site);
+  ASSERT_TRUE(provider()->registrar().GetAppById(app_id));
+  if (!previous_manifest_updates_.contains(app_id)) {
+    waiting_for_update_id_ = app_id;
+    waiting_for_update_run_loop_ = std::make_unique<base::RunLoop>();
+    Browser* browser = GetBrowserForAppId(app_id);
+    while (browser != nullptr) {
+      delegate_->CloseBrowserSynchronously(browser);
+      browser = GetBrowserForAppId(app_id);
+    }
+    waiting_for_update_run_loop_->Run();
+    waiting_for_update_run_loop_.reset();
+  }
+  AfterStateChangeAction();
+}
+
 void WebAppIntegrationTestDriver::CloseCustomToolbar() {
   if (!BeforeStateChangeAction(__FUNCTION__))
     return;
@@ -2392,18 +2411,12 @@ void WebAppIntegrationTestDriver::CheckWindowDisplayStandalone() {
 void WebAppIntegrationTestDriver::OnWebAppManifestUpdated(
     const AppId& app_id,
     base::StringPiece old_name) {
+  LOG(INFO) << "Manifest update received for " << app_id << ".";
   DCHECK_EQ(1ul, delegate_->GetAllProfiles().size())
       << "Manifest update waiting only supported on single profile tests.";
-  bool is_waiting = app_ids_with_pending_manifest_updates_.erase(app_id);
-  // The "create shortcut" behavior can cause issues with manifest update
-  // occurring when the "create shortcut" document url matches the manifest
-  // start_url. So allow random updates, but log in case of other errors.
-  if (!is_waiting) {
-    LOG(INFO) << "Received possibly unexpected manifest update for app "
-              << old_name;
-    return;
-  }
-  if (waiting_for_update_id_ && app_id == waiting_for_update_id_.value()) {
+
+  previous_manifest_updates_.insert(app_id);
+  if (waiting_for_update_id_ == app_id) {
     DCHECK(waiting_for_update_run_loop_);
     waiting_for_update_run_loop_->Quit();
     waiting_for_update_id_ = absl::nullopt;
@@ -2457,7 +2470,6 @@ void WebAppIntegrationTestDriver::AfterStateChangeAction() {
   if (delegate_->IsSyncTest())
     delegate_->AwaitWebAppQuiescence();
   FlushShortcutTasks();
-  MaybeWaitForManifestUpdates();
   after_state_change_action_state_ = ConstructStateSnapshot();
 }
 
@@ -2735,58 +2747,20 @@ void WebAppIntegrationTestDriver::UninstallPolicyAppById(const AppId& id) {
     active_app_id_.clear();
 }
 
-bool WebAppIntegrationTestDriver::AreNoAppWindowsOpen(Profile* profile,
-                                                      const AppId& app_id) {
-  auto* browser_list = BrowserList::GetInstance();
-  for (Browser* browser : *browser_list) {
-    if (browser->IsAttemptingToCloseBrowser())
-      continue;
-    if (AppBrowserController::IsForWebApp(browser, app_id))
-      return false;
-  }
-  return true;
-}
-
 void WebAppIntegrationTestDriver::ForceUpdateManifestContents(
     Site site,
     const GURL& app_url_with_manifest_param) {
-  absl::optional<AppState> app_state = GetAppBySiteMode(
-      before_state_change_action_state_.get(), profile(), site);
-  ASSERT_TRUE(app_state.has_value()) << static_cast<int>(site);
-  auto app_id = app_state->id;
+  auto app_id = GetAppIdBySiteMode(site);
   active_app_id_ = app_id;
-  app_ids_with_pending_manifest_updates_.insert(app_id);
-
   // Manifest updates must occur as the first navigation after a webapp is
   // installed, otherwise the throttle is tripped.
   ASSERT_FALSE(provider()->manifest_update_manager().IsUpdateConsumed(app_id));
+  ASSERT_FALSE(
+      provider()->manifest_update_manager().IsUpdateTaskPending(app_id));
   NavigateTabbedBrowserToSite(app_url_with_manifest_param,
                               NavigationMode::kCurrentTab);
 }
 
-void WebAppIntegrationTestDriver::MaybeWaitForManifestUpdates() {
-  if (delegate_->GetAllProfiles().size() > 1) {
-    return;
-  }
-  bool continue_checking_for_updates = true;
-  while (continue_checking_for_updates) {
-    continue_checking_for_updates = false;
-    for (const AppId& app_id : app_ids_with_pending_manifest_updates_) {
-      if (AreNoAppWindowsOpen(profile(), app_id)) {
-        waiting_for_update_id_ = absl::make_optional(app_id);
-        waiting_for_update_run_loop_ = std::make_unique<base::RunLoop>();
-        waiting_for_update_run_loop_->Run();
-        waiting_for_update_run_loop_ = nullptr;
-        DCHECK(!waiting_for_update_id_);
-        // To prevent iteration-during-modification, break and restart
-        // the loop.
-        continue_checking_for_updates = true;
-        break;
-      }
-    }
-  }
-}
-
 void WebAppIntegrationTestDriver::MaybeNavigateTabbedBrowserInScope(Site site) {
   auto browser_url = GetCurrentTab(browser())->GetURL();
   auto dest_url = GetInScopeURL(site);
@@ -3043,6 +3017,10 @@ Browser* WebAppIntegrationTest::CreateBrowser(Profile* profile) {
   return InProcessBrowserTest::CreateBrowser(profile);
 }
 
+void WebAppIntegrationTest::CloseBrowserSynchronously(Browser* browser) {
+  InProcessBrowserTest::CloseBrowserSynchronously(browser);
+}
+
 void WebAppIntegrationTest::AddBlankTabAndShow(Browser* browser) {
   InProcessBrowserTest::AddBlankTabAndShow(browser);
 }

chrome/browser/ui/views/web_apps/web_app_integration_test_driver.h

@@ -182,6 +182,7 @@ class WebAppIntegrationTestDriver : WebAppInstallManagerObserver {
    public:
     // Exposing normal functionality of testing::InProcBrowserTest:
     virtual Browser* CreateBrowser(Profile* profile) = 0;
+    virtual void CloseBrowserSynchronously(Browser* browser) = 0;
     virtual void AddBlankTabAndShow(Browser* browser) = 0;
     virtual const net::EmbeddedTestServer* EmbeddedTestServer() const = 0;
     virtual std::vector<Profile*> GetAllProfiles() = 0;
@@ -210,6 +211,7 @@ class WebAppIntegrationTestDriver : WebAppInstallManagerObserver {
 
   // State change actions:
   void AcceptAppIdUpdateDialog();
+  void AwaitManifestUpdate(Site site_mode);
   void CloseCustomToolbar();
   void ClosePwa();
   void DisableRunOnOsLogin(Site site);
@@ -341,9 +343,6 @@ class WebAppIntegrationTestDriver : WebAppInstallManagerObserver {
   void ApplyRunOnOsLoginPolicy(Site site, const char* policy);
 
   void UninstallPolicyAppById(const AppId& id);
-  // This action only works if no navigations to the given app_url occur
-  // between app installation and calls to this action.
-  bool AreNoAppWindowsOpen(Profile* profile, const AppId& app_id);
   void ForceUpdateManifestContents(Site site,
                                    const GURL& app_url_with_manifest_param);
   void MaybeWaitForManifestUpdates();
@@ -389,10 +388,6 @@ class WebAppIntegrationTestDriver : WebAppInstallManagerObserver {
 
   base::flat_set<AppId> previous_manifest_updates_;
 
-  // Variables used to facilitate waiting for manifest updates, as there isn't
-  // a formal 'action' that a user can take to wait for this, as it happens
-  // behind the scenes.
-  base::flat_set<AppId> app_ids_with_pending_manifest_updates_;
   // |waiting_for_update_*| variables are either all populated or all not
   // populated. These signify that the test is currently waiting for the
   // given |waiting_for_update_id_| to receive an update before continuing.
@@ -454,6 +449,7 @@ class WebAppIntegrationTest : public InProcessBrowserTest,
 
   // WebAppIntegrationTestDriver::TestDelegate:
   Browser* CreateBrowser(Profile* profile) override;
+  void CloseBrowserSynchronously(Browser* browser) override;
   void AddBlankTabAndShow(Browser* browser) override;
   const net::EmbeddedTestServer* EmbeddedTestServer() const override;
 

chrome/browser/web_applications/manifest_update_manager.cc

@@ -115,6 +115,10 @@ bool ManifestUpdateManager::IsUpdateConsumed(const AppId& app_id) {
   return false;
 }
 
+bool ManifestUpdateManager::IsUpdateTaskPending(const AppId& app_id) {
+  return base::Contains(tasks_, app_id);
+}
+
 // WebAppInstallManager:
 void ManifestUpdateManager::OnWebAppWillBeUninstalled(const AppId& app_id) {
   DCHECK(started_);
@@ -191,6 +195,9 @@ void ManifestUpdateManager::ResetManifestThrottleForTesting(
   if (it != last_update_check_.end()) {
     last_update_check_.erase(app_id);
   }
+  // Manifest update scheduling can still fail if there is an existing tasks.
+  // Destroy this to ensure the next load will trigger update.
+  tasks_.erase(app_id);
 }
 
 }  // namespace web_app

chrome/browser/web_applications/manifest_update_manager.h

@@ -65,6 +65,8 @@ class ManifestUpdateManager final : public WebAppInstallManagerObserver {
                    const absl::optional<AppId>& app_id,
                    content::WebContents* web_contents);
   bool IsUpdateConsumed(const AppId& app_id);
+  // bool ResetUpdateStateForTesting(const AppId& app_id)
+  bool IsUpdateTaskPending(const AppId& app_id);
 
   // WebAppInstallManagerObserver:
   void OnWebAppWillBeUninstalled(const AppId& app_id) override;

chrome/test/data/webapps_integration/wco/basic.json

@@ -0,0 +1,18 @@
+{
+  "name": "Site WCO",
+  "icons": [
+    {
+      "src": "../basic-48.png",
+      "sizes": "48x48",
+      "type": "image/png"
+    },
+    {
+      "src": "../basic-192.png",
+      "sizes": "192x192",
+      "type": "image/png"
+    }
+  ],
+  "start_url": "/webapps_integration/wco/basic.html",
+  "scope": "/web_apps/wco/",
+  "display": "standalone"
+}