Create a new helper function to extract search terms from the URL irrespective of the availablility

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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251234

[kmadhusu, 2014-02-05 03:10:14]

Please review.

Thanks.

[samarth, 2014-02-05 21:18:54]

Jered understands this code better.

[kmadhusu, 2014-02-05 22:05:55]

jered@: PTAL.

Thanks.

[Jered, 2014-02-07 03:02:26]

Sorry for the slow response...

https://codereview.chromium.org/141893009/diff/1/chrome/browser/search/instan...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/141893009/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_unittest_base.cc:53: enable_query_extraction_ =
false;
Note that query extraction is enabled unconditionally on android/ios.

https://codereview.chromium.org/141893009/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_unittest_base.cc:96: if (enable_query_extraction_)
What disables query extraction if this is false? Assuming that the default is
false would make tests break if the default changed.

https://codereview.chromium.org/141893009/diff/1/chrome/browser/search/instan...
File chrome/browser/search/instant_unittest_base.h (right):

https://codereview.chromium.org/141893009/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_unittest_base.h:28: void
SetUpWithoutCacheableNTP();
We should delete this. Is that easy to do in this CL?

https://codereview.chromium.org/141893009/diff/1/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/141893009/diff/1/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_instant_controller.cc:93: const base::string16&
search_terms =
Can we find a way to do this where we don't try to extract search terms twice?
That seems like a bad sign to me. I know the search.cc code around here is messy
but maybe this is a chance to clean it up.

https://codereview.chromium.org/141893009/diff/1/chrome/browser/ui/search/ins...
File chrome/browser/ui/search/instant_search_prerenderer.h (right):

https://codereview.chromium.org/141893009/diff/1/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_search_prerenderer.h:88: base::string16
GetSearchTermsFromURL(const GURL& url) const;
This code would be more appropriate in search.cc.

[kmadhusu, 2014-02-11 01:40:02]

jered@: Addressed comments. PTAL.

Thanks.

https://codereview.chromium.org/141893009/diff/1/chrome/browser/search/instan...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/141893009/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_unittest_base.cc:53: enable_query_extraction_ =
false;
On 2014/02/07 03:02:26, Jered wrote:
> Note that query extraction is enabled unconditionally on android/ios.

Ack.

https://codereview.chromium.org/141893009/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_unittest_base.cc:96: if (enable_query_extraction_)
On 2014/02/07 03:02:26, Jered wrote:
> What disables query extraction if this is false? Assuming that the default is
> false would make tests break if the default changed.

I agree that it is kind of tricky with a boolean member variable here. I tried a
different approach to avoid this member variable. PTAL.

https://codereview.chromium.org/141893009/diff/1/chrome/browser/search/instan...
File chrome/browser/search/instant_unittest_base.h (right):

https://codereview.chromium.org/141893009/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_unittest_base.h:28: void
SetUpWithoutCacheableNTP();
On 2014/02/07 03:02:26, Jered wrote:
> We should delete this. Is that easy to do in this CL?

Will fix it in a separate CL.

https://codereview.chromium.org/141893009/diff/1/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/141893009/diff/1/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_instant_controller.cc:93: const base::string16&
search_terms =
On 2014/02/07 03:02:26, Jered wrote:
> Can we find a way to do this where we don't try to extract search terms twice?
> That seems like a bad sign to me. I know the search.cc code around here is
messy
> but maybe this is a chance to clean it up.

Done.

https://codereview.chromium.org/141893009/diff/1/chrome/browser/ui/search/ins...
File chrome/browser/ui/search/instant_search_prerenderer.h (right):

https://codereview.chromium.org/141893009/diff/1/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_search_prerenderer.h:88: base::string16
GetSearchTermsFromURL(const GURL& url) const;
On 2014/02/07 03:02:26, Jered wrote:
> This code would be more appropriate in search.cc.

Done.

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.h:97: bool IsSuitableURLForInstant(Profile*
profile, const GURL& url);
This function deserves a better name. I would like to rename it to
IsQueryExtractionEnabledForURL. Any suggestions?

[Jered, 2014-02-11 19:10:09]

Thanks, this is much cleaner. Couple of minor comments.

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.cc:206: // Returns true if |url| passes some basic
checks that must succeed for it to be
Omit this long comment here.

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.cc:278: return IsSuitableURLForInstant(profile,
entry->GetVirtualURL()) ?
nit: This would be clearer as

