[omnibox, browser stack] Make URL comparison looser for singletons
For dispositions of singleton and switch-to-tab, allow URLs which only
differ between http and https to match. This is done to allow
suggestions of http to find their ultimate destination.
Also happens to remove the enum NavigationPararms::IGNORE_AND_STAY_PUT
since it wasn't really being used, and enum NavigationParams::
RefBehavior since we force IGNORE_REF now.
Bug: 780835
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I73e7db51fdbef710b804966b482cb2cf0d6139ab
Reviewed-on: https://chromium-review.googlesource.com/949569
Reviewed-by: Rohit Rao <[email protected]>
Reviewed-by: Istiaque Ahmed <[email protected]>
Reviewed-by: Peter Kasting <[email protected]>
Commit-Queue: Kevin Bailey <[email protected]>
Cr-Commit-Position: refs/heads/master@{#542639}diff --git a/chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc b/chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc
index bd4c870..1f6c3a2 100644
--- a/chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc
+++ b/chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc
@@ -34,6 +34,7 @@
#include "components/browser_sync/profile_sync_service.h"
#include "components/history/core/browser/history_service.h"
#include "components/omnibox/browser/autocomplete_classifier.h"
+#include "components/omnibox/browser/autocomplete_match.h"
#include "components/prefs/pref_service.h"
#include "components/signin/core/browser/signin_manager.h"
#include "components/sync/driver/sync_service_utils.h"
@@ -363,7 +364,9 @@
}
// TODO(crbug.com/46623): Maintain a map of URL->WebContents for fast look-up.
-bool ChromeAutocompleteProviderClient::IsTabOpenWithURL(const GURL& url) {
+bool ChromeAutocompleteProviderClient::IsTabOpenWithURL(
+ const GURL& url,
+ const AutocompleteInput* input) {
#if !defined(OS_ANDROID)
Browser* active_browser = BrowserList::GetInstance()->GetLastActive();
content::WebContents* active_tab = nullptr;
@@ -376,7 +379,9 @@
for (int i = 0; i < browser->tab_strip_model()->count(); ++i) {
content::WebContents* web_contents =
browser->tab_strip_model()->GetWebContentsAt(i);
- if (web_contents != active_tab && web_contents->GetVisibleURL() == url)
+ if (web_contents != active_tab &&
+ StrippedURLsAreEqual(web_contents->GetLastCommittedURL(), url,
+ input))
return true;
}
}
@@ -384,3 +389,18 @@
#endif // !defined(OS_ANDROID)
return false;
}
+
+bool ChromeAutocompleteProviderClient::StrippedURLsAreEqual(
+ const GURL& url1,
+ const GURL& url2,
+ const AutocompleteInput* input) {
+ AutocompleteInput empty_input;
+ if (!input)
+ input = &empty_input;
+ TemplateURLService* template_url_service = GetTemplateURLService();
+ return AutocompleteMatch::GURLToStrippedGURL(url1, AutocompleteInput(),
+ template_url_service,
+ base::string16()) ==
+ AutocompleteMatch::GURLToStrippedGURL(
+ url2, *input, template_url_service, base::string16());
+}
diff --git a/chrome/browser/autocomplete/chrome_autocomplete_provider_client.h b/chrome/browser/autocomplete/chrome_autocomplete_provider_client.h
index 1766fbe..9465c0d 100644
--- a/chrome/browser/autocomplete/chrome_autocomplete_provider_client.h
+++ b/chrome/browser/autocomplete/chrome_autocomplete_provider_client.h
@@ -66,13 +66,18 @@
void StartServiceWorker(const GURL& destination_url) override;
void OnAutocompleteControllerResultReady(
AutocompleteController* controller) override;
- bool IsTabOpenWithURL(const GURL& url) override;
+ bool IsTabOpenWithURL(const GURL& url,
+ const AutocompleteInput* input) override;
// For testing.
void set_storage_partition(content::StoragePartition* storage_partition) {
storage_partition_ = storage_partition;
}
+ bool StrippedURLsAreEqual(const GURL& url1,
+ const GURL& url2,
+ const AutocompleteInput* input);
+
private:
Profile* profile_;
ChromeAutocompleteSchemeClassifier scheme_classifier_;
diff --git a/chrome/browser/extensions/extension_tab_util.cc b/chrome/browser/extensions/extension_tab_util.cc
index 26486ea..f7691554 100644
--- a/chrome/browser/extensions/extension_tab_util.cc
+++ b/chrome/browser/extensions/extension_tab_util.cc
@@ -723,7 +723,7 @@
// options page to close a page that might be open to extension content.
// However, if the options page opens inside the chrome://extensions page, we
// can override an existing page.
- // Note: default ref behavior is IGNORE_REF, which is correct.
+ // Note: ref behavior is to ignore.
params.path_behavior = open_in_tab ? NavigateParams::RESPECT
: NavigateParams::IGNORE_AND_NAVIGATE;
params.url = url_to_navigate;
diff --git a/chrome/browser/ui/browser_navigator_browsertest.cc b/chrome/browser/ui/browser_navigator_browsertest.cc
index 747ace0..22fdd21 100644
--- a/chrome/browser/ui/browser_navigator_browsertest.cc
+++ b/chrome/browser/ui/browser_navigator_browsertest.cc
@@ -254,11 +254,19 @@
const GURL& url,
Browser* browser,
WindowOpenDisposition disposition) {
+ content::WindowedNotificationObserver observer(
+ content::NOTIFICATION_LOAD_STOP,
+ content::NotificationService::AllSources());
+
NavigateParams params(MakeNavigateParams(browser));
params.disposition = disposition;
params.url = url;
params.window_action = NavigateParams::SHOW_WINDOW;
Navigate(¶ms);
+
+ if (params.disposition != WindowOpenDisposition::SWITCH_TO_TAB)
+ observer.Wait();
+
return params.browser;
}
@@ -320,56 +328,6 @@
}
IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
- Disposition_SingletonTabRespectingRef) {
- GURL singleton_ref_url1("http://maps.google.com/#a");
- GURL singleton_ref_url2("http://maps.google.com/#b");
- GURL singleton_ref_url3("http://maps.google.com/");
-
- chrome::AddSelectedTabWithURL(browser(), singleton_ref_url1,
- ui::PAGE_TRANSITION_LINK);
-
- // We should have one browser with 2 tabs, 2nd selected.
- EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
- EXPECT_EQ(2, browser()->tab_strip_model()->count());
- EXPECT_EQ(1, browser()->tab_strip_model()->active_index());
-
- // Navigate to singleton_url2.
- NavigateParams params(MakeNavigateParams());
- params.disposition = WindowOpenDisposition::SINGLETON_TAB;
- params.url = singleton_ref_url2;
- Navigate(¶ms);
-
- // We should now have 2 tabs, the 2nd one selected.
- EXPECT_EQ(browser(), params.browser);
- EXPECT_EQ(2, browser()->tab_strip_model()->count());
- EXPECT_EQ(1, browser()->tab_strip_model()->active_index());
-
- // Navigate to singleton_url2, but with respect ref set.
- params = MakeNavigateParams();
- params.disposition = WindowOpenDisposition::SINGLETON_TAB;
- params.url = singleton_ref_url2;
- params.ref_behavior = NavigateParams::RESPECT_REF;
- Navigate(¶ms);
-
- // We should now have 3 tabs, the 3th one selected.
- EXPECT_EQ(browser(), params.browser);
- EXPECT_EQ(3, browser()->tab_strip_model()->count());
- EXPECT_EQ(2, browser()->tab_strip_model()->active_index());
-
- // Navigate to singleton_url3.
- params = MakeNavigateParams();
- params.disposition = WindowOpenDisposition::SINGLETON_TAB;
- params.url = singleton_ref_url3;
- params.ref_behavior = NavigateParams::RESPECT_REF;
- Navigate(¶ms);
-
- // We should now have 4 tabs, the 4th one selected.
- EXPECT_EQ(browser(), params.browser);
- EXPECT_EQ(4, browser()->tab_strip_model()->count());
- EXPECT_EQ(3, browser()->tab_strip_model()->active_index());
-}
-
-IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
Disposition_SingletonTabNoneExisting) {
GURL singleton_url1("http://maps.google.com/");
@@ -692,6 +650,31 @@
WindowOpenDisposition::SWITCH_TO_TAB);
}
+// This test verifies that IsTabOpenWithURL() and GetIndexOfExistingTab()
+// will not discriminate between http and https.
+IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, SchemeMismatchTabSwitchTest) {
+ GURL navigate_url("https://maps.google.com/");
+ GURL search_url("http://maps.google.com/");
+
+ // Generate history so the tab isn't closed.
+ NavigateHelper(GURL("chrome://dino/"), browser(),
+ WindowOpenDisposition::CURRENT_TAB);
+
+ NavigateHelper(navigate_url, browser(),
+ WindowOpenDisposition::NEW_BACKGROUND_TAB);
+
+ // We must be on another tab than the target for it to be found and
+ // switched to.
+ EXPECT_EQ(0, browser()->tab_strip_model()->active_index());
+
+ ChromeAutocompleteProviderClient client(browser()->profile());
+ EXPECT_TRUE(client.IsTabOpenWithURL(search_url, nullptr));
+
+ NavigateHelper(search_url, browser(), WindowOpenDisposition::SWITCH_TO_TAB);
+
+ EXPECT_EQ(1, browser()->tab_strip_model()->active_index());
+}
+
// This test verifies that we're picking the correct browser and tab to
// switch to. It verifies that we don't recommend the active tab, and that,
// when switching, we don't mistakenly pick the current browser.
@@ -713,7 +696,7 @@
// actively avoid it (because the user almost certainly doesn't want to
// switch to the tab they're already on). While we are not on the target
// tab, make sure the provider client recommends our other window.
- EXPECT_TRUE(client.IsTabOpenWithURL(singleton_url));
+ EXPECT_TRUE(client.IsTabOpenWithURL(singleton_url, nullptr));
// Navigate to the singleton again.
Browser* test_browser = NavigateHelper(singleton_url, middle_browser,
@@ -724,7 +707,7 @@
EXPECT_EQ(orig_browser, test_browser);
// Now that we're on the tab, make sure the provider client doesn't
// recommend it.
- EXPECT_FALSE(client.IsTabOpenWithURL(singleton_url));
+ EXPECT_FALSE(client.IsTabOpenWithURL(singleton_url, nullptr));
}
// This test verifies that "switch to tab" prefers the latest used browser,
@@ -1126,39 +1109,6 @@
}
// This test verifies that constructing params with disposition = SINGLETON_TAB
-// and IGNORE_AND_STAY_PUT opens an existing tab with the matching URL (minus
-// the path).
-IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
- Disposition_SingletonTabExistingSubPath_IgnorePath2) {
- GURL singleton_url1(GetContentSettingsURL());
- chrome::AddSelectedTabWithURL(browser(), singleton_url1,
- ui::PAGE_TRANSITION_LINK);
- chrome::AddSelectedTabWithURL(browser(), GetGoogleURL(),
- ui::PAGE_TRANSITION_LINK);
-
- // We should have one browser with 3 tabs, the 3rd selected.
- EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
- EXPECT_EQ(3, browser()->tab_strip_model()->count());
- EXPECT_EQ(2, browser()->tab_strip_model()->active_index());
-
- // Navigate to singleton_url1.
- NavigateParams params(MakeNavigateParams());
- params.disposition = WindowOpenDisposition::SINGLETON_TAB;
- params.url = GetClearBrowsingDataURL();
- params.window_action = NavigateParams::SHOW_WINDOW;
- params.path_behavior = NavigateParams::IGNORE_AND_STAY_PUT;
- Navigate(¶ms);
-
- // The middle tab should now be selected.
- EXPECT_EQ(browser(), params.browser);
- EXPECT_EQ(3, browser()->tab_strip_model()->count());
- EXPECT_EQ(1, browser()->tab_strip_model()->active_index());
- EXPECT_EQ(singleton_url1,
- ShortenUberURL(browser()->tab_strip_model()->
- GetActiveWebContents()->GetURL()));
-}
-
-// This test verifies that constructing params with disposition = SINGLETON_TAB
// and IGNORE_AND_NAVIGATE will update the current tab's URL if the currently
// selected tab is a match but has a different path.
IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
diff --git a/chrome/browser/ui/browser_navigator_params.h b/chrome/browser/ui/browser_navigator_params.h
index a4f945c8..6bd393b 100644
--- a/chrome/browser/ui/browser_navigator_params.h
+++ b/chrome/browser/ui/browser_navigator_params.h
@@ -197,20 +197,9 @@
RESPECT,
// Ignore path when finding existing tab, navigate to new URL.
IGNORE_AND_NAVIGATE,
- // Ignore path when finding existing tab, don't navigate tab.
- IGNORE_AND_STAY_PUT,
};
PathBehavior path_behavior = RESPECT;
- // What to do with the ref component of the URL for singleton navigations.
- enum RefBehavior {
- // Two URLs with differing refs are same.
- IGNORE_REF,
- // Two URLs with differing refs are different.
- RESPECT_REF,
- };
- RefBehavior ref_behavior = IGNORE_REF;
-
#if !defined(OS_ANDROID)
// [in] Specifies a Browser object where the navigation could occur or the
// tab could be added. Navigate() is not obliged to use this Browser if
diff --git a/chrome/browser/ui/chrome_pages.cc b/chrome/browser/ui/chrome_pages.cc
index ca08535..eb3e50819 100644
--- a/chrome/browser/ui/chrome_pages.cc
+++ b/chrome/browser/ui/chrome_pages.cc
@@ -201,7 +201,7 @@
base::RecordAction(UserMetricsAction("ShowBookmarkManager"));
NavigateParams params(
GetSingletonTabNavigateParams(browser, GURL(kChromeUIBookmarksURL)));
- params.path_behavior = NavigateParams::IGNORE_AND_STAY_PUT;
+ params.path_behavior = NavigateParams::IGNORE_AND_NAVIGATE;
ShowSingletonTabOverwritingNTP(browser, params);
}
diff --git a/chrome/browser/ui/singleton_tabs.cc b/chrome/browser/ui/singleton_tabs.cc
index 6019fb0..aefcb095 100644
--- a/chrome/browser/ui/singleton_tabs.cc
+++ b/chrome/browser/ui/singleton_tabs.cc
@@ -4,6 +4,7 @@
#include "chrome/browser/ui/singleton_tabs.h"
+#include "chrome/browser/autocomplete/chrome_autocomplete_provider_client.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/search/search.h"
#include "chrome/browser/ui/browser.h"
@@ -19,13 +20,11 @@
// Returns true if two URLs are equal after taking |replacements| into account.
bool CompareURLsWithReplacements(const GURL& url,
const GURL& other,
- const url::Replacements<char>& replacements) {
- if (url == other)
- return true;
-
+ const url::Replacements<char>& replacements,
+ ChromeAutocompleteProviderClient* client) {
GURL url_replaced = url.ReplaceComponents(replacements);
GURL other_replaced = other.ReplaceComponents(replacements);
- return url_replaced == other_replaced;
+ return client->StrippedURLsAreEqual(url_replaced, other_replaced, nullptr);
}
} // namespace
@@ -37,7 +36,6 @@
void ShowSingletonTabRespectRef(Browser* browser, const GURL& url) {
NavigateParams params(GetSingletonTabNavigateParams(browser, url));
- params.ref_behavior = NavigateParams::RESPECT_REF;
Navigate(¶ms);
}
@@ -89,6 +87,7 @@
content::BrowserURLHandler::GetInstance()->RewriteURLIfNecessary(
&rewritten_url, browser->profile(), &reverse_on_redirect);
+ ChromeAutocompleteProviderClient client(browser->profile());
// If there are several matches: prefer the active tab by starting there.
int start_index = std::max(0, browser->tab_strip_model()->active_index());
int tab_count = browser->tab_strip_model()->count();
@@ -109,17 +108,16 @@
&rewritten_tab_url, browser->profile(), &reverse_on_redirect);
url::Replacements<char> replacements;
- if (params.ref_behavior == NavigateParams::IGNORE_REF)
- replacements.ClearRef();
- if (params.path_behavior == NavigateParams::IGNORE_AND_NAVIGATE ||
- params.path_behavior == NavigateParams::IGNORE_AND_STAY_PUT) {
+ replacements.ClearRef();
+ if (params.path_behavior == NavigateParams::IGNORE_AND_NAVIGATE) {
replacements.ClearPath();
replacements.ClearQuery();
}
- if (CompareURLsWithReplacements(tab_url, params.url, replacements) ||
+ if (CompareURLsWithReplacements(tab_url, params.url, replacements,
+ &client) ||
CompareURLsWithReplacements(rewritten_tab_url, rewritten_url,
- replacements)) {
+ replacements, &client)) {
return tab_index;
}
}
