Revert "Revert of Reland: Switch WindowedNotificationObserver to use base::RunLoop. (patchset #3 id…

chrome/browser/ui/login/login_handler_browsertest.cc

Index: chrome/browser/ui/login/login_handler_browsertest.cc
diff --git a/chrome/browser/ui/login/login_handler_browsertest.cc b/chrome/browser/ui/login/login_handler_browsertest.cc
index aea1ff04ec2a016eb2e0905436a0b6057c07901c..a353194f9bf4b2c7a965eccd0de41f9c35eed80b 100644
--- a/chrome/browser/ui/login/login_handler_browsertest.cc
+++ b/chrome/browser/ui/login/login_handler_browsertest.cc
@@ -99,6 +99,7 @@ const char kPrefetchAuthPage[] = "/login/prefetch.html";
 
 const char kMultiRealmTestPage[] = "/login/multi_realm.html";
 const int  kMultiRealmTestRealmCount = 2;
+const int kMultiRealmTestAuthRequestsCount = 4;
 
 const char kSingleRealmTestPage[] = "/login/single_realm.html";
 
@@ -462,104 +463,102 @@ IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, TestCancelAuth_OnForward) {
 // Test handling of resources that require authentication even though
 // the page they are included on doesn't.  In this case we should only
 // present the minimal number of prompts necessary for successfully
-// displaying the page.  First we check whether cancelling works as
-// expected.
-IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, MultipleRealmCancellation) {
-  ASSERT_TRUE(embedded_test_server()->Start());
-  GURL test_page = embedded_test_server()->GetURL(kMultiRealmTestPage);
-
-  content::WebContents* contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  NavigationController* controller = &contents->GetController();
-  LoginPromptBrowserTestObserver observer;
-
-  observer.Register(content::Source<NavigationController>(controller));
-
-  WindowedLoadStopObserver load_stop_waiter(controller, 1);
-
-  {
-    WindowedAuthNeededObserver auth_needed_waiter(controller);
-    browser()->OpenURL(OpenURLParams(test_page, Referrer(),
-                                     WindowOpenDisposition::CURRENT_TAB,
-                                     ui::PAGE_TRANSITION_TYPED, false));
-    auth_needed_waiter.Wait();
+// displaying the page.
+class MultiRealmLoginPromptBrowserTest : public LoginPromptBrowserTest {

[jam, 2017/04/19 23:09:56]

I'm not familiar with this, please add a reviewer from this directory's owners

+ public:
+  void TearDownOnMainThread() override {
+    login_prompt_observer_.UnregisterAll();
+    LoginPromptBrowserTest::TearDownOnMainThread();
   }
 
-  int n_handlers = 0;
-
-  while (n_handlers < kMultiRealmTestRealmCount) {
-    WindowedAuthNeededObserver auth_needed_waiter(controller);
-
-    while (!observer.handlers().empty()) {
-      WindowedAuthCancelledObserver auth_cancelled_waiter(controller);
-      LoginHandler* handler = *observer.handlers().begin();
-
-      ASSERT_TRUE(handler);
-      n_handlers++;
-      handler->CancelAuth();
-      auth_cancelled_waiter.Wait();
-    }
-
-    if (n_handlers < kMultiRealmTestRealmCount)
-      auth_needed_waiter.Wait();
+  // Load the multi-realm test page, waits for LoginHandlers to be created, then
+  // calls |for_each_realm_func| once for each authentication realm, passing a
+  // LoginHandler for the realm as an argument. The page should stop loading
+  // after that.
+  template <class F>
+  void RunTest(const F& for_each_realm_func);
+
+  NavigationController* GetNavigationController() {
+    return &browser()
+                ->tab_strip_model()
+                ->GetActiveWebContents()
+                ->GetController();
   }
 
-  load_stop_waiter.Wait();
+  LoginPromptBrowserTestObserver* login_prompt_observer() {
+    return &login_prompt_observer_;
+  }
 
-  EXPECT_EQ(kMultiRealmTestRealmCount, n_handlers);
-  EXPECT_EQ(0, observer.auth_supplied_count());
-  EXPECT_LT(0, observer.auth_needed_count());
-  EXPECT_LT(0, observer.auth_cancelled_count());
-}
+ private:
+  LoginPromptBrowserTestObserver login_prompt_observer_;
+};
 
-// Similar to the MultipleRealmCancellation test above, but tests
-// whether supplying credentials work as exepcted.
-IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, MultipleRealmConfirmation) {
+template <class F>
+void MultiRealmLoginPromptBrowserTest::RunTest(const F& for_each_realm_func) {
   ASSERT_TRUE(embedded_test_server()->Start());
   GURL test_page = embedded_test_server()->GetURL(kMultiRealmTestPage);
 
-  content::WebContents* contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  NavigationController* controller = &contents->GetController();
-  LoginPromptBrowserTestObserver observer;
+  NavigationController* controller = GetNavigationController();
 
-  observer.Register(content::Source<NavigationController>(controller));
+  login_prompt_observer_.Register(
+      content::Source<NavigationController>(controller));
 
   WindowedLoadStopObserver load_stop_waiter(controller, 1);
-  int n_handlers = 0;
-
-  {
-    WindowedAuthNeededObserver auth_needed_waiter(controller);
 
-    browser()->OpenURL(OpenURLParams(test_page, Referrer(),
-                                     WindowOpenDisposition::CURRENT_TAB,
-                                     ui::PAGE_TRANSITION_TYPED, false));
-    auth_needed_waiter.Wait();
-  }
-
-  while (n_handlers < kMultiRealmTestRealmCount) {
-    WindowedAuthNeededObserver auth_needed_waiter(controller);
-
-    while (!observer.handlers().empty()) {
-      WindowedAuthSuppliedObserver auth_supplied_waiter(controller);
-      LoginHandler* handler = *observer.handlers().begin();
-
-      ASSERT_TRUE(handler);
-      n_handlers++;
-      SetAuthFor(handler);
-      auth_supplied_waiter.Wait();
-    }
-
-    if (n_handlers < kMultiRealmTestRealmCount)
-      auth_needed_waiter.Wait();
+  browser()->OpenURL(OpenURLParams(test_page, Referrer(),
+                                   WindowOpenDisposition::CURRENT_TAB,
+                                   ui::PAGE_TRANSITION_TYPED, false));
+
+  // Need to have LoginHandlers created for all requests that need
+  // authentication.
+  while (login_prompt_observer_.handlers().size() <
+         kMultiRealmTestAuthRequestsCount)
+    WindowedAuthNeededObserver(controller).Wait();
+
+  // Now confirm or cancel auth once per realm.
+  std::set<std::string> seen_realms;
+  for (int i = 0; i < kMultiRealmTestRealmCount; ++i) {
+    auto it = std::find_if(
+        login_prompt_observer_.handlers().begin(),
+        login_prompt_observer_.handlers().end(),
+        [&seen_realms](LoginHandler* handler) {
+          return seen_realms.count(handler->auth_info()->realm) == 0;
+        });
+    ASSERT_TRUE(it != login_prompt_observer_.handlers().end());
+    seen_realms.insert((*it)->auth_info()->realm);
+
+    for_each_realm_func(*it);
   }
 
   load_stop_waiter.Wait();
+}
+
+// Checks that cancelling works as expected.
+IN_PROC_BROWSER_TEST_F(MultiRealmLoginPromptBrowserTest,
+                       MultipleRealmCancellation) {
+  RunTest([this](LoginHandler* handler) {
+    WindowedAuthCancelledObserver waiter(GetNavigationController());
+    handler->CancelAuth();
+    waiter.Wait();
+  });
+
+  EXPECT_EQ(0, login_prompt_observer()->auth_supplied_count());
+  EXPECT_LT(0, login_prompt_observer()->auth_needed_count());

[vabr (Chromium), 2017/04/20 11:55:03]

nit: Could you add a code comment explaining why the test only checks that there
was a positive number of auth requests and auth cancellations, but does not care
about what exactly that number was or whether those were equal?

(Similarly below in MultipleRealConfirmation.)

[Alexander Semashko, 2017/04/20 14:26:32]

I don't know why is it so; I tried digging and got to the CL that added this
file 6 years ago:  https://codereview.chromium.org/5814005
Apparently, patch set 9 changed these lines, and before it there were equality
checks. I found no explanation in comments.

[vabr (Chromium), 2017/04/20 14:29:07]

Ah, OK, I missed that this was already in the old code.
That's fine, then.
Thanks!

+  EXPECT_LT(0, login_prompt_observer()->auth_cancelled_count());
+}
 
-  EXPECT_EQ(kMultiRealmTestRealmCount, n_handlers);
-  EXPECT_LT(0, observer.auth_needed_count());
-  EXPECT_LT(0, observer.auth_supplied_count());
-  EXPECT_EQ(0, observer.auth_cancelled_count());
+// Checks that supplying credentials works as exepcted.

[vabr (Chromium), 2017/04/20 11:55:03]

nit: expected

[Alexander Semashko, 2017/04/20 14:26:32]

Done.

+IN_PROC_BROWSER_TEST_F(MultiRealmLoginPromptBrowserTest,
+                       MultipleRealmConfirmation) {
+  RunTest([this](LoginHandler* handler) {
+    WindowedAuthSuppliedObserver waiter(GetNavigationController());
+    SetAuthFor(handler);
+    waiter.Wait();
+  });
+
+  EXPECT_LT(0, login_prompt_observer()->auth_needed_count());
+  EXPECT_LT(0, login_prompt_observer()->auth_supplied_count());
+  EXPECT_EQ(0, login_prompt_observer()->auth_cancelled_count());
 }
 
 // Testing for recovery from an incorrect password for the case where