diff options
Diffstat (limited to 'chrome/browser')
10 files changed, 710 insertions, 168 deletions
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc index 8f76ba8..53ab39b 100644 --- a/chrome/browser/chrome_browser_main.cc +++ b/chrome/browser/chrome_browser_main.cc @@ -1402,9 +1402,6 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() { RecordPreReadExperimentTime("Startup.BrowserOpenTabs", base::TimeTicks::Now() - browser_open_start); - // TODO(mad): Move this call in a proper place on CrOS. - // http://crosbug.com/17687 -#if !defined(OS_CHROMEOS) // If we're running tests (ui_task is non-null), then we don't want to // call FetchLanguageListFromTranslateServer or // StartRepeatedVariationsSeedFetch. @@ -1412,13 +1409,15 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() { // Request new variations seed information from server. browser_process_->variations_service()-> StartRepeatedVariationsSeedFetch(); - +#if !defined(OS_CHROMEOS) + // TODO(mad): Move this call in a proper place on CrOS. + // http://crosbug.com/17687 if (translate_manager_ != NULL) { translate_manager_->FetchLanguageListFromTranslateServer( profile_->GetPrefs()); } - } #endif + } run_message_loop_ = true; } else { diff --git a/chrome/browser/chromeos/login/wizard_controller.cc b/chrome/browser/chromeos/login/wizard_controller.cc index 09b4ed8..4943028 100644 --- a/chrome/browser/chromeos/login/wizard_controller.cc +++ b/chrome/browser/chromeos/login/wizard_controller.cc @@ -46,6 +46,7 @@ #include "chrome/common/pref_names.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/notification_service.h" +#include "content/public/browser/notification_source.h" #include "content/public/browser/notification_types.h" #include "ui/base/accelerators/accelerator.h" #include "ui/base/l10n/l10n_util.h" @@ -462,6 +463,12 @@ void WizardController::OnEulaAccepted() { MarkEulaAccepted(); bool enabled = OptionsUtil::ResolveMetricsReportingEnabled(usage_statistics_reporting_); + + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED, + content::NotificationSource(content::Source<WizardController>(this)), + content::NotificationService::NoDetails()); + CrosSettings::Get()->SetBoolean(kStatsReportingPref, enabled); if (enabled) { #if defined(USE_LINUX_BREAKPAD) diff --git a/chrome/browser/metrics/variations/resource_request_allowed_notifier.cc b/chrome/browser/metrics/variations/resource_request_allowed_notifier.cc new file mode 100644 index 0000000..302df10 --- /dev/null +++ b/chrome/browser/metrics/variations/resource_request_allowed_notifier.cc @@ -0,0 +1,131 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/metrics/variations/resource_request_allowed_notifier.h" + +#include "base/metrics/histogram.h" +#include "chrome/common/chrome_notification_types.h" +#include "content/public/browser/notification_service.h" + +#if defined(OS_CHROMEOS) +#include "chrome/browser/chromeos/login/wizard_controller.h" +#endif + +ResourceRequestAllowedNotifier::ResourceRequestAllowedNotifier() + : observer_requested_permission_(false), + was_waiting_for_network_(false), +#if defined(OS_CHROMEOS) + was_waiting_for_user_to_accept_eula_(false), +#endif + observer_(NULL) { +} + +ResourceRequestAllowedNotifier::~ResourceRequestAllowedNotifier() { + if (observer_) + net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this); +} + +void ResourceRequestAllowedNotifier::Init(Observer* observer) { + DCHECK(!observer_ && observer); + observer_ = observer; + + net::NetworkChangeNotifier::AddConnectionTypeObserver(this); + + // Check this state during initialization. It is not expected to change until + // the corresponding notification is received. + was_waiting_for_network_ = net::NetworkChangeNotifier::IsOffline(); +#if defined(OS_CHROMEOS) + was_waiting_for_user_to_accept_eula_ = NeedsEulaAcceptance(); + if (was_waiting_for_user_to_accept_eula_) { + // Note that this must listen on AllSources due to the difficulty in knowing + // when the WizardController instance is created, and to avoid over-coupling + // the Chrome OS code with the VariationsService by directly attaching as an + // observer. This is OK because WizardController is essentially a singleton. + registrar_.Add(this, chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED, + content::NotificationService::AllSources()); + } +#endif +} + +bool ResourceRequestAllowedNotifier::ResourceRequestsAllowed() { + // The observer requested permission. Return the current criteria state and + // set a flag to remind this class to notify the observer once the criteria + // is met. + observer_requested_permission_ = true; + return +#if defined(OS_CHROMEOS) + !was_waiting_for_user_to_accept_eula_ && +#endif + !was_waiting_for_network_; +} + +void ResourceRequestAllowedNotifier:: + SetWasWaitingForNetworkForTesting(bool waiting) { + was_waiting_for_network_ = waiting; +} + +#if defined(OS_CHROMEOS) +void ResourceRequestAllowedNotifier:: + SetWasWaitingForEulaForTesting(bool waiting) { + was_waiting_for_user_to_accept_eula_ = waiting; +} +#endif + +void ResourceRequestAllowedNotifier::MaybeNotifyObserver() { + // Need to ensure that all criteria are met before notifying observers. + if (observer_requested_permission_ && ResourceRequestsAllowed()) { + DVLOG(1) << "Notifying observer of state change."; + observer_->OnResourceRequestsAllowed(); + // Reset this so the observer is not informed again unless they check + // ResourceRequestsAllowed again. + observer_requested_permission_ = false; + } +} + +#if defined(OS_CHROMEOS) +bool ResourceRequestAllowedNotifier::NeedsEulaAcceptance() { +#if defined(GOOGLE_CHROME_BUILD) + return !chromeos::WizardController::IsEulaAccepted(); +#else + // On unofficial builds, there is no notion of a EULA. + return false; +#endif +} +#endif + +void ResourceRequestAllowedNotifier::OnConnectionTypeChanged( + net::NetworkChangeNotifier::ConnectionType type) { + // Only attempt to notify observers if this was previously waiting for the + // network to reconnect, and new network state is actually available. This + // prevents the notifier from notifying the observer if the notifier was never + // waiting on the network, or if the network changes from one online state + // to another (for example, Wifi to 3G, or Wifi to Wifi, if the network were + // flaky). + if (was_waiting_for_network_ && + type != net::NetworkChangeNotifier::CONNECTION_NONE) { + was_waiting_for_network_ = false; + DVLOG(1) << "Network came back online."; + MaybeNotifyObserver(); + } +} + +#if defined(OS_CHROMEOS) +void ResourceRequestAllowedNotifier::Observe( + int type, + const content::NotificationSource& source, + const content::NotificationDetails& details) { + DCHECK_EQ(chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED, type); + // This should only ever be received once. Remove it after this call. + DCHECK(!registrar_.IsEmpty()); + registrar_.Remove(this, chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED, + content::NotificationService::AllSources()); + + // This flag should have been set if this was waiting on the EULA + // notification. + DCHECK(was_waiting_for_user_to_accept_eula_); + DVLOG(1) << "EULA was accepted."; + was_waiting_for_user_to_accept_eula_ = false; + MaybeNotifyObserver(); +} +#endif diff --git a/chrome/browser/metrics/variations/resource_request_allowed_notifier.h b/chrome/browser/metrics/variations/resource_request_allowed_notifier.h new file mode 100644 index 0000000..6a7241e --- /dev/null +++ b/chrome/browser/metrics/variations/resource_request_allowed_notifier.h @@ -0,0 +1,117 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_METRICS_VARIATIONS_RESOURCE_REQUEST_ALLOWED_NOTIFIER_H_ +#define CHROME_BROWSER_METRICS_VARIATIONS_RESOURCE_REQUEST_ALLOWED_NOTIFIER_H_ + +#include "base/observer_list.h" +#include "net/base/network_change_notifier.h" + +#if defined(OS_CHROMEOS) +#include "content/public/browser/notification_observer.h" +#include "content/public/browser/notification_registrar.h" +#endif + +// This class informs an interested observer when resource requests over the +// network are permitted. +// +// Currently, the criteria for allowing resource requests include: +// 1. The network is currently available. +// 2. The EULA was accepted by the user (ChromeOS only). +// +// Interested services should add themselves as an observer of +// ResourceRequestAllowedNotifier and check ResourceRequestsAllowed() to see if +// requests are permitted. If it returns true, they can go ahead and make their +// request. If it returns false, ResourceRequestAllowedNotifier will notify the +// service when the criteria is met. +// +// If ResourceRequestsAllowed returns true the first time, +// ResourceRequestAllowedNotifier will not notify the service in the future. +// +// Note that this class handles the criteria state for a single service, so +// services should keep their own instance of this class rather than sharing a +// global instance. +class ResourceRequestAllowedNotifier : +#if defined(OS_CHROMEOS) + public content::NotificationObserver, +#endif + public net::NetworkChangeNotifier::ConnectionTypeObserver { + public: + // Observes resource request allowed state changes. + class Observer { + public: + virtual void OnResourceRequestsAllowed() = 0; + }; + + ResourceRequestAllowedNotifier(); + virtual ~ResourceRequestAllowedNotifier(); + + // Sets |observer| as the service to be notified by this instance, and + // performs initial checks on the criteria. |observer| may not be NULL. + // This is to be called immediately after construction of an instance of + // ResourceRequestAllowedNotifier to pass it the interested service. + void Init(Observer* observer); + + // Returns true iff all resource request criteria are met. If not, this call + // will set some flags so it knows to notify the observer if the criteria + // changes. Note that the observer will never be notified unless it calls this + // method first. This is virtual so it can be overriden for tests. + virtual bool ResourceRequestsAllowed(); + + void SetWasWaitingForNetworkForTesting(bool waiting); +#if defined(OS_CHROMEOS) + void SetWasWaitingForEulaForTesting(bool waiting); +#endif + + protected: + // Notifies the observer if all criteria needed for resource requests are met. + // This is protected so it can be called from subclasses for testing. + void MaybeNotifyObserver(); + +#if defined(OS_CHROMEOS) + // On official builds, returns true iff the EULA needs to be accepted. This + // always returns false on unofficial builds since there is no notion of a + // EULA. + // + // This is virtual so it can be overriden by test classes to avoid making them + // aware of the ChromeOS details. This is protected so it call be overriden in + // subclasses for testing. + virtual bool NeedsEulaAcceptance(); +#endif + + private: + // net::NetworkChangeNotifier::ConnectionTypeObserver overrides: + virtual void OnConnectionTypeChanged( + net::NetworkChangeNotifier::ConnectionType type) OVERRIDE; + +#if defined(OS_CHROMEOS) + // content::NotificationObserver overrides: + virtual void Observe(int type, + const content::NotificationSource& source, + const content::NotificationDetails& details) OVERRIDE; +#endif + + // Tracks whether or not the observer/service depending on this class actually + // requested permission to make a request or not. If it did not, then this + // class should not notify it even if the criteria is met. + bool observer_requested_permission_; + + // Tracks network connectivity criteria. + bool was_waiting_for_network_; + +#if defined(OS_CHROMEOS) + // Tracks EULA acceptance criteria. + bool was_waiting_for_user_to_accept_eula_; + + // Used to listen for the EULA accepted notification. + content::NotificationRegistrar registrar_; +#endif + + // Observing service interested in request permissions. + Observer* observer_; + + DISALLOW_COPY_AND_ASSIGN(ResourceRequestAllowedNotifier); +}; + +#endif // CHROME_BROWSER_METRICS_VARIATIONS_RESOURCE_REQUEST_ALLOWED_NOTIFIER_H_ diff --git a/chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc b/chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc new file mode 100644 index 0000000..665cf86 --- /dev/null +++ b/chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc @@ -0,0 +1,52 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h" + +TestRequestAllowedNotifier::TestRequestAllowedNotifier() : +#if defined(OS_CHROMEOS) + needs_eula_acceptance_(true), +#endif + override_requests_allowed_(false), + requests_allowed_(true) { +} + +TestRequestAllowedNotifier::~TestRequestAllowedNotifier() { +} + +#if defined(OS_CHROMEOS) +void TestRequestAllowedNotifier::SetNeedsEulaAcceptance(bool needs_acceptance) { + needs_eula_acceptance_ = needs_acceptance; +} +#endif + +void TestRequestAllowedNotifier::SetRequestsAllowedOverride(bool allowed) { + override_requests_allowed_ = true; + requests_allowed_ = allowed; +} + +void TestRequestAllowedNotifier::NotifyObserver() { + // Force the allowed state to true. This forces MaybeNotifyObserver to always + // notify observers, as MaybeNotifyObserver checks ResourceRequestsAllowed. + override_requests_allowed_ = true; + requests_allowed_ = true; + MaybeNotifyObserver(); +} + +bool TestRequestAllowedNotifier::ResourceRequestsAllowed() { + // Call ResourceRequestAllowedNotifier::ResourceRequestsAllowed once to + // simulate that the user requested permission. Only return that result if + // the override flag was set. + bool requests_allowed = + ResourceRequestAllowedNotifier::ResourceRequestsAllowed(); + if (override_requests_allowed_) + return requests_allowed_; + return requests_allowed; +} + +#if defined(OS_CHROMEOS) +bool TestRequestAllowedNotifier::NeedsEulaAcceptance() { + return needs_eula_acceptance_; +} +#endif diff --git a/chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h b/chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h new file mode 100644 index 0000000..06222fb --- /dev/null +++ b/chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h @@ -0,0 +1,52 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_METRICS_VARIATIONS_RESOURCE_REQUEST_ALLOWED_NOTIFIER_TEST_UTIL_H_ +#define CHROME_BROWSER_METRICS_VARIATIONS_RESOURCE_REQUEST_ALLOWED_NOTIFIER_TEST_UTIL_H_ + +#include "chrome/browser/metrics/variations/resource_request_allowed_notifier.h" + +// A subclass of ResourceRequestAllowedNotifier used to expose some +// functionality for testing. +// +// By default, the constructor sets this class to override +// ResourceRequestsAllowed, so its state can be set with SetRequestsAllowed. +// This is meant for higher level tests of services to ensure they adhere to the +// notifications of the ResourceRequestAllowedNotifier. Lower level tests can +// disable this by calling SetRequestsAllowedOverride with the value they want +// it to return. +class TestRequestAllowedNotifier : public ResourceRequestAllowedNotifier { + public: + TestRequestAllowedNotifier(); + virtual ~TestRequestAllowedNotifier(); + +#if defined(OS_CHROMEOS) + void SetNeedsEulaAcceptance(bool needs_acceptance); +#endif + + // Makes ResourceRequestsAllowed return |allowed| when it is called. + void SetRequestsAllowedOverride(bool allowed); + + // Notify observers that requests are allowed. This will only work if + // the observer is expecting a notification. + void NotifyObserver(); + + virtual bool ResourceRequestsAllowed() OVERRIDE; + + protected: +#if defined(OS_CHROMEOS) + virtual bool NeedsEulaAcceptance() OVERRIDE; +#endif + + private: +#if defined(OS_CHROMEOS) + bool needs_eula_acceptance_; +#endif + bool override_requests_allowed_; + bool requests_allowed_; + + DISALLOW_COPY_AND_ASSIGN(TestRequestAllowedNotifier); +}; + +#endif // CHROME_BROWSER_METRICS_VARIATIONS_RESOURCE_REQUEST_ALLOWED_NOTIFIER_TEST_UTIL_H_ diff --git a/chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc b/chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc new file mode 100644 index 0000000..782140e --- /dev/null +++ b/chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc @@ -0,0 +1,244 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h" +#include "chrome/common/chrome_notification_types.h" +#include "chrome/test/base/testing_browser_process.h" +#include "chrome/test/base/testing_pref_service.h" +#include "content/public/browser/notification_service.h" +#include "content/public/test/test_browser_thread.h" +#include "testing/gtest/include/gtest/gtest.h" + +// Override NetworkChangeNotifier to simulate connection type changes for tests. +class TestNetworkChangeNotifier : public net::NetworkChangeNotifier { + public: + TestNetworkChangeNotifier() + : net::NetworkChangeNotifier(), + connection_type_to_return_( + net::NetworkChangeNotifier::CONNECTION_UNKNOWN) { + } + + // Simulates a change of the connection type to |type|. This will notify any + // objects that are NetworkChangeNotifiers. + void SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::ConnectionType type) { + connection_type_to_return_ = type; + net::NetworkChangeNotifier::NotifyObserversOfConnectionTypeChange(); + MessageLoop::current()->RunAllPending(); + } + + private: + virtual ConnectionType GetCurrentConnectionType() const OVERRIDE { + return connection_type_to_return_; + } + + // The currently simulated network connection type. If this is set to + // CONNECTION_NONE, then NetworkChangeNotifier::IsOffline will return true. + net::NetworkChangeNotifier::ConnectionType connection_type_to_return_; + + DISALLOW_COPY_AND_ASSIGN(TestNetworkChangeNotifier); +}; + +// A test fixture class for ResourceRequestAllowedNotifier tests that require +// network state simulations. This also acts as the service implementing the +// ResourceRequestAllowedNotifier::Observer interface. +class ResourceRequestAllowedNotifierTest + : public testing::Test, + public ResourceRequestAllowedNotifier::Observer { + public: + ResourceRequestAllowedNotifierTest() + : ui_thread(content::BrowserThread::UI, &message_loop), + was_notified_(false) { +#if defined(OS_CHROMEOS) + // Set this flag to true so the Init call sets up the wait on the EULA. + SetNeedsEulaAcceptance(true); +#endif + resource_request_allowed_notifier_.Init(this); + } + ~ResourceRequestAllowedNotifierTest() { } + + bool was_notified() const { return was_notified_; } + + // ResourceRequestAllowedNotifier::Observer override: + virtual void OnResourceRequestsAllowed() OVERRIDE { + was_notified_ = true; + } + + // Network manipulation methods: + void SetWasWaitingForNetwork(bool waiting) { + resource_request_allowed_notifier_. + SetWasWaitingForNetworkForTesting(waiting); + } + + void SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::ConnectionType type) { + network_notifier.SimulateNetworkConnectionChange(type); + } + + // Simulate a resource request from the test service. + void SimulateResourceRequest() { + resource_request_allowed_notifier_.ResourceRequestsAllowed(); + } + +#if defined(OS_CHROMEOS) + // Eula manipulation methods: + void SetNeedsEulaAcceptance(bool needs_acceptance) { + resource_request_allowed_notifier_.SetNeedsEulaAcceptance(needs_acceptance); + } + + void SetWasWaitingForEula(bool waiting) { + resource_request_allowed_notifier_.SetWasWaitingForEulaForTesting(waiting); + } + + void SimulateEulaAccepted() { + SetNeedsEulaAcceptance(false); + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED, + content::NotificationService::AllSources(), + content::NotificationService::NoDetails()); + } + + // Used in tests involving the EULA. Disables both the EULA accepted state + // and the network. + void DisableEulaAndNetwork() { + SetWasWaitingForNetwork(true); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_NONE); + SetWasWaitingForEula(true); + SetNeedsEulaAcceptance(true); + } +#endif + + virtual void SetUp() OVERRIDE { + // Assume the test service has already requested permission, as all tests + // just test that criteria changes notify the server. +#if defined(OS_CHROMEOS) + // Set default EULA state to done (not waiting and EULA accepted) to + // simplify non-ChromeOS tests. + SetWasWaitingForEula(false); + SetNeedsEulaAcceptance(false); +#endif + } + + private: + MessageLoopForUI message_loop; + content::TestBrowserThread ui_thread; + TestNetworkChangeNotifier network_notifier; + TestRequestAllowedNotifier resource_request_allowed_notifier_; + bool was_notified_; + + DISALLOW_COPY_AND_ASSIGN(ResourceRequestAllowedNotifierTest); +}; + +TEST_F(ResourceRequestAllowedNotifierTest, DoNotNotifyIfOffline) { + SimulateResourceRequest(); + SetWasWaitingForNetwork(true); + SimulateNetworkConnectionChange(net::NetworkChangeNotifier::CONNECTION_NONE); + EXPECT_FALSE(was_notified()); +} + +TEST_F(ResourceRequestAllowedNotifierTest, DoNotNotifyIfOnlineToOnline) { + SimulateResourceRequest(); + SetWasWaitingForNetwork(false); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_ETHERNET); + EXPECT_FALSE(was_notified()); +} + +TEST_F(ResourceRequestAllowedNotifierTest, NotifyOnReconnect) { + SimulateResourceRequest(); + SetWasWaitingForNetwork(true); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_ETHERNET); + EXPECT_TRUE(was_notified()); +} + +TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnWardriving) { + SimulateResourceRequest(); + SetWasWaitingForNetwork(false); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_WIFI); + EXPECT_FALSE(was_notified()); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_3G); + EXPECT_FALSE(was_notified()); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_4G); + EXPECT_FALSE(was_notified()); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_WIFI); + EXPECT_FALSE(was_notified()); +} + +TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnFlakyConnection) { + SimulateResourceRequest(); + SetWasWaitingForNetwork(false); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_WIFI); + EXPECT_FALSE(was_notified()); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_NONE); + EXPECT_FALSE(was_notified()); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_WIFI); + EXPECT_FALSE(was_notified()); +} + +TEST_F(ResourceRequestAllowedNotifierTest, NoRequestNoNotify) { + // Ensure that if the observing service does not request access, it does not + // get notified, even if the criteria is met. Note that this is done by not + // calling SimulateResourceRequest here. + SetWasWaitingForNetwork(true); + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_ETHERNET); + EXPECT_FALSE(was_notified()); +} + +#if defined(OS_CHROMEOS) +TEST_F(ResourceRequestAllowedNotifierTest, EulaOnlyNetworkOffline) { + SimulateResourceRequest(); + DisableEulaAndNetwork(); + + SimulateEulaAccepted(); + EXPECT_FALSE(was_notified()); +} + +TEST_F(ResourceRequestAllowedNotifierTest, EulaFirst) { + SimulateResourceRequest(); + DisableEulaAndNetwork(); + + SimulateEulaAccepted(); + EXPECT_FALSE(was_notified()); + + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_WIFI); + EXPECT_TRUE(was_notified()); +} + +TEST_F(ResourceRequestAllowedNotifierTest, NetworkFirst) { + SimulateResourceRequest(); + DisableEulaAndNetwork(); + + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_WIFI); + EXPECT_FALSE(was_notified()); + + SimulateEulaAccepted(); + EXPECT_TRUE(was_notified()); +} + +TEST_F(ResourceRequestAllowedNotifierTest, NoRequestNoNotifyEula) { + // Ensure that if the observing service does not request access, it does not + // get notified, even if the criteria is met. Note that this is done by not + // calling SimulateResourceRequest here. + DisableEulaAndNetwork(); + + SimulateNetworkConnectionChange( + net::NetworkChangeNotifier::CONNECTION_WIFI); + EXPECT_FALSE(was_notified()); + + SimulateEulaAccepted(); + EXPECT_FALSE(was_notified()); +} +#endif // OS_CHROMEOS diff --git a/chrome/browser/metrics/variations/variations_service.cc b/chrome/browser/metrics/variations/variations_service.cc index 2b95bf6..131f646 100644 --- a/chrome/browser/metrics/variations/variations_service.cc +++ b/chrome/browser/metrics/variations/variations_service.cc @@ -11,7 +11,6 @@ #include "base/command_line.h" #include "base/memory/scoped_ptr.h" #include "base/metrics/field_trial.h" -#include "base/metrics/histogram.h" #include "base/version.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/metrics/proto/trials_seed.pb.h" @@ -100,12 +99,19 @@ GURL GetVariationsServerURL() { VariationsService::VariationsService() : variations_server_url_(GetVariationsServerURL()), create_trials_from_seed_called_(false), - was_offline_during_last_request_attempt_(false) { - net::NetworkChangeNotifier::AddConnectionTypeObserver(this); + resource_request_allowed_notifier_( + new ResourceRequestAllowedNotifier) { + resource_request_allowed_notifier_->Init(this); +} + +VariationsService::VariationsService(ResourceRequestAllowedNotifier* notifier) + : variations_server_url_(GetVariationsServerURL()), + create_trials_from_seed_called_(false), + resource_request_allowed_notifier_(notifier) { + resource_request_allowed_notifier_->Init(this); } VariationsService::~VariationsService() { - net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this); } bool VariationsService::CreateTrialsFromSeed(PrefService* local_prefs) { @@ -151,18 +157,18 @@ void VariationsService::StartRepeatedVariationsSeedFetch() { this, &VariationsService::FetchVariationsSeed); } -void VariationsService::FetchVariationsSeed() { - DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); +// static +void VariationsService::RegisterPrefs(PrefService* prefs) { + prefs->RegisterStringPref(prefs::kVariationsSeed, std::string()); + prefs->RegisterInt64Pref(prefs::kVariationsSeedDate, + base::Time().ToInternalValue()); +} - was_offline_during_last_request_attempt_ = - net::NetworkChangeNotifier::IsOffline(); - UMA_HISTOGRAM_BOOLEAN("Variations.NetworkAvailability", - !was_offline_during_last_request_attempt_); - if (was_offline_during_last_request_attempt_) { - DVLOG(1) << "Network was offline."; - return; - } +void VariationsService::SetCreateTrialsFromSeedCalledForTesting(bool called) { + create_trials_from_seed_called_ = called; +} +void VariationsService::DoActualFetch() { pending_seed_request_.reset(net::URLFetcher::Create( variations_server_url_, net::URLFetcher::GET, this)); pending_seed_request_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES | @@ -177,16 +183,15 @@ void VariationsService::FetchVariationsSeed() { pending_seed_request_->Start(); } -void VariationsService::SetWasOfflineDuringLastRequestAttemptForTesting( - bool offline) { - was_offline_during_last_request_attempt_ = offline; -} +void VariationsService::FetchVariationsSeed() { + DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); -// static -void VariationsService::RegisterPrefs(PrefService* prefs) { - prefs->RegisterStringPref(prefs::kVariationsSeed, std::string()); - prefs->RegisterInt64Pref(prefs::kVariationsSeedDate, - base::Time().ToInternalValue()); + if (!resource_request_allowed_notifier_->ResourceRequestsAllowed()) { + DVLOG(1) << "Resource requests were not allowed. Waiting for notification."; + return; + } + + DoActualFetch(); } void VariationsService::OnURLFetchComplete(const net::URLFetcher* source) { @@ -216,24 +221,17 @@ void VariationsService::OnURLFetchComplete(const net::URLFetcher* source) { StoreSeedData(seed_data, response_date, g_browser_process->local_state()); } -void VariationsService::OnConnectionTypeChanged( - net::NetworkChangeNotifier::ConnectionType type) { - // If the connection type is back online, start a request if the last request - // failed due to being offline. - if (was_offline_during_last_request_attempt_ && - type != net::NetworkChangeNotifier::CONNECTION_NONE) { - VLOG(1) << "Retrying fetch due to network reconnect."; - FetchVariationsSeed(); - - // Since FetchVariationsSeed was explicitly called here, reset the timer to - // avoid retrying for a full period. - // net::NetworkChangeNotifier::IsOffline may be inconsistent with |type|, so - // we check if FetchVariationsSeed set - // |was_offline_during_last_request_attempt_| to true before we reset the - // timer. - if (!was_offline_during_last_request_attempt_ && timer_.IsRunning()) - timer_.Reset(); - } +void VariationsService::OnResourceRequestsAllowed() { + // Note that this only attempts to fetch the seed at most once per period + // (kSeedFetchPeriodHours). This works because + // |resource_request_allowed_notifier_| only calls this method if an + // attempt was made earlier that fails (which implies that the period had + // elapsed). After a successful attempt is made, the notifier will know not + // to call this method again until another failed attempt occurs. + DVLOG(1) << "Retrying fetch."; + DoActualFetch(); + if (timer_.IsRunning()) + timer_.Reset(); } bool VariationsService::StoreSeedData(const std::string& seed_data, diff --git a/chrome/browser/metrics/variations/variations_service.h b/chrome/browser/metrics/variations/variations_service.h index 3f34121..4f55d4a 100644 --- a/chrome/browser/metrics/variations/variations_service.h +++ b/chrome/browser/metrics/variations/variations_service.h @@ -15,9 +15,9 @@ #include "base/timer.h" #include "chrome/browser/metrics/proto/study.pb.h" #include "chrome/browser/metrics/proto/trials_seed.pb.h" +#include "chrome/browser/metrics/variations/resource_request_allowed_notifier.h" #include "chrome/common/chrome_version_info.h" #include "googleurl/src/gurl.h" -#include "net/base/network_change_notifier.h" #include "net/url_request/url_fetcher_delegate.h" class PrefService; @@ -28,9 +28,14 @@ namespace chrome_variations { // new seed data from the variations server. class VariationsService : public net::URLFetcherDelegate, - public net::NetworkChangeNotifier::ConnectionTypeObserver{ + public ResourceRequestAllowedNotifier::Observer { public: VariationsService(); + + // This constructor exists for injecting a mock notifier. It is meant for + // testing only. This instance will take ownership of |notifier|. + explicit VariationsService(ResourceRequestAllowedNotifier* notifier); + virtual ~VariationsService(); // Creates field trials based on Variations Seed loaded from local prefs. If @@ -43,17 +48,16 @@ class VariationsService // |CreateTrialsFromSeed|. void StartRepeatedVariationsSeedFetch(); - // Starts the fetching process once, where |OnURLFetchComplete| is called with - // the response. If the network is down at the time, sets a flag to retry when - // the network is back online. This is virtual so it can be overriden for - // testing. - virtual void FetchVariationsSeed(); + // Register Variations related prefs in Local State. + static void RegisterPrefs(PrefService* prefs); // Exposed for testing. - void SetWasOfflineDuringLastRequestAttemptForTesting(bool offline); + void SetCreateTrialsFromSeedCalledForTesting(bool called); - // Register Variations related prefs in Local State. - static void RegisterPrefs(PrefService* prefs); + protected: + // Starts the fetching process once, where |OnURLFetchComplete| is called with + // the response. + virtual void DoActualFetch(); private: FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, CheckStudyChannel); @@ -70,12 +74,15 @@ class VariationsService FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, StoreSeed); FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, ValidateStudy); + // Checks if prerequisites for fetching the Variations seed are met, and if + // so, performs the actual fetch using |DoActualFetch|. + void FetchVariationsSeed(); + // net::URLFetcherDelegate implementation: virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE; - // net::NetworkChangeNotifier::ConnectionTypeObserver implementation. - virtual void OnConnectionTypeChanged( - net::NetworkChangeNotifier::ConnectionType type) OVERRIDE; + // ResourceRequestAllowedNotifier::Observer implementation: + virtual void OnResourceRequestsAllowed() OVERRIDE; // Store the given seed data to the given local prefs. Note that |seed_data| // is assumed to be the raw serialized protobuf data stored in a string. It @@ -146,14 +153,14 @@ class VariationsService // it gets called prior to |StartRepeatedVariationsSeedFetch|. bool create_trials_from_seed_called_; - // Tracks whether or not the last seed request attempt failed due to being - // offline. - bool was_offline_during_last_request_attempt_; - // The timer used to repeatedly ping the server. Keep this as an instance // member so if VariationsService goes out of scope, the timer is // automatically canceled. base::RepeatingTimer<VariationsService> timer_; + + // Helper class used to tell this service if it's allowed to make network + // resource requests. + scoped_ptr<ResourceRequestAllowedNotifier> resource_request_allowed_notifier_; }; } // namespace chrome_variations diff --git a/chrome/browser/metrics/variations/variations_service_unittest.cc b/chrome/browser/metrics/variations/variations_service_unittest.cc index 0de0800..c18c702 100644 --- a/chrome/browser/metrics/variations/variations_service_unittest.cc +++ b/chrome/browser/metrics/variations/variations_service_unittest.cc @@ -5,9 +5,11 @@ #include "base/base64.h" #include "base/string_split.h" #include "chrome/browser/metrics/proto/study.pb.h" +#include "chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h" #include "chrome/browser/metrics/variations/variations_service.h" #include "chrome/common/chrome_version_info.h" #include "chrome/common/pref_names.h" +#include "chrome/test/base/testing_browser_process.h" #include "chrome/test/base/testing_pref_service.h" #include "content/public/test/test_browser_thread.h" #include "testing/gtest/include/gtest/gtest.h" @@ -19,16 +21,20 @@ namespace { // A test class used to validate expected functionality in VariationsService. class TestVariationsService : public VariationsService { public: - TestVariationsService() : VariationsService(), - fetch_attempted_(false) { + explicit TestVariationsService(TestRequestAllowedNotifier* test_notifier) + : VariationsService(test_notifier), + fetch_attempted_(false) { + // Set this so StartRepeatedVariationsSeedFetch can be called in tests. + SetCreateTrialsFromSeedCalledForTesting(true); + } + + virtual ~TestVariationsService() { } - virtual ~TestVariationsService() {} bool fetch_attempted() const { return fetch_attempted_; } - void SetFetchAttempted(bool attempted) { fetch_attempted_ = attempted; } protected: - virtual void FetchVariationsSeed() OVERRIDE { + virtual void DoActualFetch() OVERRIDE { fetch_attempted_ = true; } @@ -38,32 +44,6 @@ class TestVariationsService : public VariationsService { DISALLOW_COPY_AND_ASSIGN(TestVariationsService); }; -// Override NetworkChangeNotifier to simulate connection type changes for tests. -class TestNetworkChangeNotifier : public net::NetworkChangeNotifier { - public: - TestNetworkChangeNotifier() - : net::NetworkChangeNotifier(), - connection_type_to_return_( - net::NetworkChangeNotifier::CONNECTION_UNKNOWN) { - } - - void SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::ConnectionType type) { - connection_type_to_return_ = type; - net::NetworkChangeNotifier::NotifyObserversOfConnectionTypeChange(); - MessageLoop::current()->RunAllPending(); - } - - private: - virtual ConnectionType GetCurrentConnectionType() const OVERRIDE { - return connection_type_to_return_; - } - - net::NetworkChangeNotifier::ConnectionType connection_type_to_return_; - - DISALLOW_COPY_AND_ASSIGN(TestNetworkChangeNotifier); -}; - // Converts |time| to Study proto format. int64 TimeToProtoTime(const base::Time& time) { return (time - base::Time::UnixEpoch()).InSeconds(); @@ -86,36 +66,6 @@ TrialsSeed CreateTestSeed() { } // namespace -// A test fixture class for VariationsService tests that require network state -// simulations. -class VariationsServiceNetworkTest : public testing::Test { - public: - VariationsServiceNetworkTest() - : ui_thread(content::BrowserThread::UI, &message_loop) { } - ~VariationsServiceNetworkTest() { } - - void SetWasOfflineDuringLastRequestAttempt(bool offline) { - test_service.SetWasOfflineDuringLastRequestAttemptForTesting(offline); - } - - void SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::ConnectionType type) { - notifier.SimulateNetworkConnectionChange(type); - } - - bool fetch_attempted() const { - return test_service.fetch_attempted(); - } - - private: - MessageLoopForUI message_loop; - content::TestBrowserThread ui_thread; - TestNetworkChangeNotifier notifier; - TestVariationsService test_service; - - DISALLOW_COPY_AND_ASSIGN(VariationsServiceNetworkTest); -}; - TEST(VariationsServiceTest, CheckStudyChannel) { const chrome::VersionInfo::Channel channels[] = { chrome::VersionInfo::CHANNEL_CANARY, @@ -401,7 +351,7 @@ TEST(VariationsServiceTest, LoadSeed) { ASSERT_TRUE(base::Base64Encode(serialized_seed, &base64_serialized_seed)); pref_service.SetString(prefs::kVariationsSeed, base64_serialized_seed); - VariationsService variations_service; + TestVariationsService variations_service(new TestRequestAllowedNotifier); TrialsSeed loaded_seed; EXPECT_TRUE( variations_service.LoadTrialsSeedFromPref(&pref_service, &loaded_seed)); @@ -435,7 +385,7 @@ TEST(VariationsServiceTest, StoreSeed) { TrialsSeed seed = CreateTestSeed(); - VariationsService variations_service; + TestVariationsService variations_service(new TestRequestAllowedNotifier); std::string serialized_seed; seed.SerializeToString(&serialized_seed); EXPECT_TRUE( @@ -534,53 +484,38 @@ TEST(VariationsServiceTest, ValidateStudy) { EXPECT_FALSE(valid); } -TEST_F(VariationsServiceNetworkTest, DoNotFetchIfOffline) { - SetWasOfflineDuringLastRequestAttempt(true); - SimulateNetworkConnectionChange(net::NetworkChangeNotifier::CONNECTION_NONE); - EXPECT_FALSE(fetch_attempted()); -} +TEST(VariationsServiceTest, RequestsInitiallyNotAllowed) { + MessageLoopForUI message_loop; + content::TestBrowserThread ui_thread(content::BrowserThread::UI, + &message_loop); -TEST_F(VariationsServiceNetworkTest, DoNotFetchIfOnlineToOnline) { - SetWasOfflineDuringLastRequestAttempt(false); - SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::CONNECTION_ETHERNET); - EXPECT_FALSE(fetch_attempted()); -} + // Pass ownership to TestVariationsService, but keep a weak pointer to + // manipulate it for this test. + TestRequestAllowedNotifier* test_notifier = new TestRequestAllowedNotifier; + TestVariationsService test_service(test_notifier); -TEST_F(VariationsServiceNetworkTest, FetchOnReconnect) { - SetWasOfflineDuringLastRequestAttempt(true); - SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::CONNECTION_ETHERNET); - EXPECT_TRUE(fetch_attempted()); -} + // Force the notifier to initially disallow requests. + test_notifier->SetRequestsAllowedOverride(false); + test_service.StartRepeatedVariationsSeedFetch(); + EXPECT_FALSE(test_service.fetch_attempted()); -TEST_F(VariationsServiceNetworkTest, NoFetchOnWardriving) { - SetWasOfflineDuringLastRequestAttempt(false); - SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::CONNECTION_WIFI); - EXPECT_FALSE(fetch_attempted()); - SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::CONNECTION_3G); - EXPECT_FALSE(fetch_attempted()); - SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::CONNECTION_4G); - EXPECT_FALSE(fetch_attempted()); - SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::CONNECTION_WIFI); - EXPECT_FALSE(fetch_attempted()); + test_notifier->NotifyObserver(); + EXPECT_TRUE(test_service.fetch_attempted()); } -TEST_F(VariationsServiceNetworkTest, NoFetchOnFlakyConnection) { - SetWasOfflineDuringLastRequestAttempt(false); - SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::CONNECTION_WIFI); - EXPECT_FALSE(fetch_attempted()); - SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::CONNECTION_NONE); - EXPECT_FALSE(fetch_attempted()); - SimulateNetworkConnectionChange( - net::NetworkChangeNotifier::CONNECTION_WIFI); - EXPECT_FALSE(fetch_attempted()); +TEST(VariationsServiceTest, RequestsInitiallyAllowed) { + MessageLoopForUI message_loop; + content::TestBrowserThread ui_thread(content::BrowserThread::UI, + &message_loop); + + // Pass ownership to TestVariationsService, but keep a weak pointer to + // manipulate it for this test. + TestRequestAllowedNotifier* test_notifier = new TestRequestAllowedNotifier; + TestVariationsService test_service(test_notifier); + + test_notifier->SetRequestsAllowedOverride(true); + test_service.StartRepeatedVariationsSeedFetch(); + EXPECT_TRUE(test_service.fetch_attempted()); } } // namespace chrome_variations |