Split LoginPromptBrowserTest.TestCancelAuth

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 299 matching lines @@
 
   handler1->SetAuth(base::UTF8ToUTF16(username_basic_),
                     base::UTF8ToUTF16(password_));
   handler2->SetAuth(base::UTF8ToUTF16(username_digest_),
                     base::UTF8ToUTF16(password_));
 
   EXPECT_EQ(expected_title1, title_watcher1.WaitAndGetTitle());
   EXPECT_EQ(expected_title2, title_watcher2.WaitAndGetTitle());
 }
 
-// Test login prompt cancellation.
-// Flaky on Mac crbug.com/636875
-IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, DISABLED_TestCancelAuth) {
+// Test manual login prompt cancellation.
+IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, TestCancelAuth_Manual) {
   ASSERT_TRUE(embedded_test_server()->Start());
-  GURL auth_page = embedded_test_server()->GetURL(kAuthBasicPage);
-  GURL no_auth_page_1 = embedded_test_server()->GetURL("/a");
-  GURL no_auth_page_2 = embedded_test_server()->GetURL("/b");
-  GURL no_auth_page_3 = embedded_test_server()->GetURL("/c");
+  const GURL auth_page = embedded_test_server()->GetURL(kAuthBasicPage);
 
-  content::WebContents* contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  NavigationController* controller = &contents->GetController();
+  NavigationController* controller =
+      &browser()->tab_strip_model()->GetActiveWebContents()->GetController();
 
   LoginPromptBrowserTestObserver observer;
   observer.Register(content::Source<NavigationController>(controller));
 
-  // First navigate to an unauthenticated page so we have something to
-  // go back to.
-  ui_test_utils::NavigateToURL(browser(), no_auth_page_1);
-
-  // Navigating while auth is requested is the same as cancelling.
-  {
-    // We need to wait for two LOAD_STOP events.  One for auth_page and one for
-    // no_auth_page_2.
-    WindowedLoadStopObserver load_stop_waiter(controller, 2);
-    WindowedAuthNeededObserver auth_needed_waiter(controller);
-    browser()->OpenURL(OpenURLParams(
-        auth_page, Referrer(), CURRENT_TAB, ui::PAGE_TRANSITION_TYPED,
-        false));
-    auth_needed_waiter.Wait();
-    WindowedAuthCancelledObserver auth_cancelled_waiter(controller);
-    browser()->OpenURL(OpenURLParams(
-        no_auth_page_2, Referrer(), CURRENT_TAB, ui::PAGE_TRANSITION_TYPED,
-        false));
-    auth_cancelled_waiter.Wait();
-    load_stop_waiter.Wait();
-    EXPECT_TRUE(observer.handlers().empty());
-  }
-
-  // Try navigating backwards.
-  {
-    // As above, we wait for two LOAD_STOP events; one for each navigation.
-    WindowedLoadStopObserver load_stop_waiter(controller, 2);
-    WindowedAuthNeededObserver auth_needed_waiter(controller);
-    browser()->OpenURL(OpenURLParams(
-        auth_page, Referrer(), CURRENT_TAB, ui::PAGE_TRANSITION_TYPED,
-        false));
-    auth_needed_waiter.Wait();
-    WindowedAuthCancelledObserver auth_cancelled_waiter(controller);
-    ASSERT_TRUE(chrome::CanGoBack(browser()));
-    chrome::GoBack(browser(), CURRENT_TAB);
-    auth_cancelled_waiter.Wait();
-    load_stop_waiter.Wait();
-    EXPECT_TRUE(observer.handlers().empty());
-  }
-
-  // Now add a page and go back, so we have something to go forward to.
-  ui_test_utils::NavigateToURL(browser(), no_auth_page_3);
-  {
-    WindowedLoadStopObserver load_stop_waiter(controller, 1);
-    chrome::GoBack(browser(), CURRENT_TAB);  // Should take us to page 1
-    load_stop_waiter.Wait();
-  }
-
-  {
-    // We wait for two LOAD_STOP events; one for each navigation.
-    WindowedLoadStopObserver load_stop_waiter(controller, 2);
-    WindowedAuthNeededObserver auth_needed_waiter(controller);
-    browser()->OpenURL(OpenURLParams(
-        auth_page, Referrer(), CURRENT_TAB, ui::PAGE_TRANSITION_TYPED,
-        false));
-    auth_needed_waiter.Wait();
-    WindowedAuthCancelledObserver auth_cancelled_waiter(controller);
-    ASSERT_TRUE(chrome::CanGoForward(browser()));
-    chrome::GoForward(browser(), CURRENT_TAB);  // Should take us to page 3
-    auth_cancelled_waiter.Wait();
-    load_stop_waiter.Wait();
-    EXPECT_TRUE(observer.handlers().empty());
-  }
-
-  // Now test that cancelling works as expected.
   {

[meacer, 2016/08/12 17:45:06]

No need for braces anymore.

[vabr (Chromium), 2016/08/12 19:39:37]

Done.

     WindowedLoadStopObserver load_stop_waiter(controller, 1);
     WindowedAuthNeededObserver auth_needed_waiter(controller);
-    browser()->OpenURL(OpenURLParams(
-        auth_page, Referrer(), CURRENT_TAB, ui::PAGE_TRANSITION_TYPED,
-        false));
+    browser()->OpenURL(OpenURLParams(auth_page, Referrer(), CURRENT_TAB,
+                                     ui::PAGE_TRANSITION_TYPED, false));
     auth_needed_waiter.Wait();
     WindowedAuthCancelledObserver auth_cancelled_waiter(controller);
     LoginHandler* handler = *observer.handlers().begin();
     ASSERT_TRUE(handler);
     handler->CancelAuth();
     auth_cancelled_waiter.Wait();
     load_stop_waiter.Wait();
     EXPECT_TRUE(observer.handlers().empty());
+  }
+}
+
+// Test login prompt cancellation on navigation to a new page.
+IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, TestCancelAuth_OnNavigation) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const GURL auth_page = embedded_test_server()->GetURL(kAuthBasicPage);
+  const GURL no_auth_page = embedded_test_server()->GetURL("/b");

