Add comments (and tweaks) to PendingAppManagerImplBrowserTest.AlreadyRegistered

This CL adds clarifying comments to
PendingAppManagerImplBrowserTest.AlreadyRegistered.

In addition to comments this also adds
WebAppRegistrationWaiter::AwaitNextNonFailedRegistration() to avoid
having to use /web_apps/no_service_worker.html for the sake of asserting
on a particular RegistrationResultCode. The normal install flow with
/web_apps/basic.html is flakily either kSuccess or kAlreadyInstalled as
the service worker may asynchronously register during the first install
pass.

Change-Id: I6e6aa14882424c3ccc2affa90afd2798cfaed897
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2437549Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Auto-Submit: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811824}

chrome/browser/web_applications/pending_app_manager_impl_browsertest.cc

@@ -361,24 +361,26 @@ IN_PROC_BROWSER_TEST_P(PendingAppManagerImplBrowserTest, RegistrationSkipped) {
 
 IN_PROC_BROWSER_TEST_P(PendingAppManagerImplBrowserTest, AlreadyRegistered) {
   ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Ensure service worker registered for http://embedded_test_server/web_apps/.
+  // We don't need to be installing a web app here but it's convenient just to
+  // await the service worker registration.
   {
-    // Delay service worker registration to second load to simulate it not
-    // loading during the initial install pass.
-    GURL install_url(embedded_test_server()->GetURL(
-        "/web_apps/service_worker_on_second_load.html"));
+    GURL install_url(embedded_test_server()->GetURL("/web_apps/basic.html"));
     ExternalInstallOptions install_options = CreateInstallOptions(install_url);
     install_options.force_reinstall = true;
-    install_options.bypass_service_worker_check = true;
     InstallApp(std::move(install_options));
     EXPECT_EQ(InstallResultCode::kSuccessNewInstall, result_code_.value());
     WebAppRegistrationWaiter(&pending_app_manager())
-        .AwaitNextRegistration(install_url, RegistrationResultCode::kSuccess);
+        .AwaitNextNonFailedRegistration(install_url);
+    CheckServiceWorkerStatus(
+        embedded_test_server()->GetURL("/web_apps/basic.html"),
+        content::ServiceWorkerCapability::SERVICE_WORKER_WITH_FETCH_HANDLER);
   }
 
-  CheckServiceWorkerStatus(
-      embedded_test_server()->GetURL("/web_apps/basic.html"),
-      content::ServiceWorkerCapability::SERVICE_WORKER_WITH_FETCH_HANDLER);
-
+  // With the service worker registered we install a page that doesn't register
+  // a service worker to check that the existing service worker is seen by our
+  // service worker registration step.
   {
     GURL install_url(
         embedded_test_server()->GetURL("/web_apps/no_service_worker.html"));

chrome/browser/web_applications/test/web_app_registration_waiter.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/web_applications/test/web_app_registration_waiter.h"
 
 #include "base/test/bind_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace web_app {
 
@@ -12,8 +13,11 @@ WebAppRegistrationWaiter::WebAppRegistrationWaiter(PendingAppManager* manager)
     : manager_(manager) {
   manager_->SetRegistrationCallbackForTesting(base::BindLambdaForTesting(
       [this](const GURL& install_url, RegistrationResultCode code) {
-        CHECK_EQ(install_url_, install_url);
-        CHECK_EQ(code_, code);
+        ASSERT_EQ(install_url_, install_url);
+        if (code_)
+          ASSERT_EQ(code_, code);
+        else
+          ASSERT_NE(code, RegistrationResultCode::kTimeout);
         run_loop_.Quit();
       }));
   manager_->SetRegistrationsCompleteCallbackForTesting(
@@ -32,6 +36,13 @@ void WebAppRegistrationWaiter::AwaitNextRegistration(
   run_loop_.Run();
 }
 
+void WebAppRegistrationWaiter::AwaitNextNonFailedRegistration(
+    const GURL& install_url) {
+  install_url_ = install_url;
+  code_ = base::nullopt;
+  run_loop_.Run();
+}
+
 void WebAppRegistrationWaiter::AwaitRegistrationsComplete() {
   complete_run_loop_.Run();
 }

chrome/browser/web_applications/test/web_app_registration_waiter.h

@@ -19,13 +19,16 @@ class WebAppRegistrationWaiter {
   void AwaitNextRegistration(const GURL& install_url,
                              RegistrationResultCode code);
 
+  void AwaitNextNonFailedRegistration(const GURL& install_url);
+
   void AwaitRegistrationsComplete();
 
  private:
   PendingAppManager* const manager_;
   base::RunLoop run_loop_;
   GURL install_url_;
-  RegistrationResultCode code_;
+  // If unset then check for any non failure result.
+  base::Optional<RegistrationResultCode> code_;
 
   base::RunLoop complete_run_loop_;
 };