Revert "Focus tab contents in BeginNavigation case of NTP-initiated navigations."

This reverts commit b4d501e3.

Reason for revert: This CL is on the blame list of the first
failures observed for PortalBrowserTest.FocusTransfersAcrossActivation
on Mac, and it is related to focus.

Original change's description:
> Focus tab contents in BeginNavigation case of NTP-initiated navigations.
> 
> Tab contents may get focused during some renderer-initiated navigations
> (e.g. navigations from an NTP-replacement extension - see the regression
> test added in r723022).  This CL ensures that the focus decision is
> applied not only to navigations that go through OpenURL, but also to
> navigations that go through (more freqeuent, usual) BeginNavigation.
> This change helps ensure that we retain the right focus behavior after
> more navigations switch to the BeginNavigation code path (e.g. after
> relanding ShouldFork removal in https://crrev.com/c/1949448).
> 
> The CL covers both the OpenURL and BeginNavigation code paths by
> handling the focus decisions in a newly added TabContentFocusingHelper
> class (replicating/moving the focus decisions from OpenURL-only
> Browser::UpdateUIForNavigationInTab).  Adding a new tab-helper class is
> useful, because the existing, OpenURL/BeginNavigation-shared navigation
> notifications handlers (e.g. Browser::ScheduleUIUpdate) cannot
> distinguish between replaceState and new navigations (see also new steps
> in the OmniboxFocusInteractiveTest.NtpReplacementExtension test).
> 
> While this CL changes how focus behavior is implemented, it should have
> no effect on end-to-end/high-level expectations of focus behavior (as
> observed by end users or browser tests).
> 
> browser_tests do not guarantee window activation and/or focus, but before this
> CL navigation in browser_tests would focus WebContents.  This is not happening
> after this CL, and requires moving a handful of focus-dependent tests from
> browser_tests to interactive_ui_tests.
> 
> Bug: 1029161
> Change-Id: Ib57c260b2f85f276ff1dad3aabedd59d1d864c8a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1970834
> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
> Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
> Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
> Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
> Reviewed-by: Bill Budge <bbudge@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#735048}

TBR=pkasting@chromium.org,bbudge@chromium.org,creis@chromium.org,rouslan@chromium.org,lukasza@chromium.org,karandeepb@chromium.org,bsheedy@chromium.org

Change-Id: I43362d7689548761154b80b72a0734ca02569794
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1029161, 1045594
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2020862Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735100}

chrome/browser/extensions/ntp_overridden_bubble_delegate.cc

