Don't extract search terms that the omnbox would treat as a navigation.

Doing so might confuse users into believing that the search terms were
the URL of the current page, and could cause problems if users hit enter
in the omnibox expecting to reload the page.  This also covers the case
of javascript: URLs.

BUG=163192,173483
TEST=With instant extended API enabled, paste the following URL into the omnibox and note that the whole URL is visible in the omnibox as the search results are shown. https://www.google.ca/webhp?sourceid=chrome-instant&espv=1&ie=UTF-8#hl=en&sugexp=les%3B&gs_rn=0&gs_ri=hp&cp=4&gs_id=0&xhr=t&q=http://www.shadybank.com/&pf=p&tbo=d&espv=1&output=search&sclient=chrome-search&oq=spam&gs_l=&pbx=1&bav=on.2,or.r_gc.r_pw.r_qf.&bvm=bv.41642243,d.aWc&fp=ba0fd5791f5165d1&biw=0&bih=0

Review URL: https://chromiumcodereview.appspot.com/12086058

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@183128 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 118 insertions, 5 deletions

diff --git a/chrome/browser/ui/toolbar/toolbar_model_impl.cc b/chrome/browser/ui/toolbar/toolbar_model_impl.cc
index 9daf849..04c6f3a4 100644
--- a/chrome/browser/ui/toolbar/toolbar_model_impl.cc
+++ b/chrome/browser/ui/toolbar/toolbar_model_impl.cc
@@ -7,7 +7,10 @@
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/autocomplete/autocomplete_classifier.h"
+#include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
 #include "chrome/browser/autocomplete/autocomplete_input.h"
+#include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ssl/ssl_error_info.h"
 #include "chrome/browser/ui/search/search.h"
@@ -48,8 +51,7 @@ ToolbarModelImpl::~ToolbarModelImpl() {
 string16 ToolbarModelImpl::GetText(
     bool display_search_urls_as_search_terms) const {
   if (display_search_urls_as_search_terms) {
-    string16 search_terms =
-        chrome::search::GetSearchTerms(delegate_->GetActiveWebContents());
+    string16 search_terms = GetSearchTerms();
     if (!search_terms.empty())
       return search_terms;
   }
@@ -81,8 +83,7 @@ GURL ToolbarModelImpl::GetURL() const {
 }
 
 bool ToolbarModelImpl::WouldReplaceSearchURLWithSearchTerms() const {
-  const content::WebContents* contents = delegate_->GetActiveWebContents();
-  return !chrome::search::GetSearchTerms(contents).empty();
+  return !GetSearchTerms().empty();
 }
 
 bool ToolbarModelImpl::ShouldDisplayURL() const {
@@ -220,3 +221,24 @@ Profile* ToolbarModelImpl::GetProfile() const {
       Profile::FromBrowserContext(navigation_controller->GetBrowserContext()) :
       NULL;
 }
+
+string16 ToolbarModelImpl::GetSearchTerms() const {
+  const WebContents* contents = delegate_->GetActiveWebContents();
+  string16 search_terms = chrome::search::GetSearchTerms(contents);
+
+  // Don't extract search terms that the omnibox would treat as a navigation.
+  // This might confuse users into believing that the search terms were the
+  // URL of the current page, and could cause problems if users hit enter in
+  // the omnibox expecting to reload the page.
+  if (!search_terms.empty()) {
+    AutocompleteMatch match;
+    Profile* profile =
+        Profile::FromBrowserContext(contents->GetBrowserContext());
+    AutocompleteClassifierFactory::GetForProfile(profile)->Classify(
+        search_terms, string16(), false, false, &match, NULL);
+    if (!AutocompleteMatch::IsSearchType(match.type))
+      search_terms.clear();
+  }
+
+  return search_terms;
+}
diff --git a/chrome/browser/ui/toolbar/toolbar_model_impl.h b/chrome/browser/ui/toolbar/toolbar_model_impl.h
index fda179e..91843a7 100644
--- a/chrome/browser/ui/toolbar/toolbar_model_impl.h
+++ b/chrome/browser/ui/toolbar/toolbar_model_impl.h
@@ -56,6 +56,10 @@ class ToolbarModelImpl : public ToolbarModel {
   // Helper method to extract the profile from the navigation controller.
   Profile* GetProfile() const;
 
+  // Returns search terms as in chrome::search::GetSearchTerms unless those
+  // terms would be treated by the omnibox as a navigation.
+  string16 GetSearchTerms() const;
+
   ToolbarModelDelegate* delegate_;
 
   // Whether the text in the location bar is currently being edited.
diff --git a/chrome/browser/ui/toolbar/toolbar_model_unittest.cc b/chrome/browser/ui/toolbar/toolbar_model_unittest.cc
index 7ba2409..a314c08 100644
--- a/chrome/browser/ui/toolbar/toolbar_model_unittest.cc
+++ b/chrome/browser/ui/toolbar/toolbar_model_unittest.cc
@@ -4,8 +4,11 @@
 
 #include "chrome/browser/ui/toolbar/toolbar_model.h"
 
+#include <vector>
+
 #include "base/command_line.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
 #include "chrome/browser/instant/instant_service.h"
 #include "chrome/browser/instant/instant_service_factory.h"
 #include "chrome/browser/search_engines/template_url.h"
@@ -20,6 +23,7 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
+#include "net/base/escape.h"
 
 using content::OpenURLParams;
 using content::Referrer;
@@ -108,7 +112,23 @@ struct TestItem {
     ASCIIToUTF16("tractor supply"),
     true,
     true
-  }
+  },
+  {
+    GURL("https://google.com/search?q=tractorsupply.com&espv=1"),
+    ASCIIToUTF16("https://google.com/search?q=tractorsupply.com&espv=1"),
+    ASCIIToUTF16("https://google.com/search?q=tractorsupply.com&espv=1"),
+    ASCIIToUTF16("https://google.com/search?q=tractorsupply.com&espv=1"),
+    false,
+    true
+  },
+  {
+    GURL("https://google.com/search?q=ftp://tractorsupply.ie&espv=1"),
+    ASCIIToUTF16("https://google.com/search?q=ftp://tractorsupply.ie&espv=1"),
+    ASCIIToUTF16("https://google.com/search?q=ftp://tractorsupply.ie&espv=1"),
+    ASCIIToUTF16("https://google.com/search?q=ftp://tractorsupply.ie&espv=1"),
+    false,
+    true
+  },
 };
 
 }  // end namespace
@@ -121,6 +141,8 @@ class ToolbarModelTest : public BrowserWithTestWindowTest {
     BrowserWithTestWindowTest::SetUp();
     TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
         profile(), &TemplateURLServiceFactory::BuildInstanceFor);
+    AutocompleteClassifierFactory::GetInstance()->SetTestingFactoryAndUse(
+        profile(), &AutocompleteClassifierFactory::BuildInstanceFor);
   }
   virtual void TearDown() OVERRIDE { BrowserWithTestWindowTest::TearDown(); }
 
