[omnibox] Move switch-to-tab handling into browser stack

The Omnibox was calling SwitchToTabWithURL() directly from OpenMatch()
for tab-switch suggestions. This change creates a special disposition
for the suggestions and adds support for it within the browser stack.

Bug: 780835
Change-Id: I4f48e6a7a4b46cf9c1a88baedafc87aaf763ebef
Reviewed-on: https://chromium-review.googlesource.com/809527Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Kevin Bailey <krb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523573}

chrome/browser/ui/browser_navigator.cc

@@ -22,6 +22,7 @@
 #include "chrome/browser/task_manager/web_contents_tags.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/location_bar/location_bar.h"
@@ -137,8 +138,27 @@ Browser* GetBrowserForDisposition(NavigateParams* params) {
   }
 
   Profile* profile = params->initiating_profile;
+  Browser* current_browser = params->browser;
 
   switch (params->disposition) {
+    case WindowOpenDisposition::SWITCH_TO_TAB:
+#if !defined(OS_ANDROID)
+      for (auto* browser : *BrowserList::GetInstance()) {
+        // Only look at same profile (and anonymity level).
+        if (browser->profile()->IsSameProfile(profile) &&
+            browser->profile()->GetProfileType() == profile->GetProfileType()) {
+          params->browser = browser;
+          int index = GetIndexOfExistingTab(params);
+          if (index >= 0) {
+            params->browser = current_browser;
+            params->tab_switch_hint = index;
+            return browser;
+          }
+        }
+      }
+      params->browser = current_browser;
+#endif  // !defined(OS_ANDROID)
+    // fall through
     case WindowOpenDisposition::CURRENT_TAB:
       if (params->browser)
         return params->browser;
@@ -532,7 +552,7 @@ void Navigate(NavigateParams* params) {
                                    ui::PAGE_TRANSITION_KEYWORD);
 
   // Check if this is a singleton tab that already exists
-  int singleton_index = GetIndexOfSingletonTab(params);
+  int singleton_index = GetIndexOfExistingTab(params);
 
   // Did we use a prerender?
   bool swapped_in_prerender = false;
@@ -612,6 +632,11 @@ void Navigate(NavigateParams* params) {
   }
 
   if (singleton_index >= 0) {
+    // If switching browsers, make sure it is shown.
+    if (params->disposition == WindowOpenDisposition::SWITCH_TO_TAB &&
+        params->browser != source_browser)
+      params->window_action = NavigateParams::SHOW_WINDOW;
+
     WebContents* target =
         params->browser->tab_strip_model()->GetWebContentsAt(singleton_index);
 
@@ -624,6 +649,18 @@ void Navigate(NavigateParams* params) {
 
     // If the singleton tab isn't already selected, select it.
     if (params->source_contents != params->target_contents) {
+      if (params->disposition == WindowOpenDisposition::SWITCH_TO_TAB) {
+        // Close orphaned NTPs with no history when the user switches away from
+        // them.
+        if (params->source_contents->GetController().CanGoBack() ||
+            (params->source_contents->GetLastCommittedURL().spec() !=
+                 chrome::kChromeUINewTabURL &&
+             params->source_contents->GetLastCommittedURL().spec() !=
+                 chrome::kChromeSearchLocalNtpUrl))
+          params->source_contents->Focus();
+        else
+          params->source_contents->Close();
+      }
       params->browser->tab_strip_model()->ActivateTabAt(singleton_index,
                                                         user_initiated);
     }

chrome/browser/ui/browser_navigator_browsertest.cc

@@ -1383,7 +1383,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, ViewSourceIsntSingleton) {
   NavigateParams singleton_params(browser(), GURL(chrome::kChromeUIVersionURL),
                                   ui::PAGE_TRANSITION_LINK);
   singleton_params.disposition = WindowOpenDisposition::SINGLETON_TAB;
-  EXPECT_EQ(-1, GetIndexOfSingletonTab(&singleton_params));
+  EXPECT_EQ(-1, GetIndexOfExistingTab(&singleton_params));
 }
 
 // This test verifies that browser initiated navigations can send requests

chrome/browser/ui/browser_navigator_params.h

@@ -245,6 +245,9 @@ struct NavigateParams {
   // an about:blank or a data url navigation.
   scoped_refptr<content::SiteInstance> source_site_instance;
 
+  // If non-default, provides a hint to GetIndexOfSingleton() where to start.
+  int tab_switch_hint = -1;
+
  private:
   NavigateParams();
 };

chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm

@@ -819,6 +819,11 @@ bool OmniboxViewMac::OnDoCommandBySelector(SEL cmd) {
   // |-noop:| is sent when the user presses Cmd+Return. Override the no-op
   // behavior with the proper WindowOpenDisposition.
   NSEvent* event = [NSApp currentEvent];
+  if (([event type] == NSKeyDown || [event type] == NSKeyUp) &&
+      [event keyCode] == kVK_Shift) {
+    OnShiftKeyChanged([event type] == NSKeyDown);
+    return true;
+  }
   if (cmd == @selector(insertNewline:) ||
      (cmd == @selector(noop:) &&
       ([event type] == NSKeyDown || [event type] == NSKeyUp) &&
@@ -883,6 +888,7 @@ void OmniboxViewMac::OnSetFocus(bool control_down) {
 }
 
 void OmniboxViewMac::OnKillFocus() {
+  OnShiftKeyChanged(false);
   // Tell the model to reset itself.
   model()->OnWillKillFocus();
   model()->OnKillFocus();

chrome/browser/ui/omnibox/chrome_omnibox_edit_controller.cc

@@ -39,49 +39,6 @@ void ChromeOmniboxEditController::OnInputInProgress(bool in_progress) {
   UpdateWithoutTabRestore();
 }
 
-bool ChromeOmniboxEditController::SwitchToTabWithURL(const std::string& url,
-                                                     bool close_this) {
-#if !defined(OS_ANDROID)
-  content::WebContents* old_web_contents = GetWebContents();
-  Profile* profile =
-      Profile::FromBrowserContext(old_web_contents->GetBrowserContext());
-  if (!profile)
-    return false;
-  for (auto* browser : *BrowserList::GetInstance()) {
-    if (!browser->profile()->IsSameProfile(profile) ||
-        browser->profile()->GetProfileType() != profile->GetProfileType()) {
-      continue;
-    }
-
-    for (int i = 0; i < browser->tab_strip_model()->count(); ++i) {
-      content::WebContents* new_web_contents =
-          browser->tab_strip_model()->GetWebContentsAt(i);
-      // If the tab in question is the currently active tab, skip over it,
-      // under the assumption that the user had in mind another tab with this
-      // URL.  If none is found, we'll return false and simply navigate again.
-      if (new_web_contents == old_web_contents ||
-          new_web_contents->GetLastCommittedURL() != url) {
-        continue;
-      }
-
-      if (close_this) {
-        old_web_contents->Close();
-      } else {
-        // Transfer focus, from the Omnibox, to the tab so that when focus
-        // comes back to the tab, it's focused on the content.
-        // TODO(crbug.com/46623): No idea how appropriate this approach is.  I
-        // have this feeling that we should add another command to
-        // BrowserCommandController::ExecuteCommandWithDisposition().
-        old_web_contents->Focus();
-      }
-      new_web_contents->GetDelegate()->ActivateContents(new_web_contents);
-      return true;
-    }
-  }
-#endif  // !defined(OS_ANDROID)
-  return false;
-}
-
 ChromeOmniboxEditController::ChromeOmniboxEditController(
     CommandUpdater* command_updater)
     : command_updater_(command_updater) {}

chrome/browser/ui/omnibox/chrome_omnibox_edit_controller.h

@@ -23,7 +23,6 @@ class ChromeOmniboxEditController : public OmniboxEditController {
                             ui::PageTransition transition,
                             AutocompleteMatchType::Type type) override;
   void OnInputInProgress(bool in_progress) override;
-  bool SwitchToTabWithURL(const std::string& url, bool close_this) override;
 
   // Returns the WebContents of the currently active tab.
   virtual content::WebContents* GetWebContents() = 0;

chrome/browser/ui/singleton_tabs.cc

@@ -52,7 +52,7 @@ void ShowSingletonTabOverwritingNTP(Browser* browser,
     if ((contents_url == chrome::kChromeUINewTabURL ||
          search::IsInstantNTP(contents) ||
          contents_url == url::kAboutBlankURL) &&
-        GetIndexOfSingletonTab(&local_params) < 0) {
+        GetIndexOfExistingTab(&local_params) < 0) {
       local_params.disposition = WindowOpenDisposition::CURRENT_TAB;
     }
   }
@@ -72,8 +72,9 @@ NavigateParams GetSingletonTabNavigateParams(Browser* browser,
 
 // Returns the index of an existing singleton tab in |params->browser| matching
 // the URL specified in |params|.
-int GetIndexOfSingletonTab(NavigateParams* params) {
-  if (params->disposition != WindowOpenDisposition::SINGLETON_TAB)
+int GetIndexOfExistingTab(NavigateParams* params) {
+  if (params->disposition != WindowOpenDisposition::SINGLETON_TAB &&
+      params->disposition != WindowOpenDisposition::SWITCH_TO_TAB)
     return -1;
 
   // In case the URL was rewritten by the BrowserURLHandler we need to ensure
@@ -87,8 +88,13 @@ int GetIndexOfSingletonTab(NavigateParams* params) {
       &reverse_on_redirect);
 
   // If there are several matches: prefer the active tab by starting there.
-  int start_index =
-      std::max(0, params->browser->tab_strip_model()->active_index());
+  int start_index;
+  if (params->disposition == WindowOpenDisposition::SINGLETON_TAB) {
+    start_index =
+        std::max(0, params->browser->tab_strip_model()->active_index());
+  } else {
+    start_index = std::max(0, params->tab_switch_hint);
+  }
   int tab_count = params->browser->tab_strip_model()->count();
   for (int i = 0; i < tab_count; ++i) {
     int tab_index = (start_index + i) % tab_count;

chrome/browser/ui/singleton_tabs.h

@@ -31,8 +31,8 @@ void ShowSingletonTabOverwritingNTP(Browser* browser,
 // Creates a NavigateParams struct for a singleton tab navigation.
 NavigateParams GetSingletonTabNavigateParams(Browser* browser, const GURL& url);
 
-// If the given navigational URL is a Singleton, return the tab index for it.
-// Otherwise, returns -1.
-int GetIndexOfSingletonTab(NavigateParams* params);
+// If the given navigational URL is already open in |params->browser|, return
+// the tab index for it. Otherwise, returns -1.
+int GetIndexOfExistingTab(NavigateParams* params);
 
 #endif  // CHROME_BROWSER_UI_SINGLETON_TABS_H_

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -873,6 +873,8 @@ void OmniboxViewViews::OnBlur() {
   else
     CloseOmniboxPopup();
 
+  OnShiftKeyChanged(false);
+
   // Tell the model to reset itself.
   model()->OnKillFocus();
 
@@ -988,6 +990,8 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
     // The omnibox contents may change while the control key is pressed.
     if (event.key_code() == ui::VKEY_CONTROL)
       model()->OnControlKeyChanged(false);
+    else if (event.key_code() == ui::VKEY_SHIFT)
+      OnShiftKeyChanged(false);
 
     return false;
   }
@@ -1013,6 +1017,9 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
     case ui::VKEY_CONTROL:
       model()->OnControlKeyChanged(true);
       break;
+    case ui::VKEY_SHIFT:
+      OnShiftKeyChanged(true);
+      break;
     case ui::VKEY_DELETE:
       if (shift && model()->popup_model()->IsOpen())
         model()->popup_model()->TryDeletingCurrentItem();

components/omnibox/browser/omnibox_edit_controller.cc

@@ -14,11 +14,6 @@ void OmniboxEditController::OnAutocompleteAccept(
   transition_ = transition;
 }
 
-bool OmniboxEditController::SwitchToTabWithURL(const std::string& url,
-                                               bool close_this) {
-  return false;
-}
-
 OmniboxEditController::OmniboxEditController()
     : disposition_(WindowOpenDisposition::CURRENT_TAB),
       transition_(ui::PageTransitionFromInt(

components/omnibox/browser/omnibox_edit_controller.h

@@ -31,11 +31,6 @@ class OmniboxEditController {
   virtual ToolbarModel* GetToolbarModel() = 0;
   virtual const ToolbarModel* GetToolbarModel() const = 0;
 
-  // Called to search for a tab with given URL within the profile.  If found,
-  // the tab is switched to if it is not the current tab, the current tab is
-  // closed if requested, and true is returned; returns false if not found.
-  virtual bool SwitchToTabWithURL(const std::string& url, bool close_this);
-
  protected:
   OmniboxEditController();
   virtual ~OmniboxEditController();

components/omnibox/browser/omnibox_edit_model.cc

@@ -692,17 +692,6 @@ void OmniboxEditModel::OpenMatch(AutocompleteMatch match,
     base::RecordAction(
         base::UserMetricsAction("OmniboxDestinationURLIsSearchOnDSP"));
   }
-  if (match.type == AutocompleteMatchType::TAB_SEARCH) {
-    // Only close the current tab if it's the new tab page.  Look at
-    // |permanent_text_| to do this identification.  If this fails (the
-    // previously found tab may have closed or navigated since) fall through and
-    // navigate to the URL normally.
-    // TODO(crbug.com/46623): We want to close all new tab pages (including
-    // those that are replaced by an extension), not just the built-in.
-    if (controller_->SwitchToTabWithURL(match.destination_url.spec(),
-                                        permanent_text_.empty()))
-      return;
-  }
   if (match.destination_url.is_valid()) {
     // This calls RevertAll again.
     base::AutoReset<bool> tmp(&in_revert_, true);

components/omnibox/browser/omnibox_view.cc

@@ -87,6 +87,11 @@ void OmniboxView::OpenMatch(const AutocompleteMatch& match,
   // Invalid URLs such as chrome://history can end up here.
   if (!match.destination_url.is_valid() || !model_)
     return;
+  // Unless user requests navigation, change disposition for this match type
+  // so downstream will switch tabs.
+  if (match.type == AutocompleteMatchType::TAB_SEARCH && !shift_key_down_ &&
+      disposition == WindowOpenDisposition::CURRENT_TAB)
+    disposition = WindowOpenDisposition::SWITCH_TO_TAB;
   model_->OpenMatch(
       match, disposition, alternate_nav_url, pasted_text, selected_line);
 }
@@ -187,7 +192,7 @@ OmniboxView::StateChanges OmniboxView::GetStateChanges(const State& before,
 
 OmniboxView::OmniboxView(OmniboxEditController* controller,
                          std::unique_ptr<OmniboxClient> client)
-    : controller_(controller) {
+    : controller_(controller), shift_key_down_(false) {
   // |client| can be null in tests.
   if (client) {
     model_.reset(new OmniboxEditModel(this, controller, std::move(client)));

components/omnibox/browser/omnibox_view.h

@@ -279,12 +279,15 @@ class OmniboxView {
   void UpdateTextStyle(const base::string16& display_text,
                        const AutocompleteSchemeClassifier& classifier);
 
+  void OnShiftKeyChanged(bool down) { shift_key_down_ = down; }
+
  private:
   friend class OmniboxViewMacTest;
 
   // |model_| can be NULL in tests.
   std::unique_ptr<OmniboxEditModel> model_;
   OmniboxEditController* controller_;
+  bool shift_key_down_;
 
   DISALLOW_COPY_AND_ASSIGN(OmniboxView);
 };

ui/base/mojo/window_open_disposition.mojom

@@ -21,4 +21,11 @@ enum WindowOpenDisposition {
   SAVE_TO_DISK,
   OFF_THE_RECORD,
   IGNORE_ACTION,
+  // Activates an existing tab containing the url, rather than navigating.
+  // This is similar to SINGLETON_TAB, but searches across all windows from
+  // the current profile and anonymity (instead of just the current one);
+  // closes the current tab on switching if the current tab was the NTP with
+  // no session history; and behaves like CURRENT_TAB instead of
+  // NEW_FOREGROUND_TAB when no existing tab is found.
+  SWITCH_TO_TAB,
 };

ui/base/window_open_disposition.h

@@ -22,8 +22,15 @@ enum class WindowOpenDisposition {
   SAVE_TO_DISK,
   OFF_THE_RECORD,
   IGNORE_ACTION,
+  // Activates an existing tab containing the url, rather than navigating.
+  // This is similar to SINGLETON_TAB, but searches across all windows from
+  // the current profile and anonymity (instead of just the current one);
+  // closes the current tab on switching if the current tab was the NTP with
+  // no session history; and behaves like CURRENT_TAB instead of
+  // NEW_FOREGROUND_TAB when no existing tab is found.
+  SWITCH_TO_TAB,
   // Update when adding a new disposition.
-  MAX_VALUE = IGNORE_ACTION
+  MAX_VALUE = SWITCH_TO_TAB
 };
 
 namespace ui {