[meacer, 2016/08/12 17:45:05]

You might want to use kNoAuthPage here too, instead of /b

[vabr (Chromium), 2016/08/12 19:39:37]

Done.

+
+  NavigationController* controller =
+      &browser()->tab_strip_model()->GetActiveWebContents()->GetController();
+
+  LoginPromptBrowserTestObserver observer;
+  observer.Register(content::Source<NavigationController>(controller));
+
+  // Navigating while auth is requested is the same as cancelling.
+  {

[meacer, 2016/08/12 17:45:06]

No need for braces.

[vabr (Chromium), 2016/08/12 19:39:37]

Done.
Also, I moved the comment from line 359 nearer to the code it applied to, as
with the braces removed the two comments created confusingly a single block.

+    // One LOAD_STOP event for auth_page and one for no_auth_page.
+    const int kLoadStopEvents = 2;
+    WindowedLoadStopObserver load_stop_waiter(controller, kLoadStopEvents);
+    WindowedAuthNeededObserver auth_needed_waiter(controller);
+    browser()->OpenURL(OpenURLParams(auth_page, Referrer(), CURRENT_TAB,
+                                     ui::PAGE_TRANSITION_TYPED, false));
+    auth_needed_waiter.Wait();
+    WindowedAuthCancelledObserver auth_cancelled_waiter(controller);
+    browser()->OpenURL(OpenURLParams(no_auth_page, Referrer(), CURRENT_TAB,
+                                     ui::PAGE_TRANSITION_TYPED, false));
+    auth_cancelled_waiter.Wait();
+    load_stop_waiter.Wait();
+    EXPECT_TRUE(observer.handlers().empty());
+  }
+}
+
+// Test login prompt cancellation on navigation to back.
+IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, TestCancelAuth_OnBack) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const GURL auth_page = embedded_test_server()->GetURL(kAuthBasicPage);
+  const GURL no_auth_page = embedded_test_server()->GetURL("/a");

[meacer, 2016/08/12 17:45:05]

Same here (kNoAuthPage)

[vabr (Chromium), 2016/08/12 19:39:37]

Done.

