Skip to content

Commit

Permalink
No chrome/ dependencies in TemplateURL
Browse files Browse the repository at this point in the history
Move kExtraSearchQueryParams from chrome_switches to a new file search_engines_switches.
Add 4 new methods to SearchTermsData:
 - EnableAnswersInSuggest() to replace OmniboxFieldTrial::EnableAnswersInSuggest()
 - IsShowingSearchTermsOnSearchResultsPages() to replace TemplateURL::showing_search_terms_
 - InstantExtendedEnabledParam() to replace chrome::InstantExtendedEnabledParam().
 - ForceInstantResultsParam() to replace chrome::ForceInstantResultsParam().
Add DEPS to prevent new dependencies.

BUG=386365
TEST=unit_tests
TBR=kmadhusu@chromium.org for an addition of #include in chrome/browser/search/search_unittest.cc

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@279071 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
hashimoto@chromium.org committed Jun 23, 2014
1 parent 8bb6216 commit 5c3c6e4
Show file tree
Hide file tree
Showing 18 changed files with 189 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@
#include "chrome/browser/search_engines/template_url.h"
#include "chrome/browser/search_engines/template_url_service.h"
#include "chrome/browser/search_engines/template_url_service_factory.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/test/base/testing_browser_process.h"
#include "chrome/test/base/testing_profile.h"
#include "components/metrics/proto/omnibox_event.pb.h"
#include "components/search_engines/search_engines_switches.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/notification_source.h"
Expand Down
53 changes: 52 additions & 1 deletion chrome/browser/autocomplete/history_url_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -284,14 +284,28 @@ class SearchTermsDataSnapshot : public SearchTermsData {
virtual base::string16 GetRlzParameterValue(
bool from_app_list) const OVERRIDE;
virtual std::string GetSearchClient() const OVERRIDE;
virtual bool EnableAnswersInSuggest() const OVERRIDE;
virtual bool IsShowingSearchTermsOnSearchResultsPages() const OVERRIDE;
virtual std::string InstantExtendedEnabledParam(
bool for_search) const OVERRIDE;
virtual std::string ForceInstantResultsParam(
bool for_prerender) const OVERRIDE;
virtual std::string NTPIsThemedParam() const OVERRIDE;
virtual std::string GoogleImageSearchSource() const OVERRIDE;

private:
std::string google_base_url_value_;
std::string application_locale_;
base::string16 rlz_parameter_value_;
std::string search_client_;
bool enable_answers_in_suggest_;
bool is_showing_search_terms_on_search_results_pages_;
std::string instant_extended_enabled_param_;
std::string instant_extended_enabled_param_for_search_;
std::string force_instant_results_param_;
std::string force_instant_results_param_for_prerender_;
std::string ntp_is_themed_param_;
std::string google_image_search_source_;

DISALLOW_COPY_AND_ASSIGN(SearchTermsDataSnapshot);
};
Expand All @@ -302,7 +316,20 @@ SearchTermsDataSnapshot::SearchTermsDataSnapshot(
application_locale_(search_terms_data.GetApplicationLocale()),
rlz_parameter_value_(search_terms_data.GetRlzParameterValue(false)),
search_client_(search_terms_data.GetSearchClient()),
ntp_is_themed_param_(search_terms_data.NTPIsThemedParam()) {}
enable_answers_in_suggest_(search_terms_data.EnableAnswersInSuggest()),
is_showing_search_terms_on_search_results_pages_(
search_terms_data.IsShowingSearchTermsOnSearchResultsPages()),
instant_extended_enabled_param_(
search_terms_data.InstantExtendedEnabledParam(false)),
instant_extended_enabled_param_for_search_(
search_terms_data.InstantExtendedEnabledParam(true)),
force_instant_results_param_(
search_terms_data.ForceInstantResultsParam(false)),
force_instant_results_param_for_prerender_(
search_terms_data.ForceInstantResultsParam(true)),
ntp_is_themed_param_(search_terms_data.NTPIsThemedParam()),
google_image_search_source_(search_terms_data.GoogleImageSearchSource()) {
}

SearchTermsDataSnapshot::~SearchTermsDataSnapshot() {
}
Expand All @@ -324,10 +351,34 @@ std::string SearchTermsDataSnapshot::GetSearchClient() const {
return search_client_;
}

bool SearchTermsDataSnapshot::EnableAnswersInSuggest() const {
return enable_answers_in_suggest_;
}

bool SearchTermsDataSnapshot::IsShowingSearchTermsOnSearchResultsPages() const {
return is_showing_search_terms_on_search_results_pages_;
}

std::string SearchTermsDataSnapshot::InstantExtendedEnabledParam(
bool for_search) const {
return for_search ? instant_extended_enabled_param_ :
instant_extended_enabled_param_for_search_;
}

std::string SearchTermsDataSnapshot::ForceInstantResultsParam(
bool for_prerender) const {
return for_prerender ? force_instant_results_param_ :
force_instant_results_param_for_prerender_;
}

std::string SearchTermsDataSnapshot::NTPIsThemedParam() const {
return ntp_is_themed_param_;
}

std::string SearchTermsDataSnapshot::GoogleImageSearchSource() const {
return google_image_search_source_;
}

// -----------------------------------------------------------------
// HistoryURLProvider

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/autocomplete/keyword_provider_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
#include "chrome/browser/autocomplete/keyword_provider.h"
#include "chrome/browser/search_engines/template_url.h"
#include "chrome/browser/search_engines/template_url_service.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/test/base/testing_browser_process.h"
#include "components/metrics/proto/omnibox_event.pb.h"
#include "components/search_engines/search_engines_switches.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"

Expand Down
1 change: 1 addition & 0 deletions chrome/browser/autocomplete/search_provider_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "components/google/core/browser/google_switches.h"
#include "components/metrics/proto/omnibox_event.pb.h"
#include "components/search_engines/search_engine_type.h"
#include "components/search_engines/search_engines_switches.h"
#include "components/signin/core/browser/signin_manager.h"
#include "components/sync_driver/pref_names.h"
#include "components/variations/entropy_provider.h"
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/search/search_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "chrome/test/base/browser_with_test_window_test.h"
#include "chrome/test/base/ui_test_utils.h"
#include "components/google/core/browser/google_switches.h"
#include "components/search_engines/search_engines_switches.h"
#include "components/variations/entropy_provider.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/render_view_host.h"
Expand Down
7 changes: 7 additions & 0 deletions chrome/browser/search_engines/DEPS
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
specific_include_rules = {
"template_url(_unittest)?\.": [
"-chrome",
"-content",
"+chrome/browser/search_engines/template_url.h",
],
}
30 changes: 13 additions & 17 deletions chrome/browser/search_engines/template_url.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,9 @@
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/omnibox/omnibox_field_trial.h"
#include "chrome/browser/search/search.h"
#include "chrome/common/chrome_switches.h"
#include "components/google/core/browser/google_util.h"
#include "components/metrics/proto/omnibox_input_type.pb.h"
#include "components/search_engines/search_engines_switches.h"
#include "components/search_engines/search_terms_data.h"
#include "extensions/common/constants.h"
#include "google_apis/google_api_keys.h"
Expand Down Expand Up @@ -173,11 +171,6 @@ bool IsTemplateParameterString(const std::string& param) {
(*(param.rbegin()) == kEndParameter);
}

bool ShowingSearchTermsOnSRP() {
return chrome::IsInstantExtendedAPIEnabled() &&
chrome::IsQueryExtractionEnabled();
}

} // namespace


