From 4e82187e29a6ce0310013d98f49286f0ad7d8e7c Mon Sep 17 00:00:00 2001 From: "joth@chromium.org" Date: Thu, 14 Oct 2010 11:39:50 +0000 Subject: Add extra logging to help in debugging flaky geolocation browser tests. Refactor mock location provider to send new position to provider thread, rather than writing it into shared memory that was missing locking. BUG=None TEST=Geoloc* browser and unit tests. Review URL: http://codereview.chromium.org/3749004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@62535 0039d316-1c4b-4281-b951-d872f2087c98 --- .../browser/geolocation/geolocation_browsertest.cc | 10 ++- .../geolocation/location_arbitrator_unittest.cc | 95 ++++++++-------------- .../browser/geolocation/mock_location_provider.cc | 7 +- .../browser/geolocation/mock_location_provider.h | 2 +- 4 files changed, 48 insertions(+), 66 deletions(-) (limited to 'chrome') diff --git a/chrome/browser/geolocation/geolocation_browsertest.cc b/chrome/browser/geolocation/geolocation_browsertest.cc index 60b61cb..9efac98 100644 --- a/chrome/browser/geolocation/geolocation_browsertest.cc +++ b/chrome/browser/geolocation/geolocation_browsertest.cc @@ -173,8 +173,7 @@ class GeolocationNotificationObserver : public NotificationObserver { void NotifyGeoposition(const Geoposition& geoposition) { DCHECK(MockLocationProvider::instance_); - MockLocationProvider::instance_->position_ = geoposition; - MockLocationProvider::instance_->HandlePositionChanged(); + MockLocationProvider::instance_->HandlePositionChanged(geoposition); LOG(WARNING) << "MockLocationProvider listeners updated"; } @@ -332,6 +331,11 @@ class GeolocationBrowserTest : public InProcessBrowserTest { expected, function, current_browser_->GetSelectedTabContents()); } + // InProcessBrowserTest + virtual void TearDownInProcessBrowserTestFixture() { + LOG(WARNING) << "TearDownInProcessBrowserTestFixture. Test Finished."; + } + InfoBarDelegate* infobar_; Browser* current_browser_; // path element of a URL referencing the html content for this test. @@ -466,7 +470,9 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, IFramesWithFreshPosition) { iframe_xpath_ = L"//iframe[@id='iframe_1']"; // Infobar was displayed, allow access and check there's no error code. SetInfobarResponse(iframe1_url_, true); + LOG(WARNING) << "Checking position..."; CheckGeoposition(fresh_position); + LOG(WARNING) << "...done."; } diff --git a/chrome/browser/geolocation/location_arbitrator_unittest.cc b/chrome/browser/geolocation/location_arbitrator_unittest.cc index 888254c..7a76566 100644 --- a/chrome/browser/geolocation/location_arbitrator_unittest.cc +++ b/chrome/browser/geolocation/location_arbitrator_unittest.cc @@ -53,25 +53,6 @@ class MockProviderFactory : public GeolocationArbitrator::ProviderFactory { MockLocationProvider* gps_; }; -void SetPositionFix(Geoposition* position, - double latitude, - double longitude, - double accuracy, - const base::Time& timestamp) { - ASSERT_TRUE(position); - position->error_code = Geoposition::ERROR_CODE_NONE; - position->latitude = latitude; - position->longitude = longitude; - position->accuracy = accuracy; - position->timestamp = timestamp; - ASSERT_TRUE(position->IsInitialized()); - ASSERT_TRUE(position->IsValidFix()); -} - -void SetReferencePosition(Geoposition* position) { - SetPositionFix(position, 51.0, -0.1, 400, base::Time::FromDoubleT(54321.0)); -} - double g_fake_time_now_secs = 1; base::Time GetTimeNow() { @@ -82,6 +63,25 @@ void AdvanceTimeNow(const base::TimeDelta& delta) { g_fake_time_now_secs += delta.InSecondsF(); } +void SetPositionFix(MockLocationProvider* provider, + double latitude, + double longitude, + double accuracy) { + Geoposition position; + position.error_code = Geoposition::ERROR_CODE_NONE; + position.latitude = latitude; + position.longitude = longitude; + position.accuracy = accuracy; + position.timestamp = GetTimeNow(); + ASSERT_TRUE(position.IsInitialized()); + ASSERT_TRUE(position.IsValidFix()); + provider->HandlePositionChanged(position); +} + +void SetReferencePosition(MockLocationProvider* provider) { + SetPositionFix(provider, 51.0, -0.1, 400); +} + class GeolocationLocationArbitratorTest : public testing::Test { protected: virtual void SetUp() { @@ -158,8 +158,7 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) { EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, providers_->gps_->state_); EXPECT_FALSE(observer_->last_position_.IsInitialized()); - SetReferencePosition(&providers_->cell_->position_); - providers_->cell_->HandlePositionChanged(); + SetReferencePosition(providers_->cell_); EXPECT_TRUE(observer_->last_position_.IsInitialized()); EXPECT_EQ(providers_->cell_->position_.latitude, @@ -195,8 +194,7 @@ TEST_F(GeolocationLocationArbitratorTest, SetObserverOptionsAfterFixArrives) { ASSERT_TRUE(providers_->gps_); EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, providers_->cell_->state_); EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, providers_->gps_->state_); - SetReferencePosition(&providers_->cell_->position_); - providers_->cell_->HandlePositionChanged(); + SetReferencePosition(providers_->cell_); EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, providers_->cell_->state_); EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, providers_->gps_->state_); arbitrator_->SetObserverOptions(GeolocationObserverOptions(true)); @@ -209,61 +207,48 @@ TEST_F(GeolocationLocationArbitratorTest, Arbitration) { ASSERT_TRUE(providers_->cell_); ASSERT_TRUE(providers_->gps_); - SetPositionFix(&providers_->cell_->position_, 1, 2, 150, GetTimeNow()); - providers_->cell_->HandlePositionChanged(); + SetPositionFix(providers_->cell_, 1, 2, 150); // First position available EXPECT_TRUE(observer_->last_position_.IsValidFix()); CheckLastPositionInfo(1, 2, 150); - SetPositionFix(&providers_->gps_->position_, 3, 4, 50, GetTimeNow()); - providers_->gps_->HandlePositionChanged(); + SetPositionFix(providers_->gps_, 3, 4, 50); // More accurate fix available CheckLastPositionInfo(3, 4, 50); - SetPositionFix(&providers_->cell_->position_, 5, 6, 150, GetTimeNow()); - providers_->cell_->HandlePositionChanged(); + SetPositionFix(providers_->cell_, 5, 6, 150); // New fix is available but it's less accurate, older fix should be kept. CheckLastPositionInfo(3, 4, 50); // Advance time, and notify once again AdvanceTimeNow(SwitchOnFreshnessCliff()); - providers_->cell_->HandlePositionChanged(); + providers_->cell_->HandlePositionChanged(providers_->cell_->position_); // New fix is available, less accurate but fresher CheckLastPositionInfo(5, 6, 150); // Advance time, and set a low accuracy position AdvanceTimeNow(SwitchOnFreshnessCliff()); - SetPositionFix(&providers_->cell_->position_, 5.676731, 139.629385, 1000, - GetTimeNow()); - providers_->cell_->HandlePositionChanged(); + SetPositionFix(providers_->cell_, 5.676731, 139.629385, 1000); CheckLastPositionInfo(5.676731, 139.629385, 1000); // 15 secs later, step outside. Switches to gps signal. AdvanceTimeNow(base::TimeDelta::FromSeconds(15)); - SetPositionFix(&providers_->gps_->position_, 3.5676457, 139.629198, 50, - GetTimeNow()); - providers_->gps_->HandlePositionChanged(); + SetPositionFix(providers_->gps_, 3.5676457, 139.629198, 50); CheckLastPositionInfo(3.5676457, 139.629198, 50); // 5 mins later switch cells while walking. Stay on gps. AdvanceTimeNow(base::TimeDelta::FromMinutes(5)); - SetPositionFix(&providers_->cell_->position_, 3.567832, 139.634648, 300, - GetTimeNow()); - SetPositionFix(&providers_->gps_->position_, 3.5677675, 139.632314, 50, - GetTimeNow()); - providers_->cell_->HandlePositionChanged(); - providers_->gps_->HandlePositionChanged(); + SetPositionFix(providers_->cell_, 3.567832, 139.634648, 300); + SetPositionFix(providers_->gps_, 3.5677675, 139.632314, 50); CheckLastPositionInfo(3.5677675, 139.632314, 50); // Ride train and gps signal degrades slightly. Stay on fresher gps AdvanceTimeNow(base::TimeDelta::FromMinutes(5)); - SetPositionFix(&providers_->gps_->position_, 3.5679026, 139.634777, 300, - GetTimeNow()); - providers_->gps_->HandlePositionChanged(); + SetPositionFix(providers_->gps_, 3.5679026, 139.634777, 300); CheckLastPositionInfo(3.5679026, 139.634777, 300); // 14 minutes later @@ -271,34 +256,24 @@ TEST_F(GeolocationLocationArbitratorTest, Arbitration) { // GPS reading misses a beat, but don't switch to cell yet to avoid // oscillating. - SetPositionFix(&providers_->gps_->position_, 3.5659005, 139.682579, 300, - GetTimeNow()); - providers_->gps_->HandlePositionChanged(); + SetPositionFix(providers_->gps_, 3.5659005, 139.682579, 300); AdvanceTimeNow(base::TimeDelta::FromSeconds(7)); - SetPositionFix(&providers_->cell_->position_, 3.5689579, 139.691420, 1000, - GetTimeNow()); - providers_->cell_->HandlePositionChanged(); + SetPositionFix(providers_->cell_, 3.5689579, 139.691420, 1000); CheckLastPositionInfo(3.5659005, 139.682579, 300); // 1 minute later AdvanceTimeNow(base::TimeDelta::FromMinutes(1)); // Enter tunnel. Stay on fresher gps for a moment. - SetPositionFix(&providers_->cell_->position_, 3.5657078, 139.68922, 300, - GetTimeNow()); - providers_->cell_->HandlePositionChanged(); - SetPositionFix(&providers_->gps_->position_, 3.5657104, 139.690341, 300, - GetTimeNow()); - providers_->gps_->HandlePositionChanged(); + SetPositionFix(providers_->cell_, 3.5657078, 139.68922, 300); + SetPositionFix(providers_->gps_, 3.5657104, 139.690341, 300); CheckLastPositionInfo(3.5657104, 139.690341, 300); // 2 minutes later AdvanceTimeNow(base::TimeDelta::FromMinutes(2)); // Arrive in station. Cell moves but GPS is stale. Switch to fresher cell. - SetPositionFix(&providers_->cell_->position_, 3.5658700, 139.069979, 1000, - GetTimeNow()); - providers_->cell_->HandlePositionChanged(); + SetPositionFix(providers_->cell_, 3.5658700, 139.069979, 1000); CheckLastPositionInfo(3.5658700, 139.069979, 1000); } diff --git a/chrome/browser/geolocation/mock_location_provider.cc b/chrome/browser/geolocation/mock_location_provider.cc index 8d3a73f..689ab9c 100644 --- a/chrome/browser/geolocation/mock_location_provider.cc +++ b/chrome/browser/geolocation/mock_location_provider.cc @@ -33,14 +33,15 @@ MockLocationProvider::~MockLocationProvider() { *self_ref_ = NULL; } -void MockLocationProvider::HandlePositionChanged() { +void MockLocationProvider::HandlePositionChanged(const Geoposition& position) { if (provider_loop_->BelongsToCurrentThread()) { // The location arbitrator unit tests rely on this method running // synchronously. + position_ = position; UpdateListeners(); } else { Task* task = NewRunnableMethod( - this, &MockLocationProvider::HandlePositionChanged); + this, &MockLocationProvider::HandlePositionChanged, position); provider_loop_->PostTask(FROM_HERE, task); } } @@ -102,7 +103,7 @@ class AutoMockLocationProvider : public MockLocationProvider { listeners_updated_ = true; MessageLoop::current()->PostTask( FROM_HERE, task_factory_.NewRunnableMethod( - &MockLocationProvider::HandlePositionChanged)); + &MockLocationProvider::HandlePositionChanged, position_)); } } diff --git a/chrome/browser/geolocation/mock_location_provider.h b/chrome/browser/geolocation/mock_location_provider.h index ced5d89..17f11d8 100644 --- a/chrome/browser/geolocation/mock_location_provider.h +++ b/chrome/browser/geolocation/mock_location_provider.h @@ -23,7 +23,7 @@ class MockLocationProvider : public LocationProviderBase { ~MockLocationProvider(); // Updates listeners with the new position. - void HandlePositionChanged(); + void HandlePositionChanged(const Geoposition& position); // LocationProviderBase implementation. virtual bool StartProvider(bool high_accuracy); -- cgit v1.1