diff options
author | sreeram@chromium.org <sreeram@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-02-14 03:39:32 +0000 |
---|---|---|
committer | sreeram@chromium.org <sreeram@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-02-14 03:39:32 +0000 |
commit | 7c60f50438f48400252f330c313a99897b5b7ca9 (patch) | |
tree | 1ffacb67392a7fe10a8eb0a51ef620010463d16c /chrome/browser/ui | |
parent | d0a62f0d356c745c0d40e3014910984aeeff6ef4 (diff) | |
download | chromium_src-7c60f50438f48400252f330c313a99897b5b7ca9.zip chromium_src-7c60f50438f48400252f330c313a99897b5b7ca9.tar.gz chromium_src-7c60f50438f48400252f330c313a99897b5b7ca9.tar.bz2 |
Consolidate search terms extraction and Instant process determination.
Reverts changes made to template_url.* in http://crrev.com/178755.
BUG=170390
R=dhollowa@chromium.org,pkasting@chromium.org,sky@chromium.org
TEST=none; no visible changes.
Review URL: https://chromiumcodereview.appspot.com/12250033
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@182381 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser/ui')
-rw-r--r-- | chrome/browser/ui/browser_instant_controller.cc | 18 | ||||
-rw-r--r-- | chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm | 2 | ||||
-rw-r--r-- | chrome/browser/ui/search/search.cc | 343 | ||||
-rw-r--r-- | chrome/browser/ui/search/search.h | 89 | ||||
-rw-r--r-- | chrome/browser/ui/search/search_tab_helper.cc | 62 | ||||
-rw-r--r-- | chrome/browser/ui/search/search_tab_helper.h | 16 | ||||
-rw-r--r-- | chrome/browser/ui/search/search_unittest.cc | 50 | ||||
-rw-r--r-- | chrome/browser/ui/toolbar/toolbar_model_impl.cc | 59 | ||||
-rw-r--r-- | chrome/browser/ui/toolbar/toolbar_model_impl.h | 5 | ||||
-rw-r--r-- | chrome/browser/ui/toolbar/toolbar_model_unittest.cc | 11 |
10 files changed, 384 insertions, 271 deletions
diff --git a/chrome/browser/ui/browser_instant_controller.cc b/chrome/browser/ui/browser_instant_controller.cc index 4cdc6d4..9d92283 100644 --- a/chrome/browser/ui/browser_instant_controller.cc +++ b/chrome/browser/ui/browser_instant_controller.cc @@ -83,23 +83,21 @@ void BrowserInstantController::RegisterUserPrefs( registry->RegisterBooleanPref(prefs::kInstantEnabled, false, PrefRegistrySyncable::SYNCABLE_PREF); - search::InstantExtendedDefault instant_extended_default_setting = - search::GetInstantExtendedDefaultSetting(); - - bool instant_extended_value = true; - switch (instant_extended_default_setting) { - case search::INSTANT_FORCE_ON: + bool instant_extended_default = true; + switch (search::GetInstantExtendedDefaultSetting()) { + case search::INSTANT_DEFAULT_ON: + instant_extended_default = true; break; case search::INSTANT_USE_EXISTING: - instant_extended_value = prefs->GetBoolean(prefs::kInstantEnabled); + instant_extended_default = prefs->GetBoolean(prefs::kInstantEnabled); break; - case search::INSTANT_FORCE_OFF: - instant_extended_value = false; + case search::INSTANT_DEFAULT_OFF: + instant_extended_default = false; break; } registry->RegisterBooleanPref(prefs::kInstantExtendedEnabled, - instant_extended_value, + instant_extended_default, PrefRegistrySyncable::SYNCABLE_PREF); } diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm index f42ec26..b969241 100644 --- a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm +++ b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm @@ -142,8 +142,6 @@ TEST_F(OmniboxViewMacTest, TabToAutocomplete) { } TEST_F(OmniboxViewMacTest, SetInstantSuggestion) { - chrome::search::EnableInstantExtendedAPIForTesting(); - const NSRect frame = NSMakeRect(0, 0, 50, 30); scoped_nsobject<AutocompleteTextField> field( [[AutocompleteTextField alloc] initWithFrame:frame]); diff --git a/chrome/browser/ui/search/search.cc b/chrome/browser/ui/search/search.cc index a39700e..194b6e1 100644 --- a/chrome/browser/ui/search/search.cc +++ b/chrome/browser/ui/search/search.cc @@ -7,17 +7,17 @@ #include "base/command_line.h" #include "base/metrics/field_trial.h" #include "base/string_split.h" -#include "base/string_util.h" #include "base/strings/string_number_conversions.h" +#include "chrome/browser/instant/instant_service.h" +#include "chrome/browser/instant/instant_service_factory.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/search_engines/template_url_service.h" +#include "chrome/browser/search_engines/template_url_service_factory.h" #include "chrome/common/chrome_switches.h" -#include "chrome/common/chrome_version_info.h" +#include "chrome/common/url_constants.h" #include "content/public/browser/navigation_entry.h" - -#if !defined(OS_ANDROID) -#include "chrome/browser/themes/theme_service.h" -#include "chrome/browser/themes/theme_service_factory.h" -#endif +#include "content/public/browser/render_process_host.h" +#include "content/public/browser/web_contents.h" namespace { @@ -25,12 +25,9 @@ namespace { // InstantExtended field trials are named in such a way that we can parse out // the experiment configuration from the trial's group name in order to give // us maximum flexability in running experiments. -// Field trials should be named things like "Group7 espv:2 themes:0". +// Field trial groups should be named things like "Group7 espv:2 instant:1". // The first token is always GroupN for some integer N, followed by a // space-delimited list of key:value pairs which correspond to these flags: -const char kEnableOnThemesFlagName[] = "themes"; -const bool kEnableOnThemesDefault = false; - const char kEmbeddedPageVersionFlagName[] = "espv"; const int kEmbeddedPageVersionDefault = 1; @@ -46,153 +43,270 @@ const char kGroupNumberPrefix[] = "Group"; // be ignored and Instant Extended will not be enabled by default. const char kDisablingSuffix[] = "DISABLED"; +chrome::search::InstantExtendedDefault InstantExtendedDefaultFromInt64( + int64 default_value) { + switch (default_value) { + case 0: return chrome::search::INSTANT_DEFAULT_ON; + case 1: return chrome::search::INSTANT_USE_EXISTING; + case 2: return chrome::search::INSTANT_DEFAULT_OFF; + default: return chrome::search::INSTANT_USE_EXISTING; + } +} + +TemplateURL* GetDefaultSearchProviderTemplateURL(Profile* profile) { + TemplateURLService* template_url_service = + TemplateURLServiceFactory::GetForProfile(profile); + if (template_url_service) + return template_url_service->GetDefaultSearchProvider(); + return NULL; +} + +GURL TemplateURLRefToGURL(const TemplateURLRef& ref) { + return GURL( + ref.ReplaceSearchTerms(TemplateURLRef::SearchTermsArgs(string16()))); +} + +bool MatchesOriginAndPath(const GURL& my_url, const GURL& other_url) { + return my_url.host() == other_url.host() && + my_url.port() == other_url.port() && + my_url.path() == other_url.path() && + (my_url.scheme() == other_url.scheme() || + (my_url.SchemeIs(chrome::kHttpsScheme) && + other_url.SchemeIs(chrome::kHttpScheme))); +} + +bool IsCommandLineInstantURL(const GURL& url) { + const CommandLine* cl = CommandLine::ForCurrentProcess(); + GURL instant_url(cl->GetSwitchValueASCII(switches::kInstantURL)); + return instant_url.is_valid() && MatchesOriginAndPath(url, instant_url); +} + +// Coerces the commandline Instant URL to look like a template URL, so that we +// can extract search terms from it. +GURL CoerceCommandLineURLToTemplateURL(const GURL& instant_url, + const TemplateURLRef& ref) { + GURL search_url = TemplateURLRefToGURL(ref); + + GURL::Replacements replacements; + replacements.SetSchemeStr(chrome::kHttpsScheme); + replacements.SetHostStr(search_url.host()); + replacements.SetPortStr(search_url.port()); + replacements.SetPathStr(search_url.path()); + + return instant_url.ReplaceComponents(replacements); +} + +bool MatchesAnySearchURL(const GURL& url, TemplateURL* template_url) { + GURL search_url = TemplateURLRefToGURL(template_url->url_ref()); + if (search_url.is_valid() && MatchesOriginAndPath(url, search_url)) + return true; + + // "URLCount() - 1" because we already tested url_ref above. + for (size_t i = 0; i < template_url->URLCount() - 1; ++i) { + TemplateURLRef ref(template_url, i); + search_url = TemplateURLRefToGURL(ref); + if (search_url.is_valid() && MatchesOriginAndPath(url, search_url)) + return true; + } + + return false; +} + } // namespace namespace chrome { namespace search { -// static const char kInstantExtendedSearchTermsKey[] = "search_terms"; -InstantExtendedDefault InstantExtendedDefaultFromInt64(int64 default_value) { - switch (default_value) { - case 0: return INSTANT_FORCE_ON; - case 1: return INSTANT_USE_EXISTING; - case 2: return INSTANT_FORCE_OFF; - default: return INSTANT_USE_EXISTING; - } -} +const char kLocalOmniboxPopupURL[] = + "chrome://local-omnibox-popup/local-omnibox-popup.html"; InstantExtendedDefault GetInstantExtendedDefaultSetting() { - InstantExtendedDefault default_setting = INSTANT_USE_EXISTING; - FieldTrialFlags flags; if (GetFieldTrialInfo( base::FieldTrialList::FindFullName(kInstantExtendedFieldTrialName), &flags, NULL)) { uint64 trial_default = GetUInt64ValueForFlagWithDefault( - kInstantExtendedActivationName, - kInstantExtendedActivationDefault, - flags); - default_setting = InstantExtendedDefaultFromInt64(trial_default); + kInstantExtendedActivationName, + kInstantExtendedActivationDefault, + flags); + return InstantExtendedDefaultFromInt64(trial_default); } - return default_setting; + return INSTANT_USE_EXISTING; } -// Check whether or not the Extended API should be used on the given profile. -bool IsInstantExtendedAPIEnabled(Profile* profile) { +bool IsInstantExtendedAPIEnabled(const Profile* profile) { return EmbeddedSearchPageVersion(profile) != 0; } // Determine what embedded search page version to request from the user's -// default search provider. If 0, the embedded search UI should not be enabled. -// Note that the profile object here isn't const because we need to determine -// whether or not the user has a theme installed as part of this check, and -// that logic requires a non-const profile for whatever reason. -uint64 EmbeddedSearchPageVersion(Profile* profile) { - // Incognito windows do not currently use the embedded search API. +// default search provider. If 0, the embedded search UI should not be enabled. +uint64 EmbeddedSearchPageVersion(const Profile* profile) { if (!profile || profile->IsOffTheRecord()) return 0; - // Check Finch field trials. FieldTrialFlags flags; if (GetFieldTrialInfo( base::FieldTrialList::FindFullName(kInstantExtendedFieldTrialName), &flags, NULL)) { - uint64 espv = GetUInt64ValueForFlagWithDefault( - kEmbeddedPageVersionFlagName, - kEmbeddedPageVersionDefault, - flags); - - // Check for themes. - bool has_theme = false; -#if !defined(OS_ANDROID) - has_theme = - !ThemeServiceFactory::GetForProfile(profile)->UsingDefaultTheme(); -#endif - - bool enable_for_themes = - GetBoolValueForFlagWithDefault(kEnableOnThemesFlagName, - kEnableOnThemesDefault, - flags); - if (!has_theme || enable_for_themes) - return espv; + return GetUInt64ValueForFlagWithDefault(kEmbeddedPageVersionFlagName, + kEmbeddedPageVersionDefault, + flags); } - if (CommandLine::ForCurrentProcess()->HasSwitch( - switches::kEnableInstantExtendedAPI)) { + const CommandLine* cl = CommandLine::ForCurrentProcess(); + if (cl->HasSwitch(switches::kEnableInstantExtendedAPI)) { // The user has manually flipped the about:flags switch - give the default // UI version. return kEmbeddedPageVersionDefault; } - return 0; -} -void EnableInstantExtendedAPIForTesting() { - CommandLine::ForCurrentProcess()->AppendSwitch( - switches::kEnableInstantExtendedAPI); + return 0; } -bool IsQueryExtractionEnabled(Profile* profile) { +bool IsQueryExtractionEnabled(const Profile* profile) { #if defined(OS_IOS) - return CommandLine::ForCurrentProcess()->HasSwitch( - switches::kEnableQueryExtraction); + const CommandLine* cl = CommandLine::ForCurrentProcess(); + return cl->HasSwitch(switches::kEnableQueryExtraction); #else - if (!profile || profile->IsOffTheRecord()) - return false; - // On desktop, query extraction is controlled by the instant-extended-api // flag. - bool enabled = IsInstantExtendedAPIEnabled(profile); - - // Running with --enable-query-extraction but not - // --enable-instant-extended-api is an error. - DCHECK(!(CommandLine::ForCurrentProcess()->HasSwitch( - switches::kEnableQueryExtraction) && - !enabled)); - return enabled; + return IsInstantExtendedAPIEnabled(profile); #endif } +string16 GetSearchTermsFromNavigationEntry( + const content::NavigationEntry* entry) { + string16 search_terms; + if (entry) + entry->GetExtraData(kInstantExtendedSearchTermsKey, &search_terms); + return search_terms; +} + +string16 GetSearchTerms(const content::WebContents* contents) { + if (!contents) + return string16(); + + Profile* profile = Profile::FromBrowserContext(contents->GetBrowserContext()); + if (!IsQueryExtractionEnabled(profile)) + return string16(); + + // For security reasons, don't extract search terms if the page is not being + // rendered in the privileged Instant renderer process. This is to protect + // against a malicious page somehow scripting the search results page and + // faking search terms in the URL. Random pages can't get into the Instant + // renderer and scripting doesn't work cross-process, so if the page is in + // the Instant process, we know it isn't being exploited. + const content::RenderProcessHost* process_host = + contents->GetRenderProcessHost(); + if (!process_host) + return string16(); + + const InstantService* instant_service = + InstantServiceFactory::GetForProfile(profile); + if (!instant_service) + return string16(); + + if (!instant_service->IsInstantProcess(process_host->GetID())) + return string16(); + + // Check to see if search terms have already been extracted. + const content::NavigationEntry* entry = + contents->GetController().GetVisibleEntry(); + if (!entry) + return string16(); + + string16 search_terms = GetSearchTermsFromNavigationEntry(entry); + if (!search_terms.empty()) + return search_terms; + + // Otherwise, extract from the URL. + TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile); + if (!template_url) + return string16(); + + GURL url = entry->GetVirtualURL(); + + if (IsCommandLineInstantURL(url)) + url = CoerceCommandLineURLToTemplateURL(url, template_url->url_ref()); + + if (url.SchemeIsSecure() && template_url->HasSearchTermsReplacementKey(url)) + template_url->ExtractSearchTermsFromURL(url, &search_terms); + + return search_terms; +} + +bool ShouldAssignURLToInstantRenderer(const GURL& url, Profile* profile) { + TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile); + if (!template_url) + return false; + + GURL effective_url = url; + + if (IsCommandLineInstantURL(url)) { + const TemplateURLRef& instant_url_ref = template_url->instant_url_ref(); + effective_url = CoerceCommandLineURLToTemplateURL(url, instant_url_ref); + } + + return ShouldAssignURLToInstantRendererImpl( + effective_url, + IsInstantExtendedAPIEnabled(profile), + template_url); +} + +void EnableInstantExtendedAPIForTesting() { + CommandLine* cl = CommandLine::ForCurrentProcess(); + cl->AppendSwitch(switches::kEnableInstantExtendedAPI); +} + void EnableQueryExtractionForTesting() { #if defined(OS_IOS) - CommandLine::ForCurrentProcess()->AppendSwitch( - switches::kEnableQueryExtraction); + CommandLine* cl = CommandLine::ForCurrentProcess(); + cl->AppendSwitch(switches::kEnableQueryExtraction); #else - // On desktop, query extraction is controlled by the instant-extended-api - // flag. - CommandLine::ForCurrentProcess()->AppendSwitch( - switches::kEnableInstantExtendedAPI); + EnableInstantExtendedAPIForTesting(); #endif } -string16 GetSearchTermsFromNavigationEntry( - const content::NavigationEntry* entry) { - string16 search_terms; - entry->GetExtraData(kInstantExtendedSearchTermsKey, &search_terms); - return search_terms; -} +bool ShouldAssignURLToInstantRendererImpl(const GURL& url, + bool extended_api_enabled, + TemplateURL* template_url) { + if (!url.is_valid()) + return false; + + if (url.SchemeIs(chrome::kChromeSearchScheme)) + return true; -bool IsForcedInstantURL(const GURL& url) { - CommandLine* command_line = CommandLine::ForCurrentProcess(); - if (!command_line->HasSwitch(switches::kInstantURL)) + if (extended_api_enabled && url == GURL(kLocalOmniboxPopupURL)) + return true; + + if (extended_api_enabled && !url.SchemeIsSecure()) + return false; + + if (extended_api_enabled && !template_url->HasSearchTermsReplacementKey(url)) + return false; + + GURL instant_url = TemplateURLRefToGURL(template_url->instant_url_ref()); + if (!instant_url.is_valid()) return false; - GURL instant_url(command_line->GetSwitchValueASCII(switches::kInstantURL)); - return url.scheme() == instant_url.scheme() && - url.host() == instant_url.host() && - url.port() == instant_url.port() && - url.path() == instant_url.path(); + if (MatchesOriginAndPath(url, instant_url)) + return true; + + if (extended_api_enabled && MatchesAnySearchURL(url, template_url)) + return true; + + return false; } bool GetFieldTrialInfo(const std::string& group_name, FieldTrialFlags* flags, uint64* group_number) { if (EndsWith(group_name, kDisablingSuffix, true) || - !StartsWithASCII(group_name, kGroupNumberPrefix, true)) { + !StartsWithASCII(group_name, kGroupNumberPrefix, true)) return false; - } // We have a valid trial that starts with "Group" and isn't disabled. // First extract the flags. @@ -200,8 +314,7 @@ bool GetFieldTrialInfo(const std::string& group_name, size_t first_space = group_name.find(" "); if (first_space != std::string::npos) { - // There is a flags section of the group name. Split that out and parse - // it. + // There is a flags section of the group name. Split that out and parse it. group_prefix = group_name.substr(0, first_space); if (!base::SplitStringIntoKeyValuePairs(group_name.substr(first_space), ':', ' ', flags)) { @@ -213,11 +326,10 @@ bool GetFieldTrialInfo(const std::string& group_name, // Now extract the group number, making sure we get a non-zero value. uint64 temp_group_number = 0; - if (!base::StringToUint64(group_prefix.substr(strlen(kGroupNumberPrefix)), - &temp_group_number) || - temp_group_number == 0) { + std::string group_suffix = group_prefix.substr(strlen(kGroupNumberPrefix)); + if (!base::StringToUint64(group_suffix, &temp_group_number) || + temp_group_number == 0) return false; - } if (group_number) *group_number = temp_group_number; @@ -227,10 +339,9 @@ bool GetFieldTrialInfo(const std::string& group_name, // Given a FieldTrialFlags object, returns the string value of the provided // flag. -std::string GetStringValueForFlagWithDefault( - const std::string& flag, - const std::string& default_value, - FieldTrialFlags& flags) { +std::string GetStringValueForFlagWithDefault(const std::string& flag, + const std::string& default_value, + const FieldTrialFlags& flags) { FieldTrialFlags::const_iterator i; for (i = flags.begin(); i != flags.end(); i++) { if (i->first == flag) @@ -241,19 +352,21 @@ std::string GetStringValueForFlagWithDefault( // Given a FieldTrialFlags object, returns the uint64 value of the provided // flag. -uint64 GetUInt64ValueForFlagWithDefault( - const std::string& flag, uint64 default_value, FieldTrialFlags& flags) { +uint64 GetUInt64ValueForFlagWithDefault(const std::string& flag, + uint64 default_value, + const FieldTrialFlags& flags) { uint64 value; - if (!base::StringToUint64(GetStringValueForFlagWithDefault(flag, "", flags), - &value)) - return default_value; - return value; + std::string str_value = GetStringValueForFlagWithDefault(flag, "", flags); + if (base::StringToUint64(str_value, &value)) + return value; + return default_value; } // Given a FieldTrialFlags object, returns the boolean value of the provided // flag. -bool GetBoolValueForFlagWithDefault( - const std::string& flag, bool default_value, FieldTrialFlags& flags) { +bool GetBoolValueForFlagWithDefault(const std::string& flag, + bool default_value, + const FieldTrialFlags& flags) { return !!GetUInt64ValueForFlagWithDefault(flag, default_value ? 1 : 0, flags); } diff --git a/chrome/browser/ui/search/search.h b/chrome/browser/ui/search/search.h index 888e5df..9d0532b 100644 --- a/chrome/browser/ui/search/search.h +++ b/chrome/browser/ui/search/search.h @@ -14,57 +14,79 @@ class GURL; class Profile; +class TemplateURL; namespace content { class NavigationEntry; +class WebContents; } namespace chrome { namespace search { // The key used to store search terms data in the NavigationEntry to be later -// displayed in the Omnibox. With the context of the user's exact query, +// displayed in the omnibox. With the context of the user's exact query, // InstantController sets the correct search terms to be displayed. extern const char kInstantExtendedSearchTermsKey[]; +// The URL for the local omnibox popup (rendered in a WebContents). +extern const char kLocalOmniboxPopupURL[]; + +// The default value we should assign to the instant_extended.enabled pref. +// As with other prefs, the default is used only when the user hasn't toggled +// the pref explicitly. enum InstantExtendedDefault { - INSTANT_FORCE_ON, // Force the setting on if no other setting exists. - INSTANT_USE_EXISTING, // Use same the value of the old instant.enabled pref. - INSTANT_FORCE_OFF, // Force the setting off if no other setting exists. + INSTANT_DEFAULT_ON, // Default the pref to be enabled. + INSTANT_USE_EXISTING, // Use the current value of the instant.enabled pref. + INSTANT_DEFAULT_OFF, // Default the pref to be disabled. }; // Returns an enum value indicating which mode to set the new // instant_extended.enabled pref to by default. InstantExtendedDefault GetInstantExtendedDefaultSetting(); -// Returns whether the Instant extended API is enabled for the given |profile|. -// |profile| may not be NULL. -bool IsInstantExtendedAPIEnabled(Profile* profile); +// Returns whether the Instant Extended API is enabled in this profile. +bool IsInstantExtendedAPIEnabled(const Profile* profile); -// Returns the value to pass to the &espv cgi parameter when loading the -// embedded search page from the user's default search provider. Will be +// Returns the value to pass to the &espv CGI parameter when loading the +// embedded search page from the user's default search provider. Will be // 0 if the Instant Extended API is not enabled. -uint64 EmbeddedSearchPageVersion(Profile* profile); - -// Force the instant extended API to be enabled for tests. -void EnableInstantExtendedAPIForTesting(); +uint64 EmbeddedSearchPageVersion(const Profile* profile); -// Returns whether query extraction is enabled. If -// |IsInstantExtendedAPIEnabled()| and the profile is not off the record, then -// this method will also return true. -bool IsQueryExtractionEnabled(Profile* profile); +// Returns whether query extraction is enabled. +bool IsQueryExtractionEnabled(const Profile* profile); -// Force query extraction to be enabled for tests. -void EnableQueryExtractionForTesting(); - -// Return the search terms attached to a specific NavigationEntry, or empty -// string otherwise. +// Returns the search terms attached to a specific NavigationEntry, or empty +// string otherwise. Does not consider IsQueryExtractionEnabled(), so most +// callers should use GetSearchTerms() below instead. string16 GetSearchTermsFromNavigationEntry( const content::NavigationEntry* entry); -// Returns true if |url| has the same scheme, host, port and path as the -// Instant URL set via --instant-url. -bool IsForcedInstantURL(const GURL& url); +// Returns search terms if this WebContents is a search results page. It looks +// in the visible NavigationEntry first, to see if search terms have already +// been extracted. Failing that, it tries to extract search terms from the URL. +// Returns a blank string if search terms were not found, or if search terms +// extraction is disabled for this WebContents or profile. +string16 GetSearchTerms(const content::WebContents* contents); + +// Returns true if |url| should be rendered in the Instant renderer process. +bool ShouldAssignURLToInstantRenderer(const GURL& url, Profile* profile); + +// ----------------------------------------------------- +// The following APIs are exposed for use in tests only. +// ----------------------------------------------------- + +// Forces the Instant Extended API to be enabled for tests. +void EnableInstantExtendedAPIForTesting(); + +// Forces query extraction to be enabled for tests. +void EnableQueryExtractionForTesting(); + +// Actually implements the logic for ShouldAssignURLToInstantRenderer(). +// Exposed for testing only. +bool ShouldAssignURLToInstantRendererImpl(const GURL& url, + bool extended_api_enabled, + TemplateURL* template_url); // Type for a collection of experiment configuration parameters. typedef std::vector<std::pair<std::string, std::string> > FieldTrialFlags; @@ -83,21 +105,22 @@ bool GetFieldTrialInfo(const std::string& group_name, // Given a FieldTrialFlags object, returns the string value of the provided // flag. // Exposed for testing only. -std::string GetStringValueForFlagWithDefault( - const std::string& flag, - const std::string& default_value, - FieldTrialFlags& flags); +std::string GetStringValueForFlagWithDefault(const std::string& flag, + const std::string& default_value, + const FieldTrialFlags& flags); // Given a FieldTrialFlags object, returns the uint64 value of the provided // flag. // Exposed for testing only. -uint64 GetUInt64ValueForFlagWithDefault( - const std::string& flag, uint64 default_value, FieldTrialFlags& flags); +uint64 GetUInt64ValueForFlagWithDefault(const std::string& flag, + uint64 default_value, + const FieldTrialFlags& flags); // Given a FieldTrialFlags object, returns the bool value of the provided flag. // Exposed for testing only. -bool GetBoolValueForFlagWithDefault( - const std::string& flag, bool default_value, FieldTrialFlags& flags); +bool GetBoolValueForFlagWithDefault(const std::string& flag, + bool default_value, + const FieldTrialFlags& flags); } // namespace search } // namespace chrome diff --git a/chrome/browser/ui/search/search_tab_helper.cc b/chrome/browser/ui/search/search_tab_helper.cc index 186834fa..b320565 100644 --- a/chrome/browser/ui/search/search_tab_helper.cc +++ b/chrome/browser/ui/search/search_tab_helper.cc @@ -4,55 +4,32 @@ #include "chrome/browser/ui/search/search_tab_helper.h" -#include "chrome/browser/google/google_util.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/ui/search/search.h" #include "chrome/common/url_constants.h" -#include "content/public/browser/navigation_controller.h" -#include "content/public/browser/navigation_details.h" #include "content/public/browser/navigation_entry.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/notification_types.h" -#include "content/public/browser/web_contents.h" DEFINE_WEB_CONTENTS_USER_DATA_KEY(chrome::search::SearchTabHelper); namespace { -bool IsNTP(const GURL& url) { - return url.SchemeIs(chrome::kChromeUIScheme) && - url.host() == chrome::kChromeUINewTabHost; -} - -Profile* ProfileFromWebContents(const content::WebContents* web_contents) { - return Profile::FromBrowserContext(web_contents->GetBrowserContext()); -} - -bool IsSearchEnabled(Profile* profile) { +bool IsSearchEnabled(const content::WebContents* contents) { + Profile* profile = Profile::FromBrowserContext(contents->GetBrowserContext()); return chrome::search::IsInstantExtendedAPIEnabled(profile); } +bool IsNTP(const content::WebContents* contents) { + // We can't use WebContents::GetURL() because that uses the active entry, + // whereas we want the visible entry. + const content::NavigationEntry* entry = + contents->GetController().GetVisibleEntry(); + return entry && entry->GetVirtualURL() == GURL(chrome::kChromeUINewTabURL); +} -bool IsSearchResults(const GURL& url, Profile* profile) { - if (chrome::search::IsForcedInstantURL(url)) - return true; - - // Profile can be NULL in unit tests. - TemplateURLService* template_url_service = - TemplateURLServiceFactory::GetForProfile(profile); - if (!template_url_service) - return false; - - TemplateURL* template_url = template_url_service->GetDefaultSearchProvider(); - if (!template_url) - return false; - - string16 result; - return template_url->HasSearchTermsReplacementKey(url) && - template_url->ExtractSearchTermsFromURL(url, &result) && !result.empty(); +bool IsSearchResults(const content::WebContents* contents) { + return !chrome::search::GetSearchTerms(contents).empty(); } } // namespace @@ -61,7 +38,7 @@ namespace chrome { namespace search { SearchTabHelper::SearchTabHelper(content::WebContents* web_contents) - : is_search_enabled_(IsSearchEnabled(ProfileFromWebContents(web_contents))), + : is_search_enabled_(IsSearchEnabled(web_contents)), user_input_in_progress_(false), model_(web_contents) { if (!is_search_enabled_) @@ -86,13 +63,14 @@ void SearchTabHelper::OmniboxEditModelChanged(bool user_input_in_progress, if (!user_input_in_progress && !cancelling) return; - UpdateModelBasedOnURL(web_contents()->GetURL()); + UpdateModel(); } void SearchTabHelper::NavigationEntryUpdated() { if (!is_search_enabled_) return; - UpdateModelBasedOnURL(web_contents()->GetURL()); + + UpdateModel(); } void SearchTabHelper::Observe( @@ -100,18 +78,16 @@ void SearchTabHelper::Observe( const content::NotificationSource& source, const content::NotificationDetails& details) { DCHECK_EQ(content::NOTIFICATION_NAV_ENTRY_COMMITTED, type); - content::LoadCommittedDetails* committed_details = - content::Details<content::LoadCommittedDetails>(details).ptr(); - UpdateModelBasedOnURL(committed_details->entry->GetVirtualURL()); + UpdateModel(); } -void SearchTabHelper::UpdateModelBasedOnURL(const GURL& url) { +void SearchTabHelper::UpdateModel() { Mode::Type type = Mode::MODE_DEFAULT; Mode::Origin origin = Mode::ORIGIN_DEFAULT; - if (IsNTP(url)) { + if (IsNTP(web_contents())) { type = Mode::MODE_NTP; origin = Mode::ORIGIN_NTP; - } else if (IsSearchResults(url, ProfileFromWebContents(web_contents()))) { + } else if (IsSearchResults(web_contents())) { type = Mode::MODE_SEARCH_RESULTS; origin = Mode::ORIGIN_SEARCH; } diff --git a/chrome/browser/ui/search/search_tab_helper.h b/chrome/browser/ui/search/search_tab_helper.h index 114cc66..ceb27ab 100644 --- a/chrome/browser/ui/search/search_tab_helper.h +++ b/chrome/browser/ui/search/search_tab_helper.h @@ -6,13 +6,12 @@ #define CHROME_BROWSER_UI_SEARCH_SEARCH_TAB_HELPER_H_ #include "base/basictypes.h" +#include "base/compiler_specific.h" #include "chrome/browser/ui/search/search_model.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" #include "content/public/browser/web_contents_user_data.h" -class OmniboxEditModel; - namespace content { class WebContents; } @@ -41,17 +40,18 @@ class SearchTabHelper : public content::NotificationObserver, // the notification system and shouldn't call this method. void NavigationEntryUpdated(); + private: + friend class content::WebContentsUserData<SearchTabHelper>; + + explicit SearchTabHelper(content::WebContents* web_contents); + // Overridden from content::NotificationObserver: virtual void Observe(int type, const content::NotificationSource& source, const content::NotificationDetails& details) OVERRIDE; - private: - explicit SearchTabHelper(content::WebContents* web_contents); - friend class content::WebContentsUserData<SearchTabHelper>; - - // Sets the mode of the model based on |url|. - void UpdateModelBasedOnURL(const GURL& url); + // Sets the mode of the model based on the current URL of web_contents(). + void UpdateModel(); // Returns the web contents associated with the tab that owns this helper. const content::WebContents* web_contents() const; diff --git a/chrome/browser/ui/search/search_unittest.cc b/chrome/browser/ui/search/search_unittest.cc index c91203e..7646bb7 100644 --- a/chrome/browser/ui/search/search_unittest.cc +++ b/chrome/browser/ui/search/search_unittest.cc @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "chrome/browser/search_engines/template_url.h" #include "chrome/browser/ui/search/search.h" #include "testing/gtest/include/gtest/gtest.h" @@ -65,5 +66,54 @@ TEST(EmbeddedSearchFieldTrialTest, GetFieldTrialInfo) { EXPECT_EQ(ZERO, flags.size()); } +TEST(SearchTest, ShouldAssignURLToInstantRendererImpl) { + TemplateURLData data; + data.SetURL("http://foo.com/url?bar={searchTerms}"); + data.instant_url = "http://foo.com/instant"; + data.alternate_urls.push_back("http://foo.com/alt#quux={searchTerms}"); + data.search_terms_replacement_key = "strk"; + TemplateURL url(NULL, data); + + struct { + const char* const url; + bool extended_api_enabled; + bool expected_result; + } kTestCases[] = { + {"invalid URL", false, false}, + {"unknown://scheme/path", false, false}, + {"chrome-search://foo/bar", false, true}, + {kLocalOmniboxPopupURL, false, false}, + {kLocalOmniboxPopupURL, true, true}, + {"http://foo.com/instant", false, true}, + {"http://foo.com/instant?foo=bar", false, true}, + {"https://foo.com/instant", false, true}, + {"https://foo.com/instant#foo=bar", false, true}, + {"HtTpS://fOo.CoM/instant", false, true}, + {"http://foo.com:80/instant", false, true}, + {"ftp://foo.com/instant", false, false}, + {"http://sub.foo.com/instant", false, false}, + {"http://foo.com:26/instant", false, false}, + {"http://foo.com/instant/bar", false, false}, + {"http://foo.com/Instant", false, false}, + {"https://foo.com/instant?strk", true, true}, + {"https://foo.com/instant#strk", true, true}, + {"https://foo.com/instant?strk=0", true, true}, + {"http://foo.com/instant", true, false}, + {"https://foo.com/instant", true, false}, + {"http://foo.com/instant?strk=1", true, false}, + {"https://foo.com/url?strk", true, true}, + {"https://foo.com/alt?strk", true, true}, + }; + + for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kTestCases); ++i) { + EXPECT_EQ(kTestCases[i].expected_result, + ShouldAssignURLToInstantRendererImpl( + GURL(kTestCases[i].url), + kTestCases[i].extended_api_enabled, + &url)) << kTestCases[i].url << " " + << kTestCases[i].extended_api_enabled; + } +} + } // namespace search } // namespace chrome diff --git a/chrome/browser/ui/toolbar/toolbar_model_impl.cc b/chrome/browser/ui/toolbar/toolbar_model_impl.cc index 4934114..9daf849 100644 --- a/chrome/browser/ui/toolbar/toolbar_model_impl.cc +++ b/chrome/browser/ui/toolbar/toolbar_model_impl.cc @@ -8,11 +8,7 @@ #include "base/prefs/pref_service.h" #include "base/utf_string_conversions.h" #include "chrome/browser/autocomplete/autocomplete_input.h" -#include "chrome/browser/google/google_util.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/ssl/ssl_error_info.h" #include "chrome/browser/ui/search/search.h" #include "chrome/browser/ui/toolbar/toolbar_model_delegate.h" @@ -40,26 +36,6 @@ using content::NavigationEntry; using content::SSLStatus; using content::WebContents; -namespace { - -// Coerces an instant URL to look like a regular search URL so we can extract -// query terms from the URL. -GURL ConvertInstantURLToSearchURL(const GURL& instant_url, - const TemplateURL& template_url) { - GURL search_url(template_url.url_ref().ReplaceSearchTerms( - TemplateURLRef::SearchTermsArgs(string16()))); - const std::string& scheme = search_url.scheme(); - const std::string& host = search_url.host(); - const std::string& port = search_url.port(); - GURL::Replacements replacements; - replacements.SetSchemeStr(scheme); - replacements.SetHostStr(host); - replacements.SetPortStr(port); - return instant_url.ReplaceComponents(replacements); -} - -} // namespace - ToolbarModelImpl::ToolbarModelImpl(ToolbarModelDelegate* delegate) : delegate_(delegate), input_in_progress_(false) { @@ -72,7 +48,8 @@ ToolbarModelImpl::~ToolbarModelImpl() { string16 ToolbarModelImpl::GetText( bool display_search_urls_as_search_terms) const { if (display_search_urls_as_search_terms) { - string16 search_terms = TryToExtractSearchTermsFromURL(); + string16 search_terms = + chrome::search::GetSearchTerms(delegate_->GetActiveWebContents()); if (!search_terms.empty()) return search_terms; } @@ -104,7 +81,8 @@ GURL ToolbarModelImpl::GetURL() const { } bool ToolbarModelImpl::WouldReplaceSearchURLWithSearchTerms() const { - return !TryToExtractSearchTermsFromURL().empty(); + const content::WebContents* contents = delegate_->GetActiveWebContents(); + return !chrome::search::GetSearchTerms(contents).empty(); } bool ToolbarModelImpl::ShouldDisplayURL() const { @@ -236,35 +214,6 @@ NavigationController* ToolbarModelImpl::GetNavigationController() const { return current_tab ? ¤t_tab->GetController() : NULL; } -string16 ToolbarModelImpl::TryToExtractSearchTermsFromURL() const { - GURL url = GetURL(); - Profile* profile = GetProfile(); - - // Ensure query extraction is enabled and query URL is HTTPS. - if (!profile || !chrome::search::IsQueryExtractionEnabled(profile) || - !url.SchemeIs(chrome::kHttpsScheme)) - return string16(); - - TemplateURLService* template_url_service = - TemplateURLServiceFactory::GetForProfile(profile); - - TemplateURL* template_url = template_url_service->GetDefaultSearchProvider(); - if (!template_url) - return string16(); - - // Coerce URLs set via --instant-url to look like a regular search URL so we - // can extract search terms from them. - if (chrome::search::IsForcedInstantURL(url)) - url = ConvertInstantURLToSearchURL(url, *template_url); - - if (!template_url->HasSearchTermsReplacementKey(url)) - return string16(); - - string16 result; - template_url->ExtractSearchTermsFromURL(url, &result); - return result; -} - Profile* ToolbarModelImpl::GetProfile() const { NavigationController* navigation_controller = GetNavigationController(); return navigation_controller ? diff --git a/chrome/browser/ui/toolbar/toolbar_model_impl.h b/chrome/browser/ui/toolbar/toolbar_model_impl.h index 9da3b55..fda179e 100644 --- a/chrome/browser/ui/toolbar/toolbar_model_impl.h +++ b/chrome/browser/ui/toolbar/toolbar_model_impl.h @@ -53,11 +53,6 @@ class ToolbarModelImpl : public ToolbarModel { // If this returns NULL, default values are used. content::NavigationController* GetNavigationController() const; - // Attempt to extract search terms from the current url. Called by GetText if - // |display_search_urls_as_search_terms| is true and by - // WouldReplaceSearchURLWithSearchTerms. - string16 TryToExtractSearchTermsFromURL() const; - // Helper method to extract the profile from the navigation controller. Profile* GetProfile() const; diff --git a/chrome/browser/ui/toolbar/toolbar_model_unittest.cc b/chrome/browser/ui/toolbar/toolbar_model_unittest.cc index 926cb8c..7ba2409 100644 --- a/chrome/browser/ui/toolbar/toolbar_model_unittest.cc +++ b/chrome/browser/ui/toolbar/toolbar_model_unittest.cc @@ -6,6 +6,8 @@ #include "base/command_line.h" #include "base/utf_string_conversions.h" +#include "chrome/browser/instant/instant_service.h" +#include "chrome/browser/instant/instant_service_factory.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" @@ -15,6 +17,7 @@ #include "chrome/browser/ui/toolbar/toolbar_model.h" #include "chrome/common/chrome_switches.h" #include "chrome/test/base/browser_with_test_window_test.h" +#include "content/public/browser/render_process_host.h" #include "content/public/browser/web_contents.h" #include "content/public/common/url_constants.h" @@ -126,6 +129,7 @@ class ToolbarModelTest : public BrowserWithTestWindowTest { void ResetDefaultTemplateURL() { TemplateURLData data; data.SetURL("http://google.com/search?q={searchTerms}"); + data.instant_url = "http://does/not/exist"; data.search_terms_replacement_key = "{google:instantExtendedEnabledKey}"; TemplateURL* search_template_url = new TemplateURL(profile(), data); TemplateURLService* template_url_service = @@ -158,6 +162,13 @@ class ToolbarModelTest : public BrowserWithTestWindowTest { url, Referrer(), CURRENT_TAB, content::PAGE_TRANSITION_TYPED, false)); + // Query terms replacement requires that the renderer process be a + // recognized Instant renderer. Fake it. + InstantService* instant_service = + InstantServiceFactory::GetForProfile(profile()); + int process_id = contents->GetRenderProcessHost()->GetID(); + instant_service->AddInstantProcess(process_id); + ToolbarModel* toolbar_model = browser()->toolbar_model(); // Check while loading. |