Expand Down Expand Up @@ -240,8 +233,7 @@ TemplateURLRef::TemplateURLRef(TemplateURL* owner, Type type)
valid_(false),
supports_replacements_(false),
search_term_key_location_(url::Parsed::QUERY),
prepopulated_(false),
showing_search_terms_(ShowingSearchTermsOnSRP()) {
prepopulated_(false) {
DCHECK(owner_);
DCHECK_NE(INDEXED, type_);
}
Expand All @@ -254,8 +246,7 @@ TemplateURLRef::TemplateURLRef(TemplateURL* owner, size_t index_in_owner)
valid_(false),
supports_replacements_(false),
search_term_key_location_(url::Parsed::QUERY),
prepopulated_(false),
showing_search_terms_(ShowingSearchTermsOnSRP()) {
prepopulated_(false) {
DCHECK(owner_);
DCHECK_LT(index_in_owner_, owner_->URLCount());
}
Expand Down Expand Up @@ -613,8 +604,7 @@ bool TemplateURLRef::ParseParameter(size_t start,
} else if (parameter == kGoogleSearchFieldtrialParameter) {
replacements->push_back(Replacement(GOOGLE_SEARCH_FIELDTRIAL_GROUP, start));
} else if (parameter == kGoogleSearchVersion) {
if (OmniboxFieldTrial::EnableAnswersInSuggest())
url->insert(start, "gs_rn=42&");
replacements->push_back(Replacement(GOOGLE_SEARCH_VERSION, start));
} else if (parameter == kGoogleSessionToken) {
replacements->push_back(Replacement(GOOGLE_SESSION_TOKEN, start));
} else if (parameter == kGoogleSourceIdParameter) {
Expand Down Expand Up @@ -867,7 +857,7 @@ std::string TemplateURLRef::HandleReplacements(
break;

case GOOGLE_BOOKMARK_BAR_PINNED:
if (showing_search_terms_) {
if (search_terms_data.IsShowingSearchTermsOnSearchResultsPages()) {
// Log whether the bookmark bar is pinned when the user is seeing
// InstantExtended on the SRP.
DCHECK(!i->is_post_param);
Expand Down Expand Up @@ -900,7 +890,7 @@ std::string TemplateURLRef::HandleReplacements(
case GOOGLE_FORCE_INSTANT_RESULTS:
DCHECK(!i->is_post_param);
HandleReplacement(std::string(),
chrome::ForceInstantResultsParam(
search_terms_data.ForceInstantResultsParam(
search_terms_args.force_instant_results),
*i,
&url);
Expand All @@ -915,7 +905,8 @@ std::string TemplateURLRef::HandleReplacements(
case GOOGLE_INSTANT_EXTENDED_ENABLED:
DCHECK(!i->is_post_param);
HandleReplacement(std::string(),
chrome::InstantExtendedEnabledParam(type_ == SEARCH),
search_terms_data.InstantExtendedEnabledParam(
type_ == SEARCH),
*i,
&url);
break;
Expand Down Expand Up @@ -1028,6 +1019,11 @@ std::string TemplateURLRef::HandleReplacements(
// url.insert(i->index, used_www ? "gcx=w&" : "gcx=c&");
break;

case GOOGLE_SEARCH_VERSION:
if (search_terms_data.EnableAnswersInSuggest())
HandleReplacement("gs_rn", "42", *i, &url);
break;

case GOOGLE_SESSION_TOKEN: {
std::string token = search_terms_args.session_token;
if (!token.empty())
Expand Down
5 changes: 1 addition & 4 deletions chrome/browser/search_engines/template_url.h
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ class TemplateURLRef {
GOOGLE_RLZ,
GOOGLE_SEARCH_CLIENT,
GOOGLE_SEARCH_FIELDTRIAL_GROUP,
GOOGLE_SEARCH_VERSION,
GOOGLE_SESSION_TOKEN,
GOOGLE_SUGGEST_CLIENT,
GOOGLE_SUGGEST_REQUEST_ID,
Expand Down Expand Up @@ -431,10 +432,6 @@ class TemplateURLRef {
// Whether the contained URL is a pre-populated URL.
bool prepopulated_;

// Whether search terms are shown in the omnibox on search results pages.
// This is kept as a member so it can be overridden by tests.
bool showing_search_terms_;

DISALLOW_COPY_AND_ASSIGN(TemplateURLRef);
};

Expand Down
33 changes: 26 additions & 7 deletions chrome/browser/search_engines/template_url_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/search_engines/template_url.h"
#include "chrome/common/chrome_switches.h"
#include "components/metrics/proto/omnibox_event.pb.h"
#include "components/metrics/proto/omnibox_input_type.pb.h"
#include "components/search_engines/search_engines_switches.h"
#include "components/search_engines/search_terms_data.h"
#include "testing/gtest/include/gtest/gtest.h"

Expand All @@ -28,23 +28,35 @@ class TestSearchTermsData : public SearchTermsData {
bool from_app_list) const OVERRIDE;
virtual std::string GetSearchClient() const OVERRIDE;
virtual std::string GoogleImageSearchSource() const OVERRIDE;
virtual bool EnableAnswersInSuggest() const OVERRIDE;
virtual bool IsShowingSearchTermsOnSearchResultsPages() const OVERRIDE;

void set_google_base_url(const std::string& google_base_url) {
google_base_url_ = google_base_url;
}
void set_search_client(const std::string& search_client) {
search_client_ = search_client;
}
void set_enable_answers_in_suggest(bool enable_answers_in_suggest) {
enable_answers_in_suggest_ = enable_answers_in_suggest;
}
void set_is_showing_search_terms_on_search_results_pages(bool value) {
is_showing_search_terms_on_search_results_pages_ = value;
}

private:
std::string google_base_url_;
std::string search_client_;
bool enable_answers_in_suggest_;
bool is_showing_search_terms_on_search_results_pages_;

DISALLOW_COPY_AND_ASSIGN(TestSearchTermsData);
};

TestSearchTermsData::TestSearchTermsData(const std::string& google_base_url)
: google_base_url_(google_base_url) {
: google_base_url_(google_base_url),
enable_answers_in_suggest_(false),
is_showing_search_terms_on_search_results_pages_(false) {
}

std::string TestSearchTermsData::GoogleBaseURLValue() const {
Expand All @@ -65,6 +77,14 @@ std::string TestSearchTermsData::GoogleImageSearchSource() const {
return "google_image_search_source";
}

bool TestSearchTermsData::EnableAnswersInSuggest() const {
return enable_answers_in_suggest_;
}

bool TestSearchTermsData::IsShowingSearchTermsOnSearchResultsPages() const {
return is_showing_search_terms_on_search_results_pages_;
}

// TemplateURLTest ------------------------------------------------------------

class TemplateURLTest : public testing::Test {
Expand Down Expand Up @@ -1375,19 +1395,19 @@ TEST_F(TemplateURLTest, ReflectsBookmarkBarPinned) {
TemplateURLRef::SearchTermsArgs search_terms_args(ASCIIToUTF16("foo"));

// Do not add the param when InstantExtended is suppressed on SRPs.
url.url_ref_.showing_search_terms_ = false;
search_terms_data_.set_is_showing_search_terms_on_search_results_pages(false);
std::string result = url.url_ref().ReplaceSearchTerms(search_terms_args,
search_terms_data_);
EXPECT_EQ("http://www.google.com/?q=foo", result);

// Add the param when InstantExtended is not suppressed on SRPs.
url.url_ref_.showing_search_terms_ = true;
search_terms_data_.set_is_showing_search_terms_on_search_results_pages(true);
search_terms_args.bookmark_bar_pinned = false;
result = url.url_ref().ReplaceSearchTerms(search_terms_args,
search_terms_data_);
EXPECT_EQ("http://www.google.com/?bmbp=0&q=foo", result);

url.url_ref_.showing_search_terms_ = true;
search_terms_data_.set_is_showing_search_terms_on_search_results_pages(true);
search_terms_args.bookmark_bar_pinned = true;
result = url.url_ref().ReplaceSearchTerms(search_terms_args,
search_terms_data_);
Expand All @@ -1405,8 +1425,7 @@ TEST_F(TemplateURLTest, AnswersHasVersion) {
search_terms_data_);
EXPECT_EQ("http://bar/search?q=foo&xssi=t", result);

CommandLine::ForCurrentProcess()->AppendSwitch(
switches::kEnableAnswersInSuggest);
search_terms_data_.set_enable_answers_in_suggest(true);
TemplateURL url2(data);
result = url2.url_ref().ReplaceSearchTerms(search_terms_args,
search_terms_data_);
Expand Down
19 changes: 19 additions & 0 deletions chrome/browser/search_engines/ui_thread_search_terms_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,25 @@ std::string UIThreadSearchTermsData::GetSuggestRequestIdentifier() const {
#endif
}

bool UIThreadSearchTermsData::EnableAnswersInSuggest() const {
return OmniboxFieldTrial::EnableAnswersInSuggest();
}

bool UIThreadSearchTermsData::IsShowingSearchTermsOnSearchResultsPages() const {
return chrome::IsInstantExtendedAPIEnabled() &&
chrome::IsQueryExtractionEnabled();
}

std::string UIThreadSearchTermsData::InstantExtendedEnabledParam(
bool for_search) const {
return chrome::InstantExtendedEnabledParam(for_search);
}

std::string UIThreadSearchTermsData::ForceInstantResultsParam(
bool for_prerender) const {
return chrome::ForceInstantResultsParam(for_prerender);
}

std::string UIThreadSearchTermsData::NTPIsThemedParam() const {
DCHECK(!BrowserThread::IsThreadInitialized(BrowserThread::UI) ||
BrowserThread::CurrentlyOn(BrowserThread::UI));
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/search_engines/ui_thread_search_terms_data.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ class UIThreadSearchTermsData : public SearchTermsData {
virtual std::string GetSearchClient() const OVERRIDE;
virtual std::string GetSuggestClient() const OVERRIDE;
virtual std::string GetSuggestRequestIdentifier() const OVERRIDE;
virtual bool EnableAnswersInSuggest() const OVERRIDE;
virtual bool IsShowingSearchTermsOnSearchResultsPages() const OVERRIDE;
virtual std::string InstantExtendedEnabledParam(
bool for_search) const OVERRIDE;
virtual std::string ForceInstantResultsParam(
bool for_prerender) const OVERRIDE;
virtual std::string NTPIsThemedParam() const OVERRIDE;
virtual std::string GoogleImageSearchSource() const OVERRIDE;

Expand Down
4 changes: 0 additions & 4 deletions chrome/common/chrome_switches.cc
Original file line number Diff line number Diff line change
Expand Up @@ -686,10 +686,6 @@ const char kExtensionsNotWebstore[] = "extensions-not-webstore";
// Frequency in seconds for Extensions auto-update.
const char kExtensionsUpdateFrequency[] = "extensions-update-frequency";

// Additional query params to insert in the search and instant URLs. Useful for
// testing.
const char kExtraSearchQueryParams[] = "extra-search-query-params";

// Fakes the channel of the browser for purposes of Variations filtering. This
// is to be used for testing only. Possible values are "stable", "beta", "dev"
// and "canary". Note that this only applies if the browser's reported channel
Expand Down
Loading

0 comments on commit 5c3c6e4

Please sign in to comment.