diff options
24 files changed, 456 insertions, 547 deletions
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc index a3c33ea..7213015 100644 --- a/chrome/browser/chrome_content_browser_client.cc +++ b/chrome/browser/chrome_content_browser_client.cc @@ -63,8 +63,6 @@ #include "chrome/browser/renderer_host/chrome_render_view_host_observer.h" #include "chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.h" #include "chrome/browser/search_engines/search_provider_install_state_message_filter.h" -#include "chrome/browser/search_engines/template_url_service.h" -#include "chrome/browser/search_engines/template_url_service_factory.h" #include "chrome/browser/speech/chrome_speech_recognition_manager_delegate.h" #include "chrome/browser/spellchecker/spellcheck_message_filter.h" #include "chrome/browser/ssl/ssl_add_certificate.h" @@ -73,6 +71,7 @@ #include "chrome/browser/tab_contents/tab_util.h" #include "chrome/browser/toolkit_extra_parts.h" #include "chrome/browser/ui/chrome_select_file_policy.h" +#include "chrome/browser/ui/search/search.h" #include "chrome/browser/ui/tab_contents/chrome_web_contents_view_delegate.h" #include "chrome/browser/ui/webui/chrome_web_ui_controller_factory.h" #include "chrome/browser/user_style_sheet_watcher.h" @@ -407,66 +406,22 @@ int GetCrashSignalFD(const CommandLine& command_line) { } #endif // defined(OS_POSIX) && !defined(OS_MACOSX) -// TODO(dhollowa): http://crbug.com/170390 This test exists in different places -// in Chrome. Follow-up to consolidate these various Instant URL checks. -// Returns true if |url| has the same scheme, host, and path as the instant URL -// set via --instant-url. -bool IsForcedInstantURL(const GURL& url) { - CommandLine* command_line = CommandLine::ForCurrentProcess(); - if (command_line->HasSwitch(switches::kInstantURL)) { - GURL instant_url(command_line->GetSwitchValueASCII(switches::kInstantURL)); - if (url.scheme() == instant_url.scheme() && - url.host() == instant_url.host() && - url.path() == instant_url.path()) { - return true; - } - } - return false; -} - -// Determines whether the |url| is an Instant url for the default search -// provider (set for the |profile|). -// Also returns true if an Instant "effective url" (see below) is passed in. -bool IsInstantURL(const GURL& url, Profile* profile) { - // Handle the command-line URL. - if (IsForcedInstantURL(url)) - return true; - - // A URL of this form has already be determined to be an Instant url, - // this is its "effective url". So trivially return true. - if (url.SchemeIs(chrome::kChromeSearchScheme)) - return true; - - 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; - - if (template_url->instant_url().empty()) - return false; - - return template_url->IsInstantURL(url); -} - -// Transforms the input |url| into its "effective url". The returned url +// Transforms the input |url| into its "effective URL". The returned URL // facilitates grouping process-per-site. The |url| is transformed, for // example, from // // https://www.google.com/search?espv=1&q=tractors // -// to the effective url +// to the effective URL // // chrome-search://www.google.com/search?espv=1&q=tractors // // Notice the scheme change. -// If the input is already an effective url then that same url is -// returned. -GURL GetEffectiveInstantURL(const GURL& url, Profile* profile) { - CHECK(IsInstantURL(url, profile)) << "Error granting Instant access."; +// +// If the input is already an effective URL then that same URL is returned. +GURL GetEffectiveURLForInstant(const GURL& url, Profile* profile) { + CHECK(chrome::search::ShouldAssignURLToInstantRenderer(url, profile)) + << "Error granting Instant access."; if (url.SchemeIs(chrome::kChromeSearchScheme)) return url; @@ -728,10 +683,10 @@ GURL ChromeContentBrowserClient::GetEffectiveURL( if (!profile) return url; - // If the input |url| is an Instant url, make its effective url distinct from - // other urls on the search provider's domain. - if (IsInstantURL(url, profile)) - return GetEffectiveInstantURL(url, profile); + // If the input |url| should be assigned to the Instant renderer, make its + // effective URL distinct from other URLs on the search provider's domain. + if (chrome::search::ShouldAssignURLToInstantRenderer(url, profile)) + return GetEffectiveURLForInstant(url, profile); // If the input |url| is part of an installed app, the effective URL is an // extension URL with the ID of that extension as the host. This has the @@ -767,7 +722,7 @@ bool ChromeContentBrowserClient::ShouldUseProcessPerSite( if (!profile) return false; - if (IsInstantURL(effective_url, profile)) + if (chrome::search::ShouldAssignURLToInstantRenderer(effective_url, profile)) return true; if (!effective_url.SchemeIs(extensions::kExtensionScheme)) @@ -865,7 +820,7 @@ bool ChromeContentBrowserClient::IsSuitableHost( InstantServiceFactory::GetForProfile(profile); if (instant_service && instant_service->IsInstantProcess(process_host->GetID())) - return IsInstantURL(site_url, profile); + return chrome::search::ShouldAssignURLToInstantRenderer(site_url, profile); ExtensionService* service = extensions::ExtensionSystem::Get(profile)->extension_service(); @@ -955,7 +910,8 @@ void ChromeContentBrowserClient::SiteInstanceGotProcess( // Remember the ID of the Instant process to signal the renderer process // on startup in |AppendExtraCommandLineSwitches| below. - if (IsInstantURL(site_instance->GetSiteURL(), profile)) { + if (chrome::search::ShouldAssignURLToInstantRenderer( + site_instance->GetSiteURL(), profile)) { InstantService* instant_service = InstantServiceFactory::GetForProfile(profile); if (instant_service) diff --git a/chrome/browser/instant/instant_browsertest.cc b/chrome/browser/instant/instant_browsertest.cc index b29cdd7..03d01189 100644 --- a/chrome/browser/instant/instant_browsertest.cc +++ b/chrome/browser/instant/instant_browsertest.cc @@ -112,37 +112,6 @@ IN_PROC_BROWSER_TEST_F(InstantTest, OmniboxFocusLoadsInstant) { EXPECT_EQ(preview_tab, instant()->GetPreviewContents()); } -// Test that Instant works when the URL is set via a TemplateURL (as opposed to -// --instant-url). -IN_PROC_BROWSER_TEST_F(InstantTest, SetWithTemplateURL) { - ASSERT_NO_FATAL_FAILURE(SetupInstantUsingTemplateURL()); - - // Explicitly unfocus the omnibox. - EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser())); - ui_test_utils::ClickOnView(browser(), VIEW_ID_TAB_CONTAINER); - - EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_TAB_CONTAINER)); - EXPECT_FALSE(omnibox()->model()->has_focus()); - - // Delete any existing preview. - instant()->overlay_.reset(); - EXPECT_FALSE(instant()->GetPreviewContents()); - - // Refocus the omnibox. The InstantController should've preloaded Instant. - FocusOmniboxAndWaitForInstantSupport(); - - EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_TAB_CONTAINER)); - EXPECT_TRUE(omnibox()->model()->has_focus()); - - content::WebContents* preview_tab = instant()->GetPreviewContents(); - EXPECT_TRUE(preview_tab); - - // Check that the page supports Instant, but it isn't showing. - EXPECT_TRUE(instant()->overlay_->supports_instant()); - EXPECT_FALSE(instant()->IsPreviewingSearchResults()); - EXPECT_TRUE(instant()->model()->mode().is_default()); -} - // Flakes on Windows and Mac: http://crbug.com/170677 #if defined(OS_WIN) || defined(OS_MACOSX) #define MAYBE_OnChangeEvent DISABLED_OnChangeEvent diff --git a/chrome/browser/instant/instant_controller.cc b/chrome/browser/instant/instant_controller.cc index 80f9b79..62192b6 100644 --- a/chrome/browser/instant/instant_controller.cc +++ b/chrome/browser/instant/instant_controller.cc @@ -21,6 +21,7 @@ #include "chrome/browser/search_engines/template_url_service.h" #include "chrome/browser/search_engines/template_url_service_factory.h" #include "chrome/browser/ui/browser_instant_controller.h" +#include "chrome/browser/ui/search/search.h" #include "chrome/browser/ui/search/search_tab_helper.h" #include "chrome/common/chrome_notification_types.h" #include "chrome/common/chrome_switches.h" @@ -165,10 +166,6 @@ bool IsContentsFrom(const InstantPage* page, } // namespace -// static -const char* InstantController::kLocalOmniboxPopupURL = - "chrome://local-omnibox-popup/local-omnibox-popup.html"; - InstantController::InstantController(chrome::BrowserInstantController* browser, bool extended_enabled) : browser_(browser), @@ -392,7 +389,8 @@ bool InstantController::Update(const AutocompleteMatch& match, // to a backup loader. if (extended_enabled_ && !overlay_->supports_instant() && !overlay_->IsUsingLocalPreview() && browser_->GetActiveWebContents()) { - CreateOverlay(kLocalOmniboxPopupURL, browser_->GetActiveWebContents()); + CreateOverlay(chrome::search::kLocalOmniboxPopupURL, + browser_->GetActiveWebContents()); } overlay_->Update(extended_enabled_ ? user_text : full_text, @@ -1128,7 +1126,7 @@ bool InstantController::EnsureOverlayIsCurrent(bool ignore_blacklist) { if (!GetInstantURL(profile, ignore_blacklist, &instant_url)) { // If we are in extended mode, fallback to the local popup. if (extended_enabled_) - instant_url = kLocalOmniboxPopupURL; + instant_url = chrome::search::kLocalOmniboxPopupURL; else return false; } @@ -1296,7 +1294,7 @@ bool InstantController::GetInstantURL(Profile* profile, instant_url->clear(); if (extended_enabled_ && use_local_preview_only_) { - *instant_url = kLocalOmniboxPopupURL; + *instant_url = chrome::search::kLocalOmniboxPopupURL; return true; } diff --git a/chrome/browser/instant/instant_controller.h b/chrome/browser/instant/instant_controller.h index 1a08425..a2e6749 100644 --- a/chrome/browser/instant/instant_controller.h +++ b/chrome/browser/instant/instant_controller.h @@ -67,9 +67,6 @@ class WebContents; // InstantController is owned by Browser via BrowserInstantController. class InstantController : public InstantPage::Delegate { public: - // The URL for the local omnibox popup. - static const char* kLocalOmniboxPopupURL; - InstantController(chrome::BrowserInstantController* browser, bool extended_enabled); virtual ~InstantController(); diff --git a/chrome/browser/instant/instant_extended_browsertest.cc b/chrome/browser/instant/instant_extended_browsertest.cc index 33195fc..1462bb2 100644 --- a/chrome/browser/instant/instant_extended_browsertest.cc +++ b/chrome/browser/instant/instant_extended_browsertest.cc @@ -120,8 +120,12 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest, InputShowsOverlay) { EXPECT_EQ(preview_tab, instant()->GetPreviewContents()); } +// TODO(sreeram): Enable this test once @mathp's CL lands: +// https://codereview.chromium.org/12179025/ +// // Test that omnibox text is correctly set when overlay is committed with Enter. -IN_PROC_BROWSER_TEST_F(InstantExtendedTest, OmniboxTextUponEnterCommit) { +IN_PROC_BROWSER_TEST_F(InstantExtendedTest, + DISABLED_OmniboxTextUponEnterCommit) { ASSERT_NO_FATAL_FAILURE(SetupInstant()); FocusOmniboxAndWaitForInstantSupport(); @@ -145,9 +149,13 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest, OmniboxTextUponEnterCommit) { EXPECT_EQ(ASCIIToUTF16(""), omnibox()->GetInstantSuggestion()); } +// TODO(sreeram): Enable this test once @mathp's CL lands: +// https://codereview.chromium.org/12179025/ +// // Test that omnibox text is correctly set when overlay is committed with focus // lost. -IN_PROC_BROWSER_TEST_F(InstantExtendedTest, OmniboxTextUponFocusLostCommit) { +IN_PROC_BROWSER_TEST_F(InstantExtendedTest, + DISABLED_OmniboxTextUponFocusLostCommit) { ASSERT_NO_FATAL_FAILURE(SetupInstant()); FocusOmniboxAndWaitForInstantSupport(); @@ -304,18 +312,21 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest, InputOnNTPDoesntShowOverlay) { } IN_PROC_BROWSER_TEST_F(InstantExtendedTest, ProcessIsolation) { - // Prior to setup no render process is dedicated to Instant. + // Prior to setup, Instant has an overlay with a failed "google.com" load in + // it, which is rendered in the dedicated Instant renderer process. + // + // TODO(sreeram): Fix this up when we stop doing crazy things on init. InstantService* instant_service = InstantServiceFactory::GetForProfile(browser()->profile()); ASSERT_NE(static_cast<InstantService*>(NULL), instant_service); - EXPECT_EQ(0, instant_service->GetInstantProcessCount()); + EXPECT_EQ(1, instant_service->GetInstantProcessCount()); // Setup Instant. ASSERT_NO_FATAL_FAILURE(SetupInstant()); FocusOmniboxAndWaitForInstantSupport(); - // Now there should be a registered Instant render process. - EXPECT_LT(0, instant_service->GetInstantProcessCount()); + // The registered Instant render process should still exist. + EXPECT_EQ(1, instant_service->GetInstantProcessCount()); // And the Instant overlay and ntp should live inside it. content::WebContents* preview = instant()->GetPreviewContents(); diff --git a/chrome/browser/instant/instant_overlay.cc b/chrome/browser/instant/instant_overlay.cc index ebe1a4a..15e1a0d 100644 --- a/chrome/browser/instant/instant_overlay.cc +++ b/chrome/browser/instant/instant_overlay.cc @@ -6,6 +6,7 @@ #include "base/auto_reset.h" #include "base/supports_user_data.h" +#include "chrome/browser/ui/search/search.h" #include "content/public/browser/web_contents.h" namespace { @@ -71,7 +72,7 @@ void InstantOverlay::DidNavigate( } bool InstantOverlay::IsUsingLocalPreview() const { - return instant_url_ == InstantController::kLocalOmniboxPopupURL; + return instant_url_ == chrome::search::kLocalOmniboxPopupURL; } void InstantOverlay::Update(const string16& text, diff --git a/chrome/browser/instant/instant_service_factory.cc b/chrome/browser/instant/instant_service_factory.cc index d17a235..bed382f 100644 --- a/chrome/browser/instant/instant_service_factory.cc +++ b/chrome/browser/instant/instant_service_factory.cc @@ -36,10 +36,6 @@ bool InstantServiceFactory::ServiceRedirectedInIncognito() const { return true; } -bool InstantServiceFactory::ServiceIsNULLWhileTesting() const { - return true; -} - ProfileKeyedService* InstantServiceFactory::BuildServiceInstanceFor( Profile* profile) const { return BuildInstanceFor(profile); diff --git a/chrome/browser/instant/instant_service_factory.h b/chrome/browser/instant/instant_service_factory.h index 5e40af5..df63139 100644 --- a/chrome/browser/instant/instant_service_factory.h +++ b/chrome/browser/instant/instant_service_factory.h @@ -32,7 +32,6 @@ class InstantServiceFactory : public ProfileKeyedServiceFactory { // ProfileKeyedServiceFactory: virtual bool ServiceRedirectedInIncognito() const OVERRIDE; - virtual bool ServiceIsNULLWhileTesting() const OVERRIDE; virtual ProfileKeyedService* BuildServiceInstanceFor( Profile* profile) const OVERRIDE; diff --git a/chrome/browser/instant/instant_test_utils.cc b/chrome/browser/instant/instant_test_utils.cc index 5c179bc..9b9396a 100644 --- a/chrome/browser/instant/instant_test_utils.cc +++ b/chrome/browser/instant/instant_test_utils.cc @@ -9,7 +9,6 @@ #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/pref_names.h" #include "chrome/test/base/interactive_test_utils.h" #include "chrome/test/base/ui_test_utils.h" @@ -52,16 +51,6 @@ void InstantTestModelObserver::PreviewStateChanged(const InstantModel& model) { // InstantTestBase ----------------------------------------------------------- void InstantTestBase::SetupInstant() { - CommandLine::ForCurrentProcess()->AppendSwitchASCII( - switches::kInstantURL, instant_url_.spec()); - SetupInstantUsingTemplateURL(); - - // TODO(shishir): Fix this ugly hack. - instant()->SetInstantEnabled(false, true); - instant()->SetInstantEnabled(true, false); -} - -void InstantTestBase::SetupInstantUsingTemplateURL() { TemplateURLService* service = TemplateURLServiceFactory::GetForProfile(browser()->profile()); ui_test_utils::WaitForTemplateURLServiceToLoad(service); @@ -77,7 +66,12 @@ void InstantTestBase::SetupInstantUsingTemplateURL() { TemplateURL* template_url = new TemplateURL(browser()->profile(), data); service->Add(template_url); // Takes ownership of |template_url|. service->SetDefaultSearchProvider(template_url); + browser()->profile()->GetPrefs()->SetBoolean(prefs::kInstantEnabled, true); + + // TODO(shishir): Fix this ugly hack. + instant()->SetInstantEnabled(false, true); + instant()->SetInstantEnabled(true, false); } void InstantTestBase::KillInstantRenderView() { diff --git a/chrome/browser/instant/instant_test_utils.h b/chrome/browser/instant/instant_test_utils.h index cee9243..c9554a8 100644 --- a/chrome/browser/instant/instant_test_utils.h +++ b/chrome/browser/instant/instant_test_utils.h @@ -48,7 +48,6 @@ class InstantTestBase : public InProcessBrowserTest { protected: void SetupInstant(); - void SetupInstantUsingTemplateURL(); InstantController* instant() { return browser()->instant_controller()->instant(); diff --git a/chrome/browser/policy/policy_browsertest.cc b/chrome/browser/policy/policy_browsertest.cc index c1d7115..3cc86dbb 100644 --- a/chrome/browser/policy/policy_browsertest.cc +++ b/chrome/browser/policy/policy_browsertest.cc @@ -31,6 +31,8 @@ #include "chrome/browser/extensions/crx_installer.h" #include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/extensions/extension_system.h" +#include "chrome/browser/instant/instant_service.h" +#include "chrome/browser/instant/instant_service_factory.h" #include "chrome/browser/media/media_capture_devices_dispatcher.h" #include "chrome/browser/media/media_stream_devices_controller.h" #include "chrome/browser/net/url_request_mock_util.h" @@ -76,6 +78,7 @@ #include "content/public/browser/notification_source.h" #include "content/public/browser/notification_types.h" #include "content/public/browser/plugin_service.h" +#include "content/public/browser/render_process_host.h" #include "content/public/browser/render_view_host.h" #include "content/public/browser/web_contents.h" #include "content/public/common/content_paths.h" @@ -791,12 +794,13 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, ForceSafeSearch) { IN_PROC_BROWSER_TEST_F(PolicyTest, ReplaceSearchTerms) { MakeRequestFail make_request_fail("search.example"); - chrome::search::EnableInstantExtendedAPIForTesting(); + chrome::search::EnableQueryExtractionForTesting(); // Verifies that a default search is made using the provider configured via // policy. Also checks that default search can be completely disabled. const string16 kKeyword(ASCIIToUTF16("testsearch")); const std::string kSearchURL("https://www.google.com/search?q={searchTerms}"); + const std::string kInstantURL("http://does/not/exist"); const std::string kAlternateURL0( "https://www.google.com/search#q={searchTerms}"); const std::string kAlternateURL1("https://www.google.com/#q={searchTerms}"); @@ -810,6 +814,7 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, ReplaceSearchTerms) { ASSERT_TRUE(default_search); EXPECT_NE(kKeyword, default_search->keyword()); EXPECT_NE(kSearchURL, default_search->url()); + EXPECT_NE(kInstantURL, default_search->instant_url()); EXPECT_FALSE( default_search->alternate_urls().size() == 2 && default_search->alternate_urls()[0] == kAlternateURL0 && @@ -823,6 +828,8 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, ReplaceSearchTerms) { POLICY_SCOPE_USER, base::Value::CreateStringValue(kKeyword)); policies.Set(key::kDefaultSearchProviderSearchURL, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER, base::Value::CreateStringValue(kSearchURL)); + policies.Set(key::kDefaultSearchProviderInstantURL, POLICY_LEVEL_MANDATORY, + POLICY_SCOPE_USER, base::Value::CreateStringValue(kInstantURL)); base::ListValue* alternate_urls = new base::ListValue(); alternate_urls->AppendString(kAlternateURL0); alternate_urls->AppendString(kAlternateURL1); @@ -836,10 +843,18 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, ReplaceSearchTerms) { ASSERT_TRUE(default_search); EXPECT_EQ(kKeyword, default_search->keyword()); EXPECT_EQ(kSearchURL, default_search->url()); + EXPECT_EQ(kInstantURL, default_search->instant_url()); EXPECT_EQ(2U, default_search->alternate_urls().size()); EXPECT_EQ(kAlternateURL0, default_search->alternate_urls()[0]); EXPECT_EQ(kAlternateURL1, default_search->alternate_urls()[1]); + // Query terms replacement requires that the renderer process be a recognized + // Instant renderer. Fake it. + InstantService* instant_service = + InstantServiceFactory::GetForProfile(browser()->profile()); + instant_service->AddInstantProcess(browser()->tab_strip_model()-> + GetActiveWebContents()->GetRenderProcessHost()->GetID()); + // Verify that searching from the omnibox does search term replacement with // first URL pattern. chrome::FocusLocationBar(browser()); diff --git a/chrome/browser/search_engines/template_url.cc b/chrome/browser/search_engines/template_url.cc index 24abf88..a14bbc4 100644 --- a/chrome/browser/search_engines/template_url.cc +++ b/chrome/browser/search_engines/template_url.cc @@ -416,11 +416,6 @@ std::string TemplateURLRef::DisplayURLToURLRef( return UTF16ToUTF8(result); } -const std::string& TemplateURLRef::GetScheme() const { - ParseIfNecessary(); - return scheme_; -} - const std::string& TemplateURLRef::GetHost() const { ParseIfNecessary(); return host_; @@ -524,7 +519,6 @@ bool TemplateURLRef::ExtractSearchTermsFromURL(const GURL& url, void TemplateURLRef::InvalidateCachedValues() const { supports_replacements_ = valid_ = parsed_ = false; - scheme_.clear(); host_.clear(); path_.clear(); search_term_key_.clear(); @@ -691,7 +685,6 @@ void TemplateURLRef::ParseHostAndSearchTermKey( search_terms_data.GoogleBaseSuggestURLValue()); search_term_key_.clear(); - scheme_.clear(); host_.clear(); path_.clear(); search_term_key_location_ = url_parse::Parsed::REF; @@ -707,7 +700,6 @@ void TemplateURLRef::ParseHostAndSearchTermKey( search_term_key_ = query_key.empty() ? ref_key : query_key; search_term_key_location_ = query_key.empty() ? url_parse::Parsed::REF : url_parse::Parsed::QUERY; - scheme_ = url.scheme(); host_ = url.host(); path_ = url.path(); } @@ -878,34 +870,6 @@ bool TemplateURL::HasSearchTermsReplacementKey(const GURL& url) const { return false; } -bool TemplateURL::IsInstantURL(const GURL& url) { - // If the url matches the Instant ref, there's no need to - // check the replacement-key parameter, since we know this - // is instant. - // TODO(dhollowa): http://crbug.com/170390. Consolidate Instant URL checks. - TemplateURLRef ref(this, TemplateURLRef::INSTANT); - GURL instant_url(ref.ReplaceSearchTerms( - TemplateURLRef::SearchTermsArgs(string16()))); - if (instant_url.scheme() == url.scheme() && - instant_url.host() == url.host() && - instant_url.path() == url.path()) - return true; - - // Anything else requires the existence of the replacement-key. - if (!HasSearchTermsReplacementKey(url)) - return false; - - for (size_t i = 0; i < URLCount(); ++i) { - TemplateURLRef ref(this, i); - if (ref.GetScheme() == url.scheme() && - ref.GetHost() == url.host() && - ref.GetPath() == url.path()) - return true; - } - - return false; -} - void TemplateURL::CopyFrom(const TemplateURL& other) { if (this == &other) return; diff --git a/chrome/browser/search_engines/template_url.h b/chrome/browser/search_engines/template_url.h index e81d671..d2d2c48 100644 --- a/chrome/browser/search_engines/template_url.h +++ b/chrome/browser/search_engines/template_url.h @@ -121,8 +121,7 @@ class TemplateURLRef { static std::string DisplayURLToURLRef(const string16& display_url); // If this TemplateURLRef is valid and contains one search term, this returns - // the scheme/host/path of the URL, otherwise this returns an empty string. - const std::string& GetScheme() const; + // the host/path of the URL, otherwise this returns an empty string. const std::string& GetHost() const; const std::string& GetPath() const; @@ -252,9 +251,8 @@ class TemplateURLRef { // into the string, and may be empty. mutable Replacements replacements_; - // Scheme, host, path, key and location of the search term. These are only set - // if the url contains one search term. - mutable std::string scheme_; + // Host, path, key and location of the search term. These are only set if the + // url contains one search term. mutable std::string host_; mutable std::string path_; mutable std::string search_term_key_; @@ -493,13 +491,6 @@ class TemplateURL { // InstantExtended capable URL. bool HasSearchTermsReplacementKey(const GURL& url) const; - // Returns true if the specified |url| matches the search, alternates, or - // instant url in scheme, domain, and path. In addition, the search term - // replacement key must be present in the |url|. - // This predicate is used for site isolation purposes, so has security - // implications. Seek security review if changing it. - bool IsInstantURL(const GURL& url); - private: friend class TemplateURLService; diff --git a/chrome/browser/search_engines/template_url_unittest.cc b/chrome/browser/search_engines/template_url_unittest.cc index 5550b03..3273fa5 100644 --- a/chrome/browser/search_engines/template_url_unittest.cc +++ b/chrome/browser/search_engines/template_url_unittest.cc @@ -539,37 +539,35 @@ TEST_F(TemplateURLTest, RLZ) { TEST_F(TemplateURLTest, HostAndSearchTermKey) { struct TestData { const std::string url; - const std::string scheme; const std::string host; const std::string path; const std::string search_term_key; } test_data[] = { - { "http://blah/?foo=bar&q={searchTerms}&b=x", "http", "blah", "/", "q"}, + { "http://blah/?foo=bar&q={searchTerms}&b=x", "blah", "/", "q"}, // No query key should result in empty values. - { "http://blah/{searchTerms}", "", "", "", ""}, + { "http://blah/{searchTerms}", "", "", ""}, // No term should result in empty values. - { "http://blah/", "", "", "", ""}, + { "http://blah/", "", "", ""}, // Multiple terms should result in empty values. - { "http://blah/?q={searchTerms}&x={searchTerms}", "", "", "", ""}, + { "http://blah/?q={searchTerms}&x={searchTerms}", "", "", ""}, // Term in the host shouldn't match. - { "http://{searchTerms}", "", "", "", ""}, + { "http://{searchTerms}", "", "", ""}, - { "http://blah/?q={searchTerms}", "http", "blah", "/", "q"}, - { "https://blah/?q={searchTerms}", "https", "blah", "/", "q"}, + { "http://blah/?q={searchTerms}", "blah", "/", "q"}, + { "https://blah/?q={searchTerms}", "blah", "/", "q"}, // Single term with extra chars in value should match. - { "http://blah/?q=stock:{searchTerms}", "http", "blah", "/", "q"}, + { "http://blah/?q=stock:{searchTerms}", "blah", "/", "q"}, }; for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_data); ++i) { TemplateURLData data; data.SetURL(test_data[i].url); TemplateURL url(NULL, data); - EXPECT_EQ(test_data[i].scheme, url.url_ref().GetScheme()); EXPECT_EQ(test_data[i].host, url.url_ref().GetHost()); EXPECT_EQ(test_data[i].path, url.url_ref().GetPath()); EXPECT_EQ(test_data[i].search_term_key, url.url_ref().GetSearchTermKey()); @@ -927,95 +925,3 @@ TEST_F(TemplateURLTest, HasSearchTermsReplacementKey) { EXPECT_TRUE(url.HasSearchTermsReplacementKey( GURL("http://bing.com/#espv"))); } - -TEST_F(TemplateURLTest, IsInstantURL) { - TemplateURLData data; - data.SetURL("http://google.com/?q={searchTerms}"); - data.instant_url = "http://google.com/instant#q={searchTerms}"; - data.alternate_urls.push_back("http://google.com/alt/#q={searchTerms}"); - data.alternate_urls.push_back( - "http://google.com/alt/?ext=foo&q={searchTerms}#ref=bar"); - data.search_terms_replacement_key = "espv"; - TemplateURL url(NULL, data); - - EXPECT_FALSE(url.IsInstantURL( - GURL("http://google.com/"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?espv"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/#espv"))); - - EXPECT_FALSE(url.IsInstantURL( - GURL("http://google.com/?q=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/instant?q=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/instant?x=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?q=something&espv"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?q=something&espv=1"))); - - EXPECT_FALSE(url.IsInstantURL( - GURL("https://google.com/?q=something&espv=1"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?q=something&espv=0"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?espv&q=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?espv=1&q=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?espv=0&q=something"))); - - EXPECT_FALSE(url.IsInstantURL( - GURL("http://google.com/alt/#q=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/alt/#q=something&espv"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/alt/#q=something&espv=1"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/alt/#q=something&espv=0"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/alt/#espv&q=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/alt/#espv=1&q=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/alt/#espv=0&q=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?espv#q=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?espv=1#q=something"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?q=something#espv"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/?q=something#espv=1"))); - - EXPECT_TRUE(url.IsInstantURL( - GURL("http://google.com/instant#q=something&espv=1"))); - - EXPECT_FALSE(url.IsInstantURL( - GURL("http://bing.com/?espv=1"))); - - EXPECT_FALSE(url.IsInstantURL( - GURL("http://bing.com/#espv=1"))); -} 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. |