Creates a new helper function to extract search terms from the URL...

Creates a new helper function to extract search terms from the URL irrespective of the availability of InstantExtendedEnabledParam. 

Previously, InstantSearchPrerenderer invoked chrome::GetSearchTermsFromURL() to extract search terms from the search page url. chrome::GetSearchTermsFromURL() returns a valid non-empty string iff the url has an InstantExtendedEnabledParam("&espv"). If the query extraction is turned off, the search request url will not have an InstantExtendedEnabledParam. In those cases, chrome::GetSearchTermsFromURL() returns an empty string to the InstantSearchPrerenderer.

To extract search terms for InstantSearchPrerenderer, we should not depend on the query extraction flag or the Instant support state of the page.

Follow up of crrev.com/151813002.

BUG=none
TEST=none

Review URL: https://codereview.chromium.org/141893009

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

chrome/browser/search/instant_unittest_base.cc

@@ -34,24 +34,8 @@ InstantUnitTestBase::~InstantUnitTestBase() {
 }
 
 void InstantUnitTestBase::SetUp() {
-  SetUpHelper();
-}
-
-void InstantUnitTestBase::SetUpHelper() {
   chrome::EnableQueryExtractionForTesting();
-  BrowserWithTestWindowTest::SetUp();
-
-  TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-      profile(), &TemplateURLServiceFactory::BuildInstanceFor);
-  template_url_service_ = TemplateURLServiceFactory::GetForProfile(profile());
-  ui_test_utils::WaitForTemplateURLServiceToLoad(template_url_service_);
-
-  UIThreadSearchTermsData::SetGoogleBaseURL("https://www.google.com/");
-  TestingPrefServiceSyncable* pref_service = profile()->GetTestingPrefService();
-  pref_service->SetUserPref(prefs::kLastPromptedGoogleURL,
-                            new base::StringValue("https://www.google.com/"));
-  SetDefaultSearchProvider("{google:baseURL}");
-  instant_service_ = InstantServiceFactory::GetForProfile(profile());
+  SetUpHelper();
 }
 
 void InstantUnitTestBase::TearDown() {
@@ -59,6 +43,12 @@ void InstantUnitTestBase::TearDown() {
   BrowserWithTestWindowTest::TearDown();
 }
 
+#if !defined(OS_IOS) && !defined(OS_ANDROID)
+void InstantUnitTestBase::SetUpWithoutQueryExtraction() {
+  SetUpHelper();
+}
+#endif
+
 void InstantUnitTestBase::SetDefaultSearchProvider(
     const std::string& base_url) {
   TemplateURLData data;
@@ -92,9 +82,23 @@ void InstantUnitTestBase::NotifyGoogleBaseURLUpdate(
       content::Details<GoogleURLTracker::UpdatedDetails>(&details));
 }
 
-
 bool InstantUnitTestBase::IsInstantServiceObserver(
     InstantServiceObserver* observer) {
   return instant_service_->observers_.HasObserver(observer);
 }
 
+void InstantUnitTestBase::SetUpHelper() {
+  BrowserWithTestWindowTest::SetUp();
+
+  TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+      profile(), &TemplateURLServiceFactory::BuildInstanceFor);
+  template_url_service_ = TemplateURLServiceFactory::GetForProfile(profile());
+  ui_test_utils::WaitForTemplateURLServiceToLoad(template_url_service_);
+
+  UIThreadSearchTermsData::SetGoogleBaseURL("https://www.google.com/");
+  TestingPrefServiceSyncable* pref_service = profile()->GetTestingPrefService();
+  pref_service->SetUserPref(prefs::kLastPromptedGoogleURL,
+                            new base::StringValue("https://www.google.com/"));
+  SetDefaultSearchProvider("{google:baseURL}");
+  instant_service_ = InstantServiceFactory::GetForProfile(profile());
+}

chrome/browser/search/instant_unittest_base.h

