diff options
27 files changed, 387 insertions, 470 deletions
diff --git a/chrome/browser/autocomplete/autocomplete_controller.cc b/chrome/browser/autocomplete/autocomplete_controller.cc index facdfe2..3cc8a68 100644 --- a/chrome/browser/autocomplete/autocomplete_controller.cc +++ b/chrome/browser/autocomplete/autocomplete_controller.cc @@ -464,23 +464,23 @@ void AutocompleteController::UpdateAssociatedKeywords( string16 keyword(match->GetSubstitutingExplicitlyInvokedKeyword(profile_)); if (!keyword.empty()) { keywords.insert(keyword); + continue; + } + + // Only add the keyword if the match does not have a duplicate keyword with + // a more relevant match. + keyword = match->associated_keyword.get() ? + match->associated_keyword->keyword : + keyword_provider_->GetKeywordForText(match->fill_into_edit); + if (!keyword.empty() && !keywords.count(keyword)) { + keywords.insert(keyword); + + if (!match->associated_keyword.get()) + match->associated_keyword.reset(new AutocompleteMatch( + keyword_provider_->CreateAutocompleteMatch(match->fill_into_edit, + keyword, input_))); } else { - string16 keyword = match->associated_keyword.get() ? - match->associated_keyword->keyword : - keyword_provider_->GetKeywordForText(match->fill_into_edit); - - // Only add the keyword if the match does not have a duplicate keyword - // with a more relevant match. - if (!keyword.empty() && !keywords.count(keyword)) { - keywords.insert(keyword); - - if (!match->associated_keyword.get()) - match->associated_keyword.reset(new AutocompleteMatch( - keyword_provider_->CreateAutocompleteMatch(match->fill_into_edit, - keyword, input_))); - } else { - match->associated_keyword.reset(); - } + match->associated_keyword.reset(); } } } diff --git a/chrome/browser/autocomplete/autocomplete_provider_unittest.cc b/chrome/browser/autocomplete/autocomplete_provider_unittest.cc index fea4c84..f95d1f6 100644 --- a/chrome/browser/autocomplete/autocomplete_provider_unittest.cc +++ b/chrome/browser/autocomplete/autocomplete_provider_unittest.cc @@ -189,7 +189,7 @@ class AutocompleteProviderTest : public testing::Test, scoped_ptr<AutocompleteController> controller_; private: - // content::NotificationObserver + // content::NotificationObserver: virtual void Observe(int type, const content::NotificationSource& source, const content::NotificationDetails& details) OVERRIDE; @@ -435,7 +435,7 @@ TEST_F(AutocompleteProviderTest, Query) { // Make sure the default match gets set to the highest relevance match. The // highest relevance matches should come from the second provider. - EXPECT_EQ(kResultsPerProvider * 2, result_.size()); // two providers + EXPECT_EQ(kResultsPerProvider * 2, result_.size()); ASSERT_NE(result_.end(), result_.default_match()); EXPECT_EQ(provider2, result_.default_match()->provider); } @@ -445,7 +445,7 @@ TEST_F(AutocompleteProviderTest, AssistedQueryStats) { ResetControllerWithTestProviders(false, NULL, NULL); RunTest(); - EXPECT_EQ(kResultsPerProvider * 2, result_.size()); // two providers + ASSERT_EQ(kResultsPerProvider * 2, result_.size()); // Now, check the results from the second provider, as they should not have // assisted query stats set. diff --git a/chrome/browser/autocomplete/keyword_provider.cc b/chrome/browser/autocomplete/keyword_provider.cc index 3726f8f..f6487a6 100644 --- a/chrome/browser/autocomplete/keyword_provider.cc +++ b/chrome/browser/autocomplete/keyword_provider.cc @@ -86,9 +86,8 @@ class CompareQuality { // probably better rankings than the fraction of the keyword typed. We should // always put any exact matches first no matter what, since the code in // Start() assumes this (and it makes sense). - bool operator()(const string16& keyword1, - const string16& keyword2) const { - return keyword1.length() < keyword2.length(); + bool operator()(const TemplateURL* t_url1, const TemplateURL* t_url2) const { + return t_url1->keyword().length() < t_url2->keyword().length(); } }; @@ -216,8 +215,9 @@ AutocompleteMatch KeywordProvider::CreateAutocompleteMatch( const string16& text, const string16& keyword, const AutocompleteInput& input) { - return CreateAutocompleteMatch(GetTemplateURLService(), keyword, input, - keyword.size(), SplitReplacementStringFromInput(text, true), 0); + return CreateAutocompleteMatch( + GetTemplateURLService()->GetTemplateURLForKeyword(keyword), input, + keyword.length(), SplitReplacementStringFromInput(text, true), 0); } void KeywordProvider::Start(const AutocompleteInput& input, @@ -253,8 +253,6 @@ void KeywordProvider::Start(const AutocompleteInput& input, if (!ExtractKeywordFromInput(input, &keyword, &remaining_input)) return; - TemplateURLService* model = GetTemplateURLService(); - // Get the best matches for this keyword. // // NOTE: We could cache the previous keywords and reuse them here in the @@ -265,27 +263,26 @@ void KeywordProvider::Start(const AutocompleteInput& input, // TODO(pkasting): http://b/893701 We should remember the user's use of a // search query both from the autocomplete popup and from web pages // themselves. - std::vector<string16> keyword_matches; - model->FindMatchingKeywords(keyword, - !remaining_input.empty(), - &keyword_matches); + TemplateURLService::TemplateURLVector matches; + GetTemplateURLService()->FindMatchingKeywords( + keyword, !remaining_input.empty(), &matches); - for (std::vector<string16>::iterator i(keyword_matches.begin()); - i != keyword_matches.end(); ) { - const TemplateURL* template_url = model->GetTemplateURLForKeyword(*i); + for (TemplateURLService::TemplateURLVector::iterator i(matches.begin()); + i != matches.end(); ) { + const TemplateURL* template_url = *i; // Prune any extension keywords that are disallowed in incognito mode (if // we're incognito), or disabled. if (profile_ && template_url->IsExtensionKeyword()) { ExtensionService* service = extensions::ExtensionSystem::Get(profile_)-> extension_service(); - const extensions::Extension* extension = service->GetExtensionById( - template_url->GetExtensionId(), false); + const extensions::Extension* extension = + service->GetExtensionById(template_url->GetExtensionId(), false); bool enabled = extension && (!profile_->IsOffTheRecord() || service->IsIncognitoEnabled(extension->id())); if (!enabled) { - i = keyword_matches.erase(i); + i = matches.erase(i); continue; } } @@ -293,22 +290,22 @@ void KeywordProvider::Start(const AutocompleteInput& input, // Prune any substituting keywords if there is no substitution. if (template_url->SupportsReplacement() && remaining_input.empty() && !input.allow_exact_keyword_match()) { - i = keyword_matches.erase(i); + i = matches.erase(i); continue; } ++i; } - if (keyword_matches.empty()) + if (matches.empty()) return; - std::sort(keyword_matches.begin(), keyword_matches.end(), CompareQuality()); + std::sort(matches.begin(), matches.end(), CompareQuality()); // Limit to one exact or three inexact matches, and mark them up for display // in the autocomplete popup. // Any exact match is going to be the highest quality match, and thus at the // front of our vector. - if (keyword_matches.front() == keyword) { - const TemplateURL* template_url = model->GetTemplateURLForKeyword(keyword); + if (matches.front()->keyword() == keyword) { + const TemplateURL* template_url = matches.front(); const bool is_extension_keyword = template_url->IsExtensionKeyword(); // Only create an exact match if |remaining_input| is empty or if @@ -321,9 +318,8 @@ void KeywordProvider::Start(const AutocompleteInput& input, // TODO(pkasting): We should probably check that if the user explicitly // typed a scheme, that scheme matches the one in |template_url|. - matches_.push_back(CreateAutocompleteMatch(model, keyword, input, - keyword.length(), - remaining_input, -1)); + matches_.push_back(CreateAutocompleteMatch( + template_url, input, keyword.length(), remaining_input, -1)); if (profile_ && is_extension_keyword) { if (input.matches_requested() == AutocompleteInput::ALL_MATCHES) { @@ -364,15 +360,12 @@ void KeywordProvider::Start(const AutocompleteInput& input, } } } else { - if (keyword_matches.size() > kMaxMatches) { - keyword_matches.erase(keyword_matches.begin() + kMaxMatches, - keyword_matches.end()); - } - for (std::vector<string16>::const_iterator i(keyword_matches.begin()); - i != keyword_matches.end(); ++i) { - matches_.push_back(CreateAutocompleteMatch(model, *i, - input, keyword.length(), - remaining_input, -1)); + if (matches.size() > kMaxMatches) + matches.erase(matches.begin() + kMaxMatches, matches.end()); + for (TemplateURLService::TemplateURLVector::const_iterator i( + matches.begin()); i != matches.end(); ++i) { + matches_.push_back(CreateAutocompleteMatch( + *i, input, keyword.length(), remaining_input, -1)); } } } @@ -400,10 +393,32 @@ bool KeywordProvider::ExtractKeywordFromInput(const AutocompleteInput& input, } // static -void KeywordProvider::FillInURLAndContents( - const string16& remaining_input, - const TemplateURL* element, - AutocompleteMatch* match) { +int KeywordProvider::CalculateRelevance(AutocompleteInput::Type type, + bool complete, + bool supports_replacement, + bool prefer_keyword, + bool allow_exact_keyword_match) { + // This function is responsible for scoring suggestions of keywords + // themselves and the suggestion of the verbatim query on an + // extension keyword. SearchProvider::CalculateRelevanceForKeywordVerbatim() + // scores verbatim query suggestions for non-extension keywords. + // These two functions are currently in sync, but there's no reason + // we couldn't decide in the future to score verbatim matches + // differently for extension and non-extension keywords. If you + // make such a change, however, you should update this comment to + // describe it, so it's clear why the functions diverge. + if (!complete) + return (type == AutocompleteInput::URL) ? 700 : 450; + if (!supports_replacement || (allow_exact_keyword_match && prefer_keyword)) + return 1500; + return (allow_exact_keyword_match && (type == AutocompleteInput::QUERY)) ? + 1450 : 1100; +} + +// static +void KeywordProvider::FillInURLAndContents(const string16& remaining_input, + const TemplateURL* element, + AutocompleteMatch* match) { DCHECK(!element->short_name().empty()); const TemplateURLRef& element_ref = element->url_ref(); DCHECK(element_ref.IsValid()); @@ -442,56 +457,27 @@ void KeywordProvider::FillInURLAndContents( element->short_name(), remaining_input, &content_param_offsets)); - if (content_param_offsets.size() == 2) { - AutocompleteMatch::ClassifyLocationInString(content_param_offsets[1], - remaining_input.length(), match->contents.length(), - ACMatchClassification::NONE, &match->contents_class); - } else { - // See comments on an identical NOTREACHED() in search_provider.cc. - NOTREACHED(); - } + DCHECK_EQ(2U, content_param_offsets.size()); + AutocompleteMatch::ClassifyLocationInString(content_param_offsets[1], + remaining_input.length(), match->contents.length(), + ACMatchClassification::NONE, &match->contents_class); } } -// static -int KeywordProvider::CalculateRelevance(AutocompleteInput::Type type, - bool complete, - bool supports_replacement, - bool prefer_keyword, - bool allow_exact_keyword_match) { - // This function is responsible for scoring suggestions of keywords - // themselves and the suggestion of the verbatim query on an - // extension keyword. SearchProvider::CalculateRelevanceForKeywordVerbatim() - // scores verbatim query suggestions for non-extension keywords. - // These two functions are currently in sync, but there's no reason - // we couldn't decide in the future to score verbatim matches - // differently for extension and non-extension keywords. If you - // make such a change, however, you should update this comment to - // describe it, so it's clear why the functions diverge. - if (!complete) - return (type == AutocompleteInput::URL) ? 700 : 450; - if (!supports_replacement || (allow_exact_keyword_match && prefer_keyword)) - return 1500; - return (allow_exact_keyword_match && (type == AutocompleteInput::QUERY)) ? - 1450 : 1100; -} - AutocompleteMatch KeywordProvider::CreateAutocompleteMatch( - TemplateURLService* model, - const string16& keyword, + const TemplateURL* template_url, const AutocompleteInput& input, size_t prefix_length, const string16& remaining_input, int relevance) { - DCHECK(model); - // Get keyword data from data store. - TemplateURL* element = model->GetTemplateURLForKeyword(keyword); - DCHECK(element); - const bool supports_replacement = element->url_ref().SupportsReplacement(); + DCHECK(template_url); + const bool supports_replacement = + template_url->url_ref().SupportsReplacement(); // Create an edit entry of "[keyword] [remaining input]". This is helpful // even when [remaining input] is empty, as the user can select the popup // choice and immediately begin typing in query input. + const string16& keyword = template_url->keyword(); const bool keyword_complete = (prefix_length == keyword.length()); if (relevance < 0) { relevance = @@ -505,7 +491,7 @@ AutocompleteMatch KeywordProvider::CreateAutocompleteMatch( AutocompleteMatch match(this, relevance, false, supports_replacement ? AutocompleteMatchType::SEARCH_OTHER_ENGINE : AutocompleteMatchType::HISTORY_KEYWORD); - match.fill_into_edit.assign(keyword); + match.fill_into_edit = keyword; if (!remaining_input.empty() || !keyword_complete || supports_replacement) match.fill_into_edit.push_back(L' '); match.fill_into_edit.append(remaining_input); @@ -518,7 +504,7 @@ AutocompleteMatch KeywordProvider::CreateAutocompleteMatch( // Create destination URL and popup entry content by substituting user input // into keyword templates. - FillInURLAndContents(remaining_input, element, &match); + FillInURLAndContents(remaining_input, template_url, &match); match.keyword = keyword; match.transition = content::PAGE_TRANSITION_KEYWORD; @@ -567,10 +553,10 @@ void KeywordProvider::Observe(int type, return; // This is an old result. Just ignore. string16 keyword, remaining_input; - if (!ExtractKeywordFromInput(input, &keyword, &remaining_input)) { - NOTREACHED(); - return; - } + bool result = ExtractKeywordFromInput(input, &keyword, &remaining_input); + DCHECK(result); + const TemplateURL* template_url = + model->GetTemplateURLForKeyword(keyword); // TODO(mpcomplete): consider clamping the number of suggestions to // AutocompleteProvider::kMaxMatches. @@ -585,7 +571,7 @@ void KeywordProvider::Observe(int type, int first_relevance = CalculateRelevance(input.type(), true, true, input.prefer_keyword(), input.allow_exact_keyword_match()); extension_suggest_matches_.push_back(CreateAutocompleteMatch( - model, keyword, input, keyword.length(), + template_url, input, keyword.length(), UTF8ToUTF16(suggestion.content), first_relevance - (i + 1))); AutocompleteMatch* match = &extension_suggest_matches_.back(); diff --git a/chrome/browser/autocomplete/keyword_provider.h b/chrome/browser/autocomplete/keyword_provider.h index c9caba1..9d1ba52 100644 --- a/chrome/browser/autocomplete/keyword_provider.h +++ b/chrome/browser/autocomplete/keyword_provider.h @@ -110,12 +110,6 @@ class KeywordProvider : public AutocompleteProvider, string16* keyword, string16* remaining_input); - // Fills in the "destination_url" and "contents" fields of |match| with the - // provided user input and keyword data. - static void FillInURLAndContents(const string16& remaining_input, - const TemplateURL* element, - AutocompleteMatch* match); - // Determines the relevance for some input, given its type, whether the user // typed the complete keyword, and whether the user is in "prefer keyword // matches" mode, and whether the keyword supports replacement. @@ -127,10 +121,15 @@ class KeywordProvider : public AutocompleteProvider, bool prefer_keyword, bool allow_exact_keyword_match); + // Fills in the "destination_url" and "contents" fields of |match| with the + // provided user input and keyword data. + static void FillInURLAndContents(const string16& remaining_input, + const TemplateURL* element, + AutocompleteMatch* match); + // Creates a fully marked-up AutocompleteMatch from the user's input. // If |relevance| is negative, calculate a relevance based on heuristics. - AutocompleteMatch CreateAutocompleteMatch(TemplateURLService* model, - const string16& keyword, + AutocompleteMatch CreateAutocompleteMatch(const TemplateURL* template_url, const AutocompleteInput& input, size_t prefix_length, const string16& remaining_input, diff --git a/chrome/browser/autocomplete/keyword_provider_unittest.cc b/chrome/browser/autocomplete/keyword_provider_unittest.cc index 5dec594..6a30d59 100644 --- a/chrome/browser/autocomplete/keyword_provider_unittest.cc +++ b/chrome/browser/autocomplete/keyword_provider_unittest.cc @@ -134,22 +134,22 @@ TEST_F(KeywordProviderTest, Edit) { TEST_F(KeywordProviderTest, URL) { test_data<GURL> url_cases[] = { // No query input -> empty destination URL. - {ASCIIToUTF16("z"), 1, {GURL()}}, - {ASCIIToUTF16("z \t"), 1, {GURL()}}, + {ASCIIToUTF16("z"), 1, {GURL()}}, + {ASCIIToUTF16("z \t"), 1, {GURL()}}, // Check that tokenization only collapses whitespace between first tokens // and query input, but not rest of URL, is escaped. - {ASCIIToUTF16("w bar +baz"), 2, {GURL(" +%2B?=bar+%2Bbazfoo "), - GURL("bar+%2Bbaz=z")}}, + {ASCIIToUTF16("w bar +baz"), 2, {GURL(" +%2B?=bar+%2Bbazfoo "), + GURL("bar+%2Bbaz=z")}}, // Substitution should work with various locations of the "%s". - {ASCIIToUTF16("aaa 1a2b"), 2, {GURL("http://aaaa/?aaaa=1&b=1a2b&c"), - GURL("1a2b")}}, - {ASCIIToUTF16("a 1 2 3"), 3, {GURL("aa.com?foo=1+2+3"), - GURL("bogus URL 1+2+3"), - GURL("http://aaaa/?aaaa=1&b=1+2+3&c")}}, - {ASCIIToUTF16("www.w w"), 2, {GURL(" +%2B?=wfoo "), - GURL("weaselwweasel")}}, + {ASCIIToUTF16("aaa 1a2b"), 2, {GURL("http://aaaa/?aaaa=1&b=1a2b&c"), + GURL("1a2b")}}, + {ASCIIToUTF16("a 1 2 3"), 3, {GURL("aa.com?foo=1+2+3"), + GURL("bogus URL 1+2+3"), + GURL("http://aaaa/?aaaa=1&b=1+2+3&c")}}, + {ASCIIToUTF16("www.w w"), 2, {GURL(" +%2B?=wfoo "), + GURL("weaselwweasel")}}, }; RunTest<GURL>(url_cases, arraysize(url_cases), @@ -192,26 +192,6 @@ TEST_F(KeywordProviderTest, Contents) { &AutocompleteMatch::contents); } -TEST_F(KeywordProviderTest, DISABLED_Description) { - test_data<string16> description_cases[] = { - // Whole keyword should be returned for both exact and inexact matches. - {ASCIIToUTF16("z foo"), 1, {ASCIIToUTF16("(Keyword: z)")}}, - {ASCIIToUTF16("a foo"), 3, {ASCIIToUTF16("(Keyword: aa)"), - ASCIIToUTF16("(Keyword: ab)"), - ASCIIToUTF16("(Keyword: aaaa)")}}, - {ASCIIToUTF16("ftp://www.www w"), 0, {}}, - {ASCIIToUTF16("http://www.ab w"), 1, {ASCIIToUTF16("(Keyword: ab)")}}, - - // Keyword should be returned regardless of query input. - {ASCIIToUTF16("z"), 1, {ASCIIToUTF16("(Keyword: z)")}}, - {ASCIIToUTF16("z \t"), 1, {ASCIIToUTF16("(Keyword: z)")}}, - {ASCIIToUTF16("z a b c++"), 1, {ASCIIToUTF16("(Keyword: z)")}}, - }; - - RunTest<string16>(description_cases, arraysize(description_cases), - &AutocompleteMatch::description); -} - TEST_F(KeywordProviderTest, AddKeyword) { TemplateURLData data; data.short_name = ASCIIToUTF16("Test"); diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc index eb4f27d..baf620c 100644 --- a/chrome/browser/autocomplete/search_provider.cc +++ b/chrome/browser/autocomplete/search_provider.cc @@ -256,24 +256,21 @@ SearchProvider::SearchProvider(AutocompleteProviderListener* listener, // static AutocompleteMatch SearchProvider::CreateSearchSuggestion( - Profile* profile, AutocompleteProvider* autocomplete_provider, - const AutocompleteInput& input, - const string16& query_string, - const string16& input_text, int relevance, AutocompleteMatch::Type type, - int accepted_suggestion, + const TemplateURL* template_url, + const string16& query_string, + const string16& input_text, + const AutocompleteInput& input, bool is_keyword, - const string16& keyword, + int accepted_suggestion, int omnibox_start_margin) { AutocompleteMatch match(autocomplete_provider, relevance, false, type); - // Bail out now if we don't actually have a valid provider. - match.keyword = keyword; - const TemplateURL* provider_url = match.GetTemplateURL(profile, false); - if (provider_url == NULL) + if (!template_url) return match; + match.keyword = template_url->keyword(); match.contents.assign(query_string); // We do intra-string highlighting for suggestions - the suggested segment @@ -329,7 +326,7 @@ AutocompleteMatch SearchProvider::CreateSearchSuggestion( } match.fill_into_edit.append(query_string); - const TemplateURLRef& search_url = provider_url->url_ref(); + const TemplateURLRef& search_url = template_url->url_ref(); DCHECK(search_url.SupportsReplacement()); match.search_terms_args.reset( new TemplateURLRef::SearchTermsArgs(query_string)); @@ -1373,11 +1370,11 @@ void SearchProvider::AddMatchToMap(const string16& query_string, int accepted_suggestion, bool is_keyword, MatchMap* map) { - const string16& keyword = is_keyword ? - providers_.keyword_provider() : providers_.default_provider(); - AutocompleteMatch match = CreateSearchSuggestion(profile_, this, input_, - query_string, input_text, relevance, type, accepted_suggestion, - is_keyword, keyword, omnibox_start_margin_); + const TemplateURL* template_url = is_keyword ? + providers_.GetKeywordProviderURL() : providers_.GetDefaultProviderURL(); + AutocompleteMatch match = CreateSearchSuggestion(this, relevance, type, + template_url, query_string, input_text, input_, is_keyword, + accepted_suggestion, omnibox_start_margin_); if (!match.destination_url.is_valid()) return; match.RecordAdditionalInfo(kRelevanceFromServerKey, diff --git a/chrome/browser/autocomplete/search_provider.h b/chrome/browser/autocomplete/search_provider.h index 3d9cea3..0706c1b 100644 --- a/chrome/browser/autocomplete/search_provider.h +++ b/chrome/browser/autocomplete/search_provider.h @@ -60,23 +60,30 @@ class SearchProvider : public AutocompleteProvider, SearchProvider(AutocompleteProviderListener* listener, Profile* profile); - // Returns an AutocompleteMatch representing a search for |query_string| - // using the provider identified by |keyword|. |is_keyword| should be true if - // |input| represents a keyword search (even if it's for the default search - // provider). |input_text| (the original input text) and |accepted_suggestion| - // are used to generate Assisted Query Stats. - // Returns a match with an invalid destination_url in case of any errors. + // Returns an AutocompleteMatch with the given |autocomplete_provider|, + // |relevance|, and |type|, which represents a search via |template_url| for + // |query_string|. If |template_url| is NULL, returns a match with an invalid + // destination URL. + // + // |input_text| is the original user input, which may differ from + // |query_string|; e.g. the user typed "foo" and got a search suggestion of + // "food", which we're now marking up. This is used to highlight portions of + // the match contents to distinguish locally-typed text from suggested text. + // + // |input| and |is_keyword| are necessary for various other details, like + // whether we should allow inline autocompletion and what the transition type + // should be. |accepted_suggestion| and |omnibox_start_margin| are used along + // with |input_text| to generate Assisted Query Stats. static AutocompleteMatch CreateSearchSuggestion( - Profile* profile, AutocompleteProvider* autocomplete_provider, - const AutocompleteInput& input, - const string16& query_string, - const string16& input_text, int relevance, AutocompleteMatch::Type type, - int accepted_suggestion, + const TemplateURL* template_url, + const string16& query_string, + const string16& input_text, + const AutocompleteInput& input, bool is_keyword, - const string16& keyword, + int accepted_suggestion, int omnibox_start_margin); // AutocompleteProvider: diff --git a/chrome/browser/autocomplete/search_provider_unittest.cc b/chrome/browser/autocomplete/search_provider_unittest.cc index 36a5abf9..b8dcd84 100644 --- a/chrome/browser/autocomplete/search_provider_unittest.cc +++ b/chrome/browser/autocomplete/search_provider_unittest.cc @@ -34,7 +34,8 @@ #include "net/url_request/url_request_status.h" #include "testing/gtest/include/gtest/gtest.h" -using content::BrowserThread; + +// SearchProviderTest --------------------------------------------------------- // The following environment is configured for these tests: // . The TemplateURL default_t_url_ is set as the default provider. @@ -48,26 +49,6 @@ using content::BrowserThread; class SearchProviderTest : public testing::Test, public AutocompleteProviderListener { public: - SearchProviderTest() - : default_t_url_(NULL), - term1_(ASCIIToUTF16("term1")), - keyword_t_url_(NULL), - keyword_term_(ASCIIToUTF16("keyword")), - ui_thread_(BrowserThread::UI, &message_loop_), - io_thread_(BrowserThread::IO), - quit_when_done_(false) { - io_thread_.Start(); - } - - static void SetUpTestCase(); - - static void TearDownTestCase(); - - // See description above class for what this registers. - virtual void SetUp(); - - virtual void TearDown(); - struct ResultInfo { ResultInfo() : result_type(AutocompleteMatchType::NUM_TYPES) { } @@ -78,16 +59,36 @@ class SearchProviderTest : public testing::Test, result_type(result_type), fill_into_edit(fill_into_edit) { } + const GURL gurl; const AutocompleteMatch::Type result_type; const string16 fill_into_edit; }; + struct TestData { const string16 input; const size_t num_results; const ResultInfo output[3]; }; + SearchProviderTest() + : default_t_url_(NULL), + term1_(ASCIIToUTF16("term1")), + keyword_t_url_(NULL), + keyword_term_(ASCIIToUTF16("keyword")), + ui_thread_(content::BrowserThread::UI, &message_loop_), + io_thread_(content::BrowserThread::IO), + quit_when_done_(false) { + io_thread_.Start(); + } + + static void SetUpTestCase(); + static void TearDownTestCase(); + + // See description above class for what this registers. + virtual void SetUp() OVERRIDE; + virtual void TearDown() OVERRIDE; + void RunTest(TestData* cases, int num_cases, bool prefer_keyword); protected: @@ -165,8 +166,6 @@ class SearchProviderTest : public testing::Test, // static base::FieldTrialList* SearchProviderTest::field_trial_list_ = NULL; - -// static const std::string SearchProviderTest::kNotApplicable = "Not Applicable"; // static @@ -236,6 +235,41 @@ void SearchProviderTest::SetUp() { provider_->kMinimumTimeBetweenSuggestQueriesMs = 0; } +void SearchProviderTest::TearDown() { + message_loop_.RunUntilIdle(); + + // Shutdown the provider before the profile. + provider_ = NULL; +} + +void SearchProviderTest::RunTest(TestData* cases, + int num_cases, + bool prefer_keyword) { + ACMatches matches; + for (int i = 0; i < num_cases; ++i) { + AutocompleteInput input(cases[i].input, string16::npos, string16(), GURL(), + false, prefer_keyword, true, + AutocompleteInput::ALL_MATCHES); + provider_->Start(input, false); + matches = provider_->matches(); + string16 diagnostic_details = ASCIIToUTF16("Input was: ") + cases[i].input + + ASCIIToUTF16("; prefer_keyword was: ") + + (prefer_keyword ? ASCIIToUTF16("true") : ASCIIToUTF16("false")); + EXPECT_EQ(cases[i].num_results, matches.size()) << diagnostic_details; + if (matches.size() == cases[i].num_results) { + for (size_t j = 0; j < cases[i].num_results; ++j) { + EXPECT_EQ(cases[i].output[j].gurl, matches[j].destination_url) << + diagnostic_details; + EXPECT_EQ(cases[i].output[j].result_type, matches[j].type) << + diagnostic_details; + EXPECT_EQ(cases[i].output[j].fill_into_edit, + matches[j].fill_into_edit) << + diagnostic_details; + } + } + } +} + void SearchProviderTest::OnProviderUpdate(bool updated_matches) { if (quit_when_done_ && provider_->done()) { quit_when_done_ = false; @@ -294,41 +328,6 @@ void SearchProviderTest::QueryForInputAndSetWYTMatch( wyt_match)); } -void SearchProviderTest::TearDown() { - message_loop_.RunUntilIdle(); - - // Shutdown the provider before the profile. - provider_ = NULL; -} - -void SearchProviderTest::RunTest(TestData* cases, - int num_cases, - bool prefer_keyword) { - ACMatches matches; - for (int i = 0; i < num_cases; ++i) { - AutocompleteInput input(cases[i].input, string16::npos, string16(), GURL(), - false, prefer_keyword, true, - AutocompleteInput::ALL_MATCHES); - provider_->Start(input, false); - matches = provider_->matches(); - string16 diagnostic_details = ASCIIToUTF16("Input was: ") + cases[i].input + - ASCIIToUTF16("; prefer_keyword was: ") + - (prefer_keyword ? ASCIIToUTF16("true") : ASCIIToUTF16("false")); - EXPECT_EQ(cases[i].num_results, matches.size()) << diagnostic_details; - if (matches.size() == cases[i].num_results) { - for (size_t j = 0; j < cases[i].num_results; ++j) { - EXPECT_EQ(cases[i].output[j].gurl, matches[j].destination_url) << - diagnostic_details; - EXPECT_EQ(cases[i].output[j].result_type, matches[j].type) << - diagnostic_details; - EXPECT_EQ(cases[i].output[j].fill_into_edit, - matches[j].fill_into_edit) << - diagnostic_details; - } - } - } -} - GURL SearchProviderTest::AddSearchToHistory(TemplateURL* t_url, string16 term, int visit_count) { @@ -380,7 +379,8 @@ void SearchProviderTest::FinishDefaultSuggestQuery() { default_fetcher->delegate()->OnURLFetchComplete(default_fetcher); } -// Tests ----------------------------------------------------------------------- + +// Actual Tests --------------------------------------------------------------- // Make sure we query history for the default provider and a URLFetcher is // created for the default provider suggest results. diff --git a/chrome/browser/autocomplete/zero_suggest_provider.cc b/chrome/browser/autocomplete/zero_suggest_provider.cc index ab86bff..9cc8d81 100644 --- a/chrome/browser/autocomplete/zero_suggest_provider.cc +++ b/chrome/browser/autocomplete/zero_suggest_provider.cc @@ -296,29 +296,26 @@ void ZeroSuggestProvider::FillResults( void ZeroSuggestProvider::AddSuggestResultsToMap( const SearchProvider::SuggestResults& results, - const string16& provider_keyword, + const TemplateURL* template_url, SearchProvider::MatchMap* map) { for (size_t i = 0; i < results.size(); ++i) { - AddMatchToMap(results[i].suggestion(), - provider_keyword, - results[i].relevance(), - AutocompleteMatchType::SEARCH_SUGGEST, i, map); + AddMatchToMap(results[i].relevance(), AutocompleteMatchType::SEARCH_SUGGEST, + template_url, results[i].suggestion(), i, map); } } -void ZeroSuggestProvider::AddMatchToMap(const string16& query_string, - const string16& provider_keyword, - int relevance, +void ZeroSuggestProvider::AddMatchToMap(int relevance, AutocompleteMatch::Type type, + const TemplateURL* template_url, + const string16& query_string, int accepted_suggestion, SearchProvider::MatchMap* map) { // Pass in query_string as the input_text since we don't want any bolding. // TODO(samarth|melevin): use the actual omnibox margin here as well instead // of passing in -1. AutocompleteMatch match = SearchProvider::CreateSearchSuggestion( - profile_, this, AutocompleteInput(), - query_string, query_string, relevance, type, accepted_suggestion, - false, provider_keyword, -1); + this, relevance, type, template_url, query_string, query_string, + AutocompleteInput(), false, accepted_suggestion, -1); if (!match.destination_url.is_valid()) return; @@ -415,9 +412,8 @@ void ZeroSuggestProvider::ParseSuggestResults(const Value& root_val) { &suggest_results, &navigation_results_); query_matches_map_.clear(); - const TemplateURL* default_provider = - template_url_service_->GetDefaultSearchProvider(); - AddSuggestResultsToMap(suggest_results, default_provider->keyword(), + AddSuggestResultsToMap(suggest_results, + template_url_service_->GetDefaultSearchProvider(), &query_matches_map_); } diff --git a/chrome/browser/autocomplete/zero_suggest_provider.h b/chrome/browser/autocomplete/zero_suggest_provider.h index 8b372e7..3d83a35 100644 --- a/chrome/browser/autocomplete/zero_suggest_provider.h +++ b/chrome/browser/autocomplete/zero_suggest_provider.h @@ -101,21 +101,22 @@ class ZeroSuggestProvider : public AutocompleteProvider, SearchProvider::SuggestResults* suggest_results, SearchProvider::NavigationResults* navigation_results); - // Creates AutocompleteMatches for "Search |provider_keyword| for - // <suggestion>" for all suggestions in |results|, and adds them to |map|. + // Creates AutocompleteMatches to search |template_url| for "<suggestion>" for + // all suggestions in |results|, and adds them to |map|. void AddSuggestResultsToMap(const SearchProvider::SuggestResults& results, - const string16& provider_keyword, + const TemplateURL* template_url, SearchProvider::MatchMap* map); - // Creates an AutocompleteMatch for "Search |provider_keyword| for - // |query_string|". The supplied |relevance| and |type| and - // |accepted_suggestion| will also be used to create the AutocompleteMatch. + // Creates an AutocompleteMatch with the provided |relevance| and |type| to + // search |template_url| for |query_string|. |accepted_suggestion| will be + // used to generate Assisted Query Stats. + // // Adds this match to |map|; if such a match already exists, whichever one // has lower relevance is eliminated. - void AddMatchToMap(const string16& query_string, - const string16& provider_keyword, - int relevance, + void AddMatchToMap(int relevance, AutocompleteMatch::Type type, + const TemplateURL* template_url, + const string16& query_string, int accepted_suggestion, SearchProvider::MatchMap* map); diff --git a/chrome/browser/google/google_util.cc b/chrome/browser/google/google_util.cc index 5a5befb..8211ecc 100644 --- a/chrome/browser/google/google_util.cc +++ b/chrome/browser/google/google_util.cc @@ -35,16 +35,25 @@ #define LINKDOCTOR_SERVER_REQUEST_URL std::string() #endif + +// Helpers -------------------------------------------------------------------- + namespace { const char* brand_for_testing = NULL; - bool gUseMockLinkDoctorBaseURLForTesting = false; -} // anonymous namespace +bool IsPathHomePageBase(const std::string& path) { + return (path == "/") || (path == "/webhp"); +} + +} // namespace + namespace google_util { +// Global functions ----------------------------------------------------------- + bool HasGoogleSearchQueryParam(const std::string& str) { url_parse::Component query(0, str.length()), key, value; while (url_parse::ExtractQueryKeyValue(str.c_str(), &query, &key, @@ -65,15 +74,6 @@ void SetMockLinkDoctorBaseURLForTesting() { gUseMockLinkDoctorBaseURLForTesting = true; } -BrandForTesting::BrandForTesting(const std::string& brand) : brand_(brand) { - DCHECK(brand_for_testing == NULL); - brand_for_testing = brand_.c_str(); -} - -BrandForTesting::~BrandForTesting() { - brand_for_testing = NULL; -} - GURL AppendGoogleLocaleParam(const GURL& url) { // Google does not yet recognize 'nb' for Norwegian Bokmal, but it uses // 'no' for that. @@ -152,14 +152,6 @@ bool GetReactivationBrand(std::string* brand) { #endif -bool IsGoogleDomainUrl(const GURL& url, - SubdomainPermission subdomain_permission, - PortPermission port_permission) { - return url.is_valid() && (url.SchemeIs("http") || url.SchemeIs("https")) && - (url.port().empty() || (port_permission == ALLOW_NON_STANDARD_PORTS)) && - google_util::IsGoogleHostname(url.host(), subdomain_permission); -} - bool IsGoogleHostname(const std::string& host, SubdomainPermission subdomain_permission) { size_t tld_length = net::registry_controlled_domains::GetRegistryLength( @@ -176,6 +168,14 @@ bool IsGoogleHostname(const std::string& host, return LowerCaseEqualsASCII(host_minus_tld, "www.google."); } +bool IsGoogleDomainUrl(const GURL& url, + SubdomainPermission subdomain_permission, + PortPermission port_permission) { + return url.is_valid() && (url.SchemeIs("http") || url.SchemeIs("https")) && + (url.port().empty() || (port_permission == ALLOW_NON_STANDARD_PORTS)) && + google_util::IsGoogleHostname(url.host(), subdomain_permission); +} + bool IsGoogleHomePageUrl(const GURL& url) { // First check to see if this has a Google domain. if (!IsGoogleDomainUrl(url, DISALLOW_SUBDOMAIN, DISALLOW_NON_STANDARD_PORTS)) @@ -183,12 +183,7 @@ bool IsGoogleHomePageUrl(const GURL& url) { // Make sure the path is a known home page path. std::string path(url.path()); - if (path != "/" && path != "/webhp" && - !StartsWithASCII(path, "/ig", false)) { - return false; - } - - return true; + return IsPathHomePageBase(path) || StartsWithASCII(path, "/ig", false); } bool IsGoogleSearchUrl(const GURL& url) { @@ -198,25 +193,14 @@ bool IsGoogleSearchUrl(const GURL& url) { // Make sure the path is a known search path. std::string path(url.path()); - bool has_valid_path = false; - bool is_home_page_base = false; - if (path == "/search") { - has_valid_path = true; - } else if (path == "/webhp" || path == "/") { - // Note that we allow both "/" and "" paths, but GURL spits them - // both out as just "/". - has_valid_path = true; - is_home_page_base = true; - } - if (!has_valid_path) + bool is_home_page_base = IsPathHomePageBase(path); + if (!is_home_page_base && (path != "/search")) return false; // Check for query parameter in URL parameter and hash fragment, depending on // the path type. - std::string query(url.query()); - std::string ref(url.ref()); - return HasGoogleSearchQueryParam(ref) || - (!is_home_page_base && HasGoogleSearchQueryParam(query)); + return HasGoogleSearchQueryParam(url.ref()) || + (!is_home_page_base && HasGoogleSearchQueryParam(url.query())); } bool IsOrganic(const std::string& brand) { @@ -280,4 +264,17 @@ bool IsInternetCafeBrandCode(const std::string& brand) { return found != end; } + +// BrandForTesting ------------------------------------------------------------ + +BrandForTesting::BrandForTesting(const std::string& brand) : brand_(brand) { + DCHECK(brand_for_testing == NULL); + brand_for_testing = brand_.c_str(); +} + +BrandForTesting::~BrandForTesting() { + brand_for_testing = NULL; +} + + } // namespace google_util diff --git a/chrome/browser/google/google_util.h b/chrome/browser/google/google_util.h index 0f10e31..cf7ac94 100644 --- a/chrome/browser/google/google_util.h +++ b/chrome/browser/google/google_util.h @@ -69,6 +69,11 @@ enum PortPermission { DISALLOW_NON_STANDARD_PORTS, }; +// True if |host| is "[www.]google.<TLD>" with a valid TLD. If +// |subdomain_permission| is ALLOW_SUBDOMAIN, we check against host +// "*.google.<TLD>" instead. +bool IsGoogleHostname(const std::string& host, + SubdomainPermission subdomain_permission); // True if |url| is a valid URL with a host that returns true for // IsGoogleHostname(), and an HTTP or HTTPS scheme. If |port_permission| is // DISALLOW_NON_STANDARD_PORTS, this also requires |url| to use the standard @@ -76,11 +81,6 @@ enum PortPermission { bool IsGoogleDomainUrl(const GURL& url, SubdomainPermission subdomain_permission, PortPermission port_permission); -// True if |host| is "[www.]google.<TLD>" with a valid TLD. If -// |subdomain_permission| is ALLOW_SUBDOMAIN, we check against host -// "*.google.<TLD>" instead. -bool IsGoogleHostname(const std::string& host, - SubdomainPermission subdomain_permission); // True if |url| represents a valid Google home page URL. bool IsGoogleHomePageUrl(const GURL& url); // True if |url| represents a valid Google search URL. diff --git a/chrome/browser/google/google_util_unittest.cc b/chrome/browser/google/google_util_unittest.cc index 99a1161..e300e79 100644 --- a/chrome/browser/google/google_util_unittest.cc +++ b/chrome/browser/google/google_util_unittest.cc @@ -3,7 +3,6 @@ // found in the LICENSE file. #include "base/command_line.h" -#include "base/strings/utf_string_conversions.h" #include "chrome/browser/google/google_url_tracker.h" #include "chrome/browser/google/google_util.h" #include "chrome/common/chrome_switches.h" diff --git a/chrome/browser/metro_viewer/chrome_metro_viewer_process_host_aurawin.cc b/chrome/browser/metro_viewer/chrome_metro_viewer_process_host_aurawin.cc index 7ba1f9c..6e77c6f 100644 --- a/chrome/browser/metro_viewer/chrome_metro_viewer_process_host_aurawin.cc +++ b/chrome/browser/metro_viewer/chrome_metro_viewer_process_host_aurawin.cc @@ -9,9 +9,7 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/browser_process_platform_part_aurawin.h" #include "chrome/browser/profiles/profile_manager.h" -#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/browser/search_engines/util.h" #include "chrome/browser/ui/ash/ash_init.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_finder.h" @@ -105,15 +103,8 @@ void ChromeMetroViewerProcessHost::OnOpenURL(const string16& url) { void ChromeMetroViewerProcessHost::OnHandleSearchRequest( const string16& search_string) { - const TemplateURL* default_provider = - TemplateURLServiceFactory::GetForProfile( - ProfileManager::GetDefaultProfileOrOffTheRecord())-> - GetDefaultSearchProvider(); - if (default_provider) { - const TemplateURLRef& search_url = default_provider->url_ref(); - DCHECK(search_url.SupportsReplacement()); - GURL request_url = GURL(search_url.ReplaceSearchTerms( - TemplateURLRef::SearchTermsArgs(search_string))); - OpenURL(request_url); - } + GURL url(GetDefaultSearchURLForSearchTerms( + ProfileManager::GetDefaultProfileOrOffTheRecord(), search_string)); + if (url.is_valid()) + OpenURL(url); } diff --git a/chrome/browser/search/search.cc b/chrome/browser/search/search.cc index 12a5a36..ad0c662 100644 --- a/chrome/browser/search/search.cc +++ b/chrome/browser/search/search.cc @@ -152,36 +152,35 @@ 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) It has a secure scheme. +bool IsSuitableURLForInstant(const GURL& url, const TemplateURL* template_url) { + return template_url->HasSearchTermsReplacementKey(url) && + url.SchemeIsSecure(); +} + // Returns true if |url| can be used as an Instant URL for |profile|. bool IsInstantURL(const GURL& url, Profile* profile) { - TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile); - if (!template_url) - return false; - - const TemplateURLRef& instant_url_ref = template_url->instant_url_ref(); - const bool extended_api_enabled = IsInstantExtendedAPIEnabled(); - if (!url.is_valid()) return false; - if (extended_api_enabled && !url.SchemeIsSecure()) + TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile); + if (!template_url) return false; - if (extended_api_enabled && !template_url->HasSearchTermsReplacementKey(url)) + const bool extended_api_enabled = IsInstantExtendedAPIEnabled(); + if (extended_api_enabled && !IsSuitableURLForInstant(url, template_url)) return false; + const TemplateURLRef& instant_url_ref = template_url->instant_url_ref(); const GURL instant_url = TemplateURLRefToGURL(instant_url_ref, kDisableStartMargin); - if (!instant_url.is_valid()) - return false; - - if (MatchesOriginAndPath(url, instant_url)) - return true; - - if (extended_api_enabled && MatchesAnySearchURL(url, template_url)) - return true; - - return false; + return instant_url.is_valid() && + (MatchesOriginAndPath(url, instant_url) || + (extended_api_enabled && MatchesAnySearchURL(url, template_url))); } string16 GetSearchTermsImpl(const content::WebContents* contents, @@ -200,7 +199,7 @@ string16 GetSearchTermsImpl(const content::WebContents* contents, Profile* profile = Profile::FromBrowserContext(contents->GetBrowserContext()); #if !defined(OS_IOS) && !defined(OS_ANDROID) if (!IsRenderedInInstantProcess(contents, profile) && - (contents->GetController().GetLastCommittedEntry() == entry || + ((entry == contents->GetController().GetLastCommittedEntry()) || !ShouldAssignURLToInstantRenderer(entry->GetURL(), profile))) return string16(); #endif // !defined(OS_IOS) && !defined(OS_ANDROID) @@ -262,14 +261,9 @@ bool IsQueryExtractionEnabled() { string16 GetSearchTermsFromURL(Profile* profile, const GURL& url) { string16 search_terms; - TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile); - if (!template_url) - return string16(); - - if (url.SchemeIsSecure() && template_url->HasSearchTermsReplacementKey(url)) + if (template_url && IsSuitableURLForInstant(url, template_url)) template_url->ExtractSearchTermsFromURL(url, &search_terms); - return search_terms; } @@ -437,24 +431,22 @@ GURL GetInstantURL(Profile* profile, int start_margin) { if (!IsInstantCheckboxEnabled(profile)) return GURL(); - const bool extended_api_enabled = IsInstantExtendedAPIEnabled(); - // In non-extended mode, the checkbox must be checked. + const bool extended_api_enabled = IsInstantExtendedAPIEnabled(); if (!extended_api_enabled && !IsInstantCheckboxChecked(profile)) return GURL(); TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile); GURL instant_url = TemplateURLRefToGURL(template_url->instant_url_ref(), start_margin); - if (extended_api_enabled && !instant_url.SchemeIsSecure()) { - // Extended mode requires HTTPS. Force it if necessary. - const std::string secure_scheme = chrome::kHttpsScheme; - GURL::Replacements replacements; - replacements.SetSchemeStr(secure_scheme); - instant_url = instant_url.ReplaceComponents(replacements); - } - return instant_url; + // Extended mode requires HTTPS. Force it. + if (!extended_api_enabled || instant_url.SchemeIsSecure()) + return instant_url; + GURL::Replacements replacements; + const std::string secure_scheme(chrome::kHttpsScheme); + replacements.SetSchemeStr(secure_scheme); + return instant_url.ReplaceComponents(replacements); } GURL GetLocalInstantURL(Profile* profile) { diff --git a/chrome/browser/search_engines/template_url.h b/chrome/browser/search_engines/template_url.h index 084aaef..b8149f7 100644 --- a/chrome/browser/search_engines/template_url.h +++ b/chrome/browser/search_engines/template_url.h @@ -59,8 +59,10 @@ class TemplateURLRef { // The search terms (query). const string16 search_terms; + // The original (input) query. string16 original_query; + // The optional assisted query stats, aka AQS, used for logging purposes. // This string contains impressions of all autocomplete matches shown // at the query submission time. For privacy reasons, we require the diff --git a/chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc b/chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc index 115abc7..cf6799f 100644 --- a/chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc +++ b/chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc @@ -304,13 +304,13 @@ TEST_F(TemplateURLPrepopulateDataTest, GetEngineTypeAdvanced) { TemplateURLPrepopulateData::GetEngineType(kYahooURLs[i])); } // URLs for engines not present in country-specific lists. - std::string kNigmaURL = "http://www.nigma.ru/?s={searchTerms}&arg1=value1"; EXPECT_EQ(SEARCH_ENGINE_NIGMA, - TemplateURLPrepopulateData::GetEngineType(kNigmaURL)); + TemplateURLPrepopulateData::GetEngineType( + "http://www.nigma.ru/?s={searchTerms}&arg1=value1")); // Search URL for which no prepopulated search provider exists. - std::string kExampleSearchURL = "http://example.net/search?q={searchTerms}"; EXPECT_EQ(SEARCH_ENGINE_OTHER, - TemplateURLPrepopulateData::GetEngineType(kExampleSearchURL)); + TemplateURLPrepopulateData::GetEngineType( + "http://example.net/search?q={searchTerms}")); EXPECT_EQ(SEARCH_ENGINE_OTHER, TemplateURLPrepopulateData::GetEngineType("invalid:search:url")); } diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc index f3483f6..813f488 100644 --- a/chrome/browser/search_engines/template_url_service.cc +++ b/chrome/browser/search_engines/template_url_service.cc @@ -410,7 +410,7 @@ bool TemplateURLService::CanReplaceKeyword( void TemplateURLService::FindMatchingKeywords( const string16& prefix, bool support_replacement_only, - std::vector<string16>* matches) const { + TemplateURLVector* matches) const { // Sanity check args. if (prefix.empty()) return; @@ -434,7 +434,7 @@ void TemplateURLService::FindMatchingKeywords( for (KeywordToTemplateMap::const_iterator i(match_range.first); i != match_range.second; ++i) { if (!support_replacement_only || i->second->url_ref().SupportsReplacement()) - matches->push_back(i->first); + matches->push_back(i->second); } } diff --git a/chrome/browser/search_engines/template_url_service.h b/chrome/browser/search_engines/template_url_service.h index 83826d5..724f771 100644 --- a/chrome/browser/search_engines/template_url_service.h +++ b/chrome/browser/search_engines/template_url_service.h @@ -118,12 +118,12 @@ class TemplateURLService : public WebDataServiceConsumer, const GURL& url, TemplateURL** template_url_to_replace); - // Returns (in |matches|) all keywords beginning with |prefix|, sorted - // shortest-first. If support_replacement_only is true, only keywords that - // support replacement are returned. + // Returns (in |matches|) all TemplateURLs whose keywords begin with |prefix|, + // sorted shortest keyword-first. If |support_replacement_only| is true, only + // TemplateURLs that support replacement are returned. void FindMatchingKeywords(const string16& prefix, bool support_replacement_only, - std::vector<string16>* matches) const; + TemplateURLVector* matches) const; // Looks up |keyword| and returns the element it maps to. Returns NULL if // the keyword was not found. diff --git a/chrome/browser/search_engines/util.cc b/chrome/browser/search_engines/util.cc index b477350..fbef1cc 100644 --- a/chrome/browser/search_engines/util.cc +++ b/chrome/browser/search_engines/util.cc @@ -39,6 +39,20 @@ string16 GetDefaultSearchEngineName(Profile* profile) { return default_provider->short_name(); } +GURL GetDefaultSearchURLForSearchTerms(Profile* profile, + const string16& terms) { + DCHECK(profile); + const TemplateURL* default_provider = + TemplateURLServiceFactory::GetForProfile(profile)-> + GetDefaultSearchProvider(); + if (!default_provider) + return GURL(); + const TemplateURLRef& search_url = default_provider->url_ref(); + DCHECK(search_url.SupportsReplacement()); + TemplateURLRef::SearchTermsArgs search_terms_args(terms); + return GURL(search_url.ReplaceSearchTerms(search_terms_args)); +} + void RemoveDuplicatePrepopulateIDs( WebDataService* service, const ScopedVector<TemplateURL>& prepopulated_urls, diff --git a/chrome/browser/search_engines/util.h b/chrome/browser/search_engines/util.h index 912748d..b496e2a 100644 --- a/chrome/browser/search_engines/util.h +++ b/chrome/browser/search_engines/util.h @@ -23,6 +23,10 @@ class WebDataService; // none is set. |profile| may be NULL. string16 GetDefaultSearchEngineName(Profile* profile); +// Returns a GURL that searches for |terms| using the default search engine of +// |profile|. +GURL GetDefaultSearchURLForSearchTerms(Profile* profile, const string16& terms); + // Modifies |prepopulated_url| so that it contains user-modified fields from // |original_turl|. Both URLs must have the same prepopulate_id. void MergeIntoPrepopulatedEngineData(TemplateURLData* prepopulated_url, diff --git a/chrome/browser/ui/omnibox/omnibox_controller.cc b/chrome/browser/ui/omnibox/omnibox_controller.cc index 8255044..2abb986 100644 --- a/chrome/browser/ui/omnibox/omnibox_controller.cc +++ b/chrome/browser/ui/omnibox/omnibox_controller.cc @@ -25,23 +25,6 @@ #include "extensions/common/constants.h" #include "ui/gfx/rect.h" -using predictors::AutocompleteActionPredictor; - -namespace { - -string16 GetDefaultSearchProviderKeyword(Profile* profile) { - TemplateURLService* template_url_service = - TemplateURLServiceFactory::GetForProfile(profile); - if (template_url_service) { - TemplateURL* template_url = - template_url_service->GetDefaultSearchProvider(); - if (template_url) - return template_url->keyword(); - } - return string16(); -} - -} // namespace OmniboxController::OmniboxController(OmniboxEditModel* omnibox_edit_model, Profile* profile) @@ -229,7 +212,7 @@ void OmniboxController::DoPreconnect(const AutocompleteMatch& match) { if (profile_->GetNetworkPredictor()) { profile_->GetNetworkPredictor()->AnticipateOmniboxUrl( match.destination_url, - AutocompleteActionPredictor::IsPreconnectable(match)); + predictors::AutocompleteActionPredictor::IsPreconnectable(match)); } // We could prefetch the alternate nav URL, if any, but because there // can be many of these as a user types an initial series of characters, @@ -271,11 +254,17 @@ void OmniboxController::CreateAndSetInstantMatch( string16 query_string, string16 input_text, AutocompleteMatchType::Type match_type) { - string16 keyword = GetDefaultSearchProviderKeyword(profile_); - if (keyword.empty()) - return; // CreateSearchSuggestion needs a keyword. + TemplateURLService* template_url_service = + TemplateURLServiceFactory::GetForProfile(profile_); + if (!template_url_service) + return; + + TemplateURL* template_url = + template_url_service->GetDefaultSearchProvider(); + if (!template_url) + return; current_match_ = SearchProvider::CreateSearchSuggestion( - profile_, NULL, AutocompleteInput(), query_string, input_text, 0, - match_type, 0, false, keyword, -1); + NULL, 0, match_type, template_url, query_string, input_text, + AutocompleteInput(), false, 0, -1); } diff --git a/chrome/browser/ui/omnibox/omnibox_edit_model.cc b/chrome/browser/ui/omnibox/omnibox_edit_model.cc index 6f7c1ae..8d450ee 100644 --- a/chrome/browser/ui/omnibox/omnibox_edit_model.cc +++ b/chrome/browser/ui/omnibox/omnibox_edit_model.cc @@ -896,24 +896,7 @@ void OmniboxEditModel::OnControlKeyChanged(bool pressed) { void OmniboxEditModel::OnUpOrDownKeyPressed(int count) { // NOTE: This purposefully doesn't trigger any code that resets paste_state_. - if (!popup_model()->IsOpen()) { - if (!query_in_progress()) { - // The popup is neither open nor working on a query already. So, start an - // autocomplete query for the current text. This also sets - // user_input_in_progress_ to true, which we want: if the user has started - // to interact with the popup, changing the permanent_text_ shouldn't - // change the displayed text. - // Note: This does not force the popup to open immediately. - // TODO(pkasting): We should, in fact, force this particular query to open - // the popup immediately. - if (!user_input_in_progress_) - InternalSetUserText(permanent_text_); - view_->UpdatePopup(); - } else { - // TODO(pkasting): The popup is working on a query but is not open. We - // should force it to open immediately. - } - } else { + if (popup_model()->IsOpen()) { #if defined(HTML_INSTANT_EXTENDED_POPUP) InstantController* instant = GetInstantController(); if (instant && instant->OnUpOrDownKeyPressed(count)) { @@ -922,14 +905,33 @@ void OmniboxEditModel::OnUpOrDownKeyPressed(int count) { // irrelevant, so don't process the key press ourselves. However, do stop // the autocomplete system from changing the results. autocomplete_controller()->Stop(false); - } else -#endif - { - // The popup is open, so the user should be able to interact with it - // normally. - popup_model()->Move(count); + return; } +#endif + + // The popup is open, so the user should be able to interact with it + // normally. + popup_model()->Move(count); + return; + } + + if (!query_in_progress()) { + // The popup is neither open nor working on a query already. So, start an + // autocomplete query for the current text. This also sets + // user_input_in_progress_ to true, which we want: if the user has started + // to interact with the popup, changing the permanent_text_ shouldn't change + // the displayed text. + // Note: This does not force the popup to open immediately. + // TODO(pkasting): We should, in fact, force this particular query to open + // the popup immediately. + if (!user_input_in_progress_) + InternalSetUserText(permanent_text_); + view_->UpdatePopup(); + return; } + + // TODO(pkasting): The popup is working on a query but is not open. We should + // force it to open immediately. } void OmniboxEditModel::OnPopupDataChanged( @@ -1235,11 +1237,11 @@ void OmniboxEditModel::GetInfoForCurrentText(AutocompleteMatch* match, TemplateURLServiceFactory::GetForProfile(profile_)-> GetDefaultSearchProvider(); if (default_provider && default_provider->SupportsReplacement()) { - *match = SearchProvider::CreateSearchSuggestion(profile_, - autocomplete_controller()->search_provider(), input, text, text, - 0, AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, - TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, false, - default_provider->keyword(), + *match = SearchProvider::CreateSearchSuggestion( + autocomplete_controller()->search_provider(), 0, + AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, default_provider, + text, text, input, false, + TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, controller_->GetOmniboxBounds().x()); } else { // Can't create a new search match. Leave |match| as is, with an diff --git a/chrome/browser/ui/startup/startup_browser_creator.cc b/chrome/browser/ui/startup/startup_browser_creator.cc index e64f900..f90004a 100644 --- a/chrome/browser/ui/startup/startup_browser_creator.cc +++ b/chrome/browser/ui/startup/startup_browser_creator.cc @@ -45,9 +45,7 @@ #include "chrome/browser/printing/print_dialog_cloud.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_manager.h" -#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/browser/search_engines/util.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_finder.h" #include "chrome/browser/ui/browser_window.h" @@ -384,22 +382,17 @@ std::vector<GURL> StartupBrowserCreator::GetURLsFromCommandLine( const base::FilePath& cur_dir, Profile* profile) { std::vector<GURL> urls; - const CommandLine::StringVector& params = command_line.GetArgs(); + const CommandLine::StringVector& params = command_line.GetArgs(); for (size_t i = 0; i < params.size(); ++i) { base::FilePath param = base::FilePath(params[i]); // Handle Vista way of searching - "? <search-term>" - if (param.value().size() > 2 && - param.value()[0] == '?' && param.value()[1] == ' ') { - const TemplateURL* default_provider = - TemplateURLServiceFactory::GetForProfile(profile)-> - GetDefaultSearchProvider(); - if (default_provider) { - const TemplateURLRef& search_url = default_provider->url_ref(); - DCHECK(search_url.SupportsReplacement()); - string16 search_term = param.LossyDisplayName().substr(2); - urls.push_back(GURL(search_url.ReplaceSearchTerms( - TemplateURLRef::SearchTermsArgs(search_term)))); + if ((param.value().size() > 2) && (param.value()[0] == '?') && + (param.value()[1] == ' ')) { + GURL url(GetDefaultSearchURLForSearchTerms( + profile, param.LossyDisplayName().substr(2))); + if (url.is_valid()) { + urls.push_back(url); continue; } } @@ -436,9 +429,9 @@ std::vector<GURL> StartupBrowserCreator::GetURLsFromCommandLine( // If we are in Windows 8 metro mode and were launched as a result of the // search charm or via a url navigation in metro, then fetch the // corresponding url. - GURL url = chrome::GetURLToOpen(profile); + GURL url(chrome::GetURLToOpen(profile)); if (url.is_valid()) - urls.push_back(GURL(url)); + urls.push_back(url); } #endif // OS_WIN return urls; diff --git a/chrome/browser/ui/startup/startup_browser_creator_win.cc b/chrome/browser/ui/startup/startup_browser_creator_win.cc index 0544d90..a53e900 100644 --- a/chrome/browser/ui/startup/startup_browser_creator_win.cc +++ b/chrome/browser/ui/startup/startup_browser_creator_win.cc @@ -6,9 +6,7 @@ #include "base/logging.h" #include "base/win/metro.h" -#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/browser/search_engines/util.h" #include "chrome/browser/ui/browser_finder.h" #include "chrome/browser/ui/browser_window.h" #include "chrome/browser/ui/startup/startup_browser_creator_impl.h" @@ -21,22 +19,11 @@ GURL GetURLToOpen(Profile* profile) { string16 params; base::win::MetroLaunchType launch_type = base::win::GetMetroLaunchParams(¶ms); - if ((launch_type == base::win::METRO_PROTOCOL) || - (launch_type == base::win::METRO_LAUNCH)) { + (launch_type == base::win::METRO_LAUNCH)) return GURL(params); - } else if (launch_type == base::win::METRO_SEARCH) { - const TemplateURL* default_provider = - TemplateURLServiceFactory::GetForProfile(profile)-> - GetDefaultSearchProvider(); - if (default_provider) { - const TemplateURLRef& search_url = default_provider->url_ref(); - DCHECK(search_url.SupportsReplacement()); - return GURL(search_url.ReplaceSearchTerms( - TemplateURLRef::SearchTermsArgs(params))); - } - } - return GURL(); + return (launch_type == base::win::METRO_SEARCH) ? + GetDefaultSearchURLForSearchTerms(profile, params) : GURL(); } } // namespace chrome diff --git a/chrome/browser/ui/toolbar/toolbar_model_unittest.cc b/chrome/browser/ui/toolbar/toolbar_model_unittest.cc index 0f735a8..16292ec 100644 --- a/chrome/browser/ui/toolbar/toolbar_model_unittest.cc +++ b/chrome/browser/ui/toolbar/toolbar_model_unittest.cc @@ -256,14 +256,11 @@ void ToolbarModelTest::NavigateAndCheckTextImpl(const GURL& url, TEST_F(ToolbarModelTest, ShouldDisplayURLQueryExtractionDisabled) { ASSERT_FALSE(chrome::IsQueryExtractionEnabled()) << "This test expects query extraction to be disabled."; - ResetDefaultTemplateURL(); AddTab(browser(), GURL(content::kAboutBlankURL)); for (size_t i = 0; i < arraysize(test_items); ++i) { const TestItem& test_item = test_items[i]; - NavigateAndCheckText(test_item.url, - test_item.expected_text, - test_item.expected_replace_text_inactive, - false, + NavigateAndCheckText(test_item.url, test_item.expected_text, + test_item.expected_replace_text_inactive, false, test_item.should_display); } } @@ -271,15 +268,12 @@ TEST_F(ToolbarModelTest, ShouldDisplayURLQueryExtractionDisabled) { // Test that we replace URLs when the query extraction API is enabled. TEST_F(ToolbarModelTest, ShouldDisplayURLQueryExtractionEnabled) { chrome::EnableInstantExtendedAPIForTesting(); - ResetDefaultTemplateURL(); AddTab(browser(), GURL(content::kAboutBlankURL)); for (size_t i = 0; i < arraysize(test_items); ++i) { const TestItem& test_item = test_items[i]; - NavigateAndCheckText(test_item.url, - test_item.expected_text, + NavigateAndCheckText(test_item.url, test_item.expected_text, test_item.expected_replace_text_active, - test_item.would_replace, - test_item.should_display); + test_item.would_replace, test_item.should_display); } } diff --git a/chrome/browser/ui/views/frame/browser_frame_win.cc b/chrome/browser/ui/views/frame/browser_frame_win.cc index cc0f756..7234942 100644 --- a/chrome/browser/ui/views/frame/browser_frame_win.cc +++ b/chrome/browser/ui/views/frame/browser_frame_win.cc @@ -14,9 +14,7 @@ #include "chrome/app/chrome_command_ids.h" #include "chrome/browser/lifetime/application_lifetime.h" #include "chrome/browser/profiles/profile.h" -#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/browser/search_engines/util.h" #include "chrome/browser/ui/browser_commands.h" #include "chrome/browser/ui/browser_finder.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" @@ -466,22 +464,11 @@ void BrowserFrameWin::HandleMetroNavSearchRequest(WPARAM w_param, DCHECK(browser); GURL request_url; - if (w_param) { - const wchar_t* url = reinterpret_cast<const wchar_t*>(w_param); - request_url = GURL(url); + request_url = GURL(reinterpret_cast<const wchar_t*>(w_param)); } else if (l_param) { - const wchar_t* search_string = - reinterpret_cast<const wchar_t*>(l_param); - const TemplateURL* default_provider = - TemplateURLServiceFactory::GetForProfile(browser->profile())-> - GetDefaultSearchProvider(); - if (default_provider) { - const TemplateURLRef& search_url = default_provider->url_ref(); - DCHECK(search_url.SupportsReplacement()); - request_url = GURL(search_url.ReplaceSearchTerms( - TemplateURLRef::SearchTermsArgs(search_string))); - } + request_url = GetDefaultSearchURLForSearchTerms( + browser->profile(), reinterpret_cast<const wchar_t*>(l_param)); } if (request_url.is_valid()) { browser->OpenURL(OpenURLParams(request_url, Referrer(), NEW_FOREGROUND_TAB, |