diff options
author | jered@chromium.org <jered@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-19 05:48:43 +0000 |
---|---|---|
committer | jered@chromium.org <jered@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-19 05:48:43 +0000 |
commit | 2b3ded9b1cc12a3cfb2bdf1a0f64d46fa4da6219 (patch) | |
tree | c514a2d597d7944aa787c9a2ff1a683ab0eb2971 | |
parent | d51a39a39c7d059855eb652593703ffcee8da167 (diff) | |
download | chromium_src-2b3ded9b1cc12a3cfb2bdf1a0f64d46fa4da6219.zip chromium_src-2b3ded9b1cc12a3cfb2bdf1a0f64d46fa4da6219.tar.gz chromium_src-2b3ded9b1cc12a3cfb2bdf1a0f64d46fa4da6219.tar.bz2 |
InstantExtended: Fall back if JS disabled.
This change makes Chrome fall back to the local NTP if content settings
or the global webkit pref say Javascript should be disabled. The Google
NTP already falls back if it thinks JS is disabled, correctly or
incorrectly, so this is mostly just to avoid extra network traffic.
BUG=240935
TEST=unit tests, manually tried it
Review URL: https://chromiumcodereview.appspot.com/16843012
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@207188 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | chrome/browser/ui/search/instant_controller.cc | 47 | ||||
-rw-r--r-- | chrome/browser/ui/search/instant_controller.h | 30 | ||||
-rw-r--r-- | chrome/browser/ui/search/instant_controller_unittest.cc | 254 | ||||
-rw-r--r-- | chrome/browser/ui/search/instant_page.cc | 4 | ||||
-rw-r--r-- | chrome/browser/ui/search/instant_page.h | 2 | ||||
-rw-r--r-- | tools/metrics/histograms/histograms.xml | 1 |
6 files changed, 313 insertions, 25 deletions
diff --git a/chrome/browser/ui/search/instant_controller.cc b/chrome/browser/ui/search/instant_controller.cc index d01af5d..c861a1c 100644 --- a/chrome/browser/ui/search/instant_controller.cc +++ b/chrome/browser/ui/search/instant_controller.cc @@ -7,12 +7,15 @@ #include <iterator> #include "base/metrics/histogram.h" +#include "base/prefs/pref_service.h" #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" #include "chrome/browser/autocomplete/autocomplete_provider.h" #include "chrome/browser/autocomplete/autocomplete_result.h" #include "chrome/browser/autocomplete/search_provider.h" +#include "chrome/browser/content_settings/content_settings_provider.h" +#include "chrome/browser/content_settings/host_content_settings_map.h" #include "chrome/browser/history/history_service.h" #include "chrome/browser/history/history_service_factory.h" #include "chrome/browser/history/history_tab_helper.h" @@ -30,6 +33,8 @@ #include "chrome/browser/ui/search/search_tab_helper.h" #include "chrome/common/chrome_notification_types.h" #include "chrome/common/chrome_switches.h" +#include "chrome/common/content_settings_types.h" +#include "chrome/common/pref_names.h" #include "chrome/common/url_constants.h" #include "components/sessions/serialized_navigation_entry.h" #include "content/public/browser/navigation_entry.h" @@ -1588,25 +1593,25 @@ void InstantController::ReloadStaleNTP() { } bool InstantController::ShouldSwitchToLocalNTP() const { - if (!ntp_) + if (!ntp()) + return true; + + // Assume users with Javascript disabled do not want the online experience. + if (!IsJavascriptEnabled()) return true; // Already a local page. Not calling IsLocal() because we want to distinguish // between the Google-specific and generic local NTP. - if (extended_enabled() && ntp_->instant_url() == GetLocalInstantURL()) + if (extended_enabled() && ntp()->instant_url() == GetLocalInstantURL()) return false; if (PageIsCurrent(ntp())) return false; - // TODO(shishir): This is not completely reliable. Find a better way to detect - // startup time. - const bool in_startup = !browser_->GetActiveWebContents(); - // The preloaded NTP does not support instant yet. If we're not in startup, // always fall back to the local NTP. If we are in startup, use the local NTP // (unless the finch flag to use the remote NTP is set). - return !(in_startup && chrome::ShouldPreferRemoteNTPOnStartup()); + return !(InStartup() && chrome::ShouldPreferRemoteNTPOnStartup()); } void InstantController::ResetOverlay(const std::string& instant_url) { @@ -1631,6 +1636,10 @@ InstantController::ShouldSwitchToLocalOverlay() const { if (!overlay()) return DetermineFallbackReason(NULL, std::string()); + // Assume users with Javascript disabled do not want the online experience. + if (!IsJavascriptEnabled()) + return INSTANT_FALLBACK_JAVASCRIPT_DISABLED; + if (overlay()->IsLocal()) return INSTANT_FALLBACK_NONE; @@ -1874,3 +1883,27 @@ void InstantController::PopulateInstantAutocompleteResultFromMatch( << result->description << "' '" << result->search_query << "' " << result->transition << " " << result->autocomplete_match_index; } + +bool InstantController::IsJavascriptEnabled() const { + GURL instant_url(GetInstantURL()); + GURL origin(instant_url.GetOrigin()); + ContentSetting js_setting = profile()->GetHostContentSettingsMap()-> + GetContentSetting(origin, origin, CONTENT_SETTINGS_TYPE_JAVASCRIPT, + NO_RESOURCE_IDENTIFIER); + // Javascript can be disabled either in content settings or via a WebKit + // preference, so check both. Disabling it through the Settings page affects + // content settings. I'm not sure how to disable the WebKit preference, but + // it's theoretically possible some users have it off. + bool js_content_enabled = + js_setting == CONTENT_SETTING_DEFAULT || + js_setting == CONTENT_SETTING_ALLOW; + bool js_webkit_enabled = profile()->GetPrefs()->GetBoolean( + prefs::kWebKitJavascriptEnabled); + return js_content_enabled && js_webkit_enabled; +} + +bool InstantController::InStartup() const { + // TODO(shishir): This is not completely reliable. Find a better way to detect + // startup time. + return !browser_->GetActiveWebContents(); +} diff --git a/chrome/browser/ui/search/instant_controller.h b/chrome/browser/ui/search/instant_controller.h index cdb170b..0a3d42e 100644 --- a/chrome/browser/ui/search/instant_controller.h +++ b/chrome/browser/ui/search/instant_controller.h @@ -76,7 +76,8 @@ class InstantController : public InstantPage::Delegate { INSTANT_FALLBACK_ORIGIN_PATH_MISMATCH = 3, INSTANT_FALLBACK_INSTANT_NOT_SUPPORTED = 4, INSTANT_FALLBACK_NO_OVERLAY = 5, - INSTANT_FALLBACK_MAX = 6, + INSTANT_FALLBACK_JAVASCRIPT_DISABLED = 6, + INSTANT_FALLBACK_MAX = 7, }; InstantController(BrowserInstantController* browser, @@ -251,11 +252,30 @@ class InstantController : public InstantPage::Delegate { virtual InstantTab* instant_tab() const; virtual InstantNTP* ntp() const; + virtual Profile* profile() const; + + // Returns true if Javascript is enabled and false otherwise. + virtual bool IsJavascriptEnabled() const; + + // Returns true if the browser is in startup. + virtual bool InStartup() const; + private: friend class InstantExtendedManualTest; friend class InstantTestBase; - FRIEND_TEST_ALL_PREFIXES(InstantControllerTest, - ShouldSwitchToLocalOverlayReturn); +#define UNIT_F(test) FRIEND_TEST_ALL_PREFIXES(InstantControllerTest, test) + UNIT_F(DoesNotSwitchToLocalNTPIfOnCurrentNTP); + UNIT_F(DoesNotSwitchToLocalNTPIfOnLocalNTP); + UNIT_F(IsJavascriptEnabled); + UNIT_F(IsJavascriptEnabledChecksContentSettings); + UNIT_F(IsJavascriptEnabledChecksPrefs); + UNIT_F(PrefersRemoteNTPOnStartup); + UNIT_F(ShouldSwitchToLocalOverlay); + UNIT_F(SwitchesToLocalNTPIfJSDisabled); + UNIT_F(SwitchesToLocalNTPIfNoInstantSupport); + UNIT_F(SwitchesToLocalNTPIfNoNTPReady); + UNIT_F(SwitchesToLocalNTPIfPathBad); +#undef UNIT_F FRIEND_TEST_ALL_PREFIXES(InstantTest, OmniboxFocusLoadsInstant); FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, UsesOverlayIfTabNotReady); FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, @@ -319,8 +339,6 @@ class InstantController : public InstantPage::Delegate { TypedSearchURLDoesntReuseInstantTab); #endif - Profile* profile() const; - // Overridden from InstantPage::Delegate: // TODO(shishir): We assume that the WebContent's current RenderViewHost is // the RenderViewHost being created which is not always true. Fix this. @@ -373,7 +391,7 @@ class InstantController : public InstantPage::Delegate { // Returns the local Instant URL. (Just a convenience wrapper around // chrome::GetLocalInstantURL.) - std::string GetLocalInstantURL() const; + virtual std::string GetLocalInstantURL() const; // Returns true if |page| has an up-to-date Instant URL and supports Instant. // Note that local URLs will not pass this check. diff --git a/chrome/browser/ui/search/instant_controller_unittest.cc b/chrome/browser/ui/search/instant_controller_unittest.cc index d449236..3ed131e 100644 --- a/chrome/browser/ui/search/instant_controller_unittest.cc +++ b/chrome/browser/ui/search/instant_controller_unittest.cc @@ -3,12 +3,19 @@ // found in the LICENSE file. #include "base/memory/scoped_ptr.h" -#include "base/message_loop.h" #include "base/metrics/histogram.h" #include "base/metrics/histogram_samples.h" #include "base/metrics/statistics_recorder.h" +#include "base/prefs/pref_service.h" +#include "chrome/browser/content_settings/host_content_settings_map.h" +#include "chrome/browser/search/search.h" #include "chrome/browser/ui/search/instant_controller.h" +#include "chrome/browser/ui/search/instant_ntp.h" #include "chrome/browser/ui/search/instant_overlay.h" +#include "chrome/common/content_settings.h" +#include "chrome/common/pref_names.h" +#include "chrome/test/base/testing_profile.h" +#include "content/public/test/test_browser_thread.h" #include "testing/gtest/include/gtest/gtest.h" using base::HistogramBase; @@ -45,24 +52,75 @@ class TestableInstantOverlay : public InstantOverlay { bool test_is_local_; }; +class TestableInstantNTP : public InstantNTP { + public: + TestableInstantNTP(InstantController* controller, + const std::string& instant_url) + : InstantNTP(controller, instant_url) { + } + + // Overrides from InstantPage + virtual bool supports_instant() const OVERRIDE { + return test_supports_instant_; + } + + virtual bool IsLocal() const OVERRIDE { + return test_is_local_; + }; + + virtual const std::string& instant_url() const OVERRIDE { + return test_instant_url_; + } + + void set_instant_url(const std::string& instant_url) { + test_instant_url_ = instant_url; + } + + void set_supports_instant(bool supports_instant) { + test_supports_instant_ = supports_instant; + } + + void set_is_local(bool is_local) { + test_is_local_ = is_local; + } + + private: + std::string test_instant_url_; + bool test_supports_instant_; + bool test_is_local_; +}; + class TestableInstantController : public InstantController { public: TestableInstantController() : InstantController(NULL, true), test_instant_url_("http://test_url"), - test_extended_enabled_(true) {} + test_extended_enabled_(true), + override_javascript_enabled_(true), + test_javascript_enabled_(true), + test_in_startup_(false), + test_overlay_(NULL), + test_ntp_(NULL) {} // Overrides from InstantController virtual std::string GetInstantURL() const OVERRIDE { return test_instant_url_; } + virtual std::string GetLocalInstantURL() const OVERRIDE { + return "http://local_instant_url"; + } + virtual bool extended_enabled() const OVERRIDE { return test_extended_enabled_; } virtual InstantOverlay* overlay() const OVERRIDE { - return test_overlay_.get(); + return test_overlay_; + } + + virtual InstantNTP* ntp() const OVERRIDE { + return test_ntp_; } void set_instant_url(std::string instant_url) { @@ -74,19 +132,56 @@ class TestableInstantController : public InstantController { } void set_overlay(InstantOverlay* overlay) { - test_overlay_.reset(overlay); + test_overlay_ = overlay; + } + + void set_ntp(InstantNTP* ntp) { + test_ntp_ = ntp; + } + + void set_javascript_enabled(bool javascript_enabled) { + override_javascript_enabled_ = true; + test_javascript_enabled_ = javascript_enabled; + } + + void set_override_javascript_enabled(bool override_javascript_enabled) { + override_javascript_enabled_ = override_javascript_enabled; + } + + void set_in_startup(bool in_startup) { + test_in_startup_ = in_startup; + } + + virtual bool IsJavascriptEnabled() const OVERRIDE { + if (override_javascript_enabled_) + return test_javascript_enabled_; + else + return InstantController::IsJavascriptEnabled(); + } + + virtual bool InStartup() const OVERRIDE { + return test_in_startup_; + } + + virtual Profile* profile() const OVERRIDE { + return &profile_; } private: std::string test_instant_url_; bool test_extended_enabled_; - scoped_ptr<InstantOverlay> test_overlay_; + bool override_javascript_enabled_; + bool test_javascript_enabled_; + bool test_in_startup_; + InstantOverlay* test_overlay_; + InstantNTP* test_ntp_; + mutable TestingProfile profile_; }; class InstantControllerTest : public testing::Test { public: InstantControllerTest() - : loop_(base::MessageLoop::TYPE_DEFAULT), + : ui_thread_(content::BrowserThread::UI), instant_controller_(new TestableInstantController()) { } @@ -99,11 +194,11 @@ class InstantControllerTest : public testing::Test { } private: - base::MessageLoop loop_; + content::TestBrowserThread ui_thread_; scoped_ptr<TestableInstantController> instant_controller_; }; -TEST_F(InstantControllerTest, ShouldSwitchToLocalOverlayReturn) { +TEST_F(InstantControllerTest, ShouldSwitchToLocalOverlay) { InstantController::InstantFallbackReason fallback_reason; instant_controller()->set_extended_enabled(false); @@ -115,13 +210,19 @@ TEST_F(InstantControllerTest, ShouldSwitchToLocalOverlayReturn) { ASSERT_EQ(fallback_reason, InstantController::INSTANT_FALLBACK_NO_OVERLAY); std::string instant_url("http://test_url"); - TestableInstantOverlay* test_overlay = - new TestableInstantOverlay(instant_controller(), instant_url); + scoped_ptr<TestableInstantOverlay> test_overlay( + new TestableInstantOverlay(instant_controller(), instant_url)); test_overlay->set_is_local(true); - instant_controller()->set_overlay(test_overlay); + instant_controller()->set_overlay(test_overlay.get()); fallback_reason = instant_controller()->ShouldSwitchToLocalOverlay(); ASSERT_EQ(fallback_reason, InstantController::INSTANT_FALLBACK_NONE); + instant_controller()->set_javascript_enabled(false); + fallback_reason = instant_controller()->ShouldSwitchToLocalOverlay(); + ASSERT_EQ(fallback_reason, + InstantController::INSTANT_FALLBACK_JAVASCRIPT_DISABLED); + instant_controller()->set_javascript_enabled(true); + test_overlay->set_is_local(false); instant_controller()->set_instant_url(""); fallback_reason = instant_controller()->ShouldSwitchToLocalOverlay(); @@ -143,3 +244,134 @@ TEST_F(InstantControllerTest, ShouldSwitchToLocalOverlayReturn) { fallback_reason = instant_controller()->ShouldSwitchToLocalOverlay(); ASSERT_EQ(fallback_reason, InstantController::INSTANT_FALLBACK_NONE); } + +TEST_F(InstantControllerTest, PrefersRemoteNTPOnStartup) { + std::string instant_url("http://instant_url"); + scoped_ptr<TestableInstantNTP> ntp(new TestableInstantNTP( + instant_controller(), instant_url)); + ntp->set_is_local(false); + instant_controller()->set_ntp(ntp.get()); + instant_controller()->set_javascript_enabled(true); + instant_controller()->set_extended_enabled(true); + instant_controller()->set_instant_url(instant_url); + ntp->set_instant_url(instant_url); + ntp->set_supports_instant(false); + instant_controller()->set_in_startup(true); + EXPECT_EQ(!chrome::ShouldPreferRemoteNTPOnStartup(), + instant_controller()->ShouldSwitchToLocalNTP()); +} + +TEST_F(InstantControllerTest, SwitchesToLocalNTPIfNoInstantSupport) { + std::string instant_url("http://instant_url"); + scoped_ptr<TestableInstantNTP> ntp(new TestableInstantNTP( + instant_controller(), instant_url)); + ntp.reset(new TestableInstantNTP(instant_controller(), instant_url)); + ntp->set_is_local(false); + instant_controller()->set_ntp(ntp.get()); + instant_controller()->set_javascript_enabled(true); + instant_controller()->set_extended_enabled(true); + instant_controller()->set_instant_url(instant_url); + ntp->set_instant_url(instant_url); + ntp->set_supports_instant(false); + instant_controller()->set_in_startup(false); + EXPECT_TRUE(instant_controller()->ShouldSwitchToLocalNTP()); +} + +TEST_F(InstantControllerTest, SwitchesToLocalNTPIfPathBad) { + std::string instant_url("http://instant_url"); + scoped_ptr<TestableInstantNTP> ntp(new TestableInstantNTP( + instant_controller(), instant_url)); + ntp.reset(new TestableInstantNTP(instant_controller(), instant_url)); + ntp->set_is_local(false); + instant_controller()->set_ntp(ntp.get()); + instant_controller()->set_javascript_enabled(true); + instant_controller()->set_extended_enabled(true); + instant_controller()->set_instant_url("http://bogus_url"); + ntp->set_instant_url(instant_url); + ntp->set_supports_instant(true); + instant_controller()->set_in_startup(false); + EXPECT_TRUE(instant_controller()->ShouldSwitchToLocalNTP()); +} + +TEST_F(InstantControllerTest, DoesNotSwitchToLocalNTPIfOnCurrentNTP) { + std::string instant_url("http://instant_url"); + scoped_ptr<TestableInstantNTP> ntp(new TestableInstantNTP( + instant_controller(), instant_url)); + ntp.reset(new TestableInstantNTP(instant_controller(), instant_url)); + ntp->set_is_local(false); + instant_controller()->set_ntp(ntp.get()); + instant_controller()->set_javascript_enabled(true); + instant_controller()->set_extended_enabled(true); + instant_controller()->set_instant_url(instant_url); + ntp->set_instant_url(instant_url); + ntp->set_supports_instant(true); + instant_controller()->set_in_startup(false); + EXPECT_FALSE(instant_controller()->ShouldSwitchToLocalNTP()); +} + +TEST_F(InstantControllerTest, DoesNotSwitchToLocalNTPIfOnLocalNTP) { + std::string instant_url("http://instant_url"); + scoped_ptr<TestableInstantNTP> ntp(new TestableInstantNTP( + instant_controller(), instant_url)); + ntp.reset(new TestableInstantNTP(instant_controller(), instant_url)); + ntp->set_is_local(false); + instant_controller()->set_ntp(ntp.get()); + instant_controller()->set_javascript_enabled(true); + instant_controller()->set_extended_enabled(true); + instant_controller()->set_instant_url(instant_url); + ntp->set_instant_url("http://local_instant_url"); + ntp->set_supports_instant(true); + instant_controller()->set_in_startup(false); + EXPECT_FALSE(instant_controller()->ShouldSwitchToLocalNTP()); +} + +TEST_F(InstantControllerTest, SwitchesToLocalNTPIfJSDisabled) { + std::string instant_url("http://instant_url"); + scoped_ptr<TestableInstantNTP> ntp(new TestableInstantNTP( + instant_controller(), instant_url)); + ntp.reset(new TestableInstantNTP(instant_controller(), instant_url)); + ntp->set_is_local(false); + instant_controller()->set_ntp(ntp.get()); + instant_controller()->set_javascript_enabled(false); + instant_controller()->set_extended_enabled(true); + instant_controller()->set_instant_url(instant_url); + ntp->set_instant_url("http://local_instant_url"); + ntp->set_supports_instant(true); + instant_controller()->set_in_startup(false); + EXPECT_TRUE(instant_controller()->ShouldSwitchToLocalNTP()); +} + +TEST_F(InstantControllerTest, SwitchesToLocalNTPIfNoNTPReady) { + EXPECT_TRUE(instant_controller()->ShouldSwitchToLocalNTP()); +} + +TEST_F(InstantControllerTest, IsJavascriptEnabled) { + instant_controller()->set_override_javascript_enabled(false); + EXPECT_TRUE(instant_controller()->IsJavascriptEnabled()); +} + +TEST_F(InstantControllerTest, IsJavascriptEnabledChecksContentSettings) { + instant_controller()->set_override_javascript_enabled(false); + instant_controller()->profile()->GetHostContentSettingsMap() + ->SetDefaultContentSetting(CONTENT_SETTINGS_TYPE_JAVASCRIPT, + CONTENT_SETTING_DEFAULT); + EXPECT_TRUE(instant_controller()->IsJavascriptEnabled()); + instant_controller()->profile()->GetHostContentSettingsMap() + ->SetDefaultContentSetting(CONTENT_SETTINGS_TYPE_JAVASCRIPT, + CONTENT_SETTING_ALLOW); + EXPECT_TRUE(instant_controller()->IsJavascriptEnabled()); + instant_controller()->profile()->GetHostContentSettingsMap() + ->SetDefaultContentSetting(CONTENT_SETTINGS_TYPE_JAVASCRIPT, + CONTENT_SETTING_BLOCK); + EXPECT_FALSE(instant_controller()->IsJavascriptEnabled()); +} + +TEST_F(InstantControllerTest, IsJavascriptEnabledChecksPrefs) { + instant_controller()->set_override_javascript_enabled(false); + instant_controller()->profile()->GetPrefs()->SetBoolean( + prefs::kWebKitJavascriptEnabled, true); + EXPECT_TRUE(instant_controller()->IsJavascriptEnabled()); + instant_controller()->profile()->GetPrefs()->SetBoolean( + prefs::kWebKitJavascriptEnabled, false); + EXPECT_FALSE(instant_controller()->IsJavascriptEnabled()); +} diff --git a/chrome/browser/ui/search/instant_page.cc b/chrome/browser/ui/search/instant_page.cc index 8568d2e..356ecd0 100644 --- a/chrome/browser/ui/search/instant_page.cc +++ b/chrome/browser/ui/search/instant_page.cc @@ -25,6 +25,10 @@ bool InstantPage::supports_instant() const { return supports_instant_; } +const std::string& InstantPage::instant_url() const { + return instant_url_; +} + bool InstantPage::IsLocal() const { return contents() && (contents()->GetURL() == GURL(chrome::kChromeSearchLocalNtpUrl) || diff --git a/chrome/browser/ui/search/instant_page.h b/chrome/browser/ui/search/instant_page.h index 5bb3045..341defc 100644 --- a/chrome/browser/ui/search/instant_page.h +++ b/chrome/browser/ui/search/instant_page.h @@ -111,7 +111,7 @@ class InstantPage : public content::WebContentsObserver { // Returns the Instant URL that was loaded for this page. Returns the empty // string if no URL was explicitly loaded as is the case for InstantTab. - const std::string& instant_url() const { return instant_url_; } + virtual const std::string& instant_url() const; // Returns true if the page is known to support the Instant API. This starts // out false, and is set to true whenever we get any message from the page. diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index d6c2d91..1658339 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -15001,6 +15001,7 @@ other types of suffix sets. <int value="3" label="Page not current: origin/path mismatch"/> <int value="4" label="Page not current: instant not supported"/> <int value="5" label="No overlay"/> + <int value="6" label="Javascript disabled"/> </enum> <enum name="InstantExtended_InstantNavigation" type="int"> |