+
+  NavigationController* controller =
+      &browser()->tab_strip_model()->GetActiveWebContents()->GetController();
+
+  LoginPromptBrowserTestObserver observer;
+  observer.Register(content::Source<NavigationController>(controller));
+
+  // First navigate to an unauthenticated page so we have something to
+  // go back to.
+  ui_test_utils::NavigateToURL(browser(), no_auth_page);
+
+  // Navigating back while auth is requested is the same as cancelling.

[meacer, 2016/08/12 17:45:06]

nit: No braces

[vabr (Chromium), 2016/08/12 19:39:37]

Done.

+  {
+    // One LOAD_STOP event for auth_page and one for no_auth_page.
+    const int kLoadStopEvents = 2;
+    WindowedLoadStopObserver load_stop_waiter(controller, kLoadStopEvents);
+    WindowedAuthNeededObserver auth_needed_waiter(controller);
+    browser()->OpenURL(OpenURLParams(auth_page, Referrer(), CURRENT_TAB,
+                                     ui::PAGE_TRANSITION_TYPED, false));
+    auth_needed_waiter.Wait();
+    WindowedAuthCancelledObserver auth_cancelled_waiter(controller);
+    ASSERT_TRUE(controller->CanGoBack());
+    controller->GoBack();
+    auth_cancelled_waiter.Wait();
+    load_stop_waiter.Wait();
+    EXPECT_TRUE(observer.handlers().empty());
+  }
+}
+
+// Test login prompt cancellation on navigation to forward.
+// TODO(crbug.com/636875) Flaky on Mac and Linux at least.
+IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest,
+                       DISABLED_TestCancelAuth_OnForward) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const GURL auth_page = embedded_test_server()->GetURL(kAuthBasicPage);
+  const GURL no_auth_page_1 = embedded_test_server()->GetURL("/a");

[meacer, 2016/08/12 17:45:06]

nit: Seems like these could be named kNoAuthURL1 as well:

const GURL kNoAuthURL(embedded_test_server()->GetURL(kNoAuthPage1));

[vabr (Chromium), 2016/08/12 19:39:37]

Done.

+  const GURL no_auth_page_2 = embedded_test_server()->GetURL("/c");

[meacer, 2016/08/12 17:45:06]

And here.

[vabr (Chromium), 2016/08/12 19:39:37]

Done.

+
+  NavigationController* controller =
+      &browser()->tab_strip_model()->GetActiveWebContents()->GetController();
+
+  LoginPromptBrowserTestObserver observer;
+  observer.Register(content::Source<NavigationController>(controller));
+
+  ui_test_utils::NavigateToURL(browser(), no_auth_page_1);
+
+  // Now add a page and go back, so we have something to go forward to.
+  ui_test_utils::NavigateToURL(browser(), no_auth_page_2);
+  {
+    WindowedLoadStopObserver load_stop_waiter(controller, 1);
+    ASSERT_TRUE(controller->CanGoBack());
+    controller->GoBack();
+    load_stop_waiter.Wait();
+  }
+
+  {
+    // One LOAD_STOP event for auth_page and one for no_auth_page.
+    const int kLoadStopEvents = 2;
+    WindowedLoadStopObserver load_stop_waiter(controller, kLoadStopEvents);
+    WindowedAuthNeededObserver auth_needed_waiter(controller);
+    browser()->OpenURL(OpenURLParams(auth_page, Referrer(), CURRENT_TAB,
+                                     ui::PAGE_TRANSITION_TYPED, false));
+    auth_needed_waiter.Wait();
+    WindowedAuthCancelledObserver auth_cancelled_waiter(controller);
+    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) {
   ASSERT_TRUE(embedded_test_server()->Start());
@@ skipping 1045 matching lines @@
   // out.
   EXPECT_TRUE(
       WaitForRenderFrameReady(contents->GetInterstitialPage()->GetMainFrame()));
   EXPECT_TRUE(contents->ShowingInterstitialPage());
   EXPECT_EQ(SSLBlockingPage::kTypeForTesting, contents->GetInterstitialPage()
                                                   ->GetDelegateForTesting()
                                                   ->GetTypeForTesting());
 }
 
 }  // namespace