@@ -151,6 +173,26 @@ class ToolbarModelTest : public BrowserWithTestWindowTest {
                              should_display);
   }
 
+  void NavigateAndCheckQueries(const std::vector<const char*>& queries,
+                               bool would_replace) {
+    const std::string kInstantExtendedPrefix(
+        "https://google.com/search?espv=1&q=");
+
+    chrome::search::EnableQueryExtractionForTesting();
+
+    ResetDefaultTemplateURL();
+    AddTab(browser(), GURL(chrome::kAboutBlankURL));
+    for (size_t i = 0; i < queries.size(); ++i) {
+      std::string url_string = kInstantExtendedPrefix +
+          net::EscapeQueryParamValue(queries[i], true);
+      NavigateAndCheckTextImpl(GURL(url_string), true,
+                               ASCIIToUTF16(would_replace ?
+                                            std::string(queries[i]) :
+                                            url_string),
+                               would_replace, true);
+    }
+  }
+
  private:
   void NavigateAndCheckTextImpl(const GURL& url,
                                 bool can_replace,
@@ -218,3 +260,48 @@ TEST_F(ToolbarModelTest, ShouldDisplayURLQueryExtractionEnabled) {
                          test_item.should_display);
   }
 }
+
+// Test that replacement doesn't happen for URLs.
+TEST_F(ToolbarModelTest, DoNotExtractUrls) {
+  static const char* const kQueries[] = {
+    "www.google.com ",
+    "www.google.ca",  // Oh, Canada!
+    "www.google.ca.",
+    "www.google.ca ",
+    "www.google.ca. ",
+    "something.xxx",
+    "something.travel",
+    "bankofamerica.com/",
+    "bankofamerica.com/foo",
+    "bankofamerica.com/foo bla",
+    "BankOfAmerica.BIZ/foo bla",
+    "   http:/monkeys",
+    "http://monkeys",
+    "javascript:alert(1)",
+    "JavaScript:alert(1)",
+    "localhost",
+  };
+
+  NavigateAndCheckQueries(
+      std::vector<const char*>(&kQueries[0],
+                               &kQueries[arraysize(kQueries)]), false);
+}
+
+// Test that replacement does happen for non-URLs.
+TEST_F(ToolbarModelTest, ExtractNonUrls) {
+  static const char* const kQueries[] = {
+    "facebook.com login",
+    "site:amazon.com",
+    "e.coli",
+    "info:http://www.google.com/",
+    "cache:http://www.apple.com/",
+    "9/11",
+    "<nomatch/>",
+    "ac/dc",
+    "ps/2",
+  };
+
+  NavigateAndCheckQueries(
+      std::vector<const char*>(&kQueries[0],
+                               &kQueries[arraysize(kQueries)]), true);
+}