@@ -23,6 +23,10 @@
 
 namespace {
 
+// Whether the user has been notified about extension overriding the new tab
+// page.
+const char kNtpBubbleAcknowledged[] = "ack_ntp_bubble";
+
 // Whether existing NTP extensions have been automatically acknowledged.
 const char kDidAcknowledgeExistingNtpExtensions[] =
     "ack_existing_ntp_extensions";
@@ -44,9 +48,6 @@ base::LazyInstance<std::set<std::pair<Profile*, std::string>>>::Leaky
 
 namespace extensions {
 
-const char NtpOverriddenBubbleDelegate::kNtpBubbleAcknowledged[] =
-    "ack_ntp_bubble";
-
 NtpOverriddenBubbleDelegate::NtpOverriddenBubbleDelegate(Profile* profile)
     : extensions::ExtensionMessageBubbleController::Delegate(profile),
       profile_(profile) {

chrome/browser/extensions/ntp_overridden_bubble_delegate.h

@@ -19,10 +19,6 @@ namespace extensions {
 class NtpOverriddenBubbleDelegate
     : public ExtensionMessageBubbleController::Delegate {
  public:
-  // Name of the preference that says whether the user has been notified about
-  // extension overriding the new tab page.
-  static const char kNtpBubbleAcknowledged[];
-
   explicit NtpOverriddenBubbleDelegate(Profile* profile);
   ~NtpOverriddenBubbleDelegate() override;
 

chrome/browser/spellchecker/spellcheck_mac_view_interactive_uitest.mm → chrome/browser/spellchecker/spellcheck_mac_view_browsertest.mm

@@ -3,7 +3,6 @@
 // found in the LICENSE file.
 
 #import <Cocoa/Cocoa.h>
-
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -18,14 +17,13 @@
 
 namespace {
 
-class SpellCheckMacViewInteractiveUiTest : public InProcessBrowserTest {
+class SpellCheckMacViewBrowserTest : public InProcessBrowserTest {
  public:
-  SpellCheckMacViewInteractiveUiTest() {}
+  SpellCheckMacViewBrowserTest() {}
 };
 
 #if BUILDFLAG(ENABLE_SPELLCHECK)
-IN_PROC_BROWSER_TEST_F(SpellCheckMacViewInteractiveUiTest,
-                       SpellCheckPanelVisible) {
+IN_PROC_BROWSER_TEST_F(SpellCheckMacViewBrowserTest, SpellCheckPanelVisible) {
   spellcheck::SpellCheckPanelBrowserTestHelper test_helper;
 
   ASSERT_TRUE(embedded_test_server()->Start());

chrome/browser/ui/BUILD.gn

@@ -941,8 +941,6 @@ jumbo_static_library("ui") {
       "find_bar/find_bar_platform_helper.cc",
       "find_bar/find_bar_platform_helper.h",
       "find_bar/find_bar_platform_helper_mac.mm",
-      "focus_tab_after_navigation_helper.cc",
-      "focus_tab_after_navigation_helper.h",
       "global_error/global_error.cc",
       "global_error/global_error.h",
       "global_error/global_error_bubble_view_base.h",

chrome/browser/ui/browser.cc

@@ -1007,6 +1007,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);
 
@@ -1029,6 +1030,26 @@ 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);
+
+  // Figure out if the navigating contents can take focus (potentially taking it
+  // away from other, currently-focused UI element like the omnibox).
+  // Specifically, user-initiated navigations can give focus to the tab;
+  // renderer-initiated navigations usually don't, unless the NTP triggers them
+  // (in which case they're treated similarly to a user-initiated navigation).
+  //
+  // TODO(lukasza): https://crbug.com/1034999: Try to avoid special-casing
+  // kChromeUINewTabURL below and covering it via IsNTPOrRelatedURL instead.
+  const GURL& current_url = contents->GetLastCommittedURL();
+  bool contents_can_take_focus =
+      user_initiated || current_url == GURL(chrome::kChromeUINewTabURL) ||
+      search::IsNTPOrRelatedURL(
+          current_url,
+          Profile::FromBrowserContext(contents->GetBrowserContext()));
+
+  if (contents_can_take_focus && contents_is_selected &&
+      (window()->IsActive() || action == NavigateParams::SHOW_WINDOW)) {
+    contents->SetInitialFocus();
+  }
 }
 
 void Browser::RegisterKeepAlive() {
@@ -1660,7 +1681,7 @@ bool Browser::ShouldFocusLocationBarByDefault(WebContents* source) {
 
   // This should be based on the pending entry if there is one, so that
   // back/forward navigations to the NTP are handled.  The visible entry can't
-  // be used here, since back/forward navigations are not treated as visible
+  // be used here, since back/forward navigations are not treated as  visible
   // entries to avoid URL spoofs.
   content::NavigationEntry* entry =
       source->GetController().GetPendingEntry()

chrome/browser/ui/browser.h

@@ -512,6 +512,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_navigator.cc

@@ -662,7 +662,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

-// Copyright 2020 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 "chrome/browser/ui/focus_tab_after_navigation_helper.h"
-
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/search/search.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_finder.h"
-#include "chrome/browser/ui/browser_window.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "chrome/common/webui_url_constants.h"
-#include "content/public/browser/browser_url_handler.h"
-#include "content/public/browser/navigation_handle.h"
-#include "content/public/browser/web_contents.h"
-
-FocusTabAfterNavigationHelper::FocusTabAfterNavigationHelper(
-    content::WebContents* contents)
-    : content::WebContentsObserver(contents) {}
-
-FocusTabAfterNavigationHelper::~FocusTabAfterNavigationHelper() = default;
-
-void FocusTabAfterNavigationHelper::ReadyToCommitNavigation(
-    content::NavigationHandle* navigation) {
-  // Focus the tab contents if needed.  This is done at the ReadyToCommit time
-  // to:
-  // 1) ignore same-document navigations (ReadyToCommitNavigation method is not
-  //    invoked for same-document navigations)
-  // 2) postpone moving the focus until we are ready to commit the page
-  // 3) move the focus before the page starts rendering
-  // (only 1 is a hard-requirement;  2 and 3 seem desirable but there are no
-  // known scenarios where violating these requirements would lead to bugs).
-  if (ShouldFocusTabContents(navigation))
-    web_contents()->SetInitialFocus();
-}
-
-bool FocusTabAfterNavigationHelper::ShouldFocusTabContents(
-    content::NavigationHandle* navigation) {
-  // Don't focus content in an inactive window or tab.
-  Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
-  if (!browser)
-    return false;
-  if (!browser->window()->IsActive())
-    return false;
-  if (browser->tab_strip_model()->GetActiveWebContents() != web_contents())
-    return false;
-
-  // Don't focus content after subframe navigations.
-  if (!navigation->IsInMainFrame())
-    return false;
-
-  // Browser-initiated navigations (e.g. typing in an omnibox) should focus
-  // the tab contents.
-  if (!navigation->IsRendererInitiated())
-    return true;
-
-  // Renderer-initiated navigations shouldn't focus the tab contents, unless the
-  // navigation is leaving the NTP.  See also https://crbug.com/1027719.
-  bool started_at_ntp = IsNtpURL(web_contents()->GetLastCommittedURL());
-  if (!started_at_ntp)
-    return false;
-
-  // Rewrite chrome://newtab to compare with the navigation URL.
-  GURL rewritten_ntp_url = web_contents()->GetLastCommittedURL();
-  bool ignored_reverse_on_redirect;
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
-  content::BrowserURLHandler::GetInstance()->RewriteURLIfNecessary(
-      &rewritten_ntp_url, profile, &ignored_reverse_on_redirect);
-
-  // Focus if the destination is not the NTP.
-  return !IsNtpURL(navigation->GetURL()) &&
-         (navigation->GetURL() != rewritten_ntp_url);
-}
-
-bool FocusTabAfterNavigationHelper::IsNtpURL(const GURL& url) {
-  // TODO(lukasza): https://crbug.com/1034999: Try to avoid special-casing
-  // kChromeUINewTabURL below and covering it via IsNTPOrRelatedURL instead.
-  if (url == GURL(chrome::kChromeUINewTabURL))
-    return true;
-
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
-  return search::IsNTPOrRelatedURL(url, profile);
-}
-
-WEB_CONTENTS_USER_DATA_KEY_IMPL(FocusTabAfterNavigationHelper)

chrome/browser/ui/focus_tab_after_navigation_helper.h

-// Copyright 2020 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.
-
-#ifndef CHROME_BROWSER_UI_FOCUS_TAB_AFTER_NAVIGATION_HELPER_H_
-#define CHROME_BROWSER_UI_FOCUS_TAB_AFTER_NAVIGATION_HELPER_H_
-
-#include "content/public/browser/web_contents_observer.h"
-#include "content/public/browser/web_contents_user_data.h"
-
-// FocusTabAfterNavigationHelper focuses the tab contents (potentially taking
-// focus away from other browser elements like the omnibox) after
-// 1) browser-initiated navigations (e.g. after omnibox- or bookmark-initiated
-//    navigations)
-// 2) navigations that leave NTP (e.g. after an NTP-replacement extension or
-//    third-party NTP executes window.location = ...).
-class FocusTabAfterNavigationHelper
-    : public content::WebContentsObserver,
-      public content::WebContentsUserData<FocusTabAfterNavigationHelper> {
- public:
-  ~FocusTabAfterNavigationHelper() override;
-
-  // Not copyable nor movable.
-  FocusTabAfterNavigationHelper(const FocusTabAfterNavigationHelper&) = delete;
-  FocusTabAfterNavigationHelper& operator=(
-      const FocusTabAfterNavigationHelper&) = delete;
-
-  // content::WebContentsObserver
-  void ReadyToCommitNavigation(content::NavigationHandle* navigation) override;
-
- private:
-  friend class content::WebContentsUserData<FocusTabAfterNavigationHelper>;
-
-  explicit FocusTabAfterNavigationHelper(content::WebContents* contents);
-
-  bool ShouldFocusTabContents(content::NavigationHandle* navigation);
-  bool IsNtpURL(const GURL& url);
-
-  WEB_CONTENTS_USER_DATA_KEY_DECL();
-};
-
-#endif  // CHROME_BROWSER_UI_FOCUS_TAB_AFTER_NAVIGATION_HELPER_H_

chrome/browser/ui/tab_helpers.cc

@@ -69,7 +69,6 @@
 #include "chrome/browser/ui/blocked_content/popup_blocker_tab_helper.h"
 #include "chrome/browser/ui/blocked_content/popup_opener_tab_helper.h"
 #include "chrome/browser/ui/find_bar/find_bar_state.h"
-#include "chrome/browser/ui/focus_tab_after_navigation_helper.h"
 #include "chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h"
 #include "chrome/browser/ui/navigation_correction_tab_observer.h"
 #include "chrome/browser/ui/passwords/manage_passwords_ui_controller.h"
@@ -315,7 +314,6 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
   extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
       web_contents);
   extensions::WebNavigationTabObserver::CreateForWebContents(web_contents);
-  FocusTabAfterNavigationHelper::CreateForWebContents(web_contents);
   FormInteractionTabHelper::CreateForWebContents(web_contents);
   FramebustBlockTabHelper::CreateForWebContents(web_contents);
   IntentPickerTabHelper::CreateForWebContents(web_contents);

chrome/browser/vr/webxr_vr_spatial_tracking_test.cc

@@ -12,7 +12,6 @@ namespace vr {
 // are no runtimes available.
 IN_PROC_BROWSER_TEST_F(WebXrVrRuntimelessBrowserTest,
                        TestInlineIdentityAlwaysAvailable) {
-  browser()->tab_strip_model()->GetActiveWebContents()->Focus();
   LoadUrlAndAwaitInitialization(
       GetFileUrlForHtmlTestFile("test_inline_viewer_available"));
   WaitOnJavaScriptStep();

chrome/test/BUILD.gn

@@ -1190,6 +1190,7 @@ if (!is_android) {
       "../browser/speech/extension_api/tts_extension_apitest.cc",
       "../browser/speech/speech_recognition_browsertest.cc",
       "../browser/speech/speech_recognizer_browsertest.cc",
+      "../browser/spellchecker/spellcheck_mac_view_browsertest.mm",
       "../browser/spellchecker/spellcheck_service_browsertest.cc",
       "../browser/ssl/certificate_reporting_test_utils.cc",
       "../browser/ssl/certificate_reporting_test_utils.h",
@@ -5600,7 +5601,6 @@ if (!is_android) {
       "../browser/resource_coordinator/tab_metrics_logger_interactive_uitest.cc",
       "../browser/site_isolation/site_per_process_interactive_browsertest.cc",
       "../browser/site_isolation/site_per_process_text_input_browsertest.cc",
-      "../browser/spellchecker/spellcheck_mac_view_interactive_uitest.mm",
       "../browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc",
       "../browser/ui/blocked_content/popup_blocker_browsertest.cc",
       "../browser/ui/browser_command_controller_interactive_browsertest.cc",

chrome/test/ppapi/ppapi_browsertest.cc

@@ -260,6 +260,16 @@ TEST_PPAPI_NACL(TraceEvent)
 
 TEST_PPAPI_NACL(InputEvent)
 
+// Flaky on Linux and Windows. http://crbug.com/135403
+#if defined(OS_LINUX) || defined(OS_WIN)
+#define MAYBE_ImeInputEvent DISABLED_ImeInputEvent
+#else
+#define MAYBE_ImeInputEvent ImeInputEvent
+#endif
+
+TEST_PPAPI_OUT_OF_PROCESS(MAYBE_ImeInputEvent)
+TEST_PPAPI_NACL(MAYBE_ImeInputEvent)
+
 // Graphics2D_Dev isn't supported in NaCl, only test the other interfaces
 // TODO(jhorwich) Enable when Graphics2D_Dev interfaces are proxied in NaCl.
 TEST_PPAPI_NACL(Graphics2D_InvalidResource)

chrome/test/ppapi/ppapi_interactive_browsertest.cc

@@ -9,28 +9,20 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
-#include "ppapi/shared_impl/test_utils.h"
+
+//
+// Interface tests.
+//
 
 // Disable tests under ASAN.  http://crbug.com/104832.
 // This is a bit heavy handed, but the majority of these tests fail under ASAN.
 // See bug for history.
-#if defined(ADDRESS_SANITIZER)
-#define MAYBE_MouseLock_SucceedWhenAllowed DISABLED_MouseLock_SucceedWhenAllowed
-#else
-#define MAYBE_MouseLock_SucceedWhenAllowed MouseLock_SucceedWhenAllowed
-#endif  // ADDRESS_SANITIZER
+#if !defined(ADDRESS_SANITIZER)
+
 // Disabled due to timeouts: http://crbug.com/136548
-IN_PROC_BROWSER_TEST_F(OutOfProcessPPAPITest,
-                       MAYBE_MouseLock_SucceedWhenAllowed) {
+IN_PROC_BROWSER_TEST_F(
+    OutOfProcessPPAPITest, DISABLED_MouseLock_SucceedWhenAllowed) {
   RunTestViaHTTP("MouseLock_SucceedWhenAllowed");
 }
 
-// Flaky on Linux and Windows. http://crbug.com/135403
-#if defined(OS_LINUX) || defined(OS_WIN)
-#define MAYBE_ImeInputEvent DISABLED_ImeInputEvent
-#else
-#define MAYBE_ImeInputEvent ImeInputEvent
-#endif
-IN_PROC_BROWSER_TEST_F(OutOfProcessPPAPITest, MAYBE_ImeInputEvent) {
-  RunTest(ppapi::StripTestPrefixes("ImeInputEvent"));
-}
+#endif // ADDRESS_SANITIZER