if (!IsSuitableURLForInstant(profile, entry->GetVirtualURL())
  return string16();

return ExtractSearchTermsFromURL(profile, entry->GetVirtualURL());

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.cc:406: return IsSuitableURLForInstantImpl(url,
template_url);
template_url && ...

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.h:91: // usable as an instant URL:
How about rewording this comment:
"Returns true if it is ok 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."

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.h:97: bool IsSuitableURLForInstant(Profile*
profile, const GURL& url);
On 2014/02/11 01:40:03, kmadhusu wrote:
> This function deserves a better name. I would like to rename it to
> IsQueryExtractionEnabledForURL. Any suggestions?

How about IsQueryExtractionAllowedForURL()?

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:97: return
chrome::IsSuitableURLForInstant(browser_profile, url) ?
nit:
if (!chrome::IsSuitableURLForInstant(browser_profile, url))
  return false;

return instant_.SubmitQuery(search_terms);

[kmadhusu, 2014-02-11 23:24:31]

Addressed comments. PTAL.

Thanks.

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.cc:206: // Returns true if |url| passes some basic
checks that must succeed for it to be
On 2014/02/11 19:10:10, Jered wrote:
> Omit this long comment here.

Done. Added a few lines about --google-base-url case.

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.cc:278: return IsSuitableURLForInstant(profile,
entry->GetVirtualURL()) ?
On 2014/02/11 19:10:10, Jered wrote:
> nit: This would be clearer as
> 
> if (!IsSuitableURLForInstant(profile, entry->GetVirtualURL())
>   return string16();
> 
> return ExtractSearchTermsFromURL(profile, entry->GetVirtualURL());

Done.

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.cc:406: return IsSuitableURLForInstantImpl(url,
template_url);
On 2014/02/11 19:10:10, Jered wrote:
> template_url && ...

Done.

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.h:91: // usable as an instant URL:
On 2014/02/11 19:10:10, Jered wrote:
> How about rewording this comment:
> "Returns true if it is ok 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."

Done.

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/search/s...
chrome/browser/search/search.h:97: bool IsSuitableURLForInstant(Profile*
profile, const GURL& url);
On 2014/02/11 19:10:10, Jered wrote:
> On 2014/02/11 01:40:03, kmadhusu wrote:
> > This function deserves a better name. I would like to rename it to
> > IsQueryExtractionEnabledForURL. Any suggestions?
> 
> How about IsQueryExtractionAllowedForURL()?

Done.

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/141893009/diff/650001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:97: return
chrome::IsSuitableURLForInstant(browser_profile, url) ?
On 2014/02/11 19:10:10, Jered wrote:
> nit:
> if (!chrome::IsSuitableURLForInstant(browser_profile, url))
>   return false;
> 
> return instant_.SubmitQuery(search_terms);

Done.

[Jered, 2014-02-11 23:33:28]

https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
chrome/browser/search/search.cc:209: bool IsSuitableURLForInstantImpl(const
GURL& url,
nit: Either drop the Impl or rename this to match the other function.

https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
chrome/browser/search/search_unittest.cc:810: TEST_F(SearchTest,
IsSuitableForInstant) {
Update this test case?

[kmadhusu, 2014-02-12 00:24:56]

https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
chrome/browser/search/search.cc:209: bool IsSuitableURLForInstantImpl(const
GURL& url,
On 2014/02/11 23:33:28, Jered wrote:
> nit: Either drop the Impl or rename this to match the other function.

Dropped the Impl.

https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
chrome/browser/search/search_unittest.cc:810: TEST_F(SearchTest,
IsSuitableForInstant) {
On 2014/02/11 23:33:28, Jered wrote:
> Update this test case?

Oops. Fixed.

[Jered, 2014-02-12 00:54:21]

On 2014/02/12 00:24:56, kmadhusu wrote:
>
https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
> File chrome/browser/search/search.cc (right):
> 
>
https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
> chrome/browser/search/search.cc:209: bool IsSuitableURLForInstantImpl(const
> GURL& url,
> On 2014/02/11 23:33:28, Jered wrote:
> > nit: Either drop the Impl or rename this to match the other function.
> 
> Dropped the Impl.
> 
>
https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
> File chrome/browser/search/search_unittest.cc (right):
> 
>
https://codereview.chromium.org/141893009/diff/850001/chrome/browser/search/s...
> chrome/browser/search/search_unittest.cc:810: TEST_F(SearchTest,
> IsSuitableForInstant) {
> On 2014/02/11 23:33:28, Jered wrote:
> > Update this test case?
> 
> Oops. Fixed.

lgtm

[kmadhusu, 2014-02-12 17:30:15]

Samarth: Do I need to wait for your LGTM stamp?

Thanks.

[samarth, 2014-02-12 18:02:38]

lgtm

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/i...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/i...
chrome/browser/search/instant_unittest_base.cc:48: "InstantExtended", "Group1
use_cacheable_ntp:0"));
Hmm, I think this is a no-op so we can probably kill this whole thing in a
follow-up change.

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/s...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/s...
chrome/browser/search/search.h:81: // Returns whether query extraction flag is
enabled in the field trials.
Any reason for changing this comment? I prefer the original wording as it's
simpler :)

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/s...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/s...
chrome/browser/search/search_unittest.cc:802: struct SearchURLTestCase {
s/SearchURLTestCase/ExtractSearchTermsTestCase/

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/s...
chrome/browser/search/search_unittest.cc:827: struct InstantURLTestCase {
QueryExtractionAllowedTestCase

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:78:
chrome::ExtractSearchTermsFromURL(browser_profile, url);
nit: just use profile() here?

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:83:
GetInstantSearchPrerenderer(browser_profile);
Likewise here and below

[kmadhusu, 2014-02-12 19:58:07]

samarth@: Addressed comments. Thanks.

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/i...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/i...
chrome/browser/search/instant_unittest_base.cc:48: "InstantExtended", "Group1
use_cacheable_ntp:0"));
On 2014/02/12 18:02:39, samarth wrote:
> Hmm, I think this is a no-op so we can probably kill this whole thing in a
> follow-up change.

Okay.

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/s...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/s...
chrome/browser/search/search.h:81: // Returns whether query extraction flag is
enabled in the field trials.
On 2014/02/12 18:02:39, samarth wrote:
> Any reason for changing this comment? I prefer the original wording as it's
> simpler :)

I just want to distinguish IsQueryExtractionEnabled() comments from
IsQueryExtractionAllowedForURL() comments. I don't have a strong preference.
Reverted this change.

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/s...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/s...
chrome/browser/search/search_unittest.cc:802: struct SearchURLTestCase {
On 2014/02/12 18:02:39, samarth wrote:
> s/SearchURLTestCase/ExtractSearchTermsTestCase/

Done.

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/search/s...
chrome/browser/search/search_unittest.cc:827: struct InstantURLTestCase {
On 2014/02/12 18:02:39, samarth wrote:
> QueryExtractionAllowedTestCase

Done.

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:78:
chrome::ExtractSearchTermsFromURL(browser_profile, url);
On 2014/02/12 18:02:39, samarth wrote:
> nit: just use profile() here?

Done.

https://codereview.chromium.org/141893009/diff/960001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:83:
GetInstantSearchPrerenderer(browser_profile);
On 2014/02/12 18:02:39, samarth wrote:
> Likewise here and below

Done.

[kmadhusu, 2014-02-13 17:17:36]

The CQ bit was checked by kmadhusu@chromium.org

[commit-bot: I haz the power, 2014-02-13 17:18:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/141893009/1250001

[commit-bot: I haz the power, 2014-02-13 18:47:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 18:47:56]

Retried try job too often on win_rel for step(s) printing_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[kmadhusu, 2014-02-13 23:06:34]

The CQ bit was checked by kmadhusu@chromium.org

[commit-bot: I haz the power, 2014-02-13 23:07:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/141893009/1250001

[commit-bot: I haz the power, 2014-02-13 23:07:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 23:07:23]

Failed to apply patch for chrome/browser/search/instant_unittest_base.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/search/instant_unittest_base.cc
  Hunk #1 FAILED at 34.
  Hunk #2 succeeded at 92 (offset -6 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/search/instant_unittest_base.cc.rej

Patch:       chrome/browser/search/instant_unittest_base.cc
Index: chrome/browser/search/instant_unittest_base.cc
diff --git a/chrome/browser/search/instant_unittest_base.cc
b/chrome/browser/search/instant_unittest_base.cc
index
6c2466ce3f327dbc6e4b369285635c3b45e4eb27..98095d7f740266d3b66c14cb96e6d2c99f8d447c
100644
--- a/chrome/browser/search/instant_unittest_base.cc
+++ b/chrome/browser/search/instant_unittest_base.cc
@@ -34,36 +34,27 @@ InstantUnitTestBase::~InstantUnitTestBase() {
 }
 
 void InstantUnitTestBase::SetUp() {
+  chrome::EnableQueryExtractionForTesting();
   SetUpHelper();
 }
 
+void InstantUnitTestBase::TearDown() {
+  UIThreadSearchTermsData::SetGoogleBaseURL("");
+  BrowserWithTestWindowTest::TearDown();
+}
+
 void InstantUnitTestBase::SetUpWithoutCacheableNTP() {
   ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
       "InstantExtended", "Group1 use_cacheable_ntp:0"));
-  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() {
-  UIThreadSearchTermsData::SetGoogleBaseURL("");
-  BrowserWithTestWindowTest::TearDown();
+#if !defined(OS_IOS) && !defined(OS_ANDROID)
+void InstantUnitTestBase::SetUpWithoutQueryExtraction() {
+  SetUpHelper();
 }
+#endif
 
 void InstantUnitTestBase::SetDefaultSearchProvider(
     const std::string& base_url) {
@@ -98,9 +89,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());
+}

[kmadhusu, 2014-02-13 23:12:45]

The CQ bit was checked by kmadhusu@chromium.org

[commit-bot: I haz the power, 2014-02-13 23:14:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/141893009/1590001

[commit-bot: I haz the power, 2014-02-14 02:40:26]

Change committed as 251234