Focus tab earlier for browser-initiated, //chrome-layer navigations.

This CL changes how navigations result in focusing the tab contents:

1. Browser-initiated navigations:
   - Before this CL: Focus done in FocusTabAfterNavigationHelper
     which covers both OpenURL and BeginNavigation code paths.
   - After this CL: Focus done in Browser::UpdateUIForNavigationInTab
     (similarly to pre-r737502) which covers only the OpenURL code path.

2. NTP-initiated navigations:
   - No change: Focus done in FocusTabAfterNavigationHelper.

This CL makes sure that the text in the Omnibox is not temporarily
selected during a navigation, by ensuring that the Omnibox is not
focused when OmniboxViewViews::Update runs (see also
https://crbug.com/1048742#c4 and #c5 and #c7).  Regression tests
are added for this problem.

This CL also makes sure that DevTools-initiated navigations
(browser-initiated navigations which go through the BeginNavigation
path) do not result in focus changes (see also
https://crbug.com/1048591).

This CL might also (speculating here) address the performance bugs
tracked in bugs 1048333, 1048556, 1048565, 1048988, 1048988.

Fixed: 1048742, 1048591
Bug: 1048333, 1048556, 1048565, 1048988, 1048988
Change-Id: Ia00cd9143991c260615663fae06f096e9d7cf0f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2040234
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739179}

chrome/browser/ui/browser.cc

@@ -1017,6 +1017,7 @@ bool Browser::CanSaveContents(content::WebContents* web_contents) const {
 
 void Browser::UpdateUIForNavigationInTab(WebContents* contents,
                                          ui::PageTransition transition,
+                                         NavigateParams::WindowAction action,
                                          bool user_initiated) {
   tab_strip_model_->TabNavigating(contents, transition);
 
@@ -1039,6 +1040,17 @@ void Browser::UpdateUIForNavigationInTab(WebContents* contents,
   // the throbber will show the default favicon for a split second when
   // navigating away from the new tab page.
   ScheduleUIUpdate(contents, content::INVALIDATE_TYPE_URL);
+
+  // Navigating contents can take focus (potentially taking it away from other,
+  // currently-focused UI element like the omnibox) if the navigation was
+  // initiated by the user (e.g., via omnibox, bookmarks, etc.).
+  //
+  // Note that focusing contents of NTP-initiated navigations is taken care of
+  // elsewhere - see FocusTabAfterNavigationHelper.
+  if (user_initiated && contents_is_selected &&
+      (window()->IsActive() || action == NavigateParams::SHOW_WINDOW)) {
+    contents->SetInitialFocus();
+  }
 }
 
 void Browser::RegisterKeepAlive() {

chrome/browser/ui/browser.h

@@ -513,6 +513,7 @@ class Browser : public TabStripModelObserver,
   // this Browser. Updates the UI for the start of this navigation.
   void UpdateUIForNavigationInTab(content::WebContents* contents,
                                   ui::PageTransition transition,
+                                  NavigateParams::WindowAction action,
                                   bool user_initiated);
 
   // Used to register a KeepAlive to affect the Chrome lifetime. The KeepAlive

chrome/browser/ui/browser_focus_uitest.cc

@@ -29,6 +29,7 @@
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/test/base/chrome_test_utils.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -642,6 +643,43 @@ IN_PROC_BROWSER_TEST_F(BrowserFocusTest, DISABLED_FocusAfterCrashedTab) {
   ASSERT_TRUE(IsViewFocused(VIEW_ID_TAB_CONTAINER));
 }
 
+// Tests that when omnibox triggers a navigation, then the focus is moved into
+// the current tab.
+IN_PROC_BROWSER_TEST_F(BrowserFocusTest, NavigateFromOmnibox) {
+  const GURL url = embedded_test_server()->GetURL("/title1.html");
+
+  // Focus the Omnibox.
+  ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+  chrome::FocusLocationBar(browser());
+  OmniboxView* view = browser()->window()->GetLocationBar()->GetOmniboxView();
+
+  // Simulate typing a URL into the omnibox.
+  view->SetUserText(base::UTF8ToUTF16(url.spec()));
+  EXPECT_TRUE(IsViewFocused(VIEW_ID_OMNIBOX));
+  EXPECT_FALSE(view->IsSelectAll());
+
+  // Simulate pressing Enter.
+  content::WebContents* web_contents =
+      chrome_test_utils::GetActiveWebContents(this);
+  content::TestNavigationObserver nav_observer(web_contents, 1);
+  ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_RETURN, false,
+                                              false, false, false));
+  // Verify that a navigation has started.
+  EXPECT_TRUE(web_contents->GetController().GetPendingEntry());
+  // Verify that the Omnibox text is not selected - this is a regression test
+  // for https://crbug.com/1048742.
+  EXPECT_FALSE(view->IsSelectAll());
+  // Intentionally not asserting anything about IsViewFocused in this
+  // _intermediate_ state.
+
+  // Wait for the navigation to finish and verify final, steady state.
+  nav_observer.Wait();
+  EXPECT_TRUE(nav_observer.last_navigation_succeeded());
+  EXPECT_EQ(url, web_contents->GetLastCommittedURL());
+  EXPECT_TRUE(IsViewFocused(VIEW_ID_TAB_CONTAINER));
+  EXPECT_FALSE(view->IsSelectAll());
+}
+
 // Tests that when a new tab is opened from the omnibox, the focus is moved from
 // the omnibox for the current tab.
 IN_PROC_BROWSER_TEST_F(BrowserFocusTest, NavigateFromOmniboxIntoNewTab) {

chrome/browser/ui/browser_navigator.cc

@@ -676,7 +676,8 @@ void Navigate(NavigateParams* params) {
        params->disposition == WindowOpenDisposition::CURRENT_TAB)) {
     // The navigation occurred in the source tab.
     params->browser->UpdateUIForNavigationInTab(
-        contents_to_navigate_or_insert, params->transition, user_initiated);
+        contents_to_navigate_or_insert, params->transition,
+        params->window_action, user_initiated);
   } else if (singleton_index == -1) {
     // If some non-default value is set for the index, we should tell the
     // TabStripModel to respect it.

chrome/browser/ui/focus_tab_after_navigation_helper.cc

@@ -50,10 +50,11 @@ bool FocusTabAfterNavigationHelper::ShouldFocusTabContents(
   if (!navigation->IsInMainFrame())
     return false;
 
-  // Browser-initiated navigations (e.g. typing in an omnibox) should focus
-  // the tab contents.
+  // Browser-initiated navigations (e.g. typing in an omnibox) are taken care of
+  // in Browser::UpdateUIForNavigationInTab.  See also https://crbug.com/1048591
+  // for possible regression risks related to returning |true| here.
   if (!navigation->IsRendererInitiated())
-    return true;
+    return false;
 
   // Renderer-initiated navigations shouldn't focus the tab contents, unless the
   // navigation is leaving the NTP.  See also https://crbug.com/1027719.