Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2244643002:
Split LoginPromptBrowserTest.TestCancelAuth (Closed)
|
Created:
4 years, 4 months ago by vabr (Chromium) Modified:
4 years, 4 months ago CC:
chromium-reviews, dzhioev+watch_chromium.org, achuith+watch_chromium.org, vabr+watchlistlogin_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit LoginPromptBrowserTest.TestCancelAuth
The TestCancelAuth test contains 4 different test scenarios. This CL makes them separate test cases.
The CL also changes the chrome::[Can]Go* calls to calling the navigation controller for them directly (that's what the static version did anyway) and does minor readability improvements.
BUG=636875
R=kolos@chromium.org
Committed: https://crrev.com/9544b8426ac4cc3aafdca87a56426de3974f9f04
Cr-Commit-Position: refs/heads/master@{#411751}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Comments #
Dependent Patchsets: Messages
Total messages: 21 (14 generated)
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Split LoginPromptBrowserTest.TestCancelAuth The TestCancelAuth test contains 4 different test scenarios. This CL makes them to separate test cases. BUG=636875 R=kolos@chromium.org ========== to ========== Split LoginPromptBrowserTest.TestCancelAuth The TestCancelAuth test contains 4 different test scenarios. This CL makes them separate test cases. The CL also changes the chrome::[Can]Go* calls to calling the navigation controller for them directly (that's what the static version did anyway) and does minor readability improvements. BUG=636875 R=kolos@chromium.org ==========
Hi Maxim, Could you PTAL? Thanks! Vaclav
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
vabr@chromium.org changed reviewers: + meacer@chromium.org
Maxim has been sheriffing, so I'll add a fellow OWNER Mustafa Emre Acer just in case he has time to review this instead. Cheers, Vaclav
Just some minor nits. Otherwise LGTM. Thanks! https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... File chrome/browser/ui/login/login_handler_browsertest.cc (right): https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:331: { No need for braces anymore. https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:351: const GURL no_auth_page = embedded_test_server()->GetURL("/b"); You might want to use kNoAuthPage here too, instead of /b https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:360: { No need for braces. https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:381: const GURL no_auth_page = embedded_test_server()->GetURL("/a"); Same here (kNoAuthPage) https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:393: // Navigating back while auth is requested is the same as cancelling. nit: No braces https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:417: const GURL no_auth_page_1 = embedded_test_server()->GetURL("/a"); nit: Seems like these could be named kNoAuthURL1 as well: const GURL kNoAuthURL(embedded_test_server()->GetURL(kNoAuthPage1)); https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:418: const GURL no_auth_page_2 = embedded_test_server()->GetURL("/c"); And here.
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the prompt review! Comments addressed, will send to the CQ soon. Cheers, Vaclav https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... File chrome/browser/ui/login/login_handler_browsertest.cc (right): https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:331: { On 2016/08/12 17:45:06, Mustafa Emre Acer wrote: > No need for braces anymore. Done. https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:351: const GURL no_auth_page = embedded_test_server()->GetURL("/b"); On 2016/08/12 17:45:05, Mustafa Emre Acer wrote: > You might want to use kNoAuthPage here too, instead of /b Done. https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:360: { On 2016/08/12 17:45:06, Mustafa Emre Acer wrote: > No need for braces. 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. https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:381: const GURL no_auth_page = embedded_test_server()->GetURL("/a"); On 2016/08/12 17:45:05, Mustafa Emre Acer wrote: > Same here (kNoAuthPage) Done. https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:393: // Navigating back while auth is requested is the same as cancelling. On 2016/08/12 17:45:06, Mustafa Emre Acer wrote: > nit: No braces Done. https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:417: const GURL no_auth_page_1 = embedded_test_server()->GetURL("/a"); On 2016/08/12 17:45:06, Mustafa Emre Acer wrote: > nit: Seems like these could be named kNoAuthURL1 as well: > > const GURL kNoAuthURL(embedded_test_server()->GetURL(kNoAuthPage1)); Done. https://codereview.chromium.org/2244643002/diff/1/chrome/browser/ui/login/log... chrome/browser/ui/login/login_handler_browsertest.cc:418: const GURL no_auth_page_2 = embedded_test_server()->GetURL("/c"); On 2016/08/12 17:45:06, Mustafa Emre Acer wrote: > And here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2244643002/#ps20001 (title: "Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Split LoginPromptBrowserTest.TestCancelAuth The TestCancelAuth test contains 4 different test scenarios. This CL makes them separate test cases. The CL also changes the chrome::[Can]Go* calls to calling the navigation controller for them directly (that's what the static version did anyway) and does minor readability improvements. BUG=636875 R=kolos@chromium.org ========== to ========== Split LoginPromptBrowserTest.TestCancelAuth The TestCancelAuth test contains 4 different test scenarios. This CL makes them separate test cases. The CL also changes the chrome::[Can]Go* calls to calling the navigation controller for them directly (that's what the static version did anyway) and does minor readability improvements. BUG=636875 R=kolos@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Split LoginPromptBrowserTest.TestCancelAuth The TestCancelAuth test contains 4 different test scenarios. This CL makes them separate test cases. The CL also changes the chrome::[Can]Go* calls to calling the navigation controller for them directly (that's what the static version did anyway) and does minor readability improvements. BUG=636875 R=kolos@chromium.org ========== to ========== Split LoginPromptBrowserTest.TestCancelAuth The TestCancelAuth test contains 4 different test scenarios. This CL makes them separate test cases. The CL also changes the chrome::[Can]Go* calls to calling the navigation controller for them directly (that's what the static version did anyway) and does minor readability improvements. BUG=636875 R=kolos@chromium.org Committed: https://crrev.com/9544b8426ac4cc3aafdca87a56426de3974f9f04 Cr-Commit-Position: refs/heads/master@{#411751} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9544b8426ac4cc3aafdca87a56426de3974f9f04 Cr-Commit-Position: refs/heads/master@{#411751} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
