From 03561cae90f1d99b5c54b1ef3be69f10e882b25e Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Fri, 12 Mar 2021 09:13:00 +0100 Subject: BASELINE: Update Chromium to 88.0.4324.208 Change-Id: I3ae87d23e4eff4b4a469685658740a213600c667 Reviewed-by: Allan Sandfeld Jensen --- chromium/components/search_engines/BUILD.gn | 1 + .../search_engines/default_search_manager.cc | 2 +- .../search_engines/search_engine_utils.h | 4 +- .../components/search_engines/search_terms_data.cc | 2 +- .../components/search_engines/search_terms_data.h | 3 +- chromium/components/search_engines/template_url.cc | 47 ++- chromium/components/search_engines/template_url.h | 59 +++- .../template_url_prepopulate_data.cc | 125 ++++---- .../search_engines/template_url_service.cc | 318 ++++++++------------- .../search_engines/template_url_service.h | 94 ++---- .../template_url_service_unittest.cc | 60 ++++ .../search_engines/template_url_unittest.cc | 60 +++- .../search_engines/testing_search_terms_data.cc | 4 + .../search_engines/testing_search_terms_data.h | 5 + 14 files changed, 432 insertions(+), 352 deletions(-) create mode 100644 chromium/components/search_engines/template_url_service_unittest.cc (limited to 'chromium/components/search_engines') diff --git a/chromium/components/search_engines/BUILD.gn b/chromium/components/search_engines/BUILD.gn index eb66a2d9456..d95854ac609 100644 --- a/chromium/components/search_engines/BUILD.gn +++ b/chromium/components/search_engines/BUILD.gn @@ -152,6 +152,7 @@ source_set("unit_tests") { "search_host_to_urls_map_unittest.cc", "template_url_data_unittest.cc", "template_url_prepopulate_data_unittest.cc", + "template_url_service_unittest.cc", "template_url_service_util_unittest.cc", "template_url_unittest.cc", ] diff --git a/chromium/components/search_engines/default_search_manager.cc b/chromium/components/search_engines/default_search_manager.cc index 61ffeff2717..82f5878b84a 100644 --- a/chromium/components/search_engines/default_search_manager.cc +++ b/chromium/components/search_engines/default_search_manager.cc @@ -11,7 +11,7 @@ #include #include "base/bind.h" -#include "base/bind_helpers.h" +#include "base/callback_helpers.h" #include "base/check.h" #include "base/compiler_specific.h" #include "base/i18n/case_conversion.h" diff --git a/chromium/components/search_engines/search_engine_utils.h b/chromium/components/search_engines/search_engine_utils.h index 83e0dd43dac..fbba12b2639 100644 --- a/chromium/components/search_engines/search_engine_utils.h +++ b/chromium/components/search_engines/search_engine_utils.h @@ -11,8 +11,8 @@ class GURL; namespace SearchEngineUtils { -// Like the above, but takes a GURL which is expected to represent a search URL. -// This may be called on any thread. +// Takes a GURL and returns the matching enum if it matches the URL of a +// well-known search engine. This may be called on any thread. SearchEngineType GetEngineType(const GURL& url); } // namespace SearchEngineUtils diff --git a/chromium/components/search_engines/search_terms_data.cc b/chromium/components/search_engines/search_terms_data.cc index 830f47f5f74..0005ed87a8d 100644 --- a/chromium/components/search_engines/search_terms_data.cc +++ b/chromium/components/search_engines/search_terms_data.cc @@ -46,7 +46,7 @@ std::string SearchTermsData::GetSearchClient() const { return std::string(); } -std::string SearchTermsData::GetSuggestClient() const { +std::string SearchTermsData::GetSuggestClient(bool from_ntp) const { return std::string(); } diff --git a/chromium/components/search_engines/search_terms_data.h b/chromium/components/search_engines/search_terms_data.h index 5f399b2ebd7..6fc30745273 100644 --- a/chromium/components/search_engines/search_terms_data.h +++ b/chromium/components/search_engines/search_terms_data.h @@ -40,8 +40,9 @@ class SearchTermsData { // The suggest client parameter ("client") passed with Google suggest // requests. See GetSuggestRequestIdentifier() for more details. + // |from_ntp| is true if the search is made from a non-searchbox NTP surface. // This implementation returns the empty string. - virtual std::string GetSuggestClient() const; + virtual std::string GetSuggestClient(bool from_ntp) const; // The suggest request identifier parameter ("gs_ri") passed with Google // suggest requests. Along with suggestclient (See GetSuggestClient()), diff --git a/chromium/components/search_engines/template_url.cc b/chromium/components/search_engines/template_url.cc index 2a4c441162f..f195c74444a 100644 --- a/chromium/components/search_engines/template_url.cc +++ b/chromium/components/search_engines/template_url.cc @@ -5,6 +5,7 @@ #include "components/search_engines/template_url.h" #include +#include #include #include "base/base64.h" @@ -228,7 +229,8 @@ TemplateURLRef::SearchTermsArgs::ContextualSearchParams::ContextualSearchParams( bool is_exact_search, std::string source_lang, std::string target_lang, - std::string fluent_languages) + std::string fluent_languages, + std::string related_searches_stamp) : version(version), contextual_cards_version(contextual_cards_version), home_country(home_country), @@ -237,7 +239,8 @@ TemplateURLRef::SearchTermsArgs::ContextualSearchParams::ContextualSearchParams( is_exact_search(is_exact_search), source_lang(source_lang), target_lang(target_lang), - fluent_languages(fluent_languages) {} + fluent_languages(fluent_languages), + related_searches_stamp(related_searches_stamp) {} TemplateURLRef::SearchTermsArgs::ContextualSearchParams::ContextualSearchParams( const ContextualSearchParams& other) = default; @@ -1009,6 +1012,8 @@ std::string TemplateURLRef::HandleReplacements( args.push_back("tlitetl=" + params.target_lang); if (!params.fluent_languages.empty()) args.push_back("ctxs_fls=" + params.fluent_languages); + if (!params.related_searches_stamp.empty()) + args.push_back("ctxsl_rs=" + params.related_searches_stamp); HandleReplacement(std::string(), base::JoinString(args, "&"), *i, &url); break; @@ -1118,7 +1123,7 @@ std::string TemplateURLRef::HandleReplacements( // empty string. (If we don't handle this case, we hit a // NOTREACHED below.) base::string16 rlz_string = search_terms_data.GetRlzParameterValue( - search_terms_args.from_app_list); + search_terms_args.request_source == CROS_APP_LIST); if (!rlz_string.empty()) { HandleReplacement("rlz", base::UTF16ToUTF8(rlz_string), *i, &url); } @@ -1152,7 +1157,10 @@ std::string TemplateURLRef::HandleReplacements( case GOOGLE_SUGGEST_CLIENT: HandleReplacement( - std::string(), search_terms_data.GetSuggestClient(), *i, &url); + std::string(), + search_terms_data.GetSuggestClient( + search_terms_args.request_source == NON_SEARCHBOX_NTP), + *i, &url); break; case GOOGLE_SUGGEST_REQUEST_ID: @@ -1310,6 +1318,37 @@ TemplateURL::TemplateURL(const TemplateURLData& data, TemplateURL::~TemplateURL() { } +bool TemplateURL::IsBetterThanEngineWithConflictingKeyword( + const TemplateURL* other) const { + DCHECK(other); + + auto get_sort_key = [](const TemplateURL* engine) { + return std::make_tuple( + // Policy-created engines always win over non-policy created engines. + engine->created_by_policy(), + // The integral value of the type enum is used to sort next. + // This makes extension-controlled engines win. + engine->type(), + // For engines with associated extensions; more recently installed + // extensions win. + engine->extension_info_ ? engine->extension_info_->install_time + : base::Time(), + // Prefer engines that CANNOT be auto-replaced. + !engine->safe_for_autoreplace(), + // More recently modified engines win. + engine->last_modified(), + // TODO(tommycli): This should be a tie-breaker than provides a total + // ordering of all TemplateURLs so that distributed clients resolve + // conflicts identically. This sync_guid is not globally unique today, + // so we need to fix that before we can resolve conflicts with this. + engine->sync_guid()); + }; + + // Although normally sort is done by operator<, in this case, we want the + // BETTER engine to be preceding the worse engine. + return get_sort_key(this) > get_sort_key(other); +} + // static base::string16 TemplateURL::GenerateKeyword(const GURL& url) { DCHECK(url.is_valid()); diff --git a/chromium/components/search_engines/template_url.h b/chromium/components/search_engines/template_url.h index 4a656efe31b..78a4a8fb1f0 100644 --- a/chromium/components/search_engines/template_url.h +++ b/chromium/components/search_engines/template_url.h @@ -68,6 +68,13 @@ class TemplateURLRef { // the |post_data|. See http://tools.ietf.org/html/rfc2046 for the details. typedef std::pair PostContent; + // Enumeration of the known search or suggest request sources. + enum RequestSource { + SEARCHBOX, // Omnibox or the NTP realbox. The default. + CROS_APP_LIST, // Chrome OS app list search box. + NON_SEARCHBOX_NTP, // Non-searchbox NTP surfaces. + }; + // This struct encapsulates arguments passed to // TemplateURLRef::ReplaceSearchTerms methods. By default, only search_terms // is required and is passed in the constructor. @@ -108,6 +115,10 @@ class TemplateURLRef { // is fluent in reading. This acts as an alternate set of languages // to consider translating into. The languages are ordered by // fluency, and encoded as a comma-separated list of BCP 47 languages. + // The |related_searches_stamp| string contains an information that + // indicates experiment status and server processing results so that + // can be logged in GWS Sawmill logs for offline analysis for the + // Related Searches MVP experiment. ContextualSearchParams(int version, int contextual_cards_version, std::string home_country, @@ -116,7 +127,8 @@ class TemplateURLRef { bool is_exact_search, std::string source_lang, std::string target_lang, - std::string fluent_languages); + std::string fluent_languages, + std::string related_searches_stamp); ContextualSearchParams(const ContextualSearchParams& other); ~ContextualSearchParams(); @@ -157,6 +169,11 @@ class TemplateURLRef { // Alternate target languages that the user is fluent in, encoded in a // single string. std::string fluent_languages; + + // Experiment arm and processing information for the Related Searches + // experiment. The value is an arbitrary string that starts with a + // schema version number. + std::string related_searches_stamp; }; // Estimates dynamic memory usage. @@ -228,9 +245,8 @@ class TemplateURLRef { // When searching for an image, the original size of the image. gfx::Size image_original_size; - // True if the search was made using the app list search box. Otherwise, the - // search was made using the omnibox. - bool from_app_list = false; + // Source of the search or suggest request. + RequestSource request_source = SEARCHBOX; ContextualSearchParams contextual_search_params; }; @@ -561,15 +577,19 @@ class TemplateURL { using TemplateURLVector = std::vector; using OwnedTemplateURLVector = std::vector>; + // These values are not persisted and can be freely changed. + // Their integer values are used for choosing the best engine during keyword + // conflicts, so their relative ordering should not be changed without careful + // thought about what happens during version skew. enum Type { - // Regular search engine. - NORMAL, + // Installed only on this device. Should not be synced. This is not common. + LOCAL = 0, + // Regular search engine. This is the most common. + NORMAL = 1, // Installed by extension through Override Settings API. - NORMAL_CONTROLLED_BY_EXTENSION, + NORMAL_CONTROLLED_BY_EXTENSION = 2, // The keyword associated with an extension that uses the Omnibox API. - OMNIBOX_API_EXTENSION, - // Installed only on this device. Should not be synced. - LOCAL, + OMNIBOX_API_EXTENSION = 3, }; // An AssociatedExtensionInfo represents information about the extension that @@ -606,6 +626,25 @@ class TemplateURL { ~TemplateURL(); + // For two engines with the same keyword, |this| and |other|, + // returns true if |this| is strictly better than |other|. + // + // While normal engines must all have distinct keywords, policy-created, + // extension-controlled and omnibox API engines may have the same keywords as + // each other or as normal engines. In these cases, policy-create engines + // override omnibox API engines, which override extension-controlled engines, + // which override normal engines. + // + // If there is still a conflict after this, compare by safe-for-autoreplace, + // then last modified date, then use the sync guid as a tiebreaker. + // + // TODO(tommycli): I'd like to use this to resolve Sync conflicts in the + // future, but we need a total ordering of TemplateURLs. That's not the case + // today, because the sync GUIDs are not actually globally unique, so there + // can be a genuine tie, which is not good, because then two different clients + // could choose to resolve the conflict in two different ways. + bool IsBetterThanEngineWithConflictingKeyword(const TemplateURL* other) const; + // Generates a suitable keyword for the specified url, which must be valid. // This is guaranteed not to return an empty string, since TemplateURLs should // never have an empty keyword. diff --git a/chromium/components/search_engines/template_url_prepopulate_data.cc b/chromium/components/search_engines/template_url_prepopulate_data.cc index c1368c64f91..150b548b368 100644 --- a/chromium/components/search_engines/template_url_prepopulate_data.cc +++ b/chromium/components/search_engines/template_url_prepopulate_data.cc @@ -42,16 +42,16 @@ const PrepopulatedEngine* const engines_AE[] = { &bing, &yahoo, &duckduckgo, - &yandex_ru, + &ecosia, }; // Albania const PrepopulatedEngine* const engines_AL[] = { &google, - &yahoo, &bing, + &yahoo, &duckduckgo, - &yandex_ru, + &yandex_com, }; // Argentina @@ -76,8 +76,8 @@ const PrepopulatedEngine* const engines_AT[] = { const PrepopulatedEngine* const engines_AU[] = { &google, &bing, - &duckduckgo, &yahoo_au, + &duckduckgo, &ecosia, }; @@ -87,7 +87,6 @@ const PrepopulatedEngine* const engines_BA[] = { &bing, &yahoo, &duckduckgo, - &ask, }; // Belgium @@ -123,7 +122,7 @@ const PrepopulatedEngine* const engines_BI[] = { &bing, &yahoo, &duckduckgo, - &ask, + &yandex_ru, }; // Brunei @@ -159,7 +158,7 @@ const PrepopulatedEngine* const engines_BY[] = { &yandex_by, &mail_ru, &bing, - &yahoo, + &duckduckgo, }; // Belize @@ -185,15 +184,15 @@ const PrepopulatedEngine* const engines_CH[] = { &google, &bing, &duckduckgo, - &yahoo_ch, &ecosia, + &yahoo_ch, }; // Chile const PrepopulatedEngine* const engines_CL[] = { &google, &bing, - &yahoo_cl, + &yahoo_es, &duckduckgo, &ecosia, }; @@ -203,15 +202,15 @@ const PrepopulatedEngine* const engines_CN[] = { &baidu, &sogou, &google, - &so_360, &bing, + &so_360, }; // Colombia const PrepopulatedEngine* const engines_CO[] = { &google, &bing, - &yahoo_co, + &yahoo_es, &ecosia, &duckduckgo, }; @@ -238,9 +237,9 @@ const PrepopulatedEngine* const engines_CZ[] = { const PrepopulatedEngine* const engines_DE[] = { &google, &bing, - &yahoo_de, &duckduckgo, &ecosia, + &yahoo_de, }; // Denmark @@ -264,7 +263,7 @@ const PrepopulatedEngine* const engines_DO[] = { // Algeria const PrepopulatedEngine* const engines_DZ[] = { &google, - &yahoo, + &yahoo_uk, &bing, &yandex_ru, &duckduckgo, @@ -275,8 +274,8 @@ const PrepopulatedEngine* const engines_EC[] = { &google, &bing, &yahoo, - &ecosia, &duckduckgo, + &ecosia, }; // Estonia @@ -293,7 +292,7 @@ const PrepopulatedEngine* const engines_EG[] = { &google, &yahoo, &bing, - &yandex_ru, + &yandex_com, &duckduckgo, }; @@ -348,7 +347,7 @@ const PrepopulatedEngine* const engines_GR[] = { &bing, &yahoo, &duckduckgo, - &yandex_ru, + &yandex_com, }; // Guatemala @@ -356,8 +355,8 @@ const PrepopulatedEngine* const engines_GT[] = { &google, &bing, &yahoo, - &ecosia, &duckduckgo, + &ecosia, }; // Hong Kong @@ -365,8 +364,8 @@ const PrepopulatedEngine* const engines_HK[] = { &google, &yahoo_hk, &bing, - &yandex_com, &baidu, + &duckduckgo, }; // Honduras @@ -374,8 +373,8 @@ const PrepopulatedEngine* const engines_HN[] = { &google, &bing, &yahoo, - &yandex_ru, &duckduckgo, + &ecosia, }; // Croatia @@ -384,7 +383,7 @@ const PrepopulatedEngine* const engines_HR[] = { &bing, &yahoo, &duckduckgo, - &ecosia, + &yandex_ru, }; // Hungary @@ -418,26 +417,26 @@ const PrepopulatedEngine* const engines_IE[] = { const PrepopulatedEngine* const engines_IL[] = { &google, &bing, - &yahoo, &yandex_ru, + &yahoo, &duckduckgo, }; // India const PrepopulatedEngine* const engines_IN[] = { &google, - &yahoo_in, &bing, + &yahoo_in, &duckduckgo, - &yandex_ru, + &ecosia, }; // Iraq const PrepopulatedEngine* const engines_IQ[] = { &google, - &yahoo, &bing, - &yandex_ru, + &yahoo_uk, + &yandex_com, &duckduckgo, }; @@ -446,16 +445,16 @@ const PrepopulatedEngine* const engines_IR[] = { &google, &bing, &yahoo, - &yandex_ru, &ask, + &naver, }; // Iceland const PrepopulatedEngine* const engines_IS[] = { &google, &bing, - &duckduckgo, &yahoo, + &duckduckgo, &ecosia, }; @@ -474,7 +473,7 @@ const PrepopulatedEngine* const engines_JM[] = { &bing, &yahoo, &duckduckgo, - &ask, + &ecosia, }; // Jordan @@ -501,7 +500,7 @@ const PrepopulatedEngine* const engines_KE[] = { &bing, &yahoo, &duckduckgo, - &yandex_ru, + &ecosia, }; // South Korea @@ -510,7 +509,7 @@ const PrepopulatedEngine* const engines_KR[] = { &naver, &bing, &daum, - &yahoo_jp, + &yahoo, }; // Kuwait @@ -537,7 +536,7 @@ const PrepopulatedEngine* const engines_LB[] = { &bing, &yahoo, &duckduckgo, - &ecosia, + &yandex_ru, }; // Liechtenstein @@ -570,8 +569,8 @@ const PrepopulatedEngine* const engines_LU[] = { // Latvia const PrepopulatedEngine* const engines_LV[] = { &google, - &yandex_ru, &bing, + &yandex_ru, &yahoo, &duckduckgo, }; @@ -579,8 +578,8 @@ const PrepopulatedEngine* const engines_LV[] = { // Libya const PrepopulatedEngine* const engines_LY[] = { &google, - &yahoo, &bing, + &yahoo, &yandex_com, &duckduckgo, }; @@ -588,18 +587,18 @@ const PrepopulatedEngine* const engines_LY[] = { // Morocco const PrepopulatedEngine* const engines_MA[] = { &google, - &yahoo, + &yahoo_fr, &bing, - &duckduckgo, &yandex_com, + &duckduckgo, }; // Monaco const PrepopulatedEngine* const engines_MC[] = { &google, &bing, - &duckduckgo, &yahoo, + &duckduckgo, &qwant, }; @@ -608,8 +607,8 @@ const PrepopulatedEngine* const engines_MD[] = { &google, &yandex_ru, &mail_ru, - &duckduckgo, &bing, + &yahoo, }; // Montenegro @@ -627,7 +626,7 @@ const PrepopulatedEngine* const engines_MK[] = { &bing, &yahoo, &duckduckgo, - &yandex_ru, + &baidu, }; // Mexico @@ -635,8 +634,8 @@ const PrepopulatedEngine* const engines_MX[] = { &google, &bing, &yahoo_mx, - &ecosia, &duckduckgo, + &ecosia, }; // Malaysia @@ -661,9 +660,9 @@ const PrepopulatedEngine* const engines_NI[] = { const PrepopulatedEngine* const engines_NL[] = { &google, &bing, - &yahoo_nl, &duckduckgo, - &yandex_ru, + &yahoo_nl, + &ecosia, }; // Norway @@ -672,15 +671,15 @@ const PrepopulatedEngine* const engines_NO[] = { &bing, &yahoo, &duckduckgo, - &yandex_ru, + &ecosia, }; // New Zealand const PrepopulatedEngine* const engines_NZ[] = { &google, &bing, - &duckduckgo, &yahoo_nz, + &duckduckgo, &ecosia, }; @@ -690,7 +689,7 @@ const PrepopulatedEngine* const engines_OM[] = { &bing, &yahoo, &duckduckgo, - &ecosia, + &ask, }; // Panama @@ -706,7 +705,7 @@ const PrepopulatedEngine* const engines_PA[] = { const PrepopulatedEngine* const engines_PE[] = { &google, &bing, - &yahoo_pe, + &yahoo_es, &ecosia, &duckduckgo, }; @@ -714,8 +713,8 @@ const PrepopulatedEngine* const engines_PE[] = { // Philippines const PrepopulatedEngine* const engines_PH[] = { &google, - &yahoo_ph, &bing, + &yahoo, &ecosia, &duckduckgo, }; @@ -735,7 +734,6 @@ const PrepopulatedEngine* const engines_PL[] = { &bing, &yahoo, &duckduckgo, - &yandex_ru, }; // Puerto Rico @@ -771,7 +769,7 @@ const PrepopulatedEngine* const engines_QA[] = { &bing, &yahoo, &duckduckgo, - &yandex_com, + &ecosia, }; // Romania @@ -786,8 +784,8 @@ const PrepopulatedEngine* const engines_RO[] = { // Serbia const PrepopulatedEngine* const engines_RS[] = { &google, - &yahoo, &bing, + &yahoo, &duckduckgo, &yandex_ru, }; @@ -798,7 +796,7 @@ const PrepopulatedEngine* const engines_RU[] = { &yandex_ru, &mail_ru, &bing, - &yahoo, + &duckduckgo, }; // Rwanda @@ -807,7 +805,7 @@ const PrepopulatedEngine* const engines_RW[] = { &bing, &yahoo, &duckduckgo, - &mail_ru, + &ecosia, }; // Saudi Arabia @@ -832,9 +830,9 @@ const PrepopulatedEngine* const engines_SE[] = { const PrepopulatedEngine* const engines_SG[] = { &google, &bing, - &yandex_com, - &yahoo_sg, + &yahoo, &baidu, + &duckduckgo, }; // Slovenia @@ -843,7 +841,6 @@ const PrepopulatedEngine* const engines_SI[] = { &bing, &duckduckgo, &yahoo, - &yandex_ru, }; // Slovakia @@ -876,8 +873,8 @@ const PrepopulatedEngine* const engines_SY[] = { // Thailand const PrepopulatedEngine* const engines_TH[] = { &google, - &yahoo_th, &bing, + &yahoo, &duckduckgo, &baidu, }; @@ -885,10 +882,10 @@ const PrepopulatedEngine* const engines_TH[] = { // Tunisia const PrepopulatedEngine* const engines_TN[] = { &google, - &yahoo, + &yahoo_fr, &bing, - &yandex_ru, &duckduckgo, + &yandex_ru, }; // Turkey @@ -906,7 +903,7 @@ const PrepopulatedEngine* const engines_TT[] = { &bing, &yahoo, &duckduckgo, - &ask, + &ecosia, }; // Taiwan @@ -924,7 +921,7 @@ const PrepopulatedEngine* const engines_TZ[] = { &bing, &yahoo, &duckduckgo, - &yandex_ru, + &ecosia, }; // Ukraine @@ -932,7 +929,7 @@ const PrepopulatedEngine* const engines_UA[] = { &google, &yandex_ua, &bing, - &mail_ru, + &duckduckgo, &yahoo, }; @@ -967,9 +964,9 @@ const PrepopulatedEngine* const engines_VE[] = { const PrepopulatedEngine* const engines_VN[] = { &google, &coccoc, - &yahoo, &bing, - &ecosia, + &yahoo, + &baidu, }; // Yemen @@ -987,7 +984,7 @@ const PrepopulatedEngine* const engines_ZA[] = { &bing, &yahoo, &duckduckgo, - &baidu, + &ecosia, }; // Zimbabwe @@ -995,8 +992,8 @@ const PrepopulatedEngine* const engines_ZW[] = { &google, &bing, &yahoo, - &ask, &duckduckgo, + &ask, }; // ---------------------------------------------------------------------------- diff --git a/chromium/components/search_engines/template_url_service.cc b/chromium/components/search_engines/template_url_service.cc index 65cbba22618..120df608bdc 100644 --- a/chromium/components/search_engines/template_url_service.cc +++ b/chromium/components/search_engines/template_url_service.cc @@ -7,11 +7,14 @@ #include #include "base/auto_reset.h" +#include "base/base64.h" +#include "base/base64url.h" #include "base/bind.h" #include "base/callback.h" #include "base/debug/crash_logging.h" #include "base/format_macros.h" #include "base/metrics/histogram_macros.h" +#include "base/rand_util.h" #include "base/stl_util.h" #include "base/strings/string_split.h" #include "base/strings/string_util.h" @@ -47,14 +50,26 @@ const char kDeleteSyncedEngineHistogramName[] = "Search.DeleteSyncedSearchEngine"; // Values for an enumerated histogram used to track whenever an ACTION_DELETE is -// sent to the server for search engines. +// sent to the server for search engines. These are persisted. Do not re-number. enum DeleteSyncedSearchEngineEvent { - DELETE_ENGINE_USER_ACTION, - DELETE_ENGINE_PRE_SYNC, - DELETE_ENGINE_EMPTY_FIELD, + DELETE_ENGINE_USER_ACTION = 0, + DELETE_ENGINE_PRE_SYNC = 1, + DELETE_ENGINE_EMPTY_FIELD = 2, DELETE_ENGINE_MAX, }; +const char kSearchTemplateURLEventsHistogramName[] = + "Search.TemplateURL.Events"; + +// Values for an enumerated histogram used to track TemplateURL edge cases. +// These are persisted. Do not re-number. +enum SearchTemplateURLEvent { + SYNC_DELETE_SUCCESS = 0, + SYNC_DELETE_FAIL_NONEXISTENT_ENGINE = 1, + SYNC_DELETE_FAIL_DEFAULT_SEARCH_PROVIDER = 2, + SEARCH_TEMPLATE_URL_EVENT_MAX, +}; + // Returns true iff the change in |change_list| at index |i| should not be sent // up to the server based on its GUIDs presence in |sync_data| or when compared // to changes after it in |change_list|. @@ -146,15 +161,6 @@ size_t GetRegistryLength(const base::string16& host) { net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES); } -// Returns the domain name (including registry) of a hostname. For example, -// www.google.co.uk will return google.co.uk. -base::string16 GetDomainAndRegistry(const base::string16& host) { - return base::UTF8ToUTF16( - net::registry_controlled_domains::GetDomainAndRegistry( - base::UTF16ToUTF8(host), - net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES)); -} - // For keywords that look like hostnames, returns whether KeywordProvider // should require users to type a prefix of the hostname to match against // them, rather than just the domain name portion. In other words, returns @@ -383,15 +389,6 @@ void TemplateURLService::AddMatchingKeywords( keyword_to_turl_and_length_, prefix, supports_replacement_only, matches); } -void TemplateURLService::AddMatchingDomainKeywords( - const base::string16& prefix, - bool supports_replacement_only, - TURLsAndMeaningfulLengths* matches) { - AddMatchingKeywordsHelper( - keyword_domain_to_turl_and_length_, prefix, supports_replacement_only, - matches); -} - TemplateURL* TemplateURLService::GetTemplateURLForKeyword( const base::string16& keyword) { return const_cast( @@ -401,9 +398,19 @@ TemplateURL* TemplateURLService::GetTemplateURLForKeyword( const TemplateURL* TemplateURLService::GetTemplateURLForKeyword( const base::string16& keyword) const { - auto elem(keyword_to_turl_and_length_.find(keyword)); - if (elem != keyword_to_turl_and_length_.end()) - return elem->second.first; + // Finds and returns the best match for |keyword|. + const auto match_range = keyword_to_turl_and_length_.equal_range(keyword); + if (match_range.first != match_range.second) { + // Among the matches for |keyword| in the multimap, return the best one. + return std::min_element( + match_range.first, match_range.second, + [](const auto& a, const auto& b) { + return a.second.first + ->IsBetterThanEngineWithConflictingKeyword(b.second.first); + }) + ->second.first; + } + return (!loaded_ && initial_default_search_provider_ && (initial_default_search_provider_->keyword() == keyword)) ? initial_default_search_provider_.get() @@ -1005,48 +1012,50 @@ base::Optional TemplateURLService::ProcessSyncChanges( continue; } - // Can't add an already-existing URL, or update/delete a non-existent one. - if ((iter->change_type() == syncer::SyncChange::ACTION_ADD) - ? !!existing_turl - : !existing_turl) { - error = sync_error_factory_->CreateAndUploadError(FROM_HERE, error_msg); - continue; - } - if (iter->change_type() == syncer::SyncChange::ACTION_DELETE) { - if (existing_turl == GetDefaultSearchProvider()) { - // The only way Sync can attempt to delete the default search provider - // is if we had changed the kSyncedDefaultSearchProviderGUID - // preference, but perhaps it has not yet been received. To avoid - // situations where this has come in erroneously, we will un-delete - // the current default search from the Sync data. If the pref really - // does arrive later, then default search will change to the correct - // entry, but we'll have this extra entry sitting around. The result is - // not ideal, but it prevents a far more severe bug where the default is - // unexpectedly swapped to something else. The user can safely delete - // the extra entry again later, if they choose. Most users who do not - // look at the search engines UI will not notice this. - // Note that we append a special character to the end of the keyword in - // an attempt to avoid a ping-poinging situation where receiving clients - // may try to continually delete the resurrected entry. - base::string16 updated_keyword = UniquifyKeyword(*existing_turl, true); - TemplateURLData data(existing_turl->data()); - data.SetKeyword(updated_keyword); - TemplateURL new_turl(data); - Update(existing_turl, new_turl); - - syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(new_turl); - new_changes.push_back(syncer::SyncChange(FROM_HERE, - syncer::SyncChange::ACTION_ADD, - sync_data)); - // Ignore the delete attempt. This means we never end up resetting the - // default search provider due to an ACTION_DELETE from sync. - } else { + if (!existing_turl) { + // Can't DELETE a non-existent engine, although we log it. + UMA_HISTOGRAM_ENUMERATION(kSearchTemplateURLEventsHistogramName, + SYNC_DELETE_FAIL_NONEXISTENT_ENGINE, + SEARCH_TEMPLATE_URL_EVENT_MAX); + error = sync_error_factory_->CreateAndUploadError(FROM_HERE, error_msg); + continue; + } + + // We can get an ACTION_DELETE for the default search provider if the user + // has changed the default search provider on a different machine, and we + // get the search engine update before the preference update. + // + // In this case, ignore the delete, because we never want to reset the + // default search provider as a result of ACTION_DELETE. If the preference + // update arrives later, we may be stuck with an extra search engine entry + // in this edge case, but it's better than most alternatives. + // + // In the past, we tried re-creating the deleted TemplateURL, but it was + // likely a source of duplicate search engine entries. crbug.com/1022775 + if (existing_turl != GetDefaultSearchProvider()) { Remove(existing_turl); + UMA_HISTOGRAM_ENUMERATION(kSearchTemplateURLEventsHistogramName, + SYNC_DELETE_SUCCESS, + SEARCH_TEMPLATE_URL_EVENT_MAX); + } else { + UMA_HISTOGRAM_ENUMERATION(kSearchTemplateURLEventsHistogramName, + SYNC_DELETE_FAIL_DEFAULT_SEARCH_PROVIDER, + SEARCH_TEMPLATE_URL_EVENT_MAX); } continue; } + if ((iter->change_type() == syncer::SyncChange::ACTION_ADD && + existing_turl) || + (iter->change_type() == syncer::SyncChange::ACTION_UPDATE && + !existing_turl)) { + // Can't ADD an already-existing engine, and can't UPDATE a non-existent + // engine. Early exit here to avoid ResolvingSyncKeywordConflict(). + error = sync_error_factory_->CreateAndUploadError(FROM_HERE, error_msg); + continue; + } + // Explicitly don't check for conflicts against extension keywords; in this // case the functions which modify the keyword map know how to handle the // conflicts. @@ -1081,7 +1090,6 @@ base::Optional TemplateURLService::ProcessSyncChanges( MaybeUpdateDSEViaPrefs(existing_turl); } - // If something went wrong, we want to prematurely exit to avoid pushing // inconsistent data to Sync. We return the last error we received. if (error.IsSet()) @@ -1246,6 +1254,27 @@ void TemplateURLService::ProcessTemplateURLChange( sync_processor_->ProcessSyncChanges(FROM_HERE, changes); } +std::string TemplateURLService::GetSessionToken() { + base::TimeTicks current_time(base::TimeTicks::Now()); + // Renew token if it expired. + if (current_time > token_expiration_time_) { + const size_t kTokenBytes = 12; + std::string raw_data; + base::RandBytes(base::WriteInto(&raw_data, kTokenBytes + 1), kTokenBytes); + base::Base64UrlEncode(raw_data, + base::Base64UrlEncodePolicy::INCLUDE_PADDING, + ¤t_token_); + } + + // Extend expiration time another 60 seconds. + token_expiration_time_ = current_time + base::TimeDelta::FromSeconds(60); + return current_token_; +} + +void TemplateURLService::ClearSessionToken() { + token_expiration_time_ = base::TimeTicks(); +} + // static syncer::SyncData TemplateURLService::CreateSyncDataFromTemplateURL( const TemplateURL& turl) { @@ -1440,56 +1469,17 @@ void TemplateURLService::Init(const Initializer* initializers, } } -TemplateURL* TemplateURLService::BestEngineForKeyword(TemplateURL* engine1, - TemplateURL* engine2) { - CHECK(engine1); - CHECK(engine2); - CHECK_EQ(engine1->keyword(), engine2->keyword()); - - // We should only have overlapping keywords when at least one comes from - // an extension. - CHECK(IsCreatedByExtension(engine1) || IsCreatedByExtension(engine2)); - - if (engine2->type() == engine1->type()) { - return engine1->extension_info_->install_time > - engine2->extension_info_->install_time - ? engine1 - : engine2; - } - if (engine2->type() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION) { - return engine1->type() == TemplateURL::OMNIBOX_API_EXTENSION ? engine1 - : engine2; - } - return engine2->type() == TemplateURL::OMNIBOX_API_EXTENSION ? engine2 - : engine1; -} - void TemplateURLService::RemoveFromMaps(const TemplateURL* template_url) { const base::string16& keyword = template_url->keyword(); - auto iter = keyword_to_turl_and_length_.find(keyword); - CHECK(iter != keyword_to_turl_and_length_.end()); - // The entry at |iter| may not be |template_url| if it's an extension-created - // entry with the same keyword. - if (iter->second.first == template_url) { - // We need to check whether the keyword can now be provided by another - // TemplateURL. See the comments for BestEngineForKeyword() for more - // information on extension keywords and how they can coexist with - // non-extension keywords. - TemplateURL* best_fallback = nullptr; - for (const auto& turl : template_urls_) { - if ((turl.get() != template_url) && (turl->keyword() == keyword)) { - if (best_fallback) - best_fallback = BestEngineForKeyword(best_fallback, turl.get()); - else - best_fallback = turl.get(); - } - } - RemoveFromDomainMap(template_url); - if (best_fallback) { - AddToMap(best_fallback); - AddToDomainMap(best_fallback); + + // Remove from |keyword_to_turl_and_length_|. No need to find the best + // fallback. We choose the best one as-needed from the multimap. + const auto match_range = keyword_to_turl_and_length_.equal_range(keyword); + for (auto it = match_range.first; it != match_range.second;) { + if (it->second.first == template_url) { + it = keyword_to_turl_and_length_.erase(it); } else { - keyword_to_turl_and_length_.erase(iter); + ++it; } } @@ -1505,25 +1495,13 @@ void TemplateURLService::RemoveFromMaps(const TemplateURL* template_url) { } void TemplateURLService::AddToMaps(TemplateURL* template_url) { - bool template_url_is_omnibox_api = - template_url->type() == TemplateURL::OMNIBOX_API_EXTENSION; const base::string16& keyword = template_url->keyword(); - KeywordToTURLAndMeaningfulLength::const_iterator i = - keyword_to_turl_and_length_.find(keyword); - if (i == keyword_to_turl_and_length_.end()) { - AddToMap(template_url); - AddToDomainMap(template_url); - } else { - TemplateURL* existing_url = i->second.first; - CHECK_NE(existing_url, template_url); - if (BestEngineForKeyword(existing_url, template_url) != existing_url) { - RemoveFromDomainMap(existing_url); - AddToMap(template_url); - AddToDomainMap(template_url); - } - } + keyword_to_turl_and_length_.insert(std::make_pair( + keyword, + TURLAndMeaningfulLength( + template_url, GetMeaningfulKeywordLength(keyword, template_url)))); - if (template_url_is_omnibox_api) + if (template_url->type() == TemplateURL::OMNIBOX_API_EXTENSION) return; if (!template_url->sync_guid().empty()) @@ -1533,40 +1511,6 @@ void TemplateURLService::AddToMaps(TemplateURL* template_url) { provider_map_->Add(template_url, search_terms_data()); } -void TemplateURLService::RemoveFromDomainMap(const TemplateURL* template_url) { - const base::string16 domain = GetDomainAndRegistry(template_url->keyword()); - if (domain.empty()) - return; - - const auto match_range( - keyword_domain_to_turl_and_length_.equal_range(domain)); - for (auto it(match_range.first); it != match_range.second; ) { - if (it->second.first == template_url) - it = keyword_domain_to_turl_and_length_.erase(it); - else - ++it; - } -} - -void TemplateURLService::AddToDomainMap(TemplateURL* template_url) { - const base::string16 domain = GetDomainAndRegistry(template_url->keyword()); - // Only bother adding an entry to the domain map if its key in the domain - // map would be different from the key in the regular map. - if (domain != template_url->keyword()) { - keyword_domain_to_turl_and_length_.insert(std::make_pair( - domain, - TURLAndMeaningfulLength( - template_url, GetMeaningfulKeywordLength(domain, template_url)))); - } -} - -void TemplateURLService::AddToMap(TemplateURL* template_url) { - const base::string16& keyword = template_url->keyword(); - keyword_to_turl_and_length_[keyword] = - TURLAndMeaningfulLength( - template_url, GetMeaningfulKeywordLength(keyword, template_url)); -} - void TemplateURLService::SetTemplateURLs( std::unique_ptr urls) { Scoper scoper(this); @@ -1653,29 +1597,28 @@ bool TemplateURLService::Update(TemplateURL* existing_turl, Scoper scoper(this); model_mutated_notification_pending_ = true; - base::string16 old_keyword = existing_turl->keyword(); TemplateURLID previous_id = existing_turl->id(); RemoveFromMaps(existing_turl); - // Check if new keyword conflicts with another normal engine. + // Check for new keyword conflicts with another normal engine. // This is possible when autogeneration of the keyword for a Google default // search provider at load time causes it to conflict with an existing - // keyword. In this case we delete the existing keyword if it's replaceable, - // or else undo the change in keyword for |existing_turl|. - // Conflicts with extension engines are handled in AddToMaps/RemoveFromMaps - // functions. - // Search for conflicting keyword turl before updating values of - // existing_turl. - const TemplateURL* conflicting_keyword_turl = - FindNonExtensionTemplateURLForKeyword(new_values.keyword()); - - bool keep_old_keyword = false; - if (conflicting_keyword_turl && conflicting_keyword_turl != existing_turl) { - if (CanReplace(conflicting_keyword_turl)) - Remove(conflicting_keyword_turl); - else - keep_old_keyword = true; + // keyword. If the conflicting engines are replaceable, we delete them. + // If they're not replaceable, we leave them alone, and trust AddToMaps() to + // choose the best engine to assign the keyword. + std::vector turls_to_remove; + for (const auto& turl : template_urls_) { + // TODO(tommycli): Investigate also replacing TemplateURL::LOCAL engines. + if (turl.get() != existing_turl && (turl->type() == TemplateURL::NORMAL) && + (turl->keyword() == new_values.keyword()) && CanReplace(turl.get())) { + // Remove() invalidates iterators. + turls_to_remove.push_back(turl.get()); + } + } + for (TemplateURL* turl : turls_to_remove) { + Remove(turl); } + // Update existing turl with new values. This must happen after calling // Remove(conflicting_keyword_turl) above, since otherwise during that // function RemoveFromMaps() may find |existing_turl| as an alternate engine @@ -1685,10 +1628,6 @@ bool TemplateURLService::Update(TemplateURL* existing_turl, // calling AddToMaps() below). existing_turl->CopyFrom(new_values); existing_turl->data_.id = previous_id; - if (keep_old_keyword) { - CHECK_NE(old_keyword, new_values.keyword()); - existing_turl->data_.SetKeyword(old_keyword); - } AddToMaps(existing_turl); @@ -1706,7 +1645,6 @@ bool TemplateURLService::Update(TemplateURL* existing_turl, if (default_search_provider_source_ != DefaultSearchManager::FROM_FALLBACK) MaybeUpdateDSEViaPrefs(existing_turl); - CHECK(!HasDuplicateKeywords()); return true; } @@ -1984,7 +1922,6 @@ TemplateURL* TemplateURLService::Add(std::unique_ptr template_url, if (template_url_ptr) model_mutated_notification_pending_ = true; - CHECK(!HasDuplicateKeywords()); return template_url_ptr; } @@ -2337,18 +2274,3 @@ TemplateURL* TemplateURLService::FindMatchingDefaultExtensionTemplateURL( } return nullptr; } - -bool TemplateURLService::HasDuplicateKeywords() const { - std::map keyword_to_template_url; - for (const auto& template_url : template_urls_) { - // Validate no duplicate normal engines with same keyword. - if (!IsCreatedByExtension(template_url.get())) { - if (keyword_to_template_url.find(template_url->keyword()) != - keyword_to_template_url.end()) { - return true; - } - keyword_to_template_url[template_url->keyword()] = template_url.get(); - } - } - return false; -} diff --git a/chromium/components/search_engines/template_url_service.h b/chromium/components/search_engines/template_url_service.h index e266c7a27d6..7f0210819e2 100644 --- a/chromium/components/search_engines/template_url_service.h +++ b/chromium/components/search_engines/template_url_service.h @@ -144,22 +144,12 @@ class TemplateURLService : public WebDataServiceConsumer, // Adds to |matches| all TemplateURLs whose keywords begin with |prefix|, // sorted shortest-keyword-first. If |supports_replacement_only| is true, only - // TemplateURLs that support replacement are returned. + // TemplateURLs that support replacement are returned. This method must be + // efficient, since it's run roughly once per omnibox keystroke. void AddMatchingKeywords(const base::string16& prefix, bool supports_replacement_only, TURLsAndMeaningfulLengths* matches); - // Adds to |matches| all TemplateURLs for search engines with the domain - // name part of the keyword starts with |prefix|, sorted - // shortest-domain-name-first. If |supports_replacement_only| is true, only - // TemplateURLs that support replacement are returned. Does not bother - // searching/returning keywords that would've been found with an identical - // call to FindMatchingKeywords(); i.e., doesn't search keywords for which - // the domain name is the keyword. - void AddMatchingDomainKeywords(const base::string16& prefix, - bool supports_replacement_only, - TURLsAndMeaningfulLengths* matches); - // Looks up |keyword| and returns the element it maps to. Returns NULL if // the keyword was not found. // The caller should not try to delete the returned pointer; the data store @@ -400,6 +390,13 @@ class TemplateURLService : public WebDataServiceConsumer, return *search_terms_data_; } + // Obtains a session token, regenerating if necessary. + std::string GetSessionToken(); + + // Clears the session token. Should be called when the user clears browsing + // data. + void ClearSessionToken(); + // Returns a SyncData with a sync representation of the search engine data // from |turl|. static syncer::SyncData CreateSyncDataFromTemplateURL( @@ -455,6 +452,7 @@ class TemplateURLService : public WebDataServiceConsumer, FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest, PreSyncDeletes); FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest, MergeInSyncTemplateURL); FRIEND_TEST_ALL_PREFIXES(LocationBarModelTest, GoogleBaseURL); + FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceUnitTest, SessionToken); friend class InstantUnitTestBase; friend class Scoper; @@ -464,20 +462,14 @@ class TemplateURLService : public WebDataServiceConsumer, using GUIDToTURL = std::map; // A mapping from keywords to the corresponding TemplateURLs and their - // meaningful keyword lengths. A keyword can appear only once here because - // there can be only one active TemplateURL associated with a given keyword. + // meaningful keyword lengths. This is a multimap, so the system can + // efficiently tolerate multiple engines with the same keyword, like from + // extensions. The values are not sorted from best to worst for each keyword, + // since multimaps don't sort on value. Users that want the best value for + // each key must traverse through all matching items, but we expect there to + // be below three values per key. using KeywordToTURLAndMeaningfulLength = - std::map; - - // A mapping from domain names to corresponding TemplateURLs and their - // meaningful keyword lengths. Specifically, for a keyword that is a - // hostname containing more than just a domain name, e.g., 'abc.def.com', - // the keyword is added to this map under the domain key 'def.com'. This - // means multiple keywords from the same domain share the same key, so this - // must be a multimap. - using KeywordDomainToTURLAndMeaningfulLength = std::multimap; - // Declaration of values to be used in an enumerated histogram to tally // changes to the default search provider from various entry points. In // particular, we use this to see what proportion of changes are from Sync @@ -517,36 +509,17 @@ class TemplateURLService : public WebDataServiceConsumer, void Init(const Initializer* initializers, int num_initializers); - // Given two engines with the same keyword, returns which should take - // precedence. While normal engines must all have distinct keywords, - // extension-controlled and omnibox API engines may have the same keywords as - // each other or as normal engines. In these cases, omnibox API engines - // override extension-controlled engines, which override normal engines; if - // there is still a conflict after this, the most recently-added extension - // wins. - TemplateURL* BestEngineForKeyword(TemplateURL* engine1, TemplateURL* engine2); - // Removes |template_url| from various internal maps - // (|keyword_to_turl_and_length_|, |keyword_domain_to_turl_and_length_|, - // |guid_to_turl_|, |provider_map_|). + // (|keyword_to_turl_and_length_|, |guid_to_turl_|, |provider_map_|). void RemoveFromMaps(const TemplateURL* template_url); // Adds |template_url| to various internal maps - // (|keyword_to_turl_and_length_|, |keyword_domain_to_turl_and_length_|, - // |guid_to_turl_|, |provider_map_|) if appropriate. (It might not be - // appropriate if, for instance, |template_url|'s keyword conflicts with - // the keyword of a custom search engine already existing in the maps that - // is not allowed to be replaced.) + // (|keyword_to_turl_and_length_|, |guid_to_turl_|, |provider_map_|) if + // appropriate. (It might not be appropriate if, for instance, + // |template_url|'s keyword conflicts with the keyword of a custom search + // engine already existing in the maps that is not allowed to be replaced.) void AddToMaps(TemplateURL* template_url); - // Helper function for removing an element from - // |keyword_domain_to_turl_and_length_|. - void RemoveFromDomainMap(const TemplateURL* template_url); - - // Helper fuction for adding an element to - // |keyword_domain_to_turl_and_length_| if appropriate. - void AddToDomainMap(TemplateURL* template_url); - // Helper function for adding an element to |keyword_to_turl_and_length_|. void AddToMap(TemplateURL* template_url); @@ -561,7 +534,6 @@ class TemplateURLService : public WebDataServiceConsumer, void ApplyDefaultSearchChange(const TemplateURLData* new_dse_data, DefaultSearchManager::Source source); - // Applies a DSE change. May be called at startup or after transitioning to // the loaded state. Returns true if a change actually occurred. bool ApplyDefaultSearchChangeNoMetrics(const TemplateURLData* new_dse_data, @@ -658,6 +630,10 @@ class TemplateURLService : public WebDataServiceConsumer, // * |local_turl| is created by policy. // * |prefer_local_default| is true and |local_turl| is the local default // search provider + // + // TODO(tommycli): Consolidate into using + // TemplateURL::IsBetterThanEngineWithConflictingKeyword. Likely we will + // eliminate the |prefer_local_default| mechanism. bool IsLocalTemplateURLBetter(const TemplateURL* local_turl, const TemplateURL* sync_turl, bool prefer_local_default = true) const; @@ -723,11 +699,6 @@ class TemplateURLService : public WebDataServiceConsumer, TemplateURL* FindMatchingDefaultExtensionTemplateURL( const TemplateURLData& data); - // Returns whether |template_urls_| contains more than one normal engine with - // same keyword. Used to validate state after search engines are - // added/updated. - bool HasDuplicateKeywords() const; - // ---------- Browser state related members --------------------------------- PrefService* prefs_ = nullptr; @@ -748,17 +719,6 @@ class TemplateURLService : public WebDataServiceConsumer, // Mapping from keyword to the TemplateURL. KeywordToTURLAndMeaningfulLength keyword_to_turl_and_length_; - // Mapping from keyword domain to the TemplateURL. - // Entries are only allowed here if there is a corresponding entry in - // |keyword_to_turl_and_length_|, i.e., if a template URL doesn't have an - // entry in |keyword_to_turl_and_length_| because it's subsumed by another - // template URL with an identical keyword, the template URL will not have an - // entry in this map either. This map will also not bother including entries - // for keywords in which the keyword is the domain name, with no subdomain - // before the domain name. (The ordinary |keyword_to_turl_and_length| - // suffices for that.) - KeywordDomainToTURLAndMeaningfulLength keyword_domain_to_turl_and_length_; - // Mapping from Sync GUIDs to the TemplateURL. GUIDToTURL guid_to_turl_; @@ -857,6 +817,10 @@ class TemplateURLService : public WebDataServiceConsumer, // but if no model mutation occurs, the deferred notification can be skipped. bool model_mutated_notification_pending_ = false; + // Session token management. + std::string current_token_; + base::TimeTicks token_expiration_time_; + #if defined(OS_ANDROID) // Manage and fetch the java object that wraps this TemplateURLService on // android. diff --git a/chromium/components/search_engines/template_url_service_unittest.cc b/chromium/components/search_engines/template_url_service_unittest.cc new file mode 100644 index 00000000000..c8bfe27d6e2 --- /dev/null +++ b/chromium/components/search_engines/template_url_service_unittest.cc @@ -0,0 +1,60 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#include + +#include "components/search_engines/template_url_service.h" +#include "testing/gtest/include/gtest/gtest.h" + +class TemplateURLServiceUnitTest : public testing::Test { + public: + TemplateURLServiceUnitTest() + : template_url_service_(/*initializers=*/nullptr, /*count=*/0) {} + TemplateURLService& template_url_service() { return template_url_service_; } + + private: + TemplateURLService template_url_service_; +}; + +TEST_F(TemplateURLServiceUnitTest, SessionToken) { + // Subsequent calls always get the same token. + std::string token = template_url_service().GetSessionToken(); + std::string token2 = template_url_service().GetSessionToken(); + EXPECT_EQ(token, token2); + EXPECT_FALSE(token.empty()); + + // Calls do not regenerate a token. + template_url_service().current_token_ = "PRE-EXISTING TOKEN"; + token = template_url_service().GetSessionToken(); + EXPECT_EQ(token, "PRE-EXISTING TOKEN"); + + // ... unless the token has expired. + template_url_service().current_token_.clear(); + const base::TimeDelta kSmallDelta = base::TimeDelta::FromMilliseconds(1); + template_url_service().token_expiration_time_ = + base::TimeTicks::Now() - kSmallDelta; + token = template_url_service().GetSessionToken(); + EXPECT_FALSE(token.empty()); + EXPECT_EQ(token, template_url_service().current_token_); + + // ... or cleared. + template_url_service().current_token_.clear(); + template_url_service().ClearSessionToken(); + token = template_url_service().GetSessionToken(); + EXPECT_FALSE(token.empty()); + EXPECT_EQ(token, template_url_service().current_token_); + + // The expiration time is always updated. + template_url_service().GetSessionToken(); + base::TimeTicks expiration_time_1 = + template_url_service().token_expiration_time_; + base::PlatformThread::Sleep(kSmallDelta); + template_url_service().GetSessionToken(); + base::TimeTicks expiration_time_2 = + template_url_service().token_expiration_time_; + EXPECT_GT(expiration_time_2, expiration_time_1); + EXPECT_GE(expiration_time_2, expiration_time_1 + kSmallDelta); +} diff --git a/chromium/components/search_engines/template_url_unittest.cc b/chromium/components/search_engines/template_url_unittest.cc index 78962a03bee..ef435ac629a 100644 --- a/chromium/components/search_engines/template_url_unittest.cc +++ b/chromium/components/search_engines/template_url_unittest.cc @@ -891,7 +891,7 @@ TEST_F(TemplateURLTest, RLZFromAppList) { EXPECT_TRUE(url.url_ref().IsValid(search_terms_data_)); ASSERT_TRUE(url.url_ref().SupportsReplacement(search_terms_data_)); TemplateURLRef::SearchTermsArgs args(ASCIIToUTF16("x")); - args.from_app_list = true; + args.request_source = TemplateURLRef::CROS_APP_LIST; GURL result(url.url_ref().ReplaceSearchTerms(args, search_terms_data_)); ASSERT_TRUE(result.is_valid()); EXPECT_EQ("http://bar/?rlz=" + base::UTF16ToUTF8(rlz_string) + "&x", @@ -1126,6 +1126,42 @@ TEST_F(TemplateURLTest, SearchClient) { EXPECT_EQ("http://google.com/?foobar&client=search_client&", result_2.spec()); } +TEST_F(TemplateURLTest, SuggestClient) { + const std::string base_url_str("http://google.com/?"); + const std::string query_params_str("client={google:suggestClient}"); + const std::string full_url_str = base_url_str + query_params_str; + search_terms_data_.set_google_base_url(base_url_str); + + TemplateURLData data; + data.SetURL(full_url_str); + TemplateURL url(data); + EXPECT_TRUE(url.url_ref().IsValid(search_terms_data_)); + ASSERT_FALSE(url.url_ref().SupportsReplacement(search_terms_data_)); + TemplateURLRef::SearchTermsArgs search_terms_args; + + // Check that the URL is correct when a client is not present. + GURL result( + url.url_ref().ReplaceSearchTerms(search_terms_args, search_terms_data_)); + ASSERT_TRUE(result.is_valid()); + EXPECT_EQ("http://google.com/?client=", result.spec()); + + // Check that the URL is correct when a client is present. + search_terms_data_.set_suggest_client("suggest_client"); + GURL result_2( + url.url_ref().ReplaceSearchTerms(search_terms_args, search_terms_data_)); + ASSERT_TRUE(result_2.is_valid()); + EXPECT_EQ("http://google.com/?client=suggest_client", result_2.spec()); + + // Check that the URL is correct when a suggest request is made from a + // non-searchbox NTP surface. + search_terms_args.request_source = TemplateURLRef::NON_SEARCHBOX_NTP; + GURL result_3( + url.url_ref().ReplaceSearchTerms(search_terms_args, search_terms_data_)); + ASSERT_TRUE(result_3.is_valid()); + EXPECT_EQ("http://google.com/?client=suggest_client_from_ntp", + result_3.spec()); +} + TEST_F(TemplateURLTest, GetURLNoSuggestionsURL) { TemplateURLData data; data.SetURL("http://google.com/?q={searchTerms}"); @@ -1743,7 +1779,7 @@ TEST_F(TemplateURLTest, ContextualSearchParameters) { // event. TemplateURLRef::SearchTermsArgs::ContextualSearchParams params( 2, 1, std::string(), 0, 0, false, std::string(), std::string(), - std::string()); + std::string(), std::string()); search_terms_args.contextual_search_params = params; result = url.url_ref().ReplaceSearchTerms(search_terms_args, search_terms_data_); @@ -1757,7 +1793,7 @@ TEST_F(TemplateURLTest, ContextualSearchParameters) { search_terms_args.contextual_search_params = TemplateURLRef::SearchTermsArgs::ContextualSearchParams( 2, 2, "CH", 1657713458, 5, false, std::string(), std::string(), - std::string()); + std::string(), std::string()); result = url.url_ref().ReplaceSearchTerms(search_terms_args, search_terms_data_); @@ -1774,7 +1810,7 @@ TEST_F(TemplateURLTest, ContextualSearchParameters) { search_terms_args.contextual_search_params = TemplateURLRef::SearchTermsArgs::ContextualSearchParams( 2, 1, std::string(), 0, 0, true, std::string(), std::string(), - std::string()); + std::string(), std::string()); result = url.url_ref().ReplaceSearchTerms(search_terms_args, search_terms_data_); // Find our param. @@ -1784,7 +1820,8 @@ TEST_F(TemplateURLTest, ContextualSearchParameters) { // Test source and target languages. search_terms_args.contextual_search_params = TemplateURLRef::SearchTermsArgs::ContextualSearchParams( - 2, 1, std::string(), 0, 0, true, "es", "de", std::string()); + 2, 1, std::string(), 0, 0, true, "es", "de", std::string(), + std::string()); result = url.url_ref().ReplaceSearchTerms(search_terms_args, search_terms_data_); // Find our params. @@ -1797,12 +1834,23 @@ TEST_F(TemplateURLTest, ContextualSearchParameters) { search_terms_args.contextual_search_params = TemplateURLRef::SearchTermsArgs::ContextualSearchParams( 2, 1, std::string(), 0, 0, true, std::string(), std::string(), - "es,de"); + "es,de", std::string()); result = url.url_ref().ReplaceSearchTerms(search_terms_args, search_terms_data_); // Find our param. These may actually be URL encoded. size_t fluent_pos = result.find("&ctxs_fls=es,de"); EXPECT_NE(fluent_pos, std::string::npos); + + // Test Related Searches. + search_terms_args.contextual_search_params = + TemplateURLRef::SearchTermsArgs::ContextualSearchParams( + 2, 1, std::string(), 0, 0, true, std::string(), std::string(), + std::string(), "1RbCu"); + result = + url.url_ref().ReplaceSearchTerms(search_terms_args, search_terms_data_); + // Find our param. + size_t ctxsl_rs_pos = result.find("&ctxsl_rs=1RbCu"); + EXPECT_NE(ctxsl_rs_pos, std::string::npos); } TEST_F(TemplateURLTest, GenerateKeyword) { diff --git a/chromium/components/search_engines/testing_search_terms_data.cc b/chromium/components/search_engines/testing_search_terms_data.cc index ac476a04974..dea429bae2f 100644 --- a/chromium/components/search_engines/testing_search_terms_data.cc +++ b/chromium/components/search_engines/testing_search_terms_data.cc @@ -27,6 +27,10 @@ std::string TestingSearchTermsData::GetSearchClient() const { return search_client_; } +std::string TestingSearchTermsData::GetSuggestClient(bool from_ntp) const { + return from_ntp ? suggest_client_ + "_from_ntp" : suggest_client_; +} + std::string TestingSearchTermsData::GoogleImageSearchSource() const { return "google_image_search_source"; } diff --git a/chromium/components/search_engines/testing_search_terms_data.h b/chromium/components/search_engines/testing_search_terms_data.h index 2e21f2e99e2..e7cc2be259d 100644 --- a/chromium/components/search_engines/testing_search_terms_data.h +++ b/chromium/components/search_engines/testing_search_terms_data.h @@ -16,6 +16,7 @@ class TestingSearchTermsData : public SearchTermsData { std::string GoogleBaseURLValue() const override; base::string16 GetRlzParameterValue(bool from_app_list) const override; std::string GetSearchClient() const override; + std::string GetSuggestClient(bool from_ntp) const override; std::string GoogleImageSearchSource() const override; // Estimates dynamic memory usage. @@ -28,10 +29,14 @@ class TestingSearchTermsData : public SearchTermsData { void set_search_client(const std::string& search_client) { search_client_ = search_client; } + void set_suggest_client(const std::string& suggest_client) { + suggest_client_ = suggest_client; + } private: std::string google_base_url_; std::string search_client_; + std::string suggest_client_; DISALLOW_COPY_AND_ASSIGN(TestingSearchTermsData); }; -- cgit v1.2.1