@@ -25,7 +25,10 @@ class InstantUnitTestBase : public BrowserWithTestWindowTest {
   virtual void SetUp() OVERRIDE;
   virtual void TearDown() OVERRIDE;
 
-  virtual void SetUpHelper();
+#if !defined(OS_IOS) && !defined(OS_ANDROID)
+  // Query extraction is always enabled on Android and iOS.
+  void SetUpWithoutQueryExtraction();
+#endif
 
   // Adds and sets the default search provider using the base_url.
   // The base_url should have the http[s]:// prefix and a trailing / after the
@@ -44,6 +47,9 @@ class InstantUnitTestBase : public BrowserWithTestWindowTest {
   InstantService* instant_service_;
   TemplateURLService* template_url_service_;
   scoped_ptr<base::FieldTrialList> field_trial_list_;
+
+ private:
+  void SetUpHelper();
 };
 
 #endif  // CHROME_BROWSER_SEARCH_INSTANT_UNITTEST_BASE_H_

chrome/browser/search/search.cc

@@ -203,13 +203,9 @@ bool IsRenderedInInstantProcess(const content::WebContents* contents,
   return instant_service->IsInstantProcess(process_host->GetID());
 }
 
-// Returns true if |url| passes some basic checks that must succeed for it to be
-// usable as an instant URL:
-// (1) It contains the search terms replacement key of |template_url|, which is
-//     expected to be the TemplateURL* for the default search provider.
-// (2) Either it has a secure scheme, or else the user has manually specified a
-//     --google-base-url and it uses that base URL.  (This allows testers to use
-//     --google-base-url to point at non-HTTPS servers, which eases testing.)
+// |url| should either have a secure scheme or have a non-HTTPS base URL that
+// the user specified using --google-base-url. (This allows testers to use
+// --google-base-url to point at non-HTTPS servers, which eases testing.)
 bool IsSuitableURLForInstant(const GURL& url, const TemplateURL* template_url) {
   return template_url->HasSearchTermsReplacementKey(url) &&
       (url.SchemeIsSecure() ||
@@ -273,8 +269,11 @@ base::string16 GetSearchTermsImpl(const content::WebContents* contents,
   if (!search_terms.empty())
     return search_terms;
 
+  if (!IsQueryExtractionAllowedForURL(profile, entry->GetVirtualURL()))
+    return base::string16();
+
   // Otherwise, extract from the URL.
-  return GetSearchTermsFromURL(profile, entry->GetVirtualURL());
+  return ExtractSearchTermsFromURL(profile, entry->GetVirtualURL());
 }
 
 bool IsURLAllowedForSupervisedUser(const GURL& url, Profile* profile) {
@@ -388,7 +387,7 @@ bool IsQueryExtractionEnabled() {
 #endif  // defined(OS_IOS) || defined(OS_ANDROID)
 }
 
-base::string16 GetSearchTermsFromURL(Profile* profile, const GURL& url) {
+base::string16 ExtractSearchTermsFromURL(Profile* profile, const GURL& url) {
   if (url.is_valid() && url == GetSearchResultPrefetchBaseURL(profile)) {
     // InstantSearchPrerenderer has the search query for the Instant search base
     // page.
@@ -398,13 +397,18 @@ base::string16 GetSearchTermsFromURL(Profile* profile, const GURL& url) {
     return prerenderer->get_last_query();
   }
 
-  base::string16 search_terms;
   TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile);
-  if (template_url && IsSuitableURLForInstant(url, template_url))
+  base::string16 search_terms;
+  if (template_url)
     template_url->ExtractSearchTermsFromURL(url, &search_terms);
   return search_terms;
 }
 
+bool IsQueryExtractionAllowedForURL(Profile* profile, const GURL& url) {
+  TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile);
+  return template_url && IsSuitableURLForInstant(url, template_url);
+}
+
 base::string16 GetSearchTermsFromNavigationEntry(
     const content::NavigationEntry* entry) {
   base::string16 search_terms;
@@ -454,9 +458,9 @@ bool IsNTPURL(const GURL& url, Profile* profile) {
   if (!IsInstantExtendedAPIEnabled())
     return url == GURL(chrome::kChromeUINewTabURL);
 
+  const base::string16 search_terms = ExtractSearchTermsFromURL(profile, url);
   return profile &&
-      ((IsInstantURL(url, profile) &&
-        GetSearchTermsFromURL(profile, url).empty()) ||
+      ((IsInstantURL(url, profile) && search_terms.empty()) ||
        url == GURL(chrome::kChromeSearchLocalNtpUrl));
 }
 

chrome/browser/search/search.h

@@ -89,12 +89,16 @@ std::string InstantExtendedEnabledParam(bool for_search);
 // Returns whether query extraction is enabled.
 bool IsQueryExtractionEnabled();
 
-// Extracts and returns search terms from |url|. Returns empty string if the URL
-// is not secure or doesn't have a search term replacement key.  Does not
-// consider IsQueryExtractionEnabled() and Instant support state of the page and
-// does not check for a privileged process, so most callers should use
+// Extracts and returns search terms from |url|. Does not consider
+// IsQueryExtractionEnabled() and Instant support state of the page and does
+// not check for a privileged process, so most callers should use
 // GetSearchTerms() below instead.
-base::string16 GetSearchTermsFromURL(Profile* profile, const GURL& url);
+base::string16 ExtractSearchTermsFromURL(Profile* profile, const GURL& url);
+
+// Returns true if it is okay to extract search terms from |url|. |url| must
+// have a secure scheme and must contain the search terms replacement key for
+// the default search provider.
+bool IsQueryExtractionAllowedForURL(Profile* profile, const GURL& url);
 
 // Returns the search terms attached to a specific NavigationEntry, or empty
 // string otherwise. Does not consider IsQueryExtractionEnabled() and does not

chrome/browser/search/search_unittest.cc

@@ -800,6 +800,56 @@ TEST_F(SearchTest, GetSearchResultPrefetchBaseURL) {
             GetSearchResultPrefetchBaseURL(profile()));
 }
 
+struct ExtractSearchTermsTestCase {
+  const char* url;
+  const char* expected_result;
+  const char* comment;
+};
+
+TEST_F(SearchTest, ExtractSearchTermsFromURL) {
+  const ExtractSearchTermsTestCase kTestCases[] = {
+    {chrome::kChromeSearchLocalNtpUrl,           "",    "NTP url"},
+    {"https://foo.com/instant?strk",             "",    "Invalid search url"},
+    {"https://foo.com/instant#strk",             "",    "Invalid search url"},
+    {"https://foo.com/alt#quux=foo",             "foo", "Valid search url"},
+    {"https://foo.com/alt#quux=foo&strk",        "foo", "Valid search url"}
+  };
+
+  for (size_t i = 0; i < arraysize(kTestCases); ++i) {
+    const ExtractSearchTermsTestCase& test = kTestCases[i];
+    EXPECT_EQ(
+        test.expected_result,
+        UTF16ToASCII(chrome::ExtractSearchTermsFromURL(profile(),
+                                                       GURL(test.url))))
+            << test.url << " " << test.comment;
+  }
+}
+
+struct QueryExtractionAllowedTestCase {
+  const char* url;
+  bool expected_result;
+  const char* comment;
+};
+
+TEST_F(SearchTest, IsQueryExtractionAllowedForURL) {
+  const QueryExtractionAllowedTestCase kTestCases[] = {
+    {"http://foo.com/instant?strk",       false, "HTTP URL"},
+    {"https://foo.com/instant?strk",      true,  "Valid URL"},
+    {"https://foo.com/instant?",          false,
+     "No search terms replacement key"},
+    {"https://foo.com/alt#quux=foo",      false,
+     "No search terms replacement key"},
+    {"https://foo.com/alt#quux=foo&strk", true,  "Valid search url"}
+  };
+
+  for (size_t i = 0; i < arraysize(kTestCases); ++i) {
+    const QueryExtractionAllowedTestCase& test = kTestCases[i];
+    EXPECT_EQ(test.expected_result,
+              chrome::IsQueryExtractionAllowedForURL(profile(), GURL(test.url)))
+        << test.url << " " << test.comment;
+  }
+}
+
 class SearchURLTest : public SearchTest {
  protected:
   virtual void SetSearchProvider(bool set_ntp_url, bool insecure_ntp_url)

chrome/browser/ui/browser_instant_controller.cc

@@ -73,10 +73,8 @@ bool BrowserInstantController::OpenInstant(WindowOpenDisposition disposition,
   // support for the new disposition.
   DCHECK(disposition == CURRENT_TAB) << disposition;
 
-  // If we will not be replacing search terms from this URL, don't send to
-  // InstantController.
   const base::string16& search_terms =
-      chrome::GetSearchTermsFromURL(browser_->profile(), url);
+      chrome::ExtractSearchTermsFromURL(profile(), url);
   if (search_terms.empty())
     return false;
 
@@ -93,6 +91,11 @@ bool BrowserInstantController::OpenInstant(WindowOpenDisposition disposition,
     }
   }
 
+  // If we will not be replacing search terms from this URL, don't send to
+  // InstantController.
+  if (!chrome::IsQueryExtractionAllowedForURL(profile(), url))
+    return false;
+
   return instant_.SubmitQuery(search_terms);
 }
 

chrome/browser/ui/search/instant_search_prerenderer.cc

@@ -123,7 +123,8 @@ bool InstantSearchPrerenderer::CanCommitQuery(
 bool InstantSearchPrerenderer::UsePrerenderedPage(
     const GURL& url,
     chrome::NavigateParams* params) {
-  base::string16 search_terms = chrome::GetSearchTermsFromURL(profile_, url);
+  base::string16 search_terms =
+      chrome::ExtractSearchTermsFromURL(profile_, url);
   prerender::PrerenderManager* prerender_manager =
       prerender::PrerenderManagerFactory::GetForProfile(profile_);
   if (search_terms.empty() || !params->target_contents ||

chrome/browser/ui/search/instant_search_prerenderer_unittest.cc

@@ -238,14 +238,14 @@ TEST_F(InstantSearchPrerendererTest, GetSearchTermsFromPrerenderedPage) {
   EXPECT_EQ(GURL("https://www.google.com/instant?ion=1&foo=foo#foo=foo&strk"),
             url);
   EXPECT_EQ(UTF16ToASCII(prerenderer->get_last_query()),
-            UTF16ToASCII(chrome::GetSearchTermsFromURL(profile(), url)));
+            UTF16ToASCII(chrome::ExtractSearchTermsFromURL(profile(), url)));
 
   // Assume the prerendered page prefetched search results for the query
   // "flowers".
   SetLastQuery(ASCIIToUTF16("flowers"));
   EXPECT_EQ("flowers", UTF16ToASCII(prerenderer->get_last_query()));
   EXPECT_EQ(UTF16ToASCII(prerenderer->get_last_query()),
-            UTF16ToASCII(chrome::GetSearchTermsFromURL(profile(), url)));
+            UTF16ToASCII(chrome::ExtractSearchTermsFromURL(profile(), url)));
 }
 
 TEST_F(InstantSearchPrerendererTest, PrefetchSearchResults) {
@@ -336,8 +336,8 @@ TEST_F(InstantSearchPrerendererTest, PrerenderingAllowed) {
   // used only when the underlying page doesn't support Instant.
   NavigateAndCommitActiveTab(GURL("https://www.google.com/alt#quux=foo&strk"));
   active_tab = GetActiveWebContents();
-  EXPECT_FALSE(chrome::GetSearchTermsFromURL(profile(), active_tab->GetURL())
-      .empty());
+  EXPECT_FALSE(chrome::ExtractSearchTermsFromURL(profile(),
+                                                 active_tab->GetURL()).empty());
   EXPECT_FALSE(chrome::ShouldPrefetchSearchResultsOnSRP());
   EXPECT_FALSE(prerenderer->IsAllowed(search_type_match, active_tab));
 }
@@ -448,3 +448,31 @@ TEST_F(ReuseInstantSearchBasePageTest,
   EXPECT_FALSE(prerenderer->CanCommitQuery(GetActiveWebContents(),
                                            ASCIIToUTF16("joy")));
 }
+
+#if !defined(OS_IOS) && !defined(OS_ANDROID)
+class TestUsePrerenderPage : public InstantSearchPrerendererTest {
+ protected:
+  virtual void SetUp() OVERRIDE {
+    // Disable query extraction flag in field trials.
+    ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
+        "EmbeddedSearch",
+        "Group1 strk:20 query_extraction:0 prefetch_results:1"));
+    InstantUnitTestBase::SetUpWithoutQueryExtraction();
+  }
+};
+
+TEST_F(TestUsePrerenderPage, ExtractSearchTermsAndUsePrerenderPage) {
+  PrerenderSearchQuery(ASCIIToUTF16("foo"));
+
+  // Open a search results page. Query extraction flag is disabled in field
+  // trials. Search results page URL does not contain search terms replacement
+  // key. Make sure UsePrerenderedPage() extracts the search terms from the URL
+  // and uses the prerendered page contents.
+  GURL url("https://www.google.com/alt#quux=foo");
+  browser()->OpenURL(content::OpenURLParams(url, Referrer(), CURRENT_TAB,
+                                            content::PAGE_TRANSITION_TYPED,
+                                            false));
+  EXPECT_EQ(GetPrerenderURL(), GetActiveWebContents()->GetURL());
+  EXPECT_EQ(static_cast<PrerenderHandle*>(NULL), prerender_handle());
+}
+#endif