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

chrome/browser/ui/login/login_handler_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <algorithm>
 #include <list>
 #include <map>
 
 #include "base/metrics/field_trial.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 81 matching lines @@
     const AuthInfo& info = i->second;
     handler->SetAuth(base::UTF8ToUTF16(info.username_),
                      base::UTF8ToUTF16(info.password_));
   }
 }
 
 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";
 
 const char kAuthBasicPage[] = "/auth-basic";
 const char kAuthDigestPage[] = "/auth-digest";
 
 // It does not matter what pages are selected as no-auth, as long as they exist.
 // Navigating to non-existing pages caused flakes in the past
 // (https://crbug.com/636875).
 const char kNoAuthPage1[] = "/simple.html";
@@ skipping 343 matching lines @@
   ASSERT_TRUE(controller->CanGoForward());
   controller->GoForward();
   auth_cancelled_waiter.Wait();
   load_stop_waiter.Wait();
   EXPECT_TRUE(observer.handlers().empty());
 }
 
 // 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) {
+// 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();
+  }
+
+  // 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();
+  }
+
+  LoginPromptBrowserTestObserver* login_prompt_observer() {
+    return &login_prompt_observer_;
+  }
+
+ private:
+  LoginPromptBrowserTestObserver login_prompt_observer_;
+};
+
+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);
 
-  {
-    WindowedAuthNeededObserver auth_needed_waiter(controller);
-    browser()->OpenURL(OpenURLParams(test_page, Referrer(),
-                                     WindowOpenDisposition::CURRENT_TAB,
-                                     ui::PAGE_TRANSITION_TYPED, false));
-    auth_needed_waiter.Wait();
-  }
+  browser()->OpenURL(OpenURLParams(test_page, Referrer(),
+                                   WindowOpenDisposition::CURRENT_TAB,
+                                   ui::PAGE_TRANSITION_TYPED, false));
 
-  int n_handlers = 0;
+  // Need to have LoginHandlers created for all requests that need
+  // authentication.
+  while (login_prompt_observer_.handlers().size() <
+         kMultiRealmTestAuthRequestsCount)
+    WindowedAuthNeededObserver(controller).Wait();
 
-  while (n_handlers < kMultiRealmTestRealmCount) {
-    WindowedAuthNeededObserver auth_needed_waiter(controller);
+  // 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);
 
-    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();
+    for_each_realm_func(*it);
   }
 
   load_stop_waiter.Wait();
-
-  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());
 }
 
-// Similar to the MultipleRealmCancellation test above, but tests
-// whether supplying credentials work as exepcted.
-IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, MultipleRealmConfirmation) {
-  ASSERT_TRUE(embedded_test_server()->Start());
-  GURL test_page = embedded_test_server()->GetURL(kMultiRealmTestPage);
+// Checks that cancelling works as expected.
+IN_PROC_BROWSER_TEST_F(MultiRealmLoginPromptBrowserTest,
+                       MultipleRealmCancellation) {
+  RunTest([this](LoginHandler* handler) {
+    WindowedAuthCancelledObserver waiter(GetNavigationController());
+    handler->CancelAuth();
+    waiter.Wait();
+  });
 
-  content::WebContents* contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  NavigationController* controller = &contents->GetController();
-  LoginPromptBrowserTestObserver observer;
+  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());
+}
 
-  observer.Register(content::Source<NavigationController>(controller));
+// 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();
+  });
 
-  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();
-  }
-
-  load_stop_waiter.Wait();
-
-  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());
+  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
 // there are multiple authenticated resources.
 IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, IncorrectConfirmation) {
   ASSERT_TRUE(embedded_test_server()->Start());
   GURL test_page = embedded_test_server()->GetURL(kSingleRealmTestPage);
 
   content::WebContents* contents =
       browser()->tab_strip_model()->GetActiveWebContents();
@@ skipping 926 matching lines @@
   // out.
   EXPECT_TRUE(
       WaitForRenderFrameReady(contents->GetInterstitialPage()->GetMainFrame()));
   EXPECT_TRUE(contents->ShowingInterstitialPage());
   EXPECT_EQ(SSLBlockingPage::kTypeForTesting, contents->GetInterstitialPage()
                                                   ->GetDelegateForTesting()
                                                   ->GetTypeForTesting());
 }
 
